fix: do not derive ordering for arithmetic that can overflow - #23910
Conversation
|
@buraksenn Can you please mention the devs that added those test cases in the first place for review? |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #23910 +/- ##
==========================================
+ Coverage 80.70% 80.88% +0.18%
==========================================
Files 1095 1102 +7
Lines 372554 376216 +3662
Branches 372554 376216 +3662
==========================================
+ Hits 300661 304310 +3649
+ Misses 53924 53763 -161
- Partials 17969 18143 +174 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
@mustafasrepo @berkaysynnada @geoffreyclaude can you take a look at this PR if you've time? |
# Conflicts: # datafusion/physical-expr/src/expressions/binary.rs
There was a problem hiding this comment.
@buraksenn
Nice fix. The new invariant makes sense: arithmetic orderings are only preserved when overflow cannot happen or when overflow is configured to fail. The updates across the optimizer, tests, and sqllogictests look consistent. I just have a couple of small suggestions that could make the coverage a bit stronger.
| use datafusion_expr::col as logical_col; | ||
|
|
||
| #[test] | ||
| fn test_arithmetic_ordering_overflow() -> Result<()> { |
There was a problem hiding this comment.
Nice focused regression test. One small suggestion: could we also add a couple of Minus cases covering signed minimum and unsigned underflow boundaries? The implementation now guards both Plus and Minus, but this test only exercises addition. It would help protect the subtraction path from future regressions.
There was a problem hiding this comment.
I've added signed minimum and underflow boundaries which resulted into finding a gap. Resolved that as well thanks for finding this
| (expected | false), | ||
| "{err_msg}" | ||
| ); | ||
| let may_overflow = ordering |
There was a problem hiding this comment.
I wonder if we could narrow this exemption to only the specific arithmetic ordering that may be conservatively rejected. Using any means we skip the rejection assertion for the entire multi-key requirement whenever one key contains + or -, which reduces coverage for any later non-arithmetic keys. Not blocking, since it does not affect production behavior.
There was a problem hiding this comment.
Thanks I've applied this as well
There was a problem hiding this comment.
@buraksenn, thanks for the follow-up changes.
The subtraction ordering regression coverage now includes signed-minimum overflow, unsigned underflow, safe bounded ranges, and time-minus-interval wrapping. The bounded unsigned subtraction logic also correctly compares the minimum left-hand endpoint with the maximum right-hand endpoint, which avoids deriving ordering guarantees when subtraction could underflow and wrap.
The fuzz-test exemption is now limited to the arithmetic-free ordering prefix in both the direct-ordering and projection tests, while still checking preceding non-arithmetic keys.
I did not find any new issues in the updated changes.
…23910) ## Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes apache#123` indicates that this PR will close issue apache#123. --> - Closes apache#23902. ## Rationale for this change Please check the issue for details but the main idea is that `a + b` over two sorted columns is not guaranteed to be sorted. ## What changes are included in this PR? Ordering is discarded when it overflows or wraps ## Are these changes tested? Yes, adjusted existing tests and added a regression test in `order.slt`. ## Are there any user-facing changes? no api changes
Which issue does this PR close?
Rationale for this change
Please check the issue for details but the main idea is that
a + bover twosorted columns is not guaranteed to be sorted.
What changes are included in this PR?
Ordering is discarded when it overflows or wraps
Are these changes tested?
Yes, adjusted existing tests and added a regression test in
order.slt.Are there any user-facing changes?
no api changes