Skip to content

Bypass competitive iteration in single filter bucket case #127267

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions docs/changelog/127267.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pr: 127267
summary: Bypass competitive iteration in single filter bucket case
area: "Aggregations"
type: bug
issues: [127262]
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@
import org.apache.lucene.index.LeafReaderContext;
import org.apache.lucene.search.DisiPriorityQueue;
import org.apache.lucene.search.DisiWrapper;
import org.apache.lucene.search.DisjunctionDISIApproximation;
import org.apache.lucene.search.DocIdSetIterator;
import org.apache.lucene.search.LeafCollector;
import org.apache.lucene.search.Scorable;
Expand Down Expand Up @@ -313,10 +312,9 @@ protected LeafBucketCollector getLeafCollector(AggregationExecutionContext aggCt
// and the cost of per-filter doc iterator is smaller than maxDoc, indicating that there are docs matching the main
// query but not the filter query.
final boolean hasOtherBucket = otherBucketKey != null;
final boolean usesCompetitiveIterator = (parent == null
&& hasOtherBucket == false
&& filterWrappers.isEmpty() == false
&& totalCost < aggCtx.getLeafReaderContext().reader().maxDoc());
// TODO: https://github.com/elastic/elasticsearch/issues/126955
// competitive iterator is currently broken, we would rather be slow than broken
final boolean usesCompetitiveIterator = false;

if (filterWrappers.size() == 1) {
return new SingleFilterLeafCollector(
Expand All @@ -329,12 +327,7 @@ protected LeafBucketCollector getLeafCollector(AggregationExecutionContext aggCt
);
}
// TODO: https://github.com/elastic/elasticsearch/issues/126955
// competitive iterator is currently broken, we would rather be slow than broken
return new MultiFilterLeafCollector(sub, filterWrappers, numFilters, totalNumKeys, hasOtherBucket);
// if (usesCompetitiveIterator) {
// return new MultiFilterCompetitiveLeafCollector(sub, filterWrappers, numFilters, totalNumKeys, hasOtherBucket);
// } else {
// }
}
}

Expand Down Expand Up @@ -458,46 +451,6 @@ public DocIdSetIterator competitiveIterator() {
}
}

private final class MultiFilterCompetitiveLeafCollector extends AbstractLeafCollector {

private final DisjunctionDISIApproximation disjunctionDisi;

MultiFilterCompetitiveLeafCollector(
LeafBucketCollector sub,
List<FilterMatchingDisiWrapper> filterWrappers,
int numFilters,
int totalNumKeys,
boolean hasOtherBucket
) {
super(sub, numFilters, totalNumKeys, true, hasOtherBucket);
assert filterWrappers.isEmpty() == false;
disjunctionDisi = DisjunctionDISIApproximation.of(filterWrappers, Long.MAX_VALUE);
}

public void collect(int doc, long bucket) throws IOException {
boolean matched = false;
int target = disjunctionDisi.advance(doc);
if (target == doc) {
for (DisiWrapper w = disjunctionDisi.topList(); w != null; w = w.next) {
FilterMatchingDisiWrapper topMatch = (FilterMatchingDisiWrapper) w;
if (topMatch.checkDocForMatch(doc)) {
collectBucket(sub, doc, bucketOrd(bucket, topMatch.filterOrd));
matched = true;
}
}
}

if (hasOtherBucket && false == matched) {
collectBucket(sub, doc, bucketOrd(bucket, numFilters));
}
}

@Override
public DocIdSetIterator competitiveIterator() {
return disjunctionDisi;
}
}

private static class FilterMatchingDisiWrapper extends DisiWrapper {
final int filterOrd;

Expand Down
Loading