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

Preserve ordering equivalencies on with_reorder #13770

Merged

Conversation

gokselk
Copy link
Contributor

@gokselk gokselk commented Dec 13, 2024

Which issue does this PR close?

Closes #13769.

Rationale for this change

The current implementation of with_reorder discards all existing ordering information when applying a new ordering. This is overly conservative since in many cases, parts of the existing ordering remain valid and could be preserved. Preserving valid orderings is important for query optimization as it allows the optimizer to avoid unnecessary sorting operations.

What changes are included in this PR?

  1. Enhanced with_reorder() to preserve valid suffixes from existing orderings when they don't conflict with the new ordering
  2. Added filtering of constant expressions from ordering considerations
  3. Added exprs_equal() helper method to EquivalenceGroup for comparing expressions considering equivalence classes
  4. Added comprehensive test suite covering:
    • Constant expression filtering
    • Suffix preservation
    • Equivalent expression handling
    • Incompatible prefix cases
    • Complex scenarios combining multiple features

Are these changes tested?

Yes, the PR includes extensive test coverage:

  • test_with_reorder_constant_filtering: Tests filtering of constant expressions
  • test_with_reorder_preserve_suffix: Verifies preservation of valid ordering suffixes
  • test_with_reorder_equivalent_expressions: Tests handling of equivalent expressions
  • test_with_reorder_incompatible_prefix: Ensures proper handling of incompatible orderings
  • test_with_reorder_comprehensive: Tests complex interactions between constants, equivalences, and multiple orderings

Are there any user-facing changes?

This change is an internal optimization that preserves more ordering information. While there are no direct API changes, users may notice improved query performance in scenarios where ordering information is better preserved through operations that modify sort orders.

@github-actions github-actions bot added the physical-expr Physical Expressions label Dec 13, 2024
@gokselk
Copy link
Contributor Author

gokselk commented Dec 13, 2024

cc: @berkaysynnada @ozankabak

Copy link
Contributor

@berkaysynnada berkaysynnada left a comment

Choose a reason for hiding this comment

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

LGTM

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.

I have one comment, looks good after we address it (and add a test)

@alamb
Copy link
Contributor

alamb commented Dec 20, 2024

Is this PR ready to merge?

I triggered the CI checks again

@gokselk gokselk requested a review from ozankabak December 20, 2024 05:55
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, @berkaysynnada feel free to merge after you take a final look

@berkaysynnada
Copy link
Contributor

resolved the conflicts, will be merged once CI passes

@berkaysynnada berkaysynnada merged commit b0d7cd0 into apache:main Dec 20, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
physical-expr Physical Expressions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Preserve ordering equivalencies on with_reorder
4 participants