Short Circuit And Filter Operator During Index Evaluation #14700
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #14700 +/- ##
============================================
+ Coverage 61.75% 63.83% +2.08%
- Complexity 207 1607 +1400
============================================
Files 2436 2703 +267
Lines 133233 150759 +17526
Branches 20636 23296 +2660
============================================
+ Hits 82274 96237 +13963
- Misses 44911 47323 +2412
- Partials 6048 7199 +1151
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
ankitsultana
changed the title
| @@ -401,7 +401,7 @@ public void queryStContainsWithMultipleFilters() | |||
|
|
|||
| AggregationOperator aggregationOperator = getOperator(query); | |||
| AggregationResultsBlock resultsBlock = aggregationOperator.nextBlock(); | |||
| QueriesTestUtils.testInnerSegmentExecutionStatistics(aggregationOperator.getExecutionStatistics(), 0, 2, 0, 1); | |||
| QueriesTestUtils.testInnerSegmentExecutionStatistics(aggregationOperator.getExecutionStatistics(), 0, 1, 0, 1); | |||
There was a problem hiding this comment.
This is expected. Previously we would end up evaluating both the predicates, but now we only evaluate the first one, and since that doesn't match any docs, we skip the second predicate
ankitsultana
changed the title
pinot-core/src/main/java/org/apache/pinot/core/common/BlockDocIdSet.java
Outdated
Show resolved
Hide resolved
pinot-core/src/main/java/org/apache/pinot/core/operator/docidsets/BitmapDocIdSet.java
Outdated
Show resolved
Hide resolved
ankitsultana
changed the title
ankitsultana
changed the title
There was a problem hiding this comment.
Now I remember why we didn't have isAlwaysFalse() and isAlwaysTrue() in BlockDocIdSet. We should directly create EmptyDocIdSet and MatchAllDocIdSet when we know the result is always false/true. Initially we followed this contract, but through times newer contribution didn't follow the contract.
Can you try if you can fix the violations of this contract? We should also document this contract in the interface
| /** | ||
| * For scan-based FilterBlockDocIdSet, pre-scans the documents and returns a non-scan-based FilterBlockDocIdSet. | ||
| */ | ||
| default BlockDocIdSet toNonScanDocIdSet() { | ||
| BlockDocIdIterator docIdIterator = iterator(); | ||
|
|
||
| if (docIdIterator instanceof EmptyDocIdIterator) { |
There was a problem hiding this comment.
Do we need this change? This is prioritizing the always empty filter case, which should be extremely rare
| blockDocIdSets.add(blockDocIdSet); | ||
| if (blockDocIdSet.isAlwaysFalse()) { | ||
| // Return AndDocIdSet to ensure that getNumEntriesScannedInFilter is correctly reported. | ||
| return new AndDocIdSet(blockDocIdSets, _queryOptions, true); |
There was a problem hiding this comment.
Do we need to create AndDocIdSet here? The scan shouldn't happen yet. The overhead of using AndDocIdSet is quite high
Returns early from the
AndFilterOperatorif any of the non-lazy predicate has already evaluated to an Empty Doc Id Set.Additionally, we also return early from
CombinedFilterOperatorif the main filter operator evaluated to an empty Doc Id Set.Caveats
Note that this doesn't work for scan based predicates, since those are lazily evaluated in
AndDocIdSet#iterator()for obvious reasons.Alternatives Considered
An alternative approach to this PR could be to restructure our predicate evaluation for ANDs altogether, and instead run the AND Bitmap on a running basis (i.e.
currentBitmap.and(filterOperators.get(i).getTrues()). That way, if the AND of the prefix of the predicates evaluates to an Empty Doc Id Set (!currentBitmap.cardinalityExceeds(0)), we could return early.We could then also use the idea shared by @richardstartin in #14694 to push down the currently evaluated bitmap to the index, to potentially leverage more optimizations.
I think the first step could be to add a Running
ANDfor the prefix of predicates in theAndFilterOperatorand bail early when possible, and the next step could be to push down a partial predicate to certain indexes.cc: @Jackie-Jiang
Test Plan
Unit tests have decent coverage. I am also planning to run some benchmarks and will add their results soon.
Perf Tests
Created a microbenchmark and that runs this filter (obfuscated) on one of our prod segments and the filter evaluates 2-3x faster with the patch. This is a cherry-picked example, but for any use-case that has a high ratio of
segmentsProcessedandsegmentsMatched, this optimization should be helpful. (note that Error is quite high.. but results are good enough for comparison)Without Optimization
With Optimization
Example Filter (Indicative because Obfuscated)