Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose Constraints internals #7511

Closed
tv42 opened this issue Sep 8, 2023 · 4 comments · Fixed by #7603
Closed

Expose Constraints internals #7511

tv42 opened this issue Sep 8, 2023 · 4 comments · Fixed by #7603
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@tv42
Copy link
Contributor

tv42 commented Sep 8, 2023

Describe the bug

DataFusion v29.0.0 added commit 5f03146, which contains this change:

diff --git a/datafusion/expr/src/logical_plan/ddl.rs b/datafusion/expr/src/logical_plan/ddl.rs
index e005f1147..dc247da36 100644
--- a/datafusion/expr/src/logical_plan/ddl.rs
+++ b/datafusion/expr/src/logical_plan/ddl.rs
@@ -222,8 +217,8 @@ impl Hash for CreateExternalTable {
 pub struct CreateMemoryTable {
     /// The table name
     pub name: OwnedTableReference,
-    /// The ordered list of columns in the primary key, or an empty vector if none
-    pub primary_key: Vec<Column>,
+    /// The list of constraints in the schema, such as primary key, unique, etc.
+    pub constraints: Constraints,
     /// The logical plan
     pub input: Arc<LogicalPlan>,
     /// Option to not error if table already exists

So, previously, primary_key was just a Vec, and Column was public. Now it's Constraints, and Constraints has only private fields and no accessor methods: https://docs.rs/datafusion/latest/datafusion/common/struct.Constraints.html

Previously, I could take a logical plan and introspect it. Now I can't.

Can we make Constraint public, and either make the inner of Constraints public, or add an accessor method (maybe an iterator)?

The change should be pretty simple, but I'm trying to understand the thinking behind the current API.

Cc @mustafasrepo @metesynnada @ozankabak

To Reproduce

        let statement = session_state.sql_to_statement(sql, sql_dialect)?;
        let logical_plan = session_state.statement_to_plan(statement).await?;

        match logical_plan {
            datafusion::logical_expr::LogicalPlan::Ddl(
                datafusion::logical_expr::DdlStatement::CreateMemoryTable(create_table),
            ) => {
                for key in &create_table.primary_key {
                    println!("key {} ref {:?}", key.name, key.relation);
                }
            _ => (),
        }

Expected behavior

Be able to introspect the LogicalPlan.

Additional context

No response

@tv42 tv42 added the bug Something isn't working label Sep 8, 2023
@ozankabak
Copy link
Contributor

Can we make Constraint public, and either make the inner of Constraints public, or add an accessor method (maybe an iterator)?

This sounds reasonable to me.

@alamb alamb added the good first issue Good for newcomers label Sep 19, 2023
@alamb
Copy link
Contributor

alamb commented Sep 19, 2023

I think this would be a great first issue:

  1. Add Constraints::iter() and Contraints::into_inner() --> Vec<Constraint>
  2. Write some tests

@tv42
Copy link
Contributor Author

tv42 commented Sep 19, 2023

I have a simple patch that implements Deref to &[Constraint] -- that seems like the right thing, and gives iteration for free. I need to read the contributing guide, but the code is trivial.

tv42 added a commit to tv42/arrow-datafusion that referenced this issue Sep 19, 2023
@alamb
Copy link
Contributor

alamb commented Sep 19, 2023

Thanks @tv42

alamb pushed a commit that referenced this issue Sep 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants