Visitar URL original
Add DistributeConjunctsIntoOrFilterOptimizer to push AND conjuncts into OR branches by waterWang · Pull Request #19350 · apache/pinot · GitHub
Skip to content

Add DistributeConjunctsIntoOrFilterOptimizer to push AND conjuncts into OR branches - #19350

Closed
waterWang wants to merge 1 commit into
apache:masterfrom
waterWang:fix/19339-distribute-conjuncts-into-or
Closed

waterWang wants to merge 1 commit into
apache:masterfrom
waterWang:fix/19339-distribute-conjuncts-into-or

Conversation

@waterWang

Copy link
Copy Markdown

Fixes #19339

Description

When a filter has the shape P AND (A OR B), Pinot evaluates the (A OR B) subtree independently of P. If a branch of the OR contains a predicate with no index (e.g. IN_SUBQUERY evaluated via ExpressionScanDocIdIterator), that predicate is evaluated for every document matching the branch, not only for the documents that satisfy P.

This PR adds a new DistributeConjunctsIntoOrFilterOptimizer that distributes selective conjuncts (EQUALS/IN on single columns) from an enclosing AND into each OR branch:

P AND (A OR B)  →  P AND ((P AND A) OR (P AND B))

This rewrite is sound because in three-valued logic, P ∧ (A ∨ B) ≡ P ∧ ((P ∧ A) ∨ (P ∧ B)) when the filter only passes TRUE (SQL WHERE semantics).

The bug in detail

The AndDocIdSet#iterator() method chooses between two strategies at the eager/lazy threshold. When an OR subtree contains an AND with both index-based and scan-based children, the eager path materializes the index-based children into a bitmap over the entire segment before applying the scan-based predicate. This means expensive predicates like IN_SUBQUERY are evaluated for every document in the segment, not just those matching the enclosing AND's selective predicate P.

Adding an index to a column inside an OR branch makes the query slower because it triggers the eager path:

With no index-based child in the AND under the OR: lazy path, outer merged bitmap drives it. The expensive predicate is only evaluated at documents that already match P.
With one or more index-based children: eager path, branch is fully materialized over the whole segment, ignoring P.

In production, the same query returning the same two rows went from ~500 to ~17,000,000 numEntriesScannedInFilter after adding indexes to two columns in the OR branch.

Test plan

  • Added DistributeConjunctsIntoOrFilterOptimizerTest with 5 test cases:
    • Basic distribution: P AND (A OR B) → P AND ((P AND A) OR (P AND B))
    • No distribution when there is no OR child
    • No distribution when the conjunct is not a simple column predicate
    • Branch already contains the conjunct → no redundant AND(P, P) created
    • No rewrite when there is no AND child (only OR)
  • All existing optimizer tests should continue to pass (the optimizer is added at the end of the pipeline, after all existing optimizers)

…to OR branches (apache#19339)

When a filter has the shape `P AND (A OR B)`, Pinot evaluates the `(A OR B)`
subtree independently of `P`. If a branch of the OR contains a predicate
with no index (e.g. IN_SUBQUERY evaluated via ExpressionScanDocIdIterator),
that predicate is evaluated for every document matching the branch, not
only for the documents that satisfy `P`.

This optimizer distributes selective conjuncts (EQUALS/IN on single columns)
from an enclosing AND into each OR branch:

    P AND (A OR B)  →  P AND ((P AND A) OR (P AND B))

This is sound because P ∧ (A ∨ B) ≡ P ∧ ((P ∧ A) ∨ (P ∧ B)) in three-valued
logic when the filter only passes TRUE (SQL WHERE semantics).

The optimizer runs after the merge optimizers (MergeEqInFilterOptimizer,
MergeRangeFilterOptimizer) so that merges apply to the original AND/OR
structure before distribution.

Fixes apache#19339
@Jackie-Jiang Jackie-Jiang added enhancement Improvement to existing functionality multi-stage Related to the multi-stage query engine labels Aug 25, 2026
@Jackie-Jiang Jackie-Jiang added bug Something is not working as expected enhancement Improvement to existing functionality and removed enhancement Improvement to existing functionality bug Something is not working as expected labels Aug 25, 2026
@gortiz

gortiz commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

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.

@gortiz gortiz closed this Aug 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Improvement to existing functionality multi-stage Related to the multi-stage query engine

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Predicates outside an OR do not restrict its branches, and adding an index there can cause a large regression

3 participants