Jackie-Jiang commented on PR #19390: URL: https://github.com/apache/pinot/pull/19390#issuecomment-5505570506
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. -- 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]
