fix: reverse ordering and range in NegativeExpr::get_properties - #5982
Smallfu666 wants to merge 1 commit into
Conversation
ca3fd86 to
f9d5ffc
Compare
sunchao
left a comment
There was a problem hiding this comment.
Reviewed f9d5ffcf1137cf27fe06cba8180cee5e91c0d1ca against 8c229a703ccb024a8b5b1b849a56ceef0b66a4bd. I found no verified P1/P2 issue in this revision.
Correctness
Previously, get_properties copied the child's range, ordering and preservation flags. Negation cannot generally retain those claims. The change reflects representable bounds, reverses safe ordering while retaining null placement, and clears both preservation flags. This matches the DataFusion 55.1.0 contract, with the additional guards Comet's runtime semantics require.
I compared maintained Spark 3.5/4.0 UnaryMinus, its tests and SQL floating-point ordering with Comet's unchanged evaluation paths. Legacy signed minima wrap, whereas ANSI signed minima raise errors. Consequently, an unbounded legacy integer column must remain Unordered. A bounded range excluding MIN can reverse safely. NaN remains the greatest SQL value after negation, so suppressing floating-point ordering is appropriate. Singleton remains valid because equal inputs still produce equal results.
The 15 added tests cover these property boundaries and four EquivalenceProperties scenarios, including the false [a ASC] inference from [c ASC, a ASC] with c = -a. I traced the ordering consumers and the upstream sort-removal path. Comet currently does not run DataFusion's physical optimizer, so this review does not establish a Spark SQL wrong-result reproduction.
Source validation and an independent Python arithmetic oracle passed, including all 32,896 Int8 intervals and 8/16/32/64-bit boundary examples. These are not native execution results. I did not run the Rust, Spark or JNI tests, and the author's 863-test report remains unverified. Maintained Spark 3.4/4.1 branches were unavailable. At 2026-09-16 14:31:33 UTC, CI, CodeQL and PR title checks required action with zero jobs. Only labeling had passed.
Performance
The change operates during property inference. It leaves array/scalar evaluation and Arrow kernel dispatch unchanged and adds no per-row work. Conservatively losing an ordering can retain a sort in consumers of these properties, but advertising an unsafe ordering would be incorrect. I found no material new performance risk requiring an expression microbenchmark, and no runtime speedup is claimed.
Design
Using one wraparound predicate for ordering and range keeps the two affected guarantees consistent. Treating NaN separately is useful because it prevents an ordering claim without discarding ordinary numeric bounds. Widening an unrepresentable or unsupported reflection keeps inference conservative instead of introducing a planning failure. Explicitly clearing the two preservation flags also prevents child metadata from leaking into a different expression.
Abstraction & complexity
The implementation stays local to NegativeExpr, with a small signed-minimum helper and no new configuration or framework. The focused Rust property tests exercise an internal optimizer contract that SQL result comparisons cannot directly assert. The additional duration, interval and unsigned cases defend the broader physical-expression API without expanding Spark serde support. I have no further actionable improvement to request before merge.
Which issue does this PR close?
Closes #5330.
Rationale for this change
NegativeExpr::get_propertieson main returns the child's properties with the child's own sortorder:
So for
-ait reports the same ordering asarather than the reverse, leaves the rangeunreflected, and inherits
preserves_lex_ordering: truefrom the child.EquivalenceProperties::discover_new_orderingsgates on both of those at once:Main fails both arms, so given an ordering of
[c ASC, a ASC]andc = -ait admits a false[a ASC]into the ordering equivalence class. That is the stateEnforceSortingreads when itdecides a
SortExeccan be dropped. Either arm alone would have closed the reported hole, butboth are wrong on main and both are fixed here.
Relationship to DataFusion
DataFusion 55.1.0's own
NegativeExpr::get_propertiesalready reverses the ordering, reflectsthe range, and sets both flags to false. That part of this PR is parity with upstream rather
than anything new.
The Comet-specific part is the suppression: negation is not monotonic for every type Comet hands
this hook, so the unconditional upstream answer is not always sound. DataFusion has the same
latent problem, reported upstream as apache/datafusion#24683, which is still open.
A deliberate divergence from the issue
Issue #5330's suggested fix expects
-ato reportDESCfor an ascending legacy-mode column.This PR returns
Unorderedthere instead, andtest_legacy_equivalence_properties_report_negation_as_unorderedpins that.
The reason is that
update_propertiesgives aColumnleafInterval::make_unbounded(&type),so a signed integer column's range always contains the type minimum, where
neg_wrappingis afixed point and negation is therefore not monotonic. Reporting
DESCthere is exactly the bugin apache/datafusion#24683.
This is worth stating plainly: in legacy (non-ANSI) mode, which is the Spark 3.x default,
-aon a signed integer column now reports
Unorderedunconditionally, so sort elimination throughnegation is lost across the board in that mode. That is the price of not claiming an ordering
that the data can violate.
What changes are included in this PR?
Reverse the ordering, reflect the range about zero, and set
preserves_lex_orderingandstrictly_order_preservingto false. Then suppress the claims where they would be unsound.The ordering claim and the range claim fail for different reasons, so they are decided
separately:
wrapping, where the minimum of the type is its own negation, so an ordered range reaching
that minimum keeps neither claim.
Singletonis the exception and survives wrapping, becausenegation is a function and maps equal inputs to equal outputs.
NaNbreaks only the ordering.NaNis the maximum of the sort order and negation leaves itNaN, so reversing a float ordering would send the maximum to the minimum. Negation is exactfor floats otherwise, so the bounds still reflect soundly.
ANSI mode raises an overflow error where legacy mode wraps, so it keeps the ordering claim.
Unsigned integers are routed to
neg_wrappingin either mode and get neither claim. An untypedNullrange, whichExprProperties::new_unknownreports whatever the real type is, is treatedas possibly wrapping.
Where the reflected bound is not representable, the range widens to unbounded rather than
propagating an error, because
discover_new_orderingsdoes not absorb one and would fail theplan instead.
How are these changes tested?
15 new unit tests in
negative.rs, covering both eval modes across signed, unsigned, float,decimal, duration and interval ranges.
Four go through
EquivalencePropertiesrather than calling the hook directly. One rebuilds thereported scenario:
add_ordering([c ASC, a ASC]), thenadd_equal_conditions(c, -a), thenasserts
[a ASC]is not admitted intooeq_class().For the types that take the
Nonearm ofsigned_integer_min, the tests assert the premise aswell as the conclusion, so that adding a wrapping path to
evaluatelater would fail a testrather than silently turn the ordering claim false. Duration and interval assert that the type
minimum errors instead of wrapping. Decimal is pinned separately, because
i128::MINis not arepresentable
Decimal128(38, 0)value, so that test negates the largest magnitude the typeadmits and pins the exact result.
cargo test -p datafusion-comet-spark-exprpasses 863 tests.cargo clippy -p datafusion-comet-spark-expr --all-targets -- -D warningsandcargo fmt --all -- --checkare clean.Note that CI has not exercised this branch yet, only the
labeljob has run.🤖 Generated with Claude Code