Skip to content

Fix：fix HashJoin projection swap #12967

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Oct 18, 2024
Merged

Fix：fix HashJoin projection swap #12967

merged 7 commits into from
Oct 18, 2024

Conversation

@my-vegetable-has-exploded
Copy link
Contributor

@my-vegetable-has-exploded my-vegetable-has-exploded commented Oct 16, 2024 •
edited
Loading

Which issue does this PR close?

ref #12491, #10978

If there is embeded projection in HashJoinExec, no need to add projection above hashjoin after swapping join order.

Thanks @onursatici for catching it.

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added the core Core DataFusion crate label Oct 16, 2024
@my-vegetable-has-exploded
Copy link
Contributor Author

my-vegetable-has-exploded commented Oct 16, 2024

ptal @korowa @Dandandan

korowa
korowa approved these changes Oct 16, 2024
View reviewed changes
Copy link
Contributor

@korowa korowa left a comment

Choose a reason for hiding this comment

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

Looks like a proper fix as this projection is, indeed, redundant for projected HJ.

Thank you @my-vegetable-has-exploded

@my-vegetable-has-exploded
assert hashjoinexec.
9a9aceb
@my-vegetable-has-exploded @korowa
Update datafusion/core/src/physical_optimizer/join_selection.rs
29ea1a3 
Co-authored-by: Eduard Karacharov <eduard.karacharov@gmail.com>
berkaysynnada
berkaysynnada approved these changes Oct 18, 2024
View reviewed changes
Copy link
Contributor

@berkaysynnada berkaysynnada left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, thanks @my-vegetable-has-exploded -- after you have fixed the clippy errors, we can merge this PR

@korowa korowa merged commit e9435a9 into apache:main Oct 18, 2024
24 checks passed
@my-vegetable-has-exploded my-vegetable-has-exploded deleted the fix-hash-join-projection-swap branch October 19, 2024 02:54
@my-vegetable-has-exploded
Copy link
Contributor Author

my-vegetable-has-exploded commented Oct 19, 2024

thanks @korowa @berkaysynnada

@my-vegetable-has-exploded my-vegetable-has-exploded mentioned this pull request Oct 19, 2024
Merged
@korowa korowa mentioned this pull request Oct 20, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@korowa korowa korowa approved these changes

@berkaysynnada berkaysynnada berkaysynnada approved these changes

Assignees

No one assigned

Labels

core Core DataFusion crate

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@my-vegetable-has-exploded @korowa @berkaysynnada @onursatici