jayzhan211 commented on code in PR #26747:
URL: https://github.com/apache/datafusion/pull/26747#discussion_r4434556516
##########
datafusion/physical-expr/src/equivalence/class.rs:
##########
@@ -751,6 +754,15 @@ impl EquivalenceGroup {
for (target, _) in targets.iter() {
cls.push(Arc::clone(target));
}
+ // Retain deterministic expressions as equalities so orderings
derived
+ // through strict functions can be inferred after a merge clears
orderings.
+ if !source.children().is_empty() && !is_volatile(source) {
Review Comment:
The broader rule I suggested last round has a problem, sorry about that: the
new equality survives an outer join's NULL padding. This is #26036, which so
far needed a filter like `WHERE b = coalesce(a, 0)`. With this PR a plain
projection is enough. On a padded row `x` is NULL but `CASE WHEN a IS NULL THEN
0 ELSE a END` is 0, so on this branch the outer sort below is dropped and the
query returns `1, 2, 0`. Main returns `0, 1, 2`:
```sql
SET datafusion.execution.target_partitions = 1;
CREATE TABLE padded_l(k INT) AS VALUES (1), (2), (3);
CREATE TABLE padded_r(k INT, a INT) AS VALUES (1, 1), (2, 2);
SELECT y FROM (
SELECT CASE WHEN a IS NULL THEN 0 ELSE a END AS y FROM (
SELECT rr.x, rr.a
FROM (SELECT k, a, CASE WHEN a IS NULL THEN 0 ELSE a END AS x FROM
padded_r) rr
RIGHT JOIN padded_l ON padded_l.k = rr.k
ORDER BY rr.x LIMIT 100
)
) ORDER BY y;
```
Keeping only the column members on the padded side in
`EquivalenceGroup::join` fixes both this query and the #26036 repro. With it
and the query above added to `equivalence_properties.slt` (expected `0`, `1`,
`2`), the full sqllogictest suite passes with no expected-plan changes. Could
you include it here and add `Closes #26036`?
```diff
-use crate::expressions::Literal;
+use crate::expressions::{Column, Literal};
@@ impl EquivalenceClass {
+ /// Keeps what still holds on a join side whose unmatched rows are
padded
+ /// with NULLs: padded columns stay equal to each other, but constants
and
+ /// computed members do not (`coalesce(a, 0)` is 0 where `a` is NULL).
+ fn null_pad(&mut self) {
+ self.constant = None;
+ self.exprs.retain(|expr| expr.is::<Column>());
+ }
@@ pub fn join(
if matches!(join_type, JoinType::Right |
JoinType::Full) {
- cls.constant = None;
+ cls.null_pad();
}
@@
) {
- cls.constant = None;
+ cls.null_pad();
}
```
--
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]
