Skip to content

Conversation

@berkaysynnada
Copy link
Contributor

Which issue does this PR close?

Closes #.

Rationale for this change

Output orderings and ordering equivalence classes of joins may result in suboptimal ways. There are some minor fixes to clean them up.

What changes are included in this PR?

A few util functions are added to normalize orderings and matching sort exprs with columns.

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 labels Aug 16, 2023
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.

Thank you @berkaysynnada -- this looks like a nice improvement. However, I don't see any tests or test changes in this PR. Can you please add some tests to ensure we don't accidentally break / cause a regression during some future refactor

I also think tests often help to iIllustrate what code is doing

Copy link
Contributor

@ozankabak ozankabak left a comment

Choose a reason for hiding this comment

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

LGTM now with the added tests.

Are the existing join operators "advanced" enough to let us write end-to-end SQL tests? If yes, can we add such a test? Otherwise, we can add such a test once we have a join operator that enables us to write such tests.

@mustafasrepo mustafasrepo merged commit 405458f into apache:main Aug 18, 2023
@alamb
Copy link
Contributor

alamb commented Aug 18, 2023

Are the existing join operators "advanced" enough to let us write end-to-end SQL tests? If yes, can we add such a test? Otherwise, we can add such a test once we have a join operator that enables us to write such tests.

I agree sql level tests would be ideal -- let's try and add them as a follow on PR. I can file a ticket to track this if that is helpful

@berkaysynnada berkaysynnada deleted the bug-fix/join-output-ordering branch August 18, 2023 13:50
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants