Skip to content

fix: reverse ordering and range in NegativeExpr::get_properties - #5982

Open
Smallfu666 wants to merge 1 commit into
apache:mainfrom
Smallfu666:codex/5330-negative-props-rebased
Open

Smallfu666 wants to merge 1 commit into
apache:mainfrom
Smallfu666:codex/5330-negative-props-rebased

Conversation

@Smallfu666

@Smallfu666 Smallfu666 commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes #5330.

Rationale for this change

NegativeExpr::get_properties on main returns the child's properties with the child's own sort
order:

let properties = children[0].clone().with_order(children[0].sort_properties);
Ok(properties)

So for -a it reports the same ordering as a rather than the reverse, leaves the range
unreflected, and inherits preserves_lex_ordering: true from the child.

EquivalenceProperties::discover_new_orderings gates on both of those at once:

if expr_properties.preserves_lex_ordering
    && expr_properties.sort_properties
        == SortProperties::Ordered(leading_ordering_options)

Main fails both arms, so given an ordering of [c ASC, a ASC] and c = -a it admits a false
[a ASC] into the ordering equivalence class. That is the state EnforceSorting reads when it
decides a SortExec can be dropped. Either arm alone would have closed the reported hole, but
both are wrong on main and both are fixed here.

Relationship to DataFusion

DataFusion 55.1.0's own NegativeExpr::get_properties already reverses the ordering, reflects
the 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 -a to report DESC for an ascending legacy-mode column.
This PR returns Unordered there instead, and test_legacy_equivalence_properties_report_negation_as_unordered
pins that.

The reason is that update_properties gives a Column leaf Interval::make_unbounded(&type),
so a signed integer column's range always contains the type minimum, where neg_wrapping is a
fixed point and negation is therefore not monotonic. Reporting DESC there is exactly the bug
in apache/datafusion#24683.

This is worth stating plainly: in legacy (non-ANSI) mode, which is the Spark 3.x default, -a
on a signed integer column now reports Unordered unconditionally, so sort elimination through
negation 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_ordering and
strictly_order_preserving to 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 breaks both. In legacy mode the array path negates an integer with two's complement
    wrapping, where the minimum of the type is its own negation, so an ordered range reaching
    that minimum keeps neither claim. Singleton is the exception and survives wrapping, because
    negation is a function and maps equal inputs to equal outputs.
  • NaN breaks only the ordering. NaN is the maximum of the sort order and negation leaves it
    NaN, so reversing a float ordering would send the maximum to the minimum. Negation is exact
    for 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_wrapping in either mode and get neither claim. An untyped
Null range, which ExprProperties::new_unknown reports whatever the real type is, is treated
as possibly wrapping.

Where the reflected bound is not representable, the range widens to unbounded rather than
propagating an error, because discover_new_orderings does not absorb one and would fail the
plan 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 EquivalenceProperties rather than calling the hook directly. One rebuilds the
reported scenario: add_ordering([c ASC, a ASC]), then add_equal_conditions(c, -a), then
asserts [a ASC] is not admitted into oeq_class().

For the types that take the None arm of signed_integer_min, the tests assert the premise as
well as the conclusion, so that adding a wrapping path to evaluate later would fail a test
rather 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::MIN is not a
representable Decimal128(38, 0) value, so that test negates the largest magnitude the type
admits and pins the exact result.

cargo test -p datafusion-comet-spark-expr passes 863 tests.
cargo clippy -p datafusion-comet-spark-expr --all-targets -- -D warnings and
cargo fmt --all -- --check are clean.

Note that CI has not exercised this branch yet, only the label job has run.

🤖 Generated with Claude Code

@github-actions github-actions Bot added bug Something isn't working area:expressions Expression evaluation labels Sep 16, 2026
@Smallfu666
Smallfu666 force-pushed the codex/5330-negative-props-rebased branch from ca3fd86 to f9d5ffc Compare September 16, 2026 13:56
@Smallfu666
Smallfu666 marked this pull request as ready for review September 16, 2026 13:59

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:expressions Expression evaluation bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NegativeExpr::get_properties reports child ordering and range unchanged

2 participants