raghavyadav01 commented on code in PR #19040:
URL: https://github.com/apache/pinot/pull/19040#discussion_r3706812016
##########
pinot-core/src/main/java/org/apache/pinot/core/plan/AggregationPlanNode.java:
##########
@@ -187,6 +191,15 @@ private boolean hasNullValues(AggregationFunction[]
aggregationFunctions) {
}
break;
case FUNCTION:
+ DataSource resolvedDs = resolveDataSource(argument);
+ if (resolvedDs == null) {
+ return true;
+ }
+ NullValueVectorReader resolvedNullVector =
resolvedDs.getNullValueVector();
+ if (resolvedNullVector != null &&
!resolvedNullVector.getNullBitmap().isEmpty()) {
Review Comment:
This narrows what used to be a blanket "any FUNCTION arg might be null"
assumption. Now `item()` on a materialized key whose per-key null vector is
empty (or null) falls through to `break`, so the aggregation is treated as
null-free and takes the non-scan path. That's only correct if the per-key null
vector is authoritative for absent docs — which is exactly the
mutable-vs-sealed parity question raised earlier in the review.
Not blocking, but could we add a null-handling-**on** test that pins this:
`COUNT`/`MIN`/`MAX` over a partially-present key, run on both a consuming and a
sealed segment? The existing parity test only covers null handling off.
##########
pinot-segment-local/src/main/java/org/apache/pinot/segment/local/realtime/impl/invertedindex/RealtimeInvertedIndex.java:
##########
@@ -57,6 +57,19 @@ public void add(int dictId, int docId) {
}
}
+ /// Pre-creates an empty bitmap for the next dictionary id. Used by callers
that reserve a
+ /// dictionary id upfront (e.g. OPEN_STRUCT key columns reserve dictId 0 for
the default null
+ /// value) so that subsequent contiguous [#add] calls stay aligned with
dictionary ids.
+ public void reserveNextDictId() {
Review Comment:
`reserveNextDictId()` appends unconditionally, unlike `add(dictId, docId)`
which guards on `_bitmaps.size() == dictId`. It's safe today because
`MutableKeyColumn` calls it exactly once on an empty index, but the method is
public and generically named — a second call, or an interleave with `add()`,
would silently misalign every dictId→docId bitmap after it. Worth a cheap
guard, e.g. `Preconditions.checkState(_bitmaps.isEmpty())`, to make the "only
on an empty index" contract enforced rather than assumed.
--
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]
