xiangfu0 commented on code in PR #19799:
URL: https://github.com/apache/pinot/pull/19799#discussion_r4224836207
##########
pinot-core/src/main/java/org/apache/pinot/core/startree/operator/StarTreeFilterOperator.java:
##########
@@ -233,15 +235,23 @@ private StarTreeResult traverseStarTree() {
// If all predicate columns and group-by columns are matched, we can use
aggregated document
if (remainingPredicateColumns.isEmpty() &&
remainingGroupByColumns.isEmpty()) {
- matchingDocIds.add(starTreeNode.getAggregatedDocId());
+ if (heapMatchingDocIds == null) {
+ matchingDocIds.add(starTreeNode.getAggregatedDocId());
+ } else {
+ heapMatchingDocIds.add(starTreeNode.getAggregatedDocId());
+ }
continue;
}
// For leaf node, because we haven't exhausted all predicate columns and
group-by columns, we cannot use
// the aggregated document. Add the range of documents for this node to
the bitmap, and keep track of the
// remaining predicate columns for this node
if (starTreeNode.isLeaf()) {
- matchingDocIds.add((long) starTreeNode.getStartDocId(),
starTreeNode.getEndDocId());
+ if (heapMatchingDocIds == null) {
+ // Range additions grow heap array containers geometrically. Keep
aggregate-only matches in the buffer bitmap.
+ heapMatchingDocIds = matchingDocIds.toRoaringBitmap();
Review Comment:
**[simplification, confirmed]** `matchingDocIds` is provably empty every
time this conversion runs, so the comment describes a carry-over that cannot
happen. Aggregated doc ids are only added once `remainingPredicateColumns` and
`remainingGroupByColumns` are both empty; those sets only shrink (at level
transitions), and the aggregated branch `continue`s before the `isLeaf()` check
— so once any aggregated doc is added, no leaf-range add can ever follow. The
buffer→heap handoff therefore never carries data, no test can cover a non-empty
copy, and `toRoaringBitmap()` here is equivalent to `new RoaringBitmap()`.
```suggestion
// No doc ids can precede the first leaf range (aggregated docs
are only added once all predicate and
// group-by columns are matched, and those sets only shrink), so
start an empty heap bitmap, whose array
// containers grow geometrically under repeated range additions.
heapMatchingDocIds = new RoaringBitmap();
```
--
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]
