Remove inline table scan analyzer rule #15201
Conversation
github-actions
bot
added
sql
There was a problem hiding this comment.
Thanks @jayzhan211
I have a suggestion -- let me know what you think
I would also like to run planning benchmarks on this PR -- will do later
datafusion/sql/src/select.rs
Outdated
| @@ -550,7 +550,29 @@ impl<S: ContextProvider> SqlToRel<'_, S> { | |||
| 0 => Ok(LogicalPlanBuilder::empty(true).build()?), | |||
| 1 => { | |||
| let input = from.remove(0); | |||
| self.plan_table_with_joins(input, planner_context) | |||
| let table = self.plan_table_with_joins(input, planner_context)?; | |||
There was a problem hiding this comment.
This will have the effect of only inlining table scans for SQL
Perhaps we could move the code into LogicalPlanBuilder so it also applies to dataframes?
There was a problem hiding this comment.
both dataframe and sql works well now
github-actions
bot
added
logical-expr
There was a problem hiding this comment.
Thanks @jayzhan211 -- this is looking close. I am also running benchmarks and will report back shortly
| 1 2 | ||
| 1 3 | ||
|
|
||
| query error DataFusion error: Execution error: Table 'v' doesn't exist\. |
There was a problem hiding this comment.
I think it should be drop view v rather than drop table
| } | ||
| } | ||
|
|
||
| Ok(Self::new(LogicalPlan::TableScan(table_scan))) |
There was a problem hiding this comment.
seems like the same logic should apply to scan_with_filters_fetch below
| @@ -1563,8 +1563,12 @@ async fn with_column_join_same_columns() -> Result<()> { | |||
| \n Limit: skip=0, fetch=1\ | |||
| \n Sort: t1.c1 ASC NULLS FIRST\ | |||
| \n Inner Join: t1.c1 = t2.c1\ | |||
| \n TableScan: t1\ | |||
| \n TableScan: t2", | |||
| \n SubqueryAlias: t1\ | |||
There was a problem hiding this comment.
I am still worried about this change -- I don't undersrtand why the subquery allias has not been removed or if that is a problem 🤔
Which issue does this PR close?
Analyzerwithin LogicalPlan building stage #14618Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?