Use TableReference for TableScan by alamb · Pull Request #5615 · apache/datafusion

alamb · 2023-03-15T18:06:52Z

Which issue does this PR close?

Closes #5522

Rationale for this change

After #5343 some APIs take a String and parse it like a table reference (aka apply sql normalization rules to it)

What changes are included in this PR?

TBD

Are these changes tested?

Yes, both existing tests as well as new unit tests

Are there any user-facing changes?

Theoretically yes, but he existing LogicalPlanBuilder::scan("tablename", ...) will continue to work so no major API work will be needed

alamb · 2023-03-15T19:00:01Z

cc @Jefffrey I wonder if you have time to review this PR?

Jefffrey

thanks for this, left a few nitpicks/comments

Jefffrey · 2023-03-15T19:23:25Z

datafusion/optimizer/src/inline_table_scan.rs

                    let plan = LogicalPlanBuilder::from(sub_plan.clone())
                        .project(projection_exprs)?
-                        .alias(table_name)?;
+                        .alias(table_name.to_string())?;


will subquery_alias need similar treatment to have an OwnedTableReference instead of String? or String itself should be just fine?

I think it is actually important that the subquery alias is a non parsed string here (otherwise tests fail).

This rewrite is effectively inlining the view defition as a named subquery (that doesn't make sense to be in a schema):

... FROM <view definition> as "table_name"

I will add a comment here.

Jefffrey · 2023-03-15T19:28:09Z

datafusion/common/src/dfschema.rs

+    pub fn from_qualified<'a>(
+        qualifier: impl Into<TableReference<'a>>,


im curious about why explicit lifetime is needed here?

Basically because the compiler told me to do so 😆

But seriously, I think the reason is that impl doesn't support anonymous lifetimes -- I would have to change this to use the explicit generic syntax

Here is what happens if I remove <'a>:

--> datafusion/common/src/dfschema.rs:113:45 | 113 | qualifier: impl Into<TableReference<'_>>, | ^^ expected named lifetime parameter | help: consider introducing a named lifetime parameter | 112 ~ pub fn try_from_qualified_schema<'a>( 113 ~ qualifier: impl Into<TableReference<'a>>, |

datafusion/core/src/physical_plan/planner.rs

Jefffrey · 2023-03-15T19:34:24Z

datafusion/expr/src/logical_plan/plan.rs

    }

    fn test_plan() -> LogicalPlan {
+        let table_ref: Option<TableReference<'_>> = None;


use TableReference::none() that you introduced?

Excellent call. Done in 2c3b6d7

Jefffrey · 2023-03-15T19:36:32Z

datafusion/expr/src/logical_plan/builder.rs

+    let table_source = table_source(table_schema);
+    let name = name
+        .map(|n| n.into())
+        .unwrap_or_else(|| OwnedTableReference::bare(UNNAMED_TABLE))


maybe TableReference::bare(UNNAMED_TABLE) instead since next line is to_owned_reference() anyway?

datafusion/expr/src/logical_plan/builder.rs

…erence

Co-authored-by: Jeffrey <22608443+Jefffrey@users.noreply.github.com>

…datafusion into alamb/owned_table_reference

alamb

Thank you @Jefffrey -- the review was really helpful.

alamb · 2023-03-15T21:31:18Z

datafusion/common/src/dfschema.rs

+    pub fn from_qualified<'a>(
+        qualifier: impl Into<TableReference<'a>>,


Basically because the compiler told me to do so 😆

But seriously, I think the reason is that impl doesn't support anonymous lifetimes -- I would have to change this to use the explicit generic syntax

Here is what happens if I remove <'a>:

--> datafusion/common/src/dfschema.rs:113:45 | 113 | qualifier: impl Into<TableReference<'_>>, | ^^ expected named lifetime parameter | help: consider introducing a named lifetime parameter | 112 ~ pub fn try_from_qualified_schema<'a>( 113 ~ qualifier: impl Into<TableReference<'a>>, |

datafusion/core/src/physical_plan/planner.rs

alamb · 2023-03-15T21:33:36Z

datafusion/expr/src/logical_plan/plan.rs

    }

    fn test_plan() -> LogicalPlan {
+        let table_ref: Option<TableReference<'_>> = None;


Excellent call. Done in 2c3b6d7

alamb · 2023-03-15T21:34:27Z

datafusion/optimizer/src/inline_table_scan.rs

                    let plan = LogicalPlanBuilder::from(sub_plan.clone())
                        .project(projection_exprs)?
-                        .alias(table_name)?;
+                        .alias(table_name.to_string())?;


I think it is actually important that the subquery alias is a non parsed string here (otherwise tests fail).

This rewrite is effectively inlining the view defition as a named subquery (that doesn't make sense to be in a schema):

... FROM <view definition> as "table_name"

I will add a comment here.

Jefffrey

Looks good, thanks

alamb · 2023-03-16T11:04:10Z

Thanks again for the review @Jefffrey -- since this is blocking me downstream (in IOx, where we are basically playing the role of early adopters / testers) I plan to merge this PR in later today.

alamb added the api change Changes the API exposed to users of the crate label Mar 15, 2023

github-actions bot added core Core DataFusion crate logical-expr Logical plan and expressions optimizer Optimizer rules sql SQL Planner substrait Changes to the substrait crate labels Mar 15, 2023

Use TableReference for TableScan

a9dfd29

alamb force-pushed the alamb/owned_table_reference branch from d213758 to a9dfd29 Compare March 15, 2023 18:14

revert doc changes

54b0d8a

alamb mentioned this pull request Mar 15, 2023

Minor: Add Documentation and Examples to TableReference #5616

Merged

alamb marked this pull request as ready for review March 15, 2023 18:59

Jefffrey reviewed Mar 15, 2023

View reviewed changes

alamb and others added 5 commits March 15, 2023 17:28

Merge remote-tracking branch 'apache/main' into alamb/owned_table_ref…

201d115

…erence

Use TableReference::none

2c3b6d7

Apply suggestions from code review

feac21f

Co-authored-by: Jeffrey <22608443+Jefffrey@users.noreply.github.com>

Merge branch 'alamb/owned_table_reference' of github.com:alamb/arrow-…

6aeff12

…datafusion into alamb/owned_table_reference

Add comment

ef1bc06

alamb commented Mar 15, 2023

View reviewed changes

Restore table check

c234489

Jefffrey approved these changes Mar 16, 2023

View reviewed changes

alamb merged commit 4afd67a into apache:main Mar 16, 2023

alamb deleted the alamb/owned_table_reference branch March 16, 2023 13:39

Jefffrey mentioned this pull request Apr 16, 2023

Use OwnedTableReference for subquery aliases #6022

Merged

		pub fn from_qualified<'a>(
		qualifier: impl Into<TableReference<'a>>,

Conversation

alamb commented Mar 15, 2023

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

alamb commented Mar 15, 2023

Uh oh!

Jefffrey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jefffrey left a comment

Choose a reason for hiding this comment

Uh oh!

alamb commented Mar 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants