Smallfu666 opened a new issue, #5330:
URL: https://github.com/apache/datafusion-comet/issues/5330
### 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<ExprProperties> {
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<dyn PhysicalExpr>,
SortOptions { descending: false, nulls_first: true },
)]);
let negated: Arc<dyn PhysicalExpr> = 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<ExprProperties> {
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<ExprProperties> {
+ 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.
--
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]
