Skip to content

minor: Improve equivalence handling of joins#16893

Merged
berkaysynnada merged 2 commits into
apache:mainfrom
synnada-ai:minor-join
Aug 11, 2025
Merged

minor: Improve equivalence handling of joins#16893
berkaysynnada merged 2 commits into
apache:mainfrom
synnada-ai:minor-join

Conversation

@berkaysynnada

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

  • Closes #.

Rationale for this change

While building the equivalence properties of joins, we can identify more precise orderings when the join is a LeftJoin and the join algorithm preserves the order of the left input (and similarly for RightJoin with the right input).

What changes are included in this PR?

In addition to the main change described above:

  • Replaced hash_join_swap_subrule with the actual optimizer rule it represents, as this is the rule we should be testing
  • Extended the columns in the build_sides_record_batches utility to include a new column that generates randomly ordered data. While not yet used, this column will be useful for future fuzz testing of joins.
  • Increased the visibility of reorder_output_after_swap() and swap_join_projection(), as these functions are likely to be needed across optimizer rules.

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions Bot added physical-expr Changes to the physical-expr crates core Core DataFusion crate physical-plan Changes to the physical-plan crate labels Jul 24, 2025
@berkaysynnada

Copy link
Copy Markdown
Contributor Author

@comphead @xudong963 do you have any time to review this?

@comphead comphead left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @berkaysynnada that is lgtm, however I'm not quite sure how the test asserts the changes? Should it be some performance benefit of using prefixed orderings for outer joins?

@alamb

alamb commented Aug 5, 2025

Copy link
Copy Markdown
Contributor

This PR seems stuck -- is it still waiting on some tests or is it ready to merge?

@xudong963
xudong963 self-requested a review August 6, 2025 01:46
@github-actions github-actions Bot added the sqllogictest SQL Logic Tests (.slt) label Aug 6, 2025
@berkaysynnada

Copy link
Copy Markdown
Contributor Author

I just added a test showing the purpose of this change

@berkaysynnada
berkaysynnada merged commit 3d6541d into apache:main Aug 11, 2025
27 checks passed
@alamb

alamb commented Aug 11, 2025

Copy link
Copy Markdown
Contributor

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Core DataFusion crate physical-expr Changes to the physical-expr crates physical-plan Changes to the physical-plan crate sqllogictest SQL Logic Tests (.slt)

3 participants