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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
