yashmayya opened a new pull request, #19216: URL: https://github.com/apache/pinot/pull/19216
## Problem `PinotFilterJoinRule` is a copy-paste fork of Calcite's `FilterJoinRule#perform` (it exists so filters aren't pushed into the right side of a lookup join). The fork predates [CALCITE-7373](https://issues.apache.org/jira/browse/CALCITE-7373), which is fixed in Calcite 1.42.0 — the version this repo already pins. Because the method is forked, the fix was never picked up. A conjunct like `rand() < 0.1` references no input fields, so its `InputFinder` bitmap is empty. `RelOptUtil.classifyFilters` sees an empty bitmap as a subset of the left input's fields and classifies the predicate as pushable, relocating it below the join. It is then evaluated **per left-input row instead of per join-output row**. For a join with fan-out this changes the sampled population — a matching left row keeps or drops all of its output rows together, instead of each output row being sampled independently. The same gap also fed `RelOptUtil.simplifyJoin`, which is the more damaging half. On master: ``` SELECT a.col1, b.col2 FROM a LEFT JOIN b ON a.col1 = b.col1 WHERE b.col3 > 100 * rand() -> LogicalJoin(joinType=[inner]) -- was LEFT ``` The non-deterministic predicate was treated as null-rejecting, so the left join was rewritten to an inner join and the null-padded outer rows were **dropped entirely**. `FULL JOIN` was likewise simplified to `RIGHT`. These are wrong results, not just a different plan. Reproduced on master (`EXPLAIN PLAN FOR SELECT a.col1, b.col2 FROM a JOIN b ON a.col1 = b.col1 WHERE rand() < 0.1`): ``` LogicalJoin(condition=[=($0, $1)], joinType=[inner]) PinotLogicalExchange(distribution=[hash[0]]) LogicalProject(col1=[$0]) LogicalFilter(condition=[<(RAND(), 0.1E0)]) <- pushed below the join PinotLogicalTableScan(table=[[default, a]]) ``` Pinot's `rand()` is correctly annotated `@ScalarFunction(isDeterministic = false)`, and that propagates to Calcite through `FunctionRegistry` → `PinotSqlFunction#isDeterministic`, so the upstream guard does fire once ported. `UUID_V4` / `UUID_V7` are covered too. ## Fix Port the upstream guard verbatim — skip the rule when either the filter condition or the join condition is non-deterministic. Verified byte-for-byte against `FilterJoinRule#perform` in the Calcite 1.42.0 release artifact. ## Trade-off (deliberate) The guard bails on the **whole** condition rather than per conjunct, matching upstream. So a deterministic conjunct sharing a WHERE clause with a non-deterministic one also stays above the join: ``` WHERE a.col3 > 5 AND rand() < 0.1 -> neither conjunct is pushed to the leaf ``` That costs leaf-stage filtering and shuffles more rows for those queries. Splitting per conjunct would be finer-grained, but it would be a *new* deviation in a fork whose drift is the very bug being fixed here, and it interacts with the `origAboveFilters` no-op detection that guards against repeated rule firing. Correctness first; a finer-grained version belongs upstream in Calcite. The behaviour is pinned by an explicit test case so it can't change silently. Related: keeping the filter above the join also blocks the semi-join rewrite for `IN (subquery) AND rand() < ...`, which then plans as an inner join over a distinct aggregate. Also covered by a test. ## Tests 7 cases in `join_planning_tests` + 1 in `lookup_join_planning_tests`: - non-deterministic `WHERE` on inner / left join - non-deterministic `WHERE` on the null-generating side of a **left** and a **full** join (join-type simplification suppressed) - non-deterministic `ON` condition (the second guard, reached via `JoinConditionPushRule` with a null filter) - `uuid_v4()`, to show the guard isn't `RAND`-specific - semi-join / `IN` subquery - mixed deterministic + non-deterministic conjuncts, pinning the trade-off above - lookup join, since that's why this fork exists - a **control** case asserting a purely deterministic filter is still pushed to the leaf, so the guard can't silently over-block `pinot-query-planner` 1421 tests and `pinot-query-runtime` 4491 tests pass. No existing expected plan changed — no test in either module previously used a non-deterministic function. ## Known remaining drift While diffing the fork I found the same method is also missing [CALCITE-7319](https://issues.apache.org/jira/browse/CALCITE-7319) (correlation-variable handling), also from 1.42.0. It appears inert today — Pinot decorrelates before these rules run, and the `LogicalCorrelate` shapes that survive (`UNNEST`) have an `Uncollect` right input, so a `$cor`-bearing filter never sits directly above a join. It's documented in a comment rather than fixed here, to keep this PR single-concern. The comment now uses the repo's existing grep-able `SYNCED WITH Calcite <version>` marker (as in `PinotRelDecorrelator`) so the next Calcite bump re-diffs this method. Separately, the guard only covers the `isDeterministic` axis. Pinot's independent `FunctionVolatility.VOLATILE` axis (`now()`, `ago()`, `stageId()`) is not consulted, so those filters are still pushed below joins — noted in a comment, worth a follow-up. -- 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]
