feat: Support planning subqueries with OuterReferenceColumn belongs to non-adjacent outer relations #19930
feat: Support planning subqueries with OuterReferenceColumn belongs to non-adjacent outer relations #19930mkleen wants to merge 26 commits intoapache:mainfrom
Conversation
| Projection: l.a, l.b, l.c, l.d, l.e | ||
| SubqueryAlias: l | ||
| Projection: lineitem.l_item_id AS a, lineitem.l_description AS b, lineitem.price AS c | ||
| Projection: lineitem.l_orderkey AS a, lineitem.l_item_id AS b, lineitem.l_description AS c, lineitem.l_extendedprice AS d, lineitem.price AS e |
There was a problem hiding this comment.
This is because the lineitem table is extended now.
| "o1.o_custkey".to_string(), | ||
| "o1.o_orderstatus".to_string(), | ||
| "o1.customer_id".to_string(), | ||
| "o1.o_totalprice".to_string(), |
There was a problem hiding this comment.
This is because the order table is extended now.
| assert_snapshot!( | ||
| err.strip_backtrace(), | ||
| @"Error during planning: Source table contains 3 columns but only 2 names given as column alias" | ||
| @"Error during planning: Source table contains 5 columns but only 2 names given as column alias" |
There was a problem hiding this comment.
This is because the lineitem table is extended now.
| ])), | ||
| "orders" => Ok(Schema::new(vec![ | ||
| Field::new("order_id", DataType::UInt32, false), | ||
| Field::new("o_orderkey", DataType::UInt32, false), |
There was a problem hiding this comment.
These extensions were necessary to get the tests to work.
## Which issue does this PR close? While working on #19930 , I noticed a trailing whitespace in the CROSS JOIN logical plan output. This whitespace is inconsistent with the rest of the logical plan formatting. ## Rationale for this change This change removes the unnecessary trailing whitespace in the logical plan output of a CROSS JOIN. ## What changes are included in this PR? ## Are these changes tested? Yes. ## Are there any user-facing changes? No.
99902bc to
3b1fa64
Compare
|
@alamb @duongcongtoai Could you review this when you have a moment so we can get it merged? |
af87772 to
cd81a28
Compare
|
nice, i'll take a look once i have time |
| fn table_with_column_alias() { | ||
| let sql = "SELECT a, b, c | ||
| FROM lineitem l (a, b, c)"; | ||
| let sql = "SELECT a, b, c, d, e |
There was a problem hiding this comment.
would be nice to have a sqllogictest for this PR
There was a problem hiding this comment.
ah i see you replaced them with integration test instead
There was a problem hiding this comment.
I agree we should have some sqllogictest coverage to make sure everything is hooked up right
There was a problem hiding this comment.
I added two sqllogictests back which pass without the optimizer changes, the other two remain in sql_integration.rs because they depend on the optimizer change.
| // TODO revisit this validation logic | ||
| plan_err!( | ||
| "Correlated scalar subquery in the GROUP BY clause must also be in the aggregate expressions" | ||
| "Correlated scalar subquery in the GROUP BY clause must \ |
There was a problem hiding this comment.
nit: this change is unrelated (it's in my original PR)
|
@duongcongtoai Thank you for your feedback! |
alamb
left a comment
There was a problem hiding this comment.
Thank you @mkleen and @duongcongtoai
I think this looks pretty good to me -- I have some small suggestions for API improvements, but the basic idea is 🏆
It would also be nice to restore one or two slt tests to make sure this code is connected end to end
| fn table_with_column_alias() { | ||
| let sql = "SELECT a, b, c | ||
| FROM lineitem l (a, b, c)"; | ||
| let sql = "SELECT a, b, c, d, e |
There was a problem hiding this comment.
I agree we should have some sqllogictest coverage to make sure everything is hooked up right
| /// The query schema of the outer query plan, used to resolve the columns in subquery | ||
| outer_query_schema: Option<DFSchemaRef>, | ||
|
|
||
| /// The queries schemas of outer query relations, used to resolve the outer referenced |
|
@alamb Thank you for the review. I will add your feedback soon! |
|
Thanks @mkleen -- sorry for the delay in reviewing this |
8899f47 to
5328de1
Compare
9ba50c7 to
42170bf
Compare
|
@alamb @duongcongtoai Could you please do one more review? This is the interesting change 818926d |
Which issue does this PR close?
Cleaned-up version of #18806 with:
Removed
outer_queries_schemafrom PlannerContextPlanning logic only (optimizer modifications removed)
sql logic tests moved to sql_integration.rs
Closes Supporting planning (binding) Nested correlated subquery error with a depth exceeding 1 #19816
Rationale for this change
See #18806
What changes are included in this PR?
See #18806
Are these changes tested?
Yes
Are there any user-facing changes?
outer_queries_schemais removed from PlannerContext.