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

Support filter in cross join elimination #13025

Merged
merged 4 commits into from
Oct 22, 2024

Conversation

Dandandan
Copy link
Contributor

@Dandandan Dandandan commented Oct 20, 2024

Which issue does this PR close?

Closes #7549, #4877

Rationale for this change

Simplifying optimization rule and supporting an extra case, adding more opportunity for optimization.

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt) labels Oct 20, 2024
@Dandandan Dandandan marked this pull request as ready for review October 20, 2024 21:16
@@ -253,13 +269,7 @@ fn flatten_join_inputs(
fn can_flatten_join_inputs(plan: &LogicalPlan) -> bool {
// can only flatten inner / cross joins
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps we can add the reason why only inner/cross joins can be flattened?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the reason is that the current code doesn't support that / it's not obvious how it could work for non-inner joins.

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR makes sense to me -- thank you @Dandandan and @comphead

@@ -660,7 +663,17 @@ mod tests {
.filter(col("t1.a").gt(lit(15u32)))?
.build()?;

assert_optimization_rule_fails(plan);
let expected = vec![
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this plan seems correct to me

@@ -1152,7 +1152,7 @@ logical_plan
01)Projection: t1.v0, t1.v1, t5.v2, t5.v3, t5.v4, t0.v0, t0.v1
02)--Inner Join: CAST(t1.v0 AS Float64) = t0.v1 Filter: t0.v1 + CAST(t5.v0 AS Float64) > Float64(0)
03)----Projection: t1.v0, t1.v1, t5.v0, t5.v2, t5.v3, t5.v4
04)------Inner Join: Using t1.v0 = t5.v0, t1.v1 = t5.v1
04)------Inner Join: t1.v0 = t5.v0, t1.v1 = t5.v1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This basically seems like it is the same plan to me (just with a different syntax 🤔 )

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah it's just the rule now "rewrites" it, before it didn't because of the filter in the join above it.

@Dandandan Dandandan merged commit b978cf8 into apache:main Oct 22, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle Non-Equal Condition in EliminateCrossJoin
4 participants