apache / apache/datafusion-comet

`NegativeExpr::get_properties` reports child ordering and range unchanged

Open
#5,330 1 comment 0 reactions 1 assignee Claimed by @Smallfu666 View on GitHub
area:expressions bug priority:medium
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.