Skip to content

fix: preserve sorts for wrapping integer negation - #25568

Open
haohuaijin wants to merge 7 commits into
apache:mainfrom
haohuaijin:fix/24683-negation-ordering
Open

haohuaijin wants to merge 7 commits into
apache:mainfrom
haohuaijin:fix/24683-negation-ordering

Conversation

@haohuaijin

@haohuaijin haohuaijin commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

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?

  • Stop deriving reversed ordering when the input range may contain the signed minimum, while preserving existing Singleton properties and reversal for known safe ranges.
  • Use unbounded intervals for potentially wrapping signed integer negation in forward and reverse constraint propagation, including singleton MIN ranges.
  • Add regression cases to the existing order.slt and 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?

  • A table-driven unit test covers all four signed integer widths, unbounded and MIN ranges, Singleton properties, safe reversal, and unknown input properties.
  • Four cases in order.slt cover the original incorrect ordering, MIN filter propagation, double negation on an unbounded stream, and ordering MIN values mixed with NULL.
  • The original ordering query and MIN filter reproduced failures before the fix.
  • Passed the 9 negation unit tests and order.slt, plus formatting, full Clippy, and uv run ./dev/rust_lint.sh checks.

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.

@github-actions github-actions Bot added physical-expr Changes to the physical-expr crates sqllogictest SQL Logic Tests (.slt) labels 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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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-commenter

codecov-commenter commented Sep 21, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.65%. Comparing base (76f9fde) to head (3e4cafb).

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@haohuaijin

haohuaijin commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor Author

Hi @kosiew, could you help review this follow-up to #25234 for signed-minimum negation? Thanks!

@kosiew kosiew left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@haohuaijin,

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.

Comment thread datafusion/physical-expr/src/expressions/negative.rs
@haohuaijin

Copy link
Copy Markdown
Contributor Author

Thanks @kosiew , i added the test case in 5e270ae

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

physical-expr Changes to the physical-expr crates sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unary negation can eliminate a required sort at the signed minimum

3 participants