You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "Smallfu666 (via GitHub)" <gi...@apache.org> on 2026/08/11 18:06:47 UTC
[I] `NegativeExpr::get_properties` reports child ordering and range unchanged [datafusion-comet]
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: github-unsubscribe@datafusion.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] `NegativeExpr::get_properties` reports child ordering and range unchanged [datafusion-comet]
Posted by "Smallfu666 (via GitHub)" <gi...@apache.org>.
Smallfu666 commented on issue #5330:
URL: https://github.com/apache/datafusion-comet/issues/5330#issuecomment-5257006102
take
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] `NegativeExpr::get_properties` reports child ordering and range unchanged [datafusion-comet]
Posted by "Smallfu666 (via GitHub)" <gi...@apache.org>.
Smallfu666 commented on issue #5330:
URL: https://github.com/apache/datafusion-comet/issues/5330#issuecomment-5634806014
Not working on this one, could someone unassign me?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org