Support null handling in dictionary-based and multi-value group key generation

[Jackie-Jiang]

Summary

With null handling enabled, a group-by query was routed to the no-dictionary group key generators regardless of how its columns are encoded, and the multi-value key path ignored nulls entirely. This PR makes every group key generator null-aware, so the routing can go back to being decided by column encoding alone.

Dictionary-based group key generation

With null handling enabled, a column whose segment tracks nulls reserves one further dictionary id, one past its last real id, to stand for a null value; its cardinality counts one more than the dictionary holds. A block's null rows are moved onto the reserved id before key composition (copying the block's id array, which is shared with other readers of the column), so the per-row kernels and the raw key arithmetic are untouched. The reserved id composes into the raw key like any other value and is read back out as SQL NULL.

A column that tracks no nulls reserves nothing, and neither does any column when null handling is disabled, which leaves the cardinalities, the holder selection and the block reads exactly as they are today. Dictionary-encoded group-by columns therefore keep the dictionary-based generator in both modes, instead of paying for per-row hash lookups on the no-dictionary path.

Multi-value key path of the no-dictionary generators

The int[][] overloads of both no-dictionary generators read every column without consulting its null bitmap, so a null row was grouped under the column's default null value instead of under a group of its own — for every column on that path, single-value columns included. Both overloads now recognize nulls: a null row contributes one NULL group, mirroring the single-value behavior and the row's physical storage as a one-element default value.

With that closed, the null-enabled and null-disabled twin loops in both generators are merged: every iteration on these paths pays for a hash-map operation, which dwarfs the predicted per-row null check the merge adds, and both files end up smaller than before while doing more.

Bugfix: rows misattributed once the group limit is reached

The null-enabled at-limit path of NoDictionaryMultiColumnGroupKeyGenerator resolved each column's key value but never stored it, composing whatever the key buffer held from the previous row — which, after a new group's buffer swap, is {0, 0, ...}: exactly the first group's key. With null handling enabled and numGroupsLimit reached, rows belonging to existing groups could be counted into the first group or dropped. The resolved value is now stored, same as the null-disabled twin always did.

Behavior changes

All confined to queries that enable null handling:

• Grouping by a multi-value column whose row is an empty array now returns a NULL group. Pinot ingests an empty array as null, so [] and SQL NULL are indistinguishable by the time a query reads them, and an empty multi-value row has no representation in a segment. This is not the Postgres answer, where unnest('{}') contributes no rows at all — it is the closest one reachable here, and it beats the previous behaviour of folding those rows into the column's default null value.
• Grouping by a multi-value column with null rows now returns a NULL group; previously those rows were grouped under the column's default null value.
• Rows past the group limit are attributed to their correct existing groups (the bugfix above).
• The group id upper bound for dictionary-encoded columns is now derived from cardinalities rather than defaulting to numGroupsLimit, matching the null-handling-disabled behavior.

With null handling disabled nothing changes: no id is reserved, the routing condition is the encoding check it used to be, and the per-row loops of the dictionary-based generator are byte-identical.

Tests

NullHandlingEnabledQueriesTest gains nine query-level tests covering every generator and key path with nulls: dictionary-encoded SV (including a row holding the column's default null value, which must not join the NULL group), two dictionary columns, dictionary MV, raw SV (INT and STRING, which exercise the primitive-map and object-map null keys respectively), raw MV, mixed raw/dictionary SV and MV, and a regression test for the group-limit misattribution that fails on the previous code by construction. DictionaryBasedGroupKeyGeneratorTest additionally covers IntGroupIdMap.clearAndTrim past the caching threshold.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 69.02174% with 57 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.63%. Comparing base (bb44811) to head (f402575).

Files with missing lines	Patch %	Lines
...pby/NoDictionarySingleColumnGroupKeyGenerator.java	42.85%	28 Missing and 8 partials ⚠️
...upby/NoDictionaryMultiColumnGroupKeyGenerator.java	78.72%	6 Missing and 4 partials ⚠️
...tion/groupby/DictionaryBasedGroupKeyGenerator.java	88.88%	1 Missing and 6 partials ⚠️
...egation/groupby/GroupingSetsGroupKeyGenerator.java	50.00%	1 Missing and 3 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #19390      +/-   ##
============================================
+ Coverage     67.59%   67.63%   +0.04%     
  Complexity     1430     1430              
============================================
  Files          3487     3487              
  Lines        224320   224326       +6     
  Branches      35417    35408       -9     
============================================
+ Hits         151623   151721      +98     
+ Misses        60679    60574     -105     
- Partials      12018    12031      +13     

Flag	Coverage Δ
integration	100.00% <ø> (ø)
integration1	100.00% <ø> (ø)
integration2	0.00% <ø> (ø)
java-25	67.63% <69.02%> (+0.04%)	⬆️
lane-a	100.00% <ø> (ø)
lane-b	0.00% <ø> (ø)
temurin	67.63% <69.02%> (+0.04%)	⬆️
unittests	67.63% <69.02%> (+0.04%)	⬆️
unittests1	57.74% <69.02%> (+0.03%)	⬆️
unittests2	39.35% <0.00%> (+0.02%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Jackie-Jiang]

Thanks — both confirmed and fixed in c30ab5d.

1. Dictionary-backed transforms collapse NULL into the default value. Correct, and the diagnosis is exact. ColumnContext.fromTransformFunction passes a null data source while setting dictionaryEncoded from the transform's own dictionary, and TransformBlockValSet.getNullBitmap() delegates to BaseTransformFunction.getNullBitmap, which ORs its arguments' bitmaps — so filterMv and the other dictionary-exposing transforms hand out nulls with no data source to inspect.

The nullability check is now conservative in the right direction: a column read from a segment can prove it has no nulls via its null value vector, so it still reserves nothing; anything without a data source reserves an id.

boolean tracksNulls = dataSource == null || dataSource.getNullValueVector() != null;

Regression test: testGroupByDictionaryBackedTransformKeepsNullApartFromTheDefaultNullValue, grouping by filterMv(col, 'v <= 1') over rows holding 1, NULL, and the INT default null value, asserting three distinct groups.

2. Primitive null groups excluded from group accounting. Confirmed, and both methods now return _numGroups, which counts map insertions and the external null group alike (for the object-keyed types the null key lives in the map, so the two were already equal).

One correction to the attribution, which matters for backporting: this is pre-existing on master, not new to the MV path. getKeyForNullValue() allocates outside _groupKeyMap in master's single-value path too (9 call sites), and master's getNumKeys() already returned _groupKeyMap.size(). The same ArrayIndexOutOfBoundsException reproduces on master with single-value data of that shape — this PR made it newly reachable from MV, which is presumably why it surfaced here.

Regression test: testNullGroupIsCountedWhenSizingTheResultHolder. Rather than the 10k-row block-boundary shape, it sets maxInitialResultHolderCapacity=2 with three rows — ensureCapacity grows by max(capacity * 2, capacity), so the under-reported capacityNeeded=2 is a no-op against an initial capacity of 2 and the null group's id runs off the end. Same failure, three rows.

[Jackie-Jiang]

Confirmed and fixed in 2cd84a8.

resolveColumnValueIds delegates its single-value branch to the null-aware resolveColumnIds, but the multi-value branch resolved every element straight through the on-the-fly dictionary without consulting the null bitmap. Null rows now take the reserved null component, mirroring the other MV generators:

if (_nullHandlingEnabled) {
  RoaringBitmap nullBitmap = blockValSet.getNullBitmap();
  if (nullBitmap != null && !nullBitmap.isEmpty()) {
    PeekableIntIterator nullIterator = nullBitmap.getIntIterator();
    while (nullIterator.hasNext()) {
      ids[nullIterator.next()] = _nullComponent;
    }
  }
}

Reusing the existing _nullComponent is safe and, I think, the right shape here: it already stands for a NULL key component when a grouping set excludes a column, it is already shared across every row and set, and expandGroupIds copies it into a fresh scratch array rather than writing through it. A column excluded from a set and a column whose value is NULL both render as NULL, which is why GROUPING() exists to tell them apart — so one component serving both roles matches the semantics. I extended the field's javadoc to say so.

Regression test: testGroupingSetsOverMultiValueColumnHoldingNulls, using your repro shape.

Same note as before on provenance: this one is also pre-existing on master — its MV resolver has no null-bitmap reference either. My "every group key generator is null-aware" wording in the description was wrong as written, since the grouping-sets MV path was outside the change; it is accurate now that this is fixed.

[Jackie-Jiang]

All four applied in f402575. Thanks for the careful pass — the FilteredGroupByOperator one in particular I would not have found.

Dictionary path for dict-encoded columns beside raw ones. Done — the selection is now just columnContext.isDictionaryEncoded() ? getDictionary() : null. I checked the four places it has to hold: the int[] arms already pass ids through when there is no on-the-fly dictionary, ID_FOR_NULL is -2 so it cannot collide with a dictionary id, buildKeysFromIds already branches on _dictionaries[i] != null, and the at-limit getId path stays correct because a dictionary id is always a known value while group novelty is decided by _groupKeyMap. Class javadoc now says nullness is read from the bitmap rather than from the id.

Shared generator across project operators. Comment added at the decision point, recording that FilteredGroupByOperator builds one generator and feeds it blocks from several project operators, that those operators must agree on which group-by columns can produce a null, and why they do today.

Unit coverage with true. This needed the fixture to grow a nullable column first — the segment had no nulls, so passing true would have reserved nothing and asserted nothing. Added an all-null n1 plus setDefaultNullHandlingEnabled(true) so the null value vector is actually written, then three tests: ArrayBased (asserts the upper bound is 2 — one dictionary value plus the reserved id, which is the most direct evidence the reservation happened), and LongMapBased and ArrayMapBased as you asked. Each asserts every group key reads the column back as SQL NULL, and the map-based ones assert their thread-local map is cleared on close. Existing tests are untouched: they pass their own column lists and the other columns still have no nulls.

Empty MV array. Added to the description under behavior changes, framed as you put it — one NULL group is not the Postgres answer, it is the closest one reachable given that Pinot ingests [] as null and a segment cannot represent an empty multi-value row.

On the optimizeMaxInitialResultHolderCapacity interaction: agreed it belongs to #19379. Worth noting for merge order that null handling was routing around that bug on master, so this PR removes the shield — #19379 should land first, or together.

[xiangfu0]

Documentation follow-up: pinot-contrib/pinot-docs#1026 (merged).