apache / apache/datafusion-comet
`NegativeExpr::get_properties` reports child ordering and range unchanged
- Dominant language
- Scala
- Stars
- 1.3k
- Forks
- 373
- Avg merge
- 2d 6h
- Merged PRs (30d)
- 190
Description
### Describe the bug
`NegativeExpr` implements DataFusion's `get_properties` hook, which is how the optimizer derives an
expression's ordering and value range from its children. Comet's implementation returns the child's
properties unchanged.
`native/spark-expr/src/math_funcs/negative.rs:226-230`:
```rust
/// The ordering of a [`NegativeExpr`] is simply the reverse of its child.
fn get_properties(&self, children: &[ExprProperties]) -> Result {
let properties = children[0].clone().with_order(children[0].sort_properties);
Ok(properties)
}
```
`ExprProperties::with_order` (`datafusion-expr-common-54.1.0/src/sort_properties.rs:160-163`) only
assigns the field, so passing `children[0].sort_properties` back into the child's own clone is an
identity function. Despite the doc comment, nothing is reversed:
| field | correct for `-x` | what Comet returns |
| --- | --- | --- |
| `sort_properties` | child's, `descending` flipped | child's, unflipped |
| `range` | child's, negated and swapped | child's, verbatim |
| `preserves_lex_ordering` | `false` | child's |
So when `a` is ascending, Comet reports `-a` as ascending, and when `a` has range `[1, 10]`, Comet
reports `-a` as `[1, 10]`.
DataFusion's own `NegativeExpr`
(`datafusion-physical-expr-54.1.0/src/expressions/negative.rs:164-170`) negates all three.
A repository search finds this as the only `get_properties` implementation under Comet's `native/`
tree.
### Steps to reproduce
Add to `mod tests` in `native/spark-expr/src/math_funcs/negative.rs` and run
`cargo test -p datafusion-comet-spark-expr --lib math_funcs::negative`.
```rust
#[test]
fn negation_reverses_child_ordering() {
let schema = Arc::new(Schema::new(vec![Field::new("a", DataType::Int32, true)]));
let column = Arc::new(Column::new("a", 0));
let mut eq_properties = EquivalenceProperties::new(schema);
eq_properties.add_ordering([PhysicalSortExpr::new(
Arc::clone(&column) as Arc,
SortOptions { descending: false, nulls_first: true },
)]);
let negated: Arc = Arc::new(NegativeExpr::new(column, false));
assert_eq!(
eq_properties.get_expr_properties(negated).sort_properties,
SortProperties::Ordered(SortOptions { descending: true, nulls_first: true })
);
}
```
On `main` at `bdd2aeb81`:
```
assertion `left == right` failed
left: Ordered(SortOptions { descending: false, nulls_first: true })
right: Ordered(SortOptions { descending: true, nulls_first: true })
```
This goes through `EquivalenceProperties::get_expr_properties`, so the wrong properties reach
DataFusion's property machinery rather than only the method itself. The equivalent assertion on
`range` fails the same way, with `Interval { lower: Int32(1), upper: Int32(10) }` against the
expected `[-10, -1]`.
### Expected behavior
```rust
/// The ordering of a [`NegativeExpr`] is simply the reverse of its child.
fn get_properties(&self, children: &[ExprProperties]) -> Result {
Ok(ExprProperties {
sort_properties: -children[0].sort_properties,
range: children[0].range.clone().arithmetic_negate()?,
preserves_lex_ordering: false,
})
}
```
`-SortProperties` flips only `descending` and leaves `nulls_first` alone
(`sort_properties.rs:121-130`), which is right because negation moves values and not nulls. This
matches DataFusion 54.1.0's implementation.
### Additional context
The regression entered during an API migration. `edd63efb6` (2024-06-04, #471) introduced the
expression with a correct implementation, and `fd596ed98` (2024-06-07, #403) migrated it to the new
`get_properties` API and dropped the negation:
```diff
- fn get_ordering(&self, children: &[SortProperties]) -> SortProperties {
- -children[0]
+ fn get_properties(&self, children: &[ExprProperties]) -> Result {
+ let properties = children[0].clone().with_order(children[0].sort_properties);
+ Ok(properties)
```
(then at `core/src/execution/datafusion/expressions/negative.rs`). The doc comment claiming a
reversal survived the migration, which is probably why it has read as intentional since.
`b8be7b794` (2025-04-16, #1563) carried it forward untouched during the DataFusion 47 upgrade.
The failing `EquivalenceProperties::get_expr_properties` test demonstrates that the incorrect
properties reach DataFusion's property machinery. I have not found a Spark query that produces wrong
results from this mismatch, so I am not claiming a demonstrated wrong-results bug.
I have a fix and regression tests ready and can send a PR.
Contributor guide
Assessment
This issue has not been assessed yet.