fix: preserve sorts for wrapping integer negation - #25568
Open
haohuaijin wants to merge 7 commits into
Open
haohuaijin wants to merge 7 commits into
haohuaijin wants to merge 7 commits into
Conversation
haohuaijin
commented
Sep 21, 2026
Comment on lines
+1973
to
+1975
| physical_plan | ||
| 01)SortExec: expr=[(- a@0 + b@1) DESC NULLS LAST], preserve_partitioning=[false] | ||
| 02)--DataSourceExec: file_groups={1 group: [[WORKSPACE_ROOT/datafusion/sqllogictest/data/composite_order.csv]]}, projection=[a, b], output_ordering=[a@0 + b@1 ASC NULLS LAST], file_type=csv, has_header=true |
Contributor
Author
There was a problem hiding this comment.
As a follow-up to #25220, exact column statistics could prove that a + b cannot reach INT_MIN, allowing us to safely reverse the declared ordering and eliminate this SortExec.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #25568 +/- ##
========================================
Coverage 82.65% 82.65%
========================================
Files 1147 1147
Lines 446087 446205 +118
Branches 446087 446205 +118
========================================
+ Hits 368710 368821 +111
- Misses 54990 54994 +4
- Partials 22387 22390 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
Author
kosiew
approved these changes
Oct 3, 2026
kosiew
left a comment
Contributor
There was a problem hiding this comment.
Thanks for working on this. The wrapping signed-integer negation handling looks correct, and the new tests cover the main ordering and interval-propagation cases well. I have one non-blocking test suggestion below.
Contributor
Author
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?
Closes #24683.
Related to #25407.
Rationale for this change
An ordered signed integer column does not necessarily remain ordered after negation: the minimum value wraps to itself. Treating negation as always reversing order can remove a required sort and return incorrectly ordered rows. Negating inferred interval endpoints can also raise an overflow error even though array evaluation uses wrapping negation.
What changes are included in this PR?
order.sltand update the expected plan for negation of an ordered BIGINT expression.Runtime scalar and array negation semantics are unchanged. This conservative fix can retain additional sorts when safe bounds are unavailable, including after widening casts; improving cast range propagation is left to separate work.
What is the testing strategy for this PR?
order.sltcover the original incorrect ordering, MIN filter propagation, double negation on an unbounded stream, and ordering MIN values mixed with NULL.order.slt, plus formatting, full Clippy, anduv run ./dev/rust_lint.shchecks.Are there any user-facing changes?
Queries involving signed minimum negation retain necessary sorts and no longer fail solely because an inferred negation endpoint overflows. Some range refinement and sort elimination become more conservative. No public API changes.