support simple/cross lateral joins #16015
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Alex Chi Z <iskyzh@gmail.com>
Signed-off-by: Alex Chi Z <iskyzh@gmail.com>
Signed-off-by: Alex Chi Z <iskyzh@gmail.com>
Signed-off-by: Alex Chi Z <iskyzh@gmail.com>
Signed-off-by: Alex Chi Z <iskyzh@gmail.com>
Signed-off-by: Alex Chi Z <iskyzh@gmail.com>
Signed-off-by: Alex Chi Z <iskyzh@gmail.com>
Signed-off-by: Alex Chi Z <iskyzh@gmail.com>
github-actions
bot
added
optimizer
comphead
approved these changes
May 12, 2025
There was a problem hiding this comment.
Thanks @jayzhan211 imho it is lgtm, please check some nits though
| 4 4 1 | ||
|
|
||
| query IIII | ||
| SELECT * FROM t0, LATERAL (SELECT * FROM t1 WHERE t1.v0 = 1); |
There was a problem hiding this comment.
should we also check the join syntax?
SELECT * FROM t0 JOIN LATERAL (SELECT * FROM t1 WHERE t1.v0 = 1) on true;
?
| @@ -313,6 +317,11 @@ impl TreeNodeRewriter for PullUpCorrelatedExpr { | |||
| missing_exprs.push(un_matched_row); | |||
| } | |||
| } | |||
| if aggregate.group_expr.is_empty() { | |||
| // TODO: how do we handle the case where we have pulled multiple aggregations? For example, | |||
There was a problem hiding this comment.
prob we can create a ticket for that?
| } | ||
|
|
||
| match join.right.apply_with_subqueries(|p| { | ||
| if p.contains_outer_reference() { |
There was a problem hiding this comment.
perhaps we can add a comment why queries with outer reference won't be optimized? 🤔
There was a problem hiding this comment.
not yet supported feature
| return Ok(Transformed::no(LogicalPlan::Join(join))); | ||
| } | ||
| let subquery_plan = subquery.subquery.as_ref(); | ||
| let mut pull_up = PullUpCorrelatedExpr::new().with_need_handle_count_bug(true); |
There was a problem hiding this comment.
now I'm thinking of having this .with_need_handle_count_bug to move it to unsafe or something to let users know immediately the risks?
|
FYI @Lordworms I am not sure if this is related to some of your other work |
I am doing a DelimJoin way (duckdb style) to support non-euqual lateral join like and which may got some overlap with this pr since I have to identify the join condition as well |
|
@irenjj perhaps you can review this PR as well as i cross / lateral joins are related to subqueries I think |
alamb
reviewed
May 15, 2025
There was a problem hiding this comment.
Maybe eventually it would be nice to add / adapt the tests from duckdb and make sure we get the same answers: https://github.com/duckdb/duckdb/tree/main/test/sql/subquery/lateral
|
🤖 |
alamb
reviewed
May 15, 2025
Comment on lines
42
to
44
| pub mod decorrelate; | ||
| pub mod decorrelate_lateral_join; | ||
| pub mod decorrelate_predicate_subquery; |
There was a problem hiding this comment.
It would be really nice to figure out how to combine these passes into one unified set of decorrelation code -- now we have three passes
FYI @irenjj
|
I am sorry I don't have the capacity to study LATERAL joins in detail to really review the code carefully. If you are happy with I think we should merge it. I suspect we'll have to add more general code eventually but augmenting our test cases / features along the way seems good to me |
|
🤖: Benchmark completed Details
|
|
TODOs:
|
jfahne
pushed a commit
to jfahne/datafusion
that referenced
this pull request
May 18, 2025
* support simple lateral joins Signed-off-by: Alex Chi Z <iskyzh@gmail.com> * fix explain test Signed-off-by: Alex Chi Z <iskyzh@gmail.com> * plan scalar agg correctly Signed-off-by: Alex Chi Z <iskyzh@gmail.com> * add uncorrelated query tests Signed-off-by: Alex Chi Z <iskyzh@gmail.com> * fix clippy + fmt Signed-off-by: Alex Chi Z <iskyzh@gmail.com> * make rule matching faster Signed-off-by: Alex Chi Z <iskyzh@gmail.com> * revert build_join visibility Signed-off-by: Alex Chi Z <iskyzh@gmail.com> * revert find plan outer column changes Signed-off-by: Alex Chi Z <iskyzh@gmail.com> * remove clone * address comment --------- Signed-off-by: Alex Chi Z <iskyzh@gmail.com> Co-authored-by: Alex Chi Z <iskyzh@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
address comment from #14595 to get this in.
Rationale for this change
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?