vlsi commented on PR #2253: URL: https://github.com/apache/calcite/pull/2253#issuecomment-6118568606
Thank you for working on this, @Aaaaaaron. I'm closing this PR: it has been inactive since March 2021, and `main` has changed enough that the idea needs a different shape rather than a rebase. CALCITE-4385 stays open for anyone who wants to pick it up. I rebased the change onto `main` (843f468405) and ran `:core:test` plus a few extra cases. Here is what I found, in case it helps the next attempt. **What `main` already does** - Cases 3 and 4 from the description (an OR of ANDs with common factors) are what `RexUtil.pullFactors` does. On `main` it returns the same result as `eliminateCommonExprInCondition` for both examples, keeps the operand order, and since CALCITE-739 also matches `$3 = $6` with `$6 = $3`. - CALCITE-7657 added the absorption law to `RexSimplify`, but only for a single absorbing term (`a AND (a OR b)`). Cases 1 and 2 (an AND of ORs whose common part has several terms) are still not handled, so that part of the idea is still useful. - The planner problem is real: without the production change, `testJoinEliminateCommonExpr` still fails on `main`, and the join condition stays `OR(AND(=($0, $9), =($1, 'A')), AND(=($0, $9), =($1, 'B')))`. **Problems a new PR should avoid** 1. Non-deterministic terms are merged. `(RAND() < 0.5 AND a) OR (RAND() < 0.5 AND b)` becomes `RAND() < 0.5 AND (a OR b)`, which evaluates `RAND()` once instead of twice. The absorption code from CALCITE-7657 checks `RexUtil.isDeterministic` for this reason. 2. Operand order depends on hash codes. The common terms are collected in a `HashSet`, so with 16 common terms the output starts with `=(?0.int4, 4), =(?0.int9, 9), =(?0.int3, 3)` while the input starts with `int9, int8, int7`. The tests in this PR pass only because two or three terms happen to keep their order. `pullFactors` uses a `LinkedHashMap` for this reason. 3. `JoinReduceExpressionsRule` discards the rewrite unless something else in the condition changes too. The rule puts the factored condition into `expList` before `reduceExpressions` runs, and `reduceExpressions` compares its result with that list, not with the original condition. For `(e.deptno = d.deptno OR e.sal > 1000) AND (e.deptno = d.deptno OR e.sal > 1000 OR d.name = 'x')` the new method returns `OR(=($7, $9), >($5, 1000))`, yet the rule leaves the plan unchanged. The rule test in this PR passes because the factored OR is then converted to a Sarg, which counts as a change. 4. Changing `RelOptUtil.decomposeConjunction` and `decomposeDisjunction` from `List` to `Collection` breaks binary compatibility: code compiled against the old signatures fails with `NoSuchMethodError`. An overload keeps the old callers working. 5. The new method bypasses `simplify()`. It ignores `unknownAs` and the predicates the rule computes a few lines later, and the tests call it directly, so the paranoid equivalence check that `checkSimplify` runs in `RexProgramTest` never sees the result. It is also applied only to join conditions, so the same expression in a `Filter` is not rewritten. **A possible shape for the next attempt** - Add the dual of `pullFactors`, which pulls common disjuncts out of an AND of ORs, with a `LinkedHashMap` and a determinism check. - Call both from `simplifyAnds` and `simplifyOrs`, with a term limit like `MAX_TERMS_FOR_ABSORPTION`, so filters, projects, and joins all benefit and the result goes through the paranoid check. - Test it with `checkSimplify` in `RexProgramTest`, one method per case: no common part, a common part with several terms, full absorption, a `RAND()` term that must not be merged, and nested AND/OR. - Keep the existing `decompose*` signatures. Thanks again for the effort. -- 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]
