Skip to content

NegativeExpr::get_properties reports child ordering and range unchanged #5330

Description

@Smallfu666

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:

/// 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.

#[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

/// 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:

-    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.

Metadata

Metadata

Assignees

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions