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