viirya commented on code in PR #26884:
URL: https://github.com/apache/datafusion/pull/26884#discussion_r4418464356
##########
datafusion/physical-expr/src/equivalence/properties/mod.rs:
##########
@@ -1548,7 +1548,13 @@ fn update_properties(
if !node.expr.children().is_empty() {
// We have an intermediate (non-leaf) node, account for its children:
let children_props = node.children.iter().map(|c|
c.data.clone()).collect_vec();
- node.data = node.expr.get_properties(&children_props)?;
+ // Properties that cannot be derived from the children, for example of
a
+ // function whose input may be outside its domain, are unknown. The
+ // ordering of the node itself can still be known, so do not fail here.
+ node.data = node
+ .expr
+ .get_properties(&children_props)
+ .unwrap_or_else(|_| ExprProperties::new_unknown());
Review Comment:
Minor, optional: the fallback range here is
`make_unbounded(&DataType::Null)`, so parents see an ordered child with an
untyped range and stop deriving anything. With `ordering = [sqrt(a) ASC]`,
`-sqrt(a)` and `CAST(sqrt(a) AS DOUBLE)` both come out `Unordered`.
`ScalarFunctionExpr::get_properties` already recovers the output type for an
unbounded Null range. Would something like this be worth doing here too?
```rust
.or_else(|_| {
Interval::make_unbounded(&node.expr.data_type(eq_properties.schema())?)
.map(|range| ExprProperties::new_unknown().with_range(range))
})?;
```
With it, both of those expressions keep their (reversed / same) ordering. An
unbounded typed range is what a `Column` leaf gets, so it shouldn't let a
parent assume any bounds. Fine to leave this for later if you'd rather keep the
PR minimal.
##########
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<()> {
Review Comment:
This pins down the root cause nicely. As mentioned in the summary, the
window query on `main` fails `SanityCheckPlan` because of this same bug. A
sqllogictest next to this unit test would cover the planner path that users
actually hit.
--
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]
