fix: prevent unsafe integer interval propagation

[haohuaijin]

Which issue does this PR close?

• close: Integer interval inference can incorrectly remove ORDER BY #25232

Rationale for this change

Arithmetic filters can incorrectly identify a column as constant and eliminate a required SortExec, returning rows in the wrong order. Integer multiplication can wrap, and integer division truncates, so their mathematical inverses do not necessarily describe all valid inputs.

CREATE TABLE wrap_test (a INT) AS VALUES (-2147483647), (1);
SELECT a FROM wrap_test
WHERE a * 2::INT = 2::INT ORDER BY a DESC;
-- Expected: 1, -2147483647
-- Actual before this fix: -2147483647, 1

CREATE TABLE division_test (a INT) AS VALUES (2), (3);
SELECT a FROM division_test
WHERE a / 2::INT = 1::INT ORDER BY a DESC;
-- Expected: 3, 2
-- Actual before this fix: 2, 3

Both multiplication inputs evaluate to 2; both division inputs evaluate to 1. Nevertheless, interval inference narrows a to a singleton and removes the sort. Without the filters, the sorts are retained and these queries return the correct order. With ORDER BY ... LIMIT, incorrect sort elimination can also change which rows are returned.

What changes are included in this PR?

• Return unbounded output intervals when unchecked integer addition, subtraction or multiplication may wrap.
• Skip inverse propagation for integer division, potentially wrapping arithmetic, and zero products where an operand may be zero.
• Retain interval narrowing for supported arithmetic proven safe from these cases.
• Correct a Filter statistics test whose previous lower bound excluded a valid wrapping-subtraction input.

What is the testing strategy for this PR?

• SQL regressions in filter_without_sort_exec.slt cover both reproductions and sorting a wrapped expression; verified failing before the fix and passing afterward.
• Unit tests enumerate selected Int8 domains under checked and wrapping arithmetic, ensuring forward and propagated intervals retain valid runtime values. Additional tests cover unsigned subtraction underflow and safe multiplication narrowing.
• Relevant interval, analysis and Filter tests, the complete affected SLT file, Clippy and repository lint checks pass locally.

Are there any user-facing changes?

Affected queries retain necessary sorting. Unsafe interval optimizations are skipped; SQL arithmetic semantics and public APIs are unchanged.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 99.09910% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.29%. Comparing base (be5a96e) to head (6a42c2d).
⚠️ Report is 25 commits behind head on main.

Files with missing lines	Patch %	Lines
datafusion/physical-expr/src/expressions/binary.rs	99.06%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #25234      +/-   ##
==========================================
+ Coverage   81.94%   82.29%   +0.35%     
==========================================
  Files        1135     1137       +2     
  Lines      428166   430429    +2263     
  Branches   428166   430429    +2263     
==========================================
+ Hits       350841   354216    +3375     
+ Misses      56378    54789    -1589     
- Partials    20947    21424     +477     

☔ 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]

Hi @kosiew would you mind reviewing this fix?

[kosiew]

@haohuaijin, thanks for working on this. The changes look good to me. I like that this addresses both wrapping integer arithmetic and truncating division while keeping the interval and sort-property inference conservative. The regression coverage around ordering and statistics is also helpful.

I left one non-blocking suggestion for some additional coercion-path coverage.

[haohuaijin]

Thanks @kosiew , i apply suggestion in 99eaf75

[kosiew]

🚀
@haohuaijin
Thank you for your contribution.

[haohuaijin]

Hi @kosiew, I looked into the merge queue failures. The widening change exposed two existing gaps in interval handling: negation doesn’t handle i32::MIN, and narrowing casts can fail on estimated bounds even when the actual data fits. Previously, those endpoints stayed unbounded, so these paths didn’t encounter the concrete limits. Preserving the source type’s bounds is valid, but it brought these cases into play, so we need to address them or leave widening out of this PR.

I think we have two options:

1. Remove the widening change, use Float64 for the FIFO test’s ordered column, and update the sort expectation. I tested this, and all 154 previously failing SQLite files pass.
2. Keep widening and fix how negation and narrowing casts handle those bounds.

I’m perfer 1 to keep this PR focused, then opening a separate issue for the widening optimization. What do you think? I’d like to hear your thoughts before I update the PR.

[kosiew]

@haohuaijin

I agree with option 1. The widening optimization exposes pre-existing interval-bound failures in negation and narrowing casts, and fixing both correctly needs broader boundary coverage than this sort-correctness fix. Please remove the widening change, keep the overflow/division fixes, and update the FIFO and sort expectations. We can track widening separately with targeted tests for those bound transitions.

[haohuaijin]

Hi @kosiew , i applied the option 1 and raise the follow up issue #25407

[kosiew]

@haohuaijin,

Thanks for the follow-up. The option 1 changes look good to me.

Removing the widening-integer interval-bound propagation keeps the cast endpoints conservative, and the FIFO fixture and physical-plan SLT expectation have been updated accordingly. The FIFO test now avoids the unrelated integer-bound transition, while widening integer arithmetic correctly retains the required sort.

All of my previous comments have been addressed. The earlier non-blocking suggestion around the coercion path is no longer relevant given the agreed scope reduction.

I don't see any new correctness issues introduced by these follow-up changes. Thanks for working through this!

[haohuaijin]

thanks again @kosiew