Repository navigation
Conversation
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
github-actions
Bot
removed
the
documentation
alamb
marked this pull request as ready for review
November 9, 2026 16:14
alamb
commented
Nov 9, 2026
| /// rows: they are sorted by `x`; among rows with equal `x`, sorted by `y`; | ||
| /// among rows with equal `x` and `y`, sorted by `z`. | ||
| /// | ||
| /// ## Pinning and Fixed Expressions |
Contributor
Author
There was a problem hiding this comment.
Once I went through this logic, I found the algorithm quite clever and the refactoring well done. Thank you for working on this @jayzhan211
Contributor
Author
|
I am sorry to ask you for another review @jayzhan211 but I think this is related to your attempt to try and clean up SortKeyWalk, etc cc @ozankabak and @berkaysynnada @buraksenn who may also have worked on this code and be available for a review |
alamb
commented
Nov 11, 2026
| /// For each ordering (the normal orderings of `props`, then `implied`), | ||
| /// the index of its head. Empty until the first pin, so that a walk with | ||
| /// nothing pinned allocates nothing; until then every head is at index 0. | ||
| /// The position of the head of each ordering. Empty until the first pin. |
Contributor
Author
There was a problem hiding this comment.
I found the "heads" notion quite confusing at first, but I think this captures it
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
SortKeyWalk#27234Rationale for this change
While reviewing #27191 I was very confused about how
SortKeyWalk(added in #26853) decides whether known orderings satisfy a sort requirement, and wanted to write down what I learned so others do not have to repeat it.Specifically the existing docs stated the rules but did not define the terms clearly, and had no example a reader could trace step by step.
What changes are included in this PR?
Rustdoc only, in
datafusion/physical-expr/src/equivalence/properties/walk.rs:What is the testing strategy for this PR?
CI
Are there any user-facing changes?
No.