From 51b3a81aab062056d55e3d9e6afc04c81fcd54d7 Mon Sep 17 00:00:00 2001 From: Huaijin Date: Sat, 12 Sep 2026 21:31:47 +0800 Subject: [PATCH 01/10] fix: prevent unsafe integer interval propagation --- .../physical-expr/src/expressions/binary.rs | 168 +++++++++++++++++- datafusion/physical-plan/src/filter.rs | 13 +- .../integer_interval_propagation.slt | 66 +++++++ 3 files changed, 245 insertions(+), 2 deletions(-) create mode 100644 datafusion/sqllogictest/test_files/integer_interval_propagation.slt diff --git a/datafusion/physical-expr/src/expressions/binary.rs b/datafusion/physical-expr/src/expressions/binary.rs index dfb1d136d0ff0..a203680bb5bc3 100644 --- a/datafusion/physical-expr/src/expressions/binary.rs +++ b/datafusion/physical-expr/src/expressions/binary.rs @@ -122,6 +122,23 @@ impl BinaryExpr { &self.op } + /// Mathematical intervals do not cover values produced by wrapping arithmetic. + fn integer_arithmetic_may_wrap( + &self, + left: &Interval, + right: &Interval, + result: &Interval, + ) -> bool { + !self.fail_on_overflow + && result.data_type().is_integer() + && matches!( + self.op, + Operator::Plus | Operator::Minus | Operator::Multiply + ) + && (result.is_unbounded() + || unsigned_subtraction_may_underflow(self.op, left, right, result)) + } + /// Wrapping on overflow breaks monotonicity (e.g. the sum of two /// ascending `UInt8` columns can wrap back to small values), so the /// derived ordering is kept only when overflow is impossible. `time ± @@ -700,7 +717,12 @@ impl PhysicalExpr for BinaryExpr { let left_interval = children[0]; let right_interval = children[1]; // Calculate current node's interval: - apply_operator(&self.op, left_interval, right_interval) + let result = apply_operator(&self.op, left_interval, right_interval)?; + if self.integer_arithmetic_may_wrap(left_interval, right_interval, &result) { + Interval::make_unbounded(&result.data_type()) + } else { + Ok(result) + } } fn propagate_constraints( @@ -712,6 +734,37 @@ impl PhysicalExpr for BinaryExpr { let left_interval = children[0]; let right_interval = children[1]; + if left_interval.data_type().is_integer() + && right_interval.data_type().is_integer() + && matches!( + self.op, + Operator::Plus | Operator::Minus | Operator::Multiply | Operator::Divide + ) + { + // Integer division truncates: a / 2 = 1 permits both 2 and 3. + // Wrapping arithmetic is likewise not invertible over mathematical + // intervals. Keep the input domains rather than exclude valid rows. + let contains_zero = |range: &Interval| -> Result { + Ok(range.contains(&Interval::make_zero(&range.data_type())?)? + == Interval::TRUE) + }; + // If an operand can be zero, a zero product does not constrain + // the other operand. Dividing the parent interval loses that case. + let zero_product = self.op == Operator::Multiply + && contains_zero(interval)? + && (contains_zero(left_interval)? || contains_zero(right_interval)?); + if self.op == Operator::Divide + || zero_product + || self.integer_arithmetic_may_wrap( + left_interval, + right_interval, + &apply_operator(&self.op, left_interval, right_interval)?, + ) + { + return Ok(Some(vec![])); + } + } + if self.op.eq(&Operator::And) { if interval.eq(&Interval::TRUE) { // A certainly true logical conjunction can only derive from possibly @@ -6353,6 +6406,119 @@ mod tests { } } + #[test] + fn test_integer_interval_propagation_covers_runtime_values() -> Result<()> { + // Enumerate small domains and both ends of Int8, including zero divisors, + // truncation, signed overflow and checked arithmetic. Every successful + // runtime evaluation must remain possible after interval propagation. + let domains = [ + (-3i8, 3i8), + (0, 1), + (1, 3), + (-3, -1), + (-128, -127), + (126, 127), + ]; + for checked in [false, true] { + for op in [ + Operator::Plus, + Operator::Minus, + Operator::Multiply, + Operator::Divide, + ] { + let expr = BinaryExpr::new(lit(0i8), op, lit(0i8)) + .with_fail_on_overflow(checked); + for (lo, hi) in domains { + for (rlo, rhi) in + domains.into_iter().chain([(-1, -1), (0, 0), (2, 2)]) + { + let left = Interval::make(Some(lo), Some(hi))?; + let right = Interval::make(Some(rlo), Some(rhi))?; + let bounds = expr.evaluate_bounds(&[&left, &right])?; + for a in lo..=hi { + for b in rlo..=rhi { + let result = match (op, checked) { + (Operator::Plus, false) => Some(a.wrapping_add(b)), + (Operator::Minus, false) => Some(a.wrapping_sub(b)), + (Operator::Multiply, false) => { + Some(a.wrapping_mul(b)) + } + (Operator::Plus, true) => a.checked_add(b), + (Operator::Minus, true) => a.checked_sub(b), + (Operator::Multiply, true) => a.checked_mul(b), + (Operator::Divide, _) => a.checked_div(b), + _ => unreachable!(), + }; + let Some(result) = result else { + continue; + }; + let result = Interval::make(Some(result), Some(result))?; + assert_eq!( + bounds.contains(&result)?, + Interval::TRUE, + "forward {a} {op} {b}, checked={checked}, bounds={bounds:?}" + ); + let propagated = expr + .propagate_constraints(&result, &[&left, &right])?; + let propagated = propagated + .expect("successful runtime result must be feasible"); + if !propagated.is_empty() { + assert_eq!( + propagated[0].contains(&Interval::make( + Some(a), + Some(a) + )?)?, + Interval::TRUE, + "left input excluded for {a} {op} {b}, checked={checked}: {propagated:?}" + ); + assert_eq!( + propagated[1].contains(&Interval::make( + Some(b), + Some(b) + )?)?, + Interval::TRUE, + "right input excluded for {a} {op} {b}, checked={checked}: {propagated:?}" + ); + } + } + } + } + } + } + } + Ok(()) + } + + #[test] + fn test_unsigned_subtraction_interval_underflow() -> Result<()> { + let expr = BinaryExpr::new(lit(0u8), Operator::Minus, lit(1u8)); + let left = Interval::make(Some(0u8), Some(2u8))?; + let right = Interval::make(Some(1u8), Some(1u8))?; + let wrapped = Interval::make(Some(255u8), Some(255u8))?; + assert_eq!( + expr.evaluate_bounds(&[&left, &right])?.contains(&wrapped)?, + Interval::TRUE + ); + assert_eq!( + expr.propagate_constraints(&wrapped, &[&left, &right])?, + Some(vec![]) + ); + Ok(()) + } + + #[test] + fn test_safe_integer_multiplication_still_propagates() -> Result<()> { + let expr = BinaryExpr::new(lit(0i32), Operator::Multiply, lit(2i32)); + let left = Interval::make(Some(0i32), Some(10i32))?; + let right = Interval::make(Some(2i32), Some(2i32))?; + let parent = Interval::make(Some(4i32), Some(4i32))?; + assert_eq!( + expr.propagate_constraints(&parent, &[&left, &right])?, + Some(vec![Interval::make(Some(2i32), Some(2i32))?, right]) + ); + Ok(()) + } + #[test] fn test_evaluate_bounds_int32() { let schema = Schema::new(vec![ diff --git a/datafusion/physical-plan/src/filter.rs b/datafusion/physical-plan/src/filter.rs index 12771eec78470..28c43c8d58df8 100644 --- a/datafusion/physical-plan/src/filter.rs +++ b/datafusion/physical-plan/src/filter.rs @@ -2310,6 +2310,17 @@ mod tests { )), )), )); + // i32::MIN also satisfies this predicate because subtracting 5 wraps. + // A mathematical lower bound of 5 would exclude a valid input value. + let batch = RecordBatch::try_new( + input.schema(), + vec![Arc::new(arrow::array::Int32Array::from(vec![i32::MIN]))], + )?; + let result = predicate.evaluate(&batch)?.into_array(1)?; + assert_eq!( + result.as_ref(), + &arrow::array::BooleanArray::from(vec![true]) + ); let filter: Arc = Arc::new(FilterExec::try_new(predicate, input)?); let filter_statistics = @@ -2322,7 +2333,7 @@ mod tests { // `a <= 10` rejects nulls, so `a` has no surviving nulls even // though the input statistics are entirely unknown. null_count: Precision::Exact(0), - min_value: Precision::Inexact(ScalarValue::Int32(Some(5))), + min_value: Precision::Absent, max_value: Precision::Inexact(ScalarValue::Int32(Some(10))), sum_value: Precision::Absent, distinct_count: Precision::Absent, diff --git a/datafusion/sqllogictest/test_files/integer_interval_propagation.slt b/datafusion/sqllogictest/test_files/integer_interval_propagation.slt new file mode 100644 index 0000000000000..6b7705321e271 --- /dev/null +++ b/datafusion/sqllogictest/test_files/integer_interval_propagation.slt @@ -0,0 +1,66 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +statement ok +SET datafusion.execution.target_partitions = 1; + +# Wrapping multiplication has more than one preimage of 2. The filter +# must not mark a as constant and remove either requested sort. +statement ok +CREATE TABLE wrap_test (a INT) AS VALUES (-2147483647), (1); + +query II +SELECT a, a * 2::INT FROM wrap_test ORDER BY a; +---- +-2147483647 2 +1 2 + +query I +SELECT a FROM wrap_test WHERE a * 2::INT = 2::INT ORDER BY a DESC; +---- +1 +-2147483647 + +query I +SELECT a - 2::INT AS x FROM wrap_test WHERE a * 2::INT = 2::INT ORDER BY x; +---- +-1 +2147483647 + +query II +SELECT MIN(a), MAX(a) FROM wrap_test WHERE a * 2::INT = 2::INT; +---- +-2147483647 1 + +# Integer division truncates: both 2 and 3 satisfy a / 2 = 1. +statement ok +CREATE TABLE division_test (a INT) AS VALUES (2), (3); + +query II +SELECT a, a / 2::INT FROM division_test ORDER BY a; +---- +2 1 +3 1 + +query I +SELECT a FROM division_test WHERE a / 2::INT = 1::INT ORDER BY a DESC; +---- +3 +2 + +statement ok +SET datafusion.execution.target_partitions = 4; From f043836d5f6429f9ec6a20d32f490b0b8c0ba8cf Mon Sep 17 00:00:00 2001 From: Huaijin Date: Sat, 12 Sep 2026 21:39:49 +0800 Subject: [PATCH 02/10] test: move integer interval regressions into existing filter SLT --- .../test_files/filter_without_sort_exec.slt | 50 ++++++++++++++ .../integer_interval_propagation.slt | 66 ------------------- 2 files changed, 50 insertions(+), 66 deletions(-) delete mode 100644 datafusion/sqllogictest/test_files/integer_interval_propagation.slt diff --git a/datafusion/sqllogictest/test_files/filter_without_sort_exec.slt b/datafusion/sqllogictest/test_files/filter_without_sort_exec.slt index 87e994f243183..7d18047af711f 100644 --- a/datafusion/sqllogictest/test_files/filter_without_sort_exec.slt +++ b/datafusion/sqllogictest/test_files/filter_without_sort_exec.slt @@ -189,3 +189,53 @@ physical_plan 02)--FilterExec: b@0 > 1 03)----RepartitionExec: partitioning=RoundRobinBatch(4), input_partitions=1, maintains_sort_order=true 04)------DataSourceExec: file_groups={1 group: [[WORKSPACE_ROOT/datafusion/sqllogictest/test_files/scratch/filter_without_sort_exec/cast_ordering.parquet]]}, projection=[b], output_ordering=[b@0 ASC NULLS LAST], file_type=parquet, predicate=b@0 > 1, pruning_predicate=b_null_count@1 != row_count@2 AND b_max@0 > 1, required_guarantees=[] + +statement ok +SET datafusion.execution.target_partitions = 1; + +# Wrapping multiplication has more than one preimage of 2. The filter +# must not mark a as constant and remove either requested sort. +statement ok +CREATE TABLE wrap_test (a INT) AS VALUES (-2147483647), (1); + +query II +SELECT a, a * 2::INT FROM wrap_test ORDER BY a; +---- +-2147483647 2 +1 2 + +query I +SELECT a FROM wrap_test WHERE a * 2::INT = 2::INT ORDER BY a DESC; +---- +1 +-2147483647 + +query I +SELECT a - 2::INT AS x FROM wrap_test WHERE a * 2::INT = 2::INT ORDER BY x; +---- +-1 +2147483647 + +query II +SELECT MIN(a), MAX(a) FROM wrap_test WHERE a * 2::INT = 2::INT; +---- +-2147483647 1 + +# Integer division truncates: both 2 and 3 satisfy a / 2 = 1. +statement ok +CREATE TABLE division_test (a INT) AS VALUES (2), (3); + +query II +SELECT a, a / 2::INT FROM division_test ORDER BY a; +---- +2 1 +3 1 + +query I +SELECT a FROM division_test WHERE a / 2::INT = 1::INT ORDER BY a DESC; +---- +3 +2 + +statement ok +SET datafusion.execution.target_partitions = 4; diff --git a/datafusion/sqllogictest/test_files/integer_interval_propagation.slt b/datafusion/sqllogictest/test_files/integer_interval_propagation.slt deleted file mode 100644 index 6b7705321e271..0000000000000 --- a/datafusion/sqllogictest/test_files/integer_interval_propagation.slt +++ /dev/null @@ -1,66 +0,0 @@ -# Licensed to the Apache Software Foundation (ASF) under one -# or more contributor license agreements. See the NOTICE file -# distributed with this work for additional information -# regarding copyright ownership. The ASF licenses this file -# to you under the Apache License, Version 2.0 (the -# "License"); you may not use this file except in compliance -# with the License. You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, -# software distributed under the License is distributed on an -# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY -# KIND, either express or implied. See the License for the -# specific language governing permissions and limitations -# under the License. - -statement ok -SET datafusion.execution.target_partitions = 1; - -# Wrapping multiplication has more than one preimage of 2. The filter -# must not mark a as constant and remove either requested sort. -statement ok -CREATE TABLE wrap_test (a INT) AS VALUES (-2147483647), (1); - -query II -SELECT a, a * 2::INT FROM wrap_test ORDER BY a; ----- --2147483647 2 -1 2 - -query I -SELECT a FROM wrap_test WHERE a * 2::INT = 2::INT ORDER BY a DESC; ----- -1 --2147483647 - -query I -SELECT a - 2::INT AS x FROM wrap_test WHERE a * 2::INT = 2::INT ORDER BY x; ----- --1 -2147483647 - -query II -SELECT MIN(a), MAX(a) FROM wrap_test WHERE a * 2::INT = 2::INT; ----- --2147483647 1 - -# Integer division truncates: both 2 and 3 satisfy a / 2 = 1. -statement ok -CREATE TABLE division_test (a INT) AS VALUES (2), (3); - -query II -SELECT a, a / 2::INT FROM division_test ORDER BY a; ----- -2 1 -3 1 - -query I -SELECT a FROM division_test WHERE a / 2::INT = 1::INT ORDER BY a DESC; ----- -3 -2 - -statement ok -SET datafusion.execution.target_partitions = 4; From 73297f31477e0b3d1dd31023fffa44d3b25b3fde Mon Sep 17 00:00:00 2001 From: Huaijin Date: Sat, 12 Sep 2026 23:36:40 +0800 Subject: [PATCH 03/10] update --- .../physical-expr/src/expressions/binary.rs | 62 +++++++++++++++++-- 1 file changed, 58 insertions(+), 4 deletions(-) diff --git a/datafusion/physical-expr/src/expressions/binary.rs b/datafusion/physical-expr/src/expressions/binary.rs index a203680bb5bc3..b672643572a8b 100644 --- a/datafusion/physical-expr/src/expressions/binary.rs +++ b/datafusion/physical-expr/src/expressions/binary.rs @@ -745,8 +745,7 @@ impl PhysicalExpr for BinaryExpr { // Wrapping arithmetic is likewise not invertible over mathematical // intervals. Keep the input domains rather than exclude valid rows. let contains_zero = |range: &Interval| -> Result { - Ok(range.contains(&Interval::make_zero(&range.data_type())?)? - == Interval::TRUE) + range.contains_value(ScalarValue::new_zero(&range.data_type())?) }; // If an operand can be zero, a zero product does not constrain // the other operand. Dividing the parent interval loses that case. @@ -885,7 +884,7 @@ impl PhysicalExpr for BinaryExpr { let (r_order, r_range) = (children[1].sort_properties, &children[1].range); match self.op() { Operator::Plus => { - let range = l_range.add(r_range)?; + let range = self.evaluate_bounds(&[l_range, r_range])?; Ok(ExprProperties { sort_properties: self.arithmetic_sort_properties( l_order.add(&r_order), @@ -899,7 +898,7 @@ impl PhysicalExpr for BinaryExpr { }) } Operator::Minus => { - let range = l_range.sub(r_range)?; + let range = self.evaluate_bounds(&[l_range, r_range])?; Ok(ExprProperties { sort_properties: self.arithmetic_sort_properties( l_order.sub(&r_order), @@ -6435,6 +6434,15 @@ mod tests { let left = Interval::make(Some(lo), Some(hi))?; let right = Interval::make(Some(rlo), Some(rhi))?; let bounds = expr.evaluate_bounds(&[&left, &right])?; + if matches!(op, Operator::Plus | Operator::Minus) { + let children = [left.clone(), right.clone()].map(|range| { + ExprProperties { + range, + ..ExprProperties::new_unknown() + } + }); + assert_eq!(expr.get_properties(&children)?.range, bounds); + } for a in lo..=hi { for b in rlo..=rhi { let result = match (op, checked) { @@ -6506,6 +6514,52 @@ mod tests { Ok(()) } + #[test] + fn test_nested_wrapping_arithmetic_properties() -> Result<()> { + let difference = Arc::new(BinaryExpr::new(lit(0u8), Operator::Minus, lit(1u8))); + let singleton = |value: u8| ExprProperties { + sort_properties: SortProperties::Singleton, + range: Interval::make(Some(value), Some(value)).unwrap(), + ..ExprProperties::new_unknown() + }; + let difference_props = + difference.get_properties(&[singleton(0), singleton(1)])?; + assert_eq!(difference_props.sort_properties, SortProperties::Singleton); + assert!( + difference_props + .range + .contains_value(ScalarValue::UInt8(Some(255)))? + ); + + // The inner constant wraps to 255. An incorrect range of [0, 0] + // would let the outer addition claim to preserve ascending order. + let expr = + BinaryExpr::new(Arc::new(Column::new("a", 0)), Operator::Plus, difference); + let properties = expr.get_properties(&[ + ExprProperties { + sort_properties: SortProperties::Ordered(SortOptions::default()), + range: Interval::make(Some(0u8), Some(1u8))?, + ..ExprProperties::new_unknown() + }, + difference_props, + ])?; + let batch = RecordBatch::try_new( + Arc::new(Schema::new(vec![Field::new("a", DataType::UInt8, false)])), + vec![Arc::new(UInt8Array::from(vec![0, 1]))], + )?; + let actual = expr.evaluate(&batch)?.into_array(2)?; + assert_eq!(actual.as_ref(), &UInt8Array::from(vec![255, 0])); + assert_eq!(properties.sort_properties, SortProperties::Unordered); + for value in [0u8, 255] { + assert!( + properties + .range + .contains_value(ScalarValue::UInt8(Some(value)))? + ); + } + Ok(()) + } + #[test] fn test_safe_integer_multiplication_still_propagates() -> Result<()> { let expr = BinaryExpr::new(lit(0i32), Operator::Multiply, lit(2i32)); From b6a4f3f3a89ddeafb080b521a876b472efe27fda Mon Sep 17 00:00:00 2001 From: Huaijin Date: Sun, 13 Sep 2026 17:30:15 +0800 Subject: [PATCH 04/10] fix: preserve source integer bounds in widening interval casts --- .../expr-common/src/interval_arithmetic.rs | 95 ++++++++++++++++++- .../test_files/interval_widening.slt | 67 +++++++++++++ 2 files changed, 160 insertions(+), 2 deletions(-) create mode 100644 datafusion/sqllogictest/test_files/interval_widening.slt diff --git a/datafusion/expr-common/src/interval_arithmetic.rs b/datafusion/expr-common/src/interval_arithmetic.rs index 9f1291353dc29..d0a6c9d0a72fc 100644 --- a/datafusion/expr-common/src/interval_arithmetic.rs +++ b/datafusion/expr-common/src/interval_arithmetic.rs @@ -422,9 +422,43 @@ impl Interval { data_type: &DataType, cast_options: &CastOptions, ) -> Result { + let source_type = self.data_type(); + // An unbounded integer endpoint still has a finite limit imposed by its + // type. Preserve that limit when widening so subsequent arithmetic can + // prove that it does not overflow the destination type. + use DataType::{Int8, Int16, Int32, Int64, UInt8, UInt16, UInt32, UInt64}; + let widening_integer_cast = matches!( + (&source_type, data_type), + (Int8, Int16 | Int32 | Int64) + | (Int16, Int32 | Int64) + | (Int32, Int64) + | (UInt8, UInt16 | UInt32 | UInt64 | Int16 | Int32 | Int64) + | (UInt16, UInt32 | UInt64 | Int32 | Int64) + | (UInt32, UInt64 | Int64) + ); + let lower = if widening_integer_cast && self.lower.is_null() { + get_extreme_value!( + MIN, + MIN_DECIMAL128_FOR_EACH_PRECISION, + MIN_DECIMAL256_FOR_EACH_PRECISION, + &source_type + ) + } else { + self.lower.clone() + }; + let upper = if widening_integer_cast && self.upper.is_null() { + get_extreme_value!( + MAX, + MAX_DECIMAL128_FOR_EACH_PRECISION, + MAX_DECIMAL256_FOR_EACH_PRECISION, + &source_type + ) + } else { + self.upper.clone() + }; Self::try_new( - cast_scalar_value(&self.lower, data_type, cast_options)?, - cast_scalar_value(&self.upper, data_type, cast_options)?, + cast_scalar_value(&lower, data_type, cast_options)?, + cast_scalar_value(&upper, data_type, cast_options)?, ) } @@ -2358,6 +2392,63 @@ mod tests { Ok(()) } + #[test] + fn test_widening_integer_cast_bounds() -> Result<()> { + use DataType::{Int8, Int16, Int32, Int64, UInt8, UInt16, UInt32, UInt64}; + use arrow::compute::CastOptions; + + let types = [ + (Int8, i8::MIN as i128, i8::MAX as i128), + (Int16, i16::MIN as i128, i16::MAX as i128), + (Int32, i32::MIN as i128, i32::MAX as i128), + (Int64, i64::MIN as i128, i64::MAX as i128), + (UInt8, 0, u8::MAX as i128), + (UInt16, 0, u16::MAX as i128), + (UInt32, 0, u32::MAX as i128), + (UInt64, 0, u64::MAX as i128), + ]; + for (source, min, max) in &types { + for (target, target_min, target_max) in &types { + if source == target || min < target_min || max > target_max { + continue; + } + // Every source that can widen fits in Int64. + let lower = ScalarValue::Int64(Some(*min as i64)).cast_to(source)?; + let upper = ScalarValue::Int64(Some(*max as i64)).cast_to(source)?; + let zero = ScalarValue::new_zero(source)?; + let unbounded = ScalarValue::try_from(source)?; + for (lo, hi, expected_lo, expected_hi) in [ + ( + unbounded.clone(), + unbounded.clone(), + lower.clone(), + upper.clone(), + ), + (zero.clone(), unbounded.clone(), zero.clone(), upper.clone()), + (unbounded.clone(), zero.clone(), lower.clone(), zero.clone()), + (zero.clone(), zero.clone(), zero.clone(), zero.clone()), + ] { + let actual = Interval::try_new(lo, hi)? + .cast_to(target, &CastOptions::default())?; + let expected = Interval::try_new( + expected_lo.cast_to(target)?, + expected_hi.cast_to(target)?, + )?; + assert_eq!(actual, expected, "{source:?} -> {target:?}"); + } + } + } + // Same-type and narrowing casts retain their existing unbounded behavior. + let unbounded = Interval::make_unbounded(&Int64)?; + for target in [Int64, Int32] { + assert_eq!( + unbounded.cast_to(&target, &CastOptions::default())?, + Interval::make_unbounded(&target)? + ); + } + Ok(()) + } + #[test] fn test_new_interval() -> Result<()> { use ScalarValue::*; diff --git a/datafusion/sqllogictest/test_files/interval_widening.slt b/datafusion/sqllogictest/test_files/interval_widening.slt new file mode 100644 index 0000000000000..5e6b8f2ea7203 --- /dev/null +++ b/datafusion/sqllogictest/test_files/interval_widening.slt @@ -0,0 +1,67 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +# Widening Int32 to Int64 preserves the source type's upper bound. +statement ok +SET datafusion.execution.target_partitions = 1; + +statement ok +SET datafusion.explain.physical_plan_only = true; + +statement ok +CREATE TABLE widening_input (a INT) AS +VALUES (2147483647), (1), (2147483646), (2147483647); + +# The predicate implies a = Int32::MAX, so ORDER BY a needs no sort. +query TT +EXPLAIN SELECT a FROM widening_input +WHERE CAST(a AS BIGINT) + 1 > 2147483647 ORDER BY a; +---- +physical_plan +01)FilterExec: CAST(a@0 AS Int64) + 1 > 2147483647 +02)--DataSourceExec: partitions=1, partition_sizes=[1] + +query I +SELECT a FROM widening_input +WHERE CAST(a AS BIGINT) + 1 > 2147483647 ORDER BY a; +---- +2147483647 +2147483647 + +# Two distinct values can pass this predicate; the sort must remain. +query TT +EXPLAIN SELECT a FROM widening_input +WHERE CAST(a AS BIGINT) + 1 > 2147483646 ORDER BY a; +---- +physical_plan +01)SortExec: expr=[a@0 ASC NULLS LAST], preserve_partitioning=[false] +02)--FilterExec: CAST(a@0 AS Int64) + 1 > 2147483646 +03)----DataSourceExec: partitions=1, partition_sizes=[1] + +query I +SELECT a FROM widening_input +WHERE CAST(a AS BIGINT) + 1 > 2147483646 ORDER BY a; +---- +2147483646 +2147483647 +2147483647 + +statement ok +SET datafusion.execution.target_partitions = 4; + +statement ok +RESET datafusion.explain.physical_plan_only; From 3ccde0598f6f6ef565cbde4ffd403ede78606430 Mon Sep 17 00:00:00 2001 From: Huaijin Date: Sun, 13 Sep 2026 17:32:54 +0800 Subject: [PATCH 05/10] test: move interval widening cases into existing filter SLT --- .../test_files/filter_without_sort_exec.slt | 45 +++++++++++++ .../test_files/interval_widening.slt | 67 ------------------- 2 files changed, 45 insertions(+), 67 deletions(-) delete mode 100644 datafusion/sqllogictest/test_files/interval_widening.slt diff --git a/datafusion/sqllogictest/test_files/filter_without_sort_exec.slt b/datafusion/sqllogictest/test_files/filter_without_sort_exec.slt index 7d18047af711f..aa17b82c67b9c 100644 --- a/datafusion/sqllogictest/test_files/filter_without_sort_exec.slt +++ b/datafusion/sqllogictest/test_files/filter_without_sort_exec.slt @@ -237,5 +237,50 @@ SELECT a FROM division_test WHERE a / 2::INT = 1::INT ORDER BY a DESC; 3 2 +# Widening Int32 to Int64 preserves the source type's upper bound. +statement ok +SET datafusion.explain.physical_plan_only = true; + +statement ok +CREATE TABLE widening_input (a INT) AS +VALUES (2147483647), (1), (2147483646), (2147483647); + +# The predicate implies a = Int32::MAX, so ORDER BY a needs no sort. +query TT +EXPLAIN SELECT a FROM widening_input +WHERE CAST(a AS BIGINT) + 1 > 2147483647 ORDER BY a; +---- +physical_plan +01)FilterExec: CAST(a@0 AS Int64) + 1 > 2147483647 +02)--DataSourceExec: partitions=1, partition_sizes=[1] + +query I +SELECT a FROM widening_input +WHERE CAST(a AS BIGINT) + 1 > 2147483647 ORDER BY a; +---- +2147483647 +2147483647 + +# Two distinct values can pass this predicate; the sort must remain. +query TT +EXPLAIN SELECT a FROM widening_input +WHERE CAST(a AS BIGINT) + 1 > 2147483646 ORDER BY a; +---- +physical_plan +01)SortExec: expr=[a@0 ASC NULLS LAST], preserve_partitioning=[false] +02)--FilterExec: CAST(a@0 AS Int64) + 1 > 2147483646 +03)----DataSourceExec: partitions=1, partition_sizes=[1] + +query I +SELECT a FROM widening_input +WHERE CAST(a AS BIGINT) + 1 > 2147483646 ORDER BY a; +---- +2147483646 +2147483647 +2147483647 + statement ok SET datafusion.execution.target_partitions = 4; + +statement ok +RESET datafusion.explain.physical_plan_only; diff --git a/datafusion/sqllogictest/test_files/interval_widening.slt b/datafusion/sqllogictest/test_files/interval_widening.slt deleted file mode 100644 index 5e6b8f2ea7203..0000000000000 --- a/datafusion/sqllogictest/test_files/interval_widening.slt +++ /dev/null @@ -1,67 +0,0 @@ -# Licensed to the Apache Software Foundation (ASF) under one -# or more contributor license agreements. See the NOTICE file -# distributed with this work for additional information -# regarding copyright ownership. The ASF licenses this file -# to you under the Apache License, Version 2.0 (the -# "License"); you may not use this file except in compliance -# with the License. You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, -# software distributed under the License is distributed on an -# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY -# KIND, either express or implied. See the License for the -# specific language governing permissions and limitations -# under the License. - -# Widening Int32 to Int64 preserves the source type's upper bound. -statement ok -SET datafusion.execution.target_partitions = 1; - -statement ok -SET datafusion.explain.physical_plan_only = true; - -statement ok -CREATE TABLE widening_input (a INT) AS -VALUES (2147483647), (1), (2147483646), (2147483647); - -# The predicate implies a = Int32::MAX, so ORDER BY a needs no sort. -query TT -EXPLAIN SELECT a FROM widening_input -WHERE CAST(a AS BIGINT) + 1 > 2147483647 ORDER BY a; ----- -physical_plan -01)FilterExec: CAST(a@0 AS Int64) + 1 > 2147483647 -02)--DataSourceExec: partitions=1, partition_sizes=[1] - -query I -SELECT a FROM widening_input -WHERE CAST(a AS BIGINT) + 1 > 2147483647 ORDER BY a; ----- -2147483647 -2147483647 - -# Two distinct values can pass this predicate; the sort must remain. -query TT -EXPLAIN SELECT a FROM widening_input -WHERE CAST(a AS BIGINT) + 1 > 2147483646 ORDER BY a; ----- -physical_plan -01)SortExec: expr=[a@0 ASC NULLS LAST], preserve_partitioning=[false] -02)--FilterExec: CAST(a@0 AS Int64) + 1 > 2147483646 -03)----DataSourceExec: partitions=1, partition_sizes=[1] - -query I -SELECT a FROM widening_input -WHERE CAST(a AS BIGINT) + 1 > 2147483646 ORDER BY a; ----- -2147483646 -2147483647 -2147483647 - -statement ok -SET datafusion.execution.target_partitions = 4; - -statement ok -RESET datafusion.explain.physical_plan_only; From 112e1024cbc2d3a6f780c7f7a31801fde33d3a23 Mon Sep 17 00:00:00 2001 From: Huaijin Date: Sun, 13 Sep 2026 18:22:05 +0800 Subject: [PATCH 06/10] test: improve integer interval coverage --- .../expr-common/src/interval_arithmetic.rs | 64 ++++++----- .../physical-expr/src/expressions/binary.rs | 108 ++++++++++-------- datafusion/physical-plan/src/filter.rs | 5 +- 3 files changed, 102 insertions(+), 75 deletions(-) diff --git a/datafusion/expr-common/src/interval_arithmetic.rs b/datafusion/expr-common/src/interval_arithmetic.rs index d0a6c9d0a72fc..ba492806386ff 100644 --- a/datafusion/expr-common/src/interval_arithmetic.rs +++ b/datafusion/expr-common/src/interval_arithmetic.rs @@ -437,22 +437,12 @@ impl Interval { | (UInt32, UInt64 | Int64) ); let lower = if widening_integer_cast && self.lower.is_null() { - get_extreme_value!( - MIN, - MIN_DECIMAL128_FOR_EACH_PRECISION, - MIN_DECIMAL256_FOR_EACH_PRECISION, - &source_type - ) + ScalarValue::min(&source_type).expect("integer types have a minimum") } else { self.lower.clone() }; let upper = if widening_integer_cast && self.upper.is_null() { - get_extreme_value!( - MAX, - MAX_DECIMAL128_FOR_EACH_PRECISION, - MAX_DECIMAL256_FOR_EACH_PRECISION, - &source_type - ) + ScalarValue::max(&source_type).expect("integer types have a maximum") } else { self.upper.clone() }; @@ -2393,7 +2383,7 @@ mod tests { } #[test] - fn test_widening_integer_cast_bounds() -> Result<()> { + fn test_widening_integer_cast_bounds() { use DataType::{Int8, Int16, Int32, Int64, UInt8, UInt16, UInt32, UInt64}; use arrow::compute::CastOptions; @@ -2413,10 +2403,14 @@ mod tests { continue; } // Every source that can widen fits in Int64. - let lower = ScalarValue::Int64(Some(*min as i64)).cast_to(source)?; - let upper = ScalarValue::Int64(Some(*max as i64)).cast_to(source)?; - let zero = ScalarValue::new_zero(source)?; - let unbounded = ScalarValue::try_from(source)?; + let lower = ScalarValue::Int64(Some(*min as i64)) + .cast_to(source) + .unwrap(); + let upper = ScalarValue::Int64(Some(*max as i64)) + .cast_to(source) + .unwrap(); + let zero = ScalarValue::new_zero(source).unwrap(); + let unbounded = ScalarValue::try_from(source).unwrap(); for (lo, hi, expected_lo, expected_hi) in [ ( unbounded.clone(), @@ -2428,25 +2422,43 @@ mod tests { (unbounded.clone(), zero.clone(), lower.clone(), zero.clone()), (zero.clone(), zero.clone(), zero.clone(), zero.clone()), ] { - let actual = Interval::try_new(lo, hi)? - .cast_to(target, &CastOptions::default())?; + let actual = Interval::try_new(lo, hi) + .unwrap() + .cast_to(target, &CastOptions::default()) + .unwrap(); let expected = Interval::try_new( - expected_lo.cast_to(target)?, - expected_hi.cast_to(target)?, - )?; + expected_lo.cast_to(target).unwrap(), + expected_hi.cast_to(target).unwrap(), + ) + .unwrap(); assert_eq!(actual, expected, "{source:?} -> {target:?}"); } } } // Same-type and narrowing casts retain their existing unbounded behavior. - let unbounded = Interval::make_unbounded(&Int64)?; + let unbounded = Interval::make_unbounded(&Int64).unwrap(); for target in [Int64, Int32] { assert_eq!( - unbounded.cast_to(&target, &CastOptions::default())?, - Interval::make_unbounded(&target)? + unbounded.cast_to(&target, &CastOptions::default()).unwrap(), + Interval::make_unbounded(&target).unwrap() ); } - Ok(()) + } + + #[test] + fn test_integer_interval_cast_overflow() { + use arrow::compute::CastOptions; + + let options = CastOptions { + safe: false, + ..Default::default() + }; + // Check failures at either endpoint, including an upper endpoint that + // overflows after the lower endpoint has been successfully cast. + for (lower, upper) in [(-129i16, 0i16), (0, 128)] { + let interval = Interval::make(Some(lower), Some(upper)).unwrap(); + assert!(interval.cast_to(&DataType::Int8, &options).is_err()); + } } #[test] diff --git a/datafusion/physical-expr/src/expressions/binary.rs b/datafusion/physical-expr/src/expressions/binary.rs index b672643572a8b..21098df377681 100644 --- a/datafusion/physical-expr/src/expressions/binary.rs +++ b/datafusion/physical-expr/src/expressions/binary.rs @@ -6406,7 +6406,7 @@ mod tests { } #[test] - fn test_integer_interval_propagation_covers_runtime_values() -> Result<()> { + fn test_integer_interval_propagation_covers_runtime_values() { // Enumerate small domains and both ends of Int8, including zero divisors, // truncation, signed overflow and checked arithmetic. Every successful // runtime evaluation must remain possible after interval propagation. @@ -6431,9 +6431,9 @@ mod tests { for (rlo, rhi) in domains.into_iter().chain([(-1, -1), (0, 0), (2, 2)]) { - let left = Interval::make(Some(lo), Some(hi))?; - let right = Interval::make(Some(rlo), Some(rhi))?; - let bounds = expr.evaluate_bounds(&[&left, &right])?; + let left = Interval::make(Some(lo), Some(hi)).unwrap(); + let right = Interval::make(Some(rlo), Some(rhi)).unwrap(); + let bounds = expr.evaluate_bounds(&[&left, &right]).unwrap(); if matches!(op, Operator::Plus | Operator::Minus) { let children = [left.clone(), right.clone()].map(|range| { ExprProperties { @@ -6441,7 +6441,10 @@ mod tests { ..ExprProperties::new_unknown() } }); - assert_eq!(expr.get_properties(&children)?.range, bounds); + assert_eq!( + expr.get_properties(&children).unwrap().range, + bounds + ); } for a in lo..=hi { for b in rlo..=rhi { @@ -6460,30 +6463,34 @@ mod tests { let Some(result) = result else { continue; }; - let result = Interval::make(Some(result), Some(result))?; + let result = + Interval::make(Some(result), Some(result)).unwrap(); assert_eq!( - bounds.contains(&result)?, + bounds.contains(&result).unwrap(), Interval::TRUE, "forward {a} {op} {b}, checked={checked}, bounds={bounds:?}" ); let propagated = expr - .propagate_constraints(&result, &[&left, &right])?; + .propagate_constraints(&result, &[&left, &right]) + .unwrap(); let propagated = propagated .expect("successful runtime result must be feasible"); if !propagated.is_empty() { assert_eq!( - propagated[0].contains(&Interval::make( - Some(a), - Some(a) - )?)?, + propagated[0] + .contains( + Interval::make(Some(a), Some(a)).unwrap() + ) + .unwrap(), Interval::TRUE, "left input excluded for {a} {op} {b}, checked={checked}: {propagated:?}" ); assert_eq!( - propagated[1].contains(&Interval::make( - Some(b), - Some(b) - )?)?, + propagated[1] + .contains( + Interval::make(Some(b), Some(b)).unwrap() + ) + .unwrap(), Interval::TRUE, "right input excluded for {a} {op} {b}, checked={checked}: {propagated:?}" ); @@ -6494,83 +6501,90 @@ mod tests { } } } - Ok(()) } #[test] - fn test_unsigned_subtraction_interval_underflow() -> Result<()> { + fn test_unsigned_subtraction_interval_underflow() { let expr = BinaryExpr::new(lit(0u8), Operator::Minus, lit(1u8)); - let left = Interval::make(Some(0u8), Some(2u8))?; - let right = Interval::make(Some(1u8), Some(1u8))?; - let wrapped = Interval::make(Some(255u8), Some(255u8))?; + let left = Interval::make(Some(0u8), Some(2u8)).unwrap(); + let right = Interval::make(Some(1u8), Some(1u8)).unwrap(); + let wrapped = Interval::make(Some(255u8), Some(255u8)).unwrap(); assert_eq!( - expr.evaluate_bounds(&[&left, &right])?.contains(&wrapped)?, + expr.evaluate_bounds(&[&left, &right]) + .unwrap() + .contains(&wrapped) + .unwrap(), Interval::TRUE ); assert_eq!( - expr.propagate_constraints(&wrapped, &[&left, &right])?, + expr.propagate_constraints(&wrapped, &[&left, &right]) + .unwrap(), Some(vec![]) ); - Ok(()) } #[test] - fn test_nested_wrapping_arithmetic_properties() -> Result<()> { + fn test_nested_wrapping_arithmetic_properties() { let difference = Arc::new(BinaryExpr::new(lit(0u8), Operator::Minus, lit(1u8))); let singleton = |value: u8| ExprProperties { sort_properties: SortProperties::Singleton, range: Interval::make(Some(value), Some(value)).unwrap(), ..ExprProperties::new_unknown() }; - let difference_props = - difference.get_properties(&[singleton(0), singleton(1)])?; + let difference_props = difference + .get_properties(&[singleton(0), singleton(1)]) + .unwrap(); assert_eq!(difference_props.sort_properties, SortProperties::Singleton); assert!( difference_props .range - .contains_value(ScalarValue::UInt8(Some(255)))? + .contains_value(ScalarValue::UInt8(Some(255))) + .unwrap() ); // The inner constant wraps to 255. An incorrect range of [0, 0] // would let the outer addition claim to preserve ascending order. let expr = BinaryExpr::new(Arc::new(Column::new("a", 0)), Operator::Plus, difference); - let properties = expr.get_properties(&[ - ExprProperties { - sort_properties: SortProperties::Ordered(SortOptions::default()), - range: Interval::make(Some(0u8), Some(1u8))?, - ..ExprProperties::new_unknown() - }, - difference_props, - ])?; + let properties = expr + .get_properties(&[ + ExprProperties { + sort_properties: SortProperties::Ordered(SortOptions::default()), + range: Interval::make(Some(0u8), Some(1u8)).unwrap(), + ..ExprProperties::new_unknown() + }, + difference_props, + ]) + .unwrap(); let batch = RecordBatch::try_new( Arc::new(Schema::new(vec![Field::new("a", DataType::UInt8, false)])), vec![Arc::new(UInt8Array::from(vec![0, 1]))], - )?; - let actual = expr.evaluate(&batch)?.into_array(2)?; + ) + .unwrap(); + let actual = expr.evaluate(&batch).unwrap().into_array(2).unwrap(); assert_eq!(actual.as_ref(), &UInt8Array::from(vec![255, 0])); assert_eq!(properties.sort_properties, SortProperties::Unordered); for value in [0u8, 255] { assert!( properties .range - .contains_value(ScalarValue::UInt8(Some(value)))? + .contains_value(ScalarValue::UInt8(Some(value))) + .unwrap() ); } - Ok(()) } #[test] - fn test_safe_integer_multiplication_still_propagates() -> Result<()> { + fn test_safe_integer_multiplication_still_propagates() { let expr = BinaryExpr::new(lit(0i32), Operator::Multiply, lit(2i32)); - let left = Interval::make(Some(0i32), Some(10i32))?; - let right = Interval::make(Some(2i32), Some(2i32))?; - let parent = Interval::make(Some(4i32), Some(4i32))?; + let left = Interval::make(Some(0i32), Some(10i32)).unwrap(); + let right = Interval::make(Some(2i32), Some(2i32)).unwrap(); + let parent = Interval::make(Some(4i32), Some(4i32)).unwrap(); assert_eq!( - expr.propagate_constraints(&parent, &[&left, &right])?, - Some(vec![Interval::make(Some(2i32), Some(2i32))?, right]) + expr.propagate_constraints(&parent, &[&left, &right]) + .unwrap(), + Some(vec![Interval::make(Some(2i32), Some(2i32)).unwrap(), right]) ); - Ok(()) } #[test] diff --git a/datafusion/physical-plan/src/filter.rs b/datafusion/physical-plan/src/filter.rs index 28c43c8d58df8..4ed11f5ec1a9a 100644 --- a/datafusion/physical-plan/src/filter.rs +++ b/datafusion/physical-plan/src/filter.rs @@ -2315,8 +2315,9 @@ mod tests { let batch = RecordBatch::try_new( input.schema(), vec![Arc::new(arrow::array::Int32Array::from(vec![i32::MIN]))], - )?; - let result = predicate.evaluate(&batch)?.into_array(1)?; + ) + .unwrap(); + let result = predicate.evaluate(&batch).unwrap().into_array(1).unwrap(); assert_eq!( result.as_ref(), &arrow::array::BooleanArray::from(vec![true]) From 8d49098b2a3c43cf792337fc5312869ff48ad420 Mon Sep 17 00:00:00 2001 From: Huaijin Date: Sun, 13 Sep 2026 19:44:02 +0800 Subject: [PATCH 07/10] test: complete integer interval patch coverage --- .../physical-expr/src/expressions/binary.rs | 114 +++++++++++++----- 1 file changed, 86 insertions(+), 28 deletions(-) diff --git a/datafusion/physical-expr/src/expressions/binary.rs b/datafusion/physical-expr/src/expressions/binary.rs index 21098df377681..a34b4b1b75f30 100644 --- a/datafusion/physical-expr/src/expressions/binary.rs +++ b/datafusion/physical-expr/src/expressions/binary.rs @@ -745,23 +745,41 @@ impl PhysicalExpr for BinaryExpr { // Wrapping arithmetic is likewise not invertible over mathematical // intervals. Keep the input domains rather than exclude valid rows. let contains_zero = |range: &Interval| -> Result { - range.contains_value(ScalarValue::new_zero(&range.data_type())?) + let zero = ScalarValue::new_zero(&range.data_type())?; + Ok(range + .contains_value(zero) + .expect("zero has the interval's type")) }; // If an operand can be zero, a zero product does not constrain // the other operand. Dividing the parent interval loses that case. let zero_product = self.op == Operator::Multiply && contains_zero(interval)? - && (contains_zero(left_interval)? || contains_zero(right_interval)?); - if self.op == Operator::Divide - || zero_product - || self.integer_arithmetic_may_wrap( - left_interval, - right_interval, - &apply_operator(&self.op, left_interval, right_interval)?, - ) - { + && (contains_zero(left_interval) + .expect("integer intervals support zero") + || contains_zero(right_interval) + .expect("integer intervals support zero")); + if self.op == Operator::Divide || zero_product { return Ok(Some(vec![])); } + return apply_operator(&self.op, left_interval, right_interval).and_then( + |result| { + if self.integer_arithmetic_may_wrap( + left_interval, + right_interval, + &result, + ) { + Ok(Some(vec![])) + } else { + propagate_arithmetic( + &self.op, + interval, + left_interval, + right_interval, + ) + .map(|bounds| bounds.map(|(left, right)| vec![left, right])) + } + }, + ); } if self.op.eq(&Operator::And) { @@ -6418,13 +6436,24 @@ mod tests { (-128, -127), (126, 127), ]; + type Arithmetic = fn(i8, i8) -> Option; for checked in [false, true] { - for op in [ - Operator::Plus, - Operator::Minus, - Operator::Multiply, - Operator::Divide, - ] { + let operations: [(Operator, Arithmetic); 4] = if checked { + [ + (Operator::Plus, i8::checked_add), + (Operator::Minus, i8::checked_sub), + (Operator::Multiply, i8::checked_mul), + (Operator::Divide, i8::checked_div), + ] + } else { + [ + (Operator::Plus, |a, b| Some(a.wrapping_add(b))), + (Operator::Minus, |a, b| Some(a.wrapping_sub(b))), + (Operator::Multiply, |a, b| Some(a.wrapping_mul(b))), + (Operator::Divide, i8::checked_div), + ] + }; + for (op, evaluate) in operations { let expr = BinaryExpr::new(lit(0i8), op, lit(0i8)) .with_fail_on_overflow(checked); for (lo, hi) in domains { @@ -6448,18 +6477,7 @@ mod tests { } for a in lo..=hi { for b in rlo..=rhi { - let result = match (op, checked) { - (Operator::Plus, false) => Some(a.wrapping_add(b)), - (Operator::Minus, false) => Some(a.wrapping_sub(b)), - (Operator::Multiply, false) => { - Some(a.wrapping_mul(b)) - } - (Operator::Plus, true) => a.checked_add(b), - (Operator::Minus, true) => a.checked_sub(b), - (Operator::Multiply, true) => a.checked_mul(b), - (Operator::Divide, _) => a.checked_div(b), - _ => unreachable!(), - }; + let result = evaluate(a, b); let Some(result) = result else { continue; }; @@ -6503,6 +6521,46 @@ mod tests { } } + #[test] + fn test_integer_interval_error_propagation() { + let integer = Interval::make(Some(1i32), Some(2i32)).unwrap(); + let boolean = Interval::TRUE; + for op in [Operator::Plus, Operator::Minus] { + let expr = BinaryExpr::new(lit(1i32), op, lit(true)); + let expected = apply_operator(&op, &integer, &boolean) + .unwrap_err() + .to_string(); + assert_eq!( + expr.evaluate_bounds(&[&integer, &boolean]) + .unwrap_err() + .to_string(), + expected + ); + let children = + [integer.clone(), boolean.clone()].map(|range| ExprProperties { + range, + ..ExprProperties::new_unknown() + }); + assert_eq!( + expr.get_properties(&children).unwrap_err().to_string(), + expected + ); + } + + // A nonnumeric parent cannot describe an integer product. Preserve + // the error from constructing zero for its unsupported type. + let parent = Interval::make_unbounded(&DataType::Utf8).unwrap(); + let expr = BinaryExpr::new(lit(1i32), Operator::Multiply, lit(2i32)); + assert_eq!( + expr.propagate_constraints(&parent, &[&integer, &integer]) + .unwrap_err() + .to_string(), + ScalarValue::new_zero(&DataType::Utf8) + .unwrap_err() + .to_string() + ); + } + #[test] fn test_unsigned_subtraction_interval_underflow() { let expr = BinaryExpr::new(lit(0u8), Operator::Minus, lit(1u8)); From b4c540c4a3f00bf3d4a297ee798a2fc6f51778b1 Mon Sep 17 00:00:00 2001 From: Huaijin Date: Sun, 13 Sep 2026 21:28:26 +0800 Subject: [PATCH 08/10] update --- .../physical-expr/src/expressions/binary.rs | 101 +++++++++--------- 1 file changed, 50 insertions(+), 51 deletions(-) diff --git a/datafusion/physical-expr/src/expressions/binary.rs b/datafusion/physical-expr/src/expressions/binary.rs index a34b4b1b75f30..fedd9cd438bef 100644 --- a/datafusion/physical-expr/src/expressions/binary.rs +++ b/datafusion/physical-expr/src/expressions/binary.rs @@ -745,41 +745,23 @@ impl PhysicalExpr for BinaryExpr { // Wrapping arithmetic is likewise not invertible over mathematical // intervals. Keep the input domains rather than exclude valid rows. let contains_zero = |range: &Interval| -> Result { - let zero = ScalarValue::new_zero(&range.data_type())?; - Ok(range - .contains_value(zero) - .expect("zero has the interval's type")) + range.contains_value(ScalarValue::new_zero(&range.data_type())?) }; // If an operand can be zero, a zero product does not constrain // the other operand. Dividing the parent interval loses that case. let zero_product = self.op == Operator::Multiply && contains_zero(interval)? - && (contains_zero(left_interval) - .expect("integer intervals support zero") - || contains_zero(right_interval) - .expect("integer intervals support zero")); - if self.op == Operator::Divide || zero_product { + && (contains_zero(left_interval)? || contains_zero(right_interval)?); + if self.op == Operator::Divide + || zero_product + || self.integer_arithmetic_may_wrap( + left_interval, + right_interval, + &apply_operator(&self.op, left_interval, right_interval)?, + ) + { return Ok(Some(vec![])); } - return apply_operator(&self.op, left_interval, right_interval).and_then( - |result| { - if self.integer_arithmetic_may_wrap( - left_interval, - right_interval, - &result, - ) { - Ok(Some(vec![])) - } else { - propagate_arithmetic( - &self.op, - interval, - left_interval, - right_interval, - ) - .map(|bounds| bounds.map(|(left, right)| vec![left, right])) - } - }, - ); } if self.op.eq(&Operator::And) { @@ -6436,24 +6418,13 @@ mod tests { (-128, -127), (126, 127), ]; - type Arithmetic = fn(i8, i8) -> Option; for checked in [false, true] { - let operations: [(Operator, Arithmetic); 4] = if checked { - [ - (Operator::Plus, i8::checked_add), - (Operator::Minus, i8::checked_sub), - (Operator::Multiply, i8::checked_mul), - (Operator::Divide, i8::checked_div), - ] - } else { - [ - (Operator::Plus, |a, b| Some(a.wrapping_add(b))), - (Operator::Minus, |a, b| Some(a.wrapping_sub(b))), - (Operator::Multiply, |a, b| Some(a.wrapping_mul(b))), - (Operator::Divide, i8::checked_div), - ] - }; - for (op, evaluate) in operations { + for op in [ + Operator::Plus, + Operator::Minus, + Operator::Multiply, + Operator::Divide, + ] { let expr = BinaryExpr::new(lit(0i8), op, lit(0i8)) .with_fail_on_overflow(checked); for (lo, hi) in domains { @@ -6477,7 +6448,18 @@ mod tests { } for a in lo..=hi { for b in rlo..=rhi { - let result = evaluate(a, b); + let result = match (op, checked) { + (Operator::Plus, false) => Some(a.wrapping_add(b)), + (Operator::Minus, false) => Some(a.wrapping_sub(b)), + (Operator::Multiply, false) => { + Some(a.wrapping_mul(b)) + } + (Operator::Plus, true) => a.checked_add(b), + (Operator::Minus, true) => a.checked_sub(b), + (Operator::Multiply, true) => a.checked_mul(b), + (Operator::Divide, _) => a.checked_div(b), + _ => unreachable!(), + }; let Some(result) = result else { continue; }; @@ -6523,17 +6505,18 @@ mod tests { #[test] fn test_integer_interval_error_propagation() { + // Compare error messages without backtraces, which differ by call path. let integer = Interval::make(Some(1i32), Some(2i32)).unwrap(); let boolean = Interval::TRUE; for op in [Operator::Plus, Operator::Minus] { let expr = BinaryExpr::new(lit(1i32), op, lit(true)); let expected = apply_operator(&op, &integer, &boolean) .unwrap_err() - .to_string(); + .strip_backtrace(); assert_eq!( expr.evaluate_bounds(&[&integer, &boolean]) .unwrap_err() - .to_string(), + .strip_backtrace(), expected ); let children = @@ -6542,7 +6525,9 @@ mod tests { ..ExprProperties::new_unknown() }); assert_eq!( - expr.get_properties(&children).unwrap_err().to_string(), + expr.get_properties(&children) + .unwrap_err() + .strip_backtrace(), expected ); } @@ -6554,10 +6539,10 @@ mod tests { assert_eq!( expr.propagate_constraints(&parent, &[&integer, &integer]) .unwrap_err() - .to_string(), + .strip_backtrace(), ScalarValue::new_zero(&DataType::Utf8) .unwrap_err() - .to_string() + .strip_backtrace() ); } @@ -6632,6 +6617,20 @@ mod tests { } } + #[test] + fn test_integer_comparison_still_propagates() { + // Integer comparisons must bypass the arithmetic overflow guard and + // still narrow their inputs: a = 5 restricts a in [0, 10] to [5, 5]. + let expr = BinaryExpr::new(lit(0i32), Operator::Eq, lit(5i32)); + let left = Interval::make(Some(0i32), Some(10i32)).unwrap(); + let right = Interval::make(Some(5i32), Some(5i32)).unwrap(); + assert_eq!( + expr.propagate_constraints(&Interval::TRUE, &[&left, &right]) + .unwrap(), + Some(vec![right.clone(), right]) + ); + } + #[test] fn test_safe_integer_multiplication_still_propagates() { let expr = BinaryExpr::new(lit(0i32), Operator::Multiply, lit(2i32)); From 186d8bb82dacc0d07a56ca2dae8b5e1a491b7a7c Mon Sep 17 00:00:00 2001 From: Huaijin Date: Sun, 13 Sep 2026 21:51:27 +0800 Subject: [PATCH 09/10] update --- .../physical-expr/src/expressions/binary.rs | 27 ++++++++++--------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/datafusion/physical-expr/src/expressions/binary.rs b/datafusion/physical-expr/src/expressions/binary.rs index fedd9cd438bef..1117d3fdd5fc1 100644 --- a/datafusion/physical-expr/src/expressions/binary.rs +++ b/datafusion/physical-expr/src/expressions/binary.rs @@ -6410,6 +6410,7 @@ mod tests { // Enumerate small domains and both ends of Int8, including zero divisors, // truncation, signed overflow and checked arithmetic. Every successful // runtime evaluation must remain possible after interval propagation. + let batch = RecordBatch::new_empty(Arc::new(Schema::empty())); let domains = [ (-3i8, 3i8), (0, 1), @@ -6448,21 +6449,21 @@ mod tests { } for a in lo..=hi { for b in rlo..=rhi { - let result = match (op, checked) { - (Operator::Plus, false) => Some(a.wrapping_add(b)), - (Operator::Minus, false) => Some(a.wrapping_sub(b)), - (Operator::Multiply, false) => { - Some(a.wrapping_mul(b)) + // Use the execution kernel as the oracle rather than + // duplicating its checked and wrapping arithmetic. + let runtime = BinaryExpr::new(lit(a), op, lit(b)) + .with_fail_on_overflow(checked); + let result = match runtime.evaluate(&batch) { + Ok(value) => value.into_array(1).unwrap(), + Err(error) => { + assert!( + checked || op == Operator::Divide, + "wrapping {a} {op} {b} failed: {error}" + ); + continue; } - (Operator::Plus, true) => a.checked_add(b), - (Operator::Minus, true) => a.checked_sub(b), - (Operator::Multiply, true) => a.checked_mul(b), - (Operator::Divide, _) => a.checked_div(b), - _ => unreachable!(), - }; - let Some(result) = result else { - continue; }; + let result = result.as_primitive::().value(0); let result = Interval::make(Some(result), Some(result)).unwrap(); assert_eq!( From 99eaf75bd5592dcd839d3d881e375702ef7b6506 Mon Sep 17 00:00:00 2001 From: Huaijin Date: Tue, 15 Sep 2026 13:49:05 +0800 Subject: [PATCH 10/10] apply suggestion --- .../expr-common/src/interval_arithmetic.rs | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/datafusion/expr-common/src/interval_arithmetic.rs b/datafusion/expr-common/src/interval_arithmetic.rs index ba492806386ff..82577fb72a26c 100644 --- a/datafusion/expr-common/src/interval_arithmetic.rs +++ b/datafusion/expr-common/src/interval_arithmetic.rs @@ -2445,6 +2445,40 @@ mod tests { } } + #[test] + fn test_widening_integer_coercion_arithmetic() -> Result<()> { + let unsigned = Interval::make_unbounded(&DataType::UInt8)?; + let two = Interval::make(Some(2_i16), Some(2_i16))?; + + // Mixed arithmetic must widen UInt8 to [0, 255] in Int16 before + // computing the result, including when it is the right operand. + let product = Interval::make(Some(0_i16), Some(510_i16))?; + assert_eq!(unsigned.mul(&two)?, product); + assert_eq!(two.mul(&unsigned)?, product); + assert_eq!( + unsigned.div(&two)?, + Interval::make(Some(0_i16), Some(127_i16))? + ); + Ok(()) + } + + #[test] + fn test_widening_integer_coercion_comparison() -> Result<()> { + let unsigned = Interval::make_unbounded(&DataType::UInt8)?; + let signed = Interval::make(Some(-1_i16), Some(256_i16))?; + let widened = Interval::make(Some(0_i16), Some(255_i16))?; + + // Comparison coercion must preserve both limits of the UInt8 domain. + assert_eq!(unsigned.intersect(&signed)?, Some(widened.clone())); + assert_eq!(signed.intersect(&unsigned)?, Some(widened.clone())); + assert_eq!(widened.contains(&unsigned)?, Interval::TRUE); + for value in [-1_i16, 256_i16] { + let outside = Interval::make(Some(value), Some(value))?; + assert_eq!(unsigned.contains(&outside)?, Interval::FALSE); + } + Ok(()) + } + #[test] fn test_integer_interval_cast_overflow() { use arrow::compute::CastOptions;