1fanwang opened a new pull request, #23615:
URL: https://github.com/apache/datafusion/pull/23615

   ## Which issue does this PR close?
   
   - Closes #14943.
   
   Revives #21870, which was closed by the stale bot. The direction was agreed 
with `@asolimando` and `sql_planner` was benchmarked by `@alamb` there 
([results](https://github.com/apache/datafusion/pull/21870#issuecomment-4338614104),
 no measurable change). Rebased on `main`.
   
   ## Rationale for this change
   
   `SimplifyExpressions` disables canonicalization for `LogicalPlan::Join` (see 
#8780), so the AND/OR dedup in `expr_contains_inner` — which compares each 
conjunct/disjunct leaf with structural `==` — cannot recognize duplicates that 
differ only by commutative operand order (`A = B` vs `B = A`). Simplifying 
inside a join filter (the delta-rs MERGE case reported in #14943) keeps the 
duplicate across simplifier cycles because nothing normalizes operand order 
first.
   
   `@alamb` pointed at the fix in the issue thread: CSE already dedups `A = B` 
/ `B = A` via `NormalizeEq`; route the simplifier's leaf comparison through the 
same trait.
   
   ## What changes are included in this PR?
   
   `expr_contains_inner` compares leaves with `Expr::normalize_eq` instead of 
`==`. `NormalizeEq` handles `+`, `*`, `&`, `|`, `^`, `=`, `!=` commutatively 
and falls back to structural `==` for everything else, so non-commutative rules 
and the existing `!needle.is_volatile()` guard are unchanged. A regression test 
covers the `AND`, `OR`, and 3-conjunct nested forms.
   
   ## Are these changes tested?
   
   Yes. The new `test_simplify_swapped_operands_in_and_or_no_canonicalize` 
fails on `main` (the duplicate passes through unchanged) and passes with the 
fix. The full `datafusion-optimizer` lib suite and `cargo clippy` are clean, 
and the `sql_planner` benchmark linked above shows no measurable change.
   
   ## Are there any user-facing changes?
   
   No public API changes. AND/OR chains containing commutative-equivalent 
duplicates now collapse even when the simplifier's canonicalizer is disabled 
(currently the `LogicalPlan::Join` path). Canonicalize-on paths produce the 
same output as before.
   


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

Reply via email to