kosiew commented on code in PR #26884:
URL: https://github.com/apache/datafusion/pull/26884#discussion_r4421834626
##########
datafusion/physical-expr/src/equivalence/properties/dependency.rs:
##########
@@ -592,6 +592,28 @@ mod tests {
Ok(())
}
+ /// `sqrt(a)` has no ordering that can be derived from `a`, which may be
+ /// negative, so its ordering function fails. A plan sorted on `sqrt(a)`
+ /// still satisfies a requirement on `sqrt(a)`.
+ #[test]
+ fn test_known_ordering_when_expr_properties_fail() -> Result<()> {
+ let schema =
+ Arc::new(Schema::new(vec![Field::new("a", DataType::Float64,
true)]));
+ let sqrt_a: Arc<dyn PhysicalExpr> =
Arc::new(ScalarFunctionExpr::try_new(
+ datafusion_functions::math::sqrt(),
+ vec![col("a", &schema)?],
+ &schema,
+ Arc::new(ConfigOptions::default()),
+ )?);
+ let ordering: LexOrdering =
Review Comment:
Optional: could we test each ASC/DESC and NULLS FIRST/LAST combination using
a fresh `EquivalenceProperties` instance? Identical requirements should
succeed, while changing either option should fail. Since `sqrt(a)` is nullable,
this would verify that recovery preserves sort options rather than merely
recognizing the expression as ordered.
##########
datafusion/physical-expr/src/equivalence/properties/dependency.rs:
##########
@@ -592,6 +592,28 @@ mod tests {
Ok(())
}
+ /// `sqrt(a)` has no ordering that can be derived from `a`, which may be
+ /// negative, so its ordering function fails. A plan sorted on `sqrt(a)`
+ /// still satisfies a requirement on `sqrt(a)`.
+ #[test]
+ fn test_known_ordering_when_expr_properties_fail() -> Result<()> {
+ let schema =
+ Arc::new(Schema::new(vec![Field::new("a", DataType::Float64,
true)]));
+ let sqrt_a: Arc<dyn PhysicalExpr> =
Arc::new(ScalarFunctionExpr::try_new(
+ datafusion_functions::math::sqrt(),
+ vec![col("a", &schema)?],
+ &schema,
+ Arc::new(ConfigOptions::default()),
+ )?);
+ let ordering: LexOrdering =
+ [PhysicalSortExpr::new_default(Arc::clone(&sqrt_a)).asc()].into();
+ let mut eq_properties =
EquivalenceProperties::new(Arc::clone(&schema));
Review Comment:
Optional: could we add negative controls using the same schema and
expression? Verify that `sqrt(a)` ordering is not satisfied when no ordering is
registered or when only `a` is ordered, since the unbounded input range
prevents `sqrt_order` from deriving ordering. This would confirm that recovery
does not manufacture ordering guarantees.
--
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]
