gortiz commented on PR #19350: URL: https://github.com/apache/pinot/pull/19350#issuecomment-5479363265
Thanks for picking this up so quickly, @waterWang, and for the write-up in the description — the diagnosis is exactly right, and it's a correct rewrite. It's actually sound more generally than you claim: strong Kleene logic is a distributive lattice with an idempotent meet, so `P ∧ ((P∧A) ∨ (P∧B)) ≡ P ∧ (A∨B)` holds unconditionally, including underneath a `NOT` — not only because `WHERE` passes TRUE. After spending more time on the problem I don't think the planner rewrite is the right *primary* fix, and I've opened #19408 with option 1 instead. My reasoning, in the order I'd weight it: **It doesn't remove the cliff.** The bug I care most about in #19339 is that *adding an index to a column inside an OR branch makes a query slower*, because the branch flips to the eager path in `AndDocIdSet#iterator()`. After this rewrite that is still true — the branch still goes eager, it just has `P` inside it now. The execution-layer fix removes the threshold instead of arranging for the right predicate to be on the other side of it. **It taxes the common case to help a rare one.** For a plain `P AND (A OR B)` with `A` and `B` both indexed — most OR queries — the rewrite is pure overhead: `P`'s filter operator is constructed and its bitmap read three times instead of once, plus two extra intersections. `FilterPlanNode`'s evaluator cache shares the dictionary lookup, but not the index read or the bitmap work. The optimizer doesn't check whether any branch actually contains something scan-based, which is the only case where the duplication pays for itself. **Its reach is narrower than the issue needs.** Only single-column EQ/IN is distributed, so it can't reach the case @alexch2000 reported above — his selective block is `OR(AND(name, flow_id, action_id), AND(name, flow_id))`, and the only conjunct available to distribute is a `device_os IN (...)` that matches most rows. And because the recursion is post-order, an AND nested inside an OR branch is rewritten before its parent distributes into it, so `P` never reaches an OR below the first level. None of this is a criticism of the implementation — it's that the layer is wrong for this particular bug. A planner rewrite has to decide, without segment knowledge, whether duplicating a predicate is cheap; at execution time the AND already *has* the matching document ids and can simply hand them down, with no heuristic, no duplicated index lookups, and at any nesting depth. Two smaller notes for whatever you work on next, since they'd apply to any version of this: the optimizer runs unconditionally, where #19339 asked for a query option (#19408 ships disabled by default for exactly this reason — the push-down materializes the filter result, so it needs to stay off for queries that stop at their LIMIT); and it inserts the same `Expression` object into several branches, which is worth avoiding in a tree that later passes mutate in place. I'd really like you to stay involved — a review of #19408 from someone who has already been through this code would be genuinely useful. And there are two clear follow-ups if you want a bigger piece: pushing the restriction into the range index itself via `RangeBitmap`'s context parameter (@richardstartin's suggestion on the issue, which nothing in Pinot uses today), and making the push-down stream instead of materialize so the mode gating can be deleted entirely. Happy to write either up as an issue. -- 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]
