fix: sort InListExpr terms before building pruning predicate OR chain - #25311
Open
Rich-T-kid wants to merge 2 commits into
Open
Rich-T-kid wants to merge 2 commits into
Rich-T-kid wants to merge 2 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #25311 +/- ##
=========================================
Coverage 81.91% 81.92%
=========================================
Files 1134 1135 +1
Lines 425708 427773 +2065
Branches 425708 427773 +2065
=========================================
+ Hits 348726 350451 +1725
- Misses 56305 56378 +73
- Partials 20677 20944 +267 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
asolimando
reviewed
Sep 15, 2026
| let change_expr = in_list | ||
| .list() | ||
| let mut sorted_list: Vec<_> = in_list.list().to_vec(); | ||
| sorted_list.sort_by_key(|a| a.to_string()); |
Member
There was a problem hiding this comment.
The to_string part can be costly, is there a cheaper alternative maybe? Anyway the ordering doesn't really matter, anything stable that is cheaper would do, like hashing.
In case we want to keep it, we should probably be using sort_by_cached_key if the to_string part is finally expensive.
Contributor
Author
There was a problem hiding this comment.
@asolimando updated this in the latest commit
Rich-T-kid
force-pushed
the
rich-T-kid/fix/sort-inlist-pruning-predicate
branch
from
September 15, 2026 20:40
791592c to
7f40357
Compare
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
3 participants
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?
Rationale for this change
build_predicate_expressionfolds InListExpr values into an OR/AND chain in execution order, which is non-deterministic when the list is built from a hash join's build side. This makes the pruning predicate's string representation unstable between runs, causing flaky snapshot tests.What changes are included in this PR?
In
build_predicate_expression(datafusion/pruning/src/pruning_predicate.rs), IN-list items are sorted by display string before being folded into the OR/AND chain, making the output deterministic without changing pruning behavior.What is the testing strategy for this PR?
I think this is straight forward enough to not warrant a test. can create one if anyone disagrees
Are there any user-facing changes?
somewhat. existing code doesnt break but moving forward users can expect deterministic formating