alamb commented on code in PR #11363:
URL: https://github.com/apache/datafusion/pull/11363#discussion_r1672247911
##########
datafusion/physical-expr/src/equivalence/properties.rs:
##########
@@ -2454,30 +2478,49 @@ mod tests {
];
for case in cases {
- let mut properties = base_properties
- .clone()
-
.add_constants(case.constants.into_iter().map(ConstExpr::from));
- for [left, right] in &case.equal_conditions {
- properties.add_equal_conditions(left, right)?
- }
-
- let sort = case
- .sort_columns
- .iter()
- .map(|&name| {
- col(name, &schema).map(|col| PhysicalSortExpr {
- expr: col,
- options: SortOptions::default(),
+ // Construct the equivalence properties in different orders
Review Comment:
I also verified test coverage by running these tests without the code
changes and verified they do in fact fail ✅
```
thread
'equivalence::properties::tests::test_eliminate_redundant_monotonic_sorts'
panicked at datafusion/physical-expr/src/equivalence/properties.rs:2493:17:
assertion `left == right` failed: failed test '(a, b, c) -> (c)'
left: false
right: true
stack backtrace:
0: rust_begin_unwind
at
/rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/std/src/panicking.rs:652:5
1: core::panicking::panic_fmt
at
/rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/core/src/panicking.rs:72:14
2: core::panicking::assert_failed_inner
3: core::panicking::assert_failed
at
/rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/core/src/panicking.rs:364:5
4:
datafusion_physical_expr::equivalence::properties::tests::test_eliminate_redundant_monotonic_sorts
at ./src/equivalence/properties.rs:2493:17
5:
datafusion_physical_expr::equivalence::properties::tests::test_eliminate_redundant_monotonic_sorts::{{closure}}
at ./src/equivalence/properties.rs:2382:54
6: core::ops::function::FnOnce::call_once
at
/rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/core/src/ops/function.rs:250:5
7: core::ops::function::FnOnce::call_once
at
/rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose
backtrace.
error: test failed, to rerun pass `--lib`
error: 1 target failed:
`--lib`
```
##########
datafusion/physical-expr/src/equivalence/properties.rs:
##########
@@ -223,56 +223,11 @@ impl EquivalenceProperties {
}
}
- // Discover new valid orderings in light of the new equality. For a
discussion, see:
- // https://github.com/apache/datafusion/issues/9812
- let mut new_orderings = vec![];
- for ordering in self.normalized_oeq_class().iter() {
- let expressions = if left.eq(&ordering[0].expr) {
- // Left expression is leading ordering
- Some((ordering[0].options, right))
- } else if right.eq(&ordering[0].expr) {
- // Right expression is leading ordering
- Some((ordering[0].options, left))
- } else {
- None
- };
- if let Some((leading_ordering, other_expr)) = expressions {
- // Currently, we only handle expressions with a single child.
- // TODO: It should be possible to handle expressions orderings
like
- // f(a, b, c), a, b, c if f is monotonic in all
arguments.
- // First expression after leading ordering
- if let Some(next_expr) = ordering.get(1) {
- let children = other_expr.children();
- if children.len() == 1
- && children[0].eq(&next_expr.expr)
- && SortProperties::Ordered(leading_ordering)
- == other_expr
- .get_properties(&[ExprProperties {
- sort_properties: SortProperties::Ordered(
- leading_ordering,
- ),
- range: Interval::make_unbounded(
- &other_expr.data_type(&self.schema)?,
- )?,
- }])?
- .sort_properties
- {
- // Assume existing ordering is [a ASC, b ASC]
- // When equality a = f(b) is given, If we know that
given ordering `[b ASC]`, ordering `[f(b) ASC]` is valid,
- // then we can deduce that ordering `[b ASC]` is also
valid.
- // Hence, ordering `[b ASC]` can be added to the state
as valid ordering.
- // (e.g. existing ordering where leading ordering is
removed)
- new_orderings.push(ordering[1..].to_vec());
- }
- }
- }
- }
- if !new_orderings.is_empty() {
- self.oeq_class.add_new_orderings(new_orderings);
- }
-
// Add equal expressions to the state
self.eq_group.add_equal_conditions(left, right);
+
+ // Discover any new orderings
+ self.discover_new_orderings(left)?;
Review Comment:
❤️
--
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-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
