Smallfu666 opened a new pull request, #5982:
URL: https://github.com/apache/datafusion-comet/pull/5982
## Which issue does this PR close?
Closes #5330.
## Rationale for this change
`NegativeExpr::get_properties` returned the child's properties with the
child's own sort order:
```rust
let properties = children[0].clone().with_order(children[0].sort_properties);
Ok(properties)
```
So for `-a` it reported the same ordering as `a` rather than the reverse,
left the range
unreflected, and inherited `preserves_lex_ordering` as `true` from the child.
That last part is the one with teeth.
`EquivalenceProperties::discover_new_orderings` gates on
that flag, so given an ordering of `[c ASC, a ASC]` and `c = -a` it admitted
a false `[a ASC]`
into the ordering equivalence class. That is the state `EnforceSorting`
reads when it decides a
`SortExec` can be dropped.
## What changes are included in this PR?
Reverse the ordering, reflect the range about zero, and set
`preserves_lex_ordering` to false.
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 a range
reaching that minimum
gets neither claim. An unbounded range, which is what
`EquivalenceProperties` hands this hook
for a column, always reaches it.
- `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.
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?
14 new unit tests in `negative.rs`, covering both eval modes across signed,
unsigned, float,
decimal, duration and interval ranges.
Two of them 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 that `[a ASC]` is not admitted
into `oeq_class()`.
It fails on unpatched main.
`cargo test -p datafusion-comet-spark-expr` passes 851 tests.
`cargo clippy -p datafusion-comet-spark-expr --all-targets -- -D warnings`
and
`cargo fmt --all -- --check` are clean.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
