Re: [PR] feat(open_struct): query-layer wiring for OPEN_STRUCT columns (PR 3/4) [pinot]

["raghavyadav01 (via GitHub)" <gi...@apache.org>]

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: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org