You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "comphead (via GitHub)" <gi...@apache.org> on 2026/10/23 00:37:03 UTC
Re: [PR] fix: reverse ordering and range in NegativeExpr::get_properties [datafusion-comet]
comphead commented on code in PR #5982:
URL: https://github.com/apache/datafusion-comet/pull/5982#discussion_r4354978651
##########
native/spark-expr/src/math_funcs/negative.rs:
##########
@@ -70,6 +70,61 @@ impl NegativeExpr {
pub fn arg(&self) -> &Arc<dyn PhysicalExpr> {
&self.arg
}
+
+ /// Wraparound breaks both monotonicity and the reflected bounds, which is why one
+ /// predicate drives two decisions.
+ ///
+ /// This mirrors the type dispatch in `evaluate`, and rests on the invariant that
+ /// `evaluate` raises an error for every type it does not hand to a kernel that can
+ /// actually wrap. `neg_wrapping` is also the float path, but negation is exact there, so
+ /// taking it is not by itself a reason to answer `true`. A new genuinely wrapping path in
+ /// `evaluate` has to be reflected here.
+ fn negation_can_wrap(&self, range: &Interval) -> bool {
+ let data_type = range.data_type();
+ // `evaluate`'s array path routes an unsigned integer to `neg_wrapping` in either mode,
+ // and a `Null` range carries no type to rule anything out:
+ // `ExprProperties::new_unknown` reports one for an expression of any type.
+ if data_type.is_unsigned_integer() || data_type == DataType::Null {
+ return true;
+ }
+ // ANSI mode raises an overflow error where legacy mode wraps.
+ if self.fail_on_error {
+ return false;
+ }
+ // `neg_wrapping` wraps exactly the `downcast_integer!` types and hands everything else to
+ // `neg`, which errors instead of wrapping. The unsigned half already returned above, so
+ // what is left of that set is exactly `is_signed_integer()`. Gating on it ties this
+ // predicate to the kernel: a type that grows a wrapping path in `evaluate` cannot start
+ // claiming a reversed ordering by default, it has to be admitted here deliberately.
+ if !data_type.is_signed_integer() {
+ return false;
+ }
+ match signed_integer_min(&data_type) {
Review Comment:
`DataType::is_signed_integer` already limits this to `Int8` through `Int64`, and `signed_integer_min` lists exactly those four, so I could not find a way to reach the `None => true` arm. `ScalarValue::min(&data_type)` returns the same values (it exists in the DataFusion source I checked), so this could be `ScalarValue::min(&data_type).is_none_or(|min| range.lower().is_null() || range.lower() <= &min)` and the helper could go. That also removes a hand-maintained list that has to stay in step with the predicate. I know the `None` arm is the conservative fallback that was asked for, so please ignore this if you prefer to keep it explicit. I did not compile it, so please check `is_none_or` against the repo's Rust version.
##########
native/spark-expr/src/math_funcs/negative.rs:
##########
@@ -434,6 +540,433 @@ mod tests {
}
}
+ /// The legacy scalar path disagrees with the legacy array path right beside it. `evaluate`
+ /// skips the overflow checks when `fail_on_error` is false and hands the scalar to
+ /// `ScalarValue::arithmetic_negate`, which negates with `neg_checked` whatever the mode, so
+ /// the type minimum errors here while the array path wraps it. Spark wraps in non-ANSI, so
+ /// the array path is the one that matches. Both halves are pinned rather than fixed, since
+ /// the scalar path predates this change. Tracked in
+ /// <https://github.com/apache/datafusion-comet/issues/6015>.
+ #[test]
+ fn test_legacy_scalar_negation_of_min_errors_unlike_the_array_path() {
+ for scalar in [
+ ScalarValue::Int8(Some(i8::MIN)),
+ ScalarValue::Int16(Some(i16::MIN)),
+ ScalarValue::Int32(Some(i32::MIN)),
+ ScalarValue::Int64(Some(i64::MIN)),
+ ] {
+ assert!(
+ eval_scalar(scalar.clone(), false).is_err(),
+ "{scalar:?}: legacy scalar negation of the type minimum errors today"
+ );
+ }
+
+ // Same value, same mode, array path: wraps onto itself instead of erroring.
+ let array: ArrayRef = Arc::new(Int32Array::from(vec![i32::MIN]));
+ let ColumnarValue::Array(result) = eval_array(array, false).unwrap() else {
+ panic!("expected array result")
+ };
+ assert_eq!(result.as_primitive::<Int32Type>().value(0), i32::MIN);
+ }
+
+ fn ordered(descending: bool, nulls_first: bool) -> SortProperties {
+ SortProperties::Ordered(SortOptions {
+ descending,
+ nulls_first,
+ })
+ }
+
+ fn int32_interval(lower: i32, upper: i32) -> Interval {
+ Interval::try_new(
+ ScalarValue::Int32(Some(lower)),
+ ScalarValue::Int32(Some(upper)),
+ )
+ .unwrap()
+ }
+
+ fn unbounded(data_type: &DataType) -> Interval {
+ Interval::make_unbounded(data_type).unwrap()
+ }
+
+ fn child_properties(sort_properties: SortProperties, range: Interval) -> ExprProperties {
+ ExprProperties {
+ sort_properties,
+ range,
+ // `EquivalenceProperties::discover_new_orderings` builds its child properties
+ // this way, so it is the value the result has to override.
+ preserves_lex_ordering: true,
+ strictly_order_preserving: true,
+ }
+ }
+
+ fn negate_properties(child: ExprProperties, fail_on_error: bool) -> Result<ExprProperties> {
+ NegativeExpr::new(Arc::new(Column::new("a", 0)), fail_on_error).get_properties(&[child])
+ }
+
+ /// Nulls are not moved by negation, so `nulls_first` carries over unflipped.
+ #[test]
+ fn test_get_properties_reverses_child_ordering() {
Review Comment:
`test_get_properties_reverses_child_ordering`, `test_get_properties_negates_child_range` and `test_legacy_get_properties_reverses_ordering_when_range_excludes_int_min` check the same hook on the same shape of input, each asserting one half of the result. A single table-driven test over `(child sort, child range, fail_on_error)` to `(sort, range)` would cover them and could absorb the `holds_int_min` and singleton rows too.
##########
native/spark-expr/src/math_funcs/negative.rs:
##########
@@ -434,6 +540,433 @@ mod tests {
}
}
+ /// The legacy scalar path disagrees with the legacy array path right beside it. `evaluate`
+ /// skips the overflow checks when `fail_on_error` is false and hands the scalar to
+ /// `ScalarValue::arithmetic_negate`, which negates with `neg_checked` whatever the mode, so
+ /// the type minimum errors here while the array path wraps it. Spark wraps in non-ANSI, so
+ /// the array path is the one that matches. Both halves are pinned rather than fixed, since
+ /// the scalar path predates this change. Tracked in
+ /// <https://github.com/apache/datafusion-comet/issues/6015>.
+ #[test]
+ fn test_legacy_scalar_negation_of_min_errors_unlike_the_array_path() {
+ for scalar in [
+ ScalarValue::Int8(Some(i8::MIN)),
+ ScalarValue::Int16(Some(i16::MIN)),
+ ScalarValue::Int32(Some(i32::MIN)),
+ ScalarValue::Int64(Some(i64::MIN)),
+ ] {
+ assert!(
+ eval_scalar(scalar.clone(), false).is_err(),
+ "{scalar:?}: legacy scalar negation of the type minimum errors today"
+ );
+ }
+
+ // Same value, same mode, array path: wraps onto itself instead of erroring.
+ let array: ArrayRef = Arc::new(Int32Array::from(vec![i32::MIN]));
+ let ColumnarValue::Array(result) = eval_array(array, false).unwrap() else {
+ panic!("expected array result")
+ };
+ assert_eq!(result.as_primitive::<Int32Type>().value(0), i32::MIN);
+ }
+
+ fn ordered(descending: bool, nulls_first: bool) -> SortProperties {
+ SortProperties::Ordered(SortOptions {
+ descending,
+ nulls_first,
+ })
+ }
+
+ fn int32_interval(lower: i32, upper: i32) -> Interval {
+ Interval::try_new(
+ ScalarValue::Int32(Some(lower)),
+ ScalarValue::Int32(Some(upper)),
+ )
+ .unwrap()
+ }
+
+ fn unbounded(data_type: &DataType) -> Interval {
+ Interval::make_unbounded(data_type).unwrap()
+ }
+
+ fn child_properties(sort_properties: SortProperties, range: Interval) -> ExprProperties {
+ ExprProperties {
+ sort_properties,
+ range,
+ // `EquivalenceProperties::discover_new_orderings` builds its child properties
+ // this way, so it is the value the result has to override.
+ preserves_lex_ordering: true,
+ strictly_order_preserving: true,
+ }
+ }
+
+ fn negate_properties(child: ExprProperties, fail_on_error: bool) -> Result<ExprProperties> {
+ NegativeExpr::new(Arc::new(Column::new("a", 0)), fail_on_error).get_properties(&[child])
+ }
+
+ /// Nulls are not moved by negation, so `nulls_first` carries over unflipped.
+ #[test]
+ fn test_get_properties_reverses_child_ordering() {
+ for (child, expected) in [
+ (ordered(false, true), ordered(true, true)),
+ (ordered(true, false), ordered(false, false)),
+ ] {
+ let props =
+ negate_properties(child_properties(child, int32_interval(1, 10)), false).unwrap();
+ assert_eq!(props.sort_properties, expected);
+ }
+ }
+
+ #[test]
+ fn test_get_properties_negates_child_range() {
+ let props = negate_properties(
+ child_properties(SortProperties::Unordered, int32_interval(1, 10)),
+ false,
+ )
+ .unwrap();
+ assert_eq!(props.range, int32_interval(-10, -1));
+ assert!(!props.preserves_lex_ordering);
+ }
+
+ #[test]
+ fn test_legacy_get_properties_drops_ordering_when_range_holds_int_min() {
+ let props = negate_properties(
+ child_properties(ordered(false, true), int32_interval(i32::MIN, i32::MIN + 1)),
+ false,
+ )
+ .unwrap();
+ assert_eq!(props.sort_properties, SortProperties::Unordered);
+ assert_eq!(props.range, unbounded(&DataType::Int32));
+ }
+
+ #[test]
+ fn test_legacy_get_properties_reverses_ordering_when_range_excludes_int_min() {
+ let props = negate_properties(
+ child_properties(ordered(false, true), int32_interval(i32::MIN + 1, 10)),
+ false,
+ )
+ .unwrap();
+ assert_eq!(props.sort_properties, ordered(true, true));
+ assert_eq!(props.range, int32_interval(-10, i32::MAX));
+ }
+
+ #[test]
+ fn test_legacy_get_properties_keeps_a_singleton_child_singleton() {
+ let props = negate_properties(
+ child_properties(
+ SortProperties::Singleton,
+ int32_interval(i32::MIN, i32::MIN),
+ ),
+ false,
+ )
+ .unwrap();
+ assert_eq!(props.sort_properties, SortProperties::Singleton);
+ assert_eq!(props.range, unbounded(&DataType::Int32));
+ }
+
+ /// `ExprProperties::new_unknown` reports a `Null` range whatever the expression's real
+ /// type, so a `Null` range cannot rule out wrapping.
+ #[test]
+ fn test_get_properties_claims_no_ordering_for_an_untyped_range() {
+ for fail_on_error in [false, true] {
+ let props = negate_properties(
+ child_properties(ordered(false, true), ExprProperties::new_unknown().range),
+ fail_on_error,
+ )
+ .unwrap();
+ assert_eq!(props.sort_properties, SortProperties::Unordered);
+ assert_eq!(props.range, unbounded(&DataType::Null));
+ }
+ }
+
+ /// ANSI mode raises an overflow error instead of wrapping, so a range reaching down to
+ /// `i32::MIN` still reverses the ordering. Its reflection is not representable.
+ #[test]
+ fn test_ansi_get_properties_reverses_ordering_when_range_holds_int_min() {
+ let props = negate_properties(
+ child_properties(ordered(false, true), int32_interval(i32::MIN, 10)),
+ true,
+ )
+ .unwrap();
+ assert_eq!(props.sort_properties, ordered(true, true));
+ assert_eq!(props.range, unbounded(&DataType::Int32));
+ }
+
+ /// `evaluate` routes an unsigned integer to `neg_wrapping` even under ANSI, so `0` negates
+ /// to itself and the wrapped bounds are out of reach as well.
+ #[test]
+ fn test_get_properties_claims_nothing_for_an_unsigned_range() {
+ for fail_on_error in [false, true] {
+ let props = negate_properties(
+ child_properties(ordered(false, true), unbounded(&DataType::UInt32)),
+ fail_on_error,
+ )
+ .unwrap();
+ assert_eq!(props.sort_properties, SortProperties::Unordered);
+ assert_eq!(props.range, unbounded(&DataType::UInt32));
+ }
+ }
+
+ /// `NaN` is the maximum of the sort order and negation leaves it `NaN`, both asserted here,
+ /// so reversing a float ordering would have to send the maximum to the minimum. Negation is
+ /// otherwise exact, so the bounds reflect: two separate questions, and only ordering fails.
+ #[test]
+ fn test_get_properties_reflects_a_float_range_but_claims_no_ordering() {
+ let nan: ArrayRef = Arc::new(Float64Array::from(vec![f64::NAN]));
+ let inf: ArrayRef = Arc::new(Float64Array::from(vec![f64::INFINITY]));
+ assert!(gt(&nan, &inf).unwrap().value(0), "NaN is the order maximum");
+
+ let floats: ArrayRef = Arc::new(Float64Array::from(vec![1.0, 2.0, f64::NAN]));
+ let ColumnarValue::Array(negated) = eval_array(floats, false).unwrap() else {
+ panic!("expected array result")
+ };
+ let negated = negated.as_primitive::<Float64Type>();
+ assert_eq!(negated.value(0), -1.0);
+ assert!(negated.value(2).is_nan(), "the maximum is a fixed point");
+
+ let float_interval = |lower: f64, upper: f64| {
+ Interval::try_new(
+ ScalarValue::Float64(Some(lower)),
+ ScalarValue::Float64(Some(upper)),
+ )
+ .unwrap()
+ };
+ for fail_on_error in [false, true] {
+ let props = negate_properties(
+ child_properties(ordered(false, true), float_interval(1.0, 10.0)),
+ fail_on_error,
+ )
+ .unwrap();
+ assert_eq!(props.sort_properties, SortProperties::Unordered);
+ assert_eq!(props.range, float_interval(-10.0, -1.0));
+
+ // The ordering answer is `Singleton` either way, so this pins the range alone.
+ let props = negate_properties(
+ child_properties(SortProperties::Singleton, float_interval(1.5, 1.5)),
+ fail_on_error,
+ )
+ .unwrap();
+ assert_eq!(props.sort_properties, SortProperties::Singleton);
+ assert_eq!(props.range, float_interval(-1.5, -1.5));
+ }
+ }
+
+ /// An unbounded decimal range is made of null decimal bounds, which
+ /// `ScalarValue::arithmetic_negate` returns unchanged, so the range stays the full range and
+ /// the ordering claim survives without turning a decimal negation into a planning error:
+ /// `EquivalenceProperties::discover_new_orderings` propagates an error out of this hook with
+ /// `?`, so `add_equal_conditions` below would fail the plan instead.
+ #[test]
+ fn test_get_properties_widens_a_decimal_range_rather_than_failing_a_plan() {
Review Comment:
The name says the range widens, but the body's last assertion is that the unbounded range comes back unchanged, and the thread says decimal bounds negate without error. The `add_equal_conditions` half repeats `test_negation_does_not_admit_a_false_ordering_into_the_equivalence_class`, and the `negate_properties` half repeats the ANSI assertion in `test_get_properties_reverses_ordering_for_decimal`. Could this test go, with the unbounded-range assertion added to the decimal test?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org