xiangfu0 opened a new pull request, #19379: URL: https://github.com/apache/pinot/pull/19379
## Problem With the query option `optimizeMaxInitialResultHolderCapacity=true`, `DictionaryBasedGroupKeyGenerator` shrinks the group-by cardinality product to the IN/EQ predicate sizes and then uses the shrunk value both for holder-type selection and for `_globalGroupIdUpperBound`. This is unsound in three ways: 1. **`ArrayIndexOutOfBoundsException`**: `ArrayBasedHolder` uses raw dictionary-id mixed-radix products as group ids directly, so its `_flags` array (sized to the shrunk bound) is indexed by ids up to the full cardinality product. Repro: dict INT column `dInt` with cardinality 1000, `SET optimizeMaxInitialResultHolderCapacity=true; SELECT dInt, COUNT(*) FROM t WHERE dInt IN (1,2,3,4,5) GROUP BY dInt` throws `AIOOBE: Index 5 out of bounds for length 5` in `ArrayBasedHolder.markGroups`. 2. **Silent wrong results**: resetting `longOverflow = false` and shrinking the product can downgrade the holder to `IntMapBasedHolder`/`LongMapBasedHolder`, whose raw-key arithmetic still uses the full cardinalities — the keys overflow int/long and distinct groups collide. 3. **Multi-value group-by columns**: a row matching an IN/EQ predicate on an MV column contributes one group per value inside the row, not only the matching values, so the predicate size is not a valid bound on the number of distinct groups (this also affects the two no-dictionary group key generators). ## Fix - Holder-type selection always uses the **full** cardinality product (raw keys span the full mixed-radix space regardless of the filter). The predicate-derived value only caps `_globalGroupIdUpperBound`, which is a valid dense group-count bound for the map-based holders. - When the optimized bound shrinks below the full product, `ArrayBasedHolder` falls back to `IntMapBasedHolder`, which maps the sparse raw keys onto dense group ids. Inside the `ArrayBasedHolder` branch, `_globalGroupIdUpperBound == cardinalityProduct` now holds by construction. This deliberately trades a hash lookup per row for the smaller result holders the opt-in option asks for; the alternative (keep `ArrayBasedHolder` with the full bound whenever the product fits under `arrayBasedThreshold`) would silently turn the option into a no-op for every small-cardinality group-by, changing its documented, tested behavior. With the option off (the default), holder selection is bit-identical to before. - `DefaultGroupByExecutor#getGroupByExpressionSizesFromPredicates` excludes multi-value group-by expressions from the predicate-size map, covering all three group key generators, and `DictionaryBasedGroupKeyGenerator` also self-enforces the exclusion (it computes the bound from its own `_isSingleValueColumn`/`_cardinalities` arrays, which additionally removes the per-query `cardinalityMap` allocation and keeps the bound comparable to the cardinality product when the same expression appears multiple times in the GROUP BY). Known adjacent follow-up (not in this PR): `NoDictionaryMultiColumnGroupKeyGenerator` does not cap its optimized upper bound by `numGroupsLimit` (not a correctness issue — the limit is enforced separately at group creation — but it oversizes result holder max capacity), and `NoDictionaryGroupKeyGeneratorTest` has no coverage with a non-null predicate-size map. ## Tests `DictionaryBasedGroupKeyGeneratorTest` additions (each verified to fail before the fix): - `testOptimizedUpperBoundSmallerThanCardinalityProduct` — the AIOOBE repro; now served by `IntMapBasedHolder`. - `testOptimizedUpperBoundKeepsLongMapBasedHolder` / `testOptimizedUpperBoundKeepsArrayMapBasedHolder` — the optimization must not downgrade the holder type across the int/long overflow boundaries. - `testOptimizedUpperBoundMatchingCardinalityProductKeepsArrayBasedHolder` — no deoptimization when the predicates do not prove fewer groups. - `testOptimizedUpperBoundIgnoresMultiValuePredicates` — MV predicate sizes must not shrink the bound. - The pre-existing `testGetGroupByResultHolderCapacity` now also runs `process()` over the block, so all 12 capacity cases exercise actual key generation (8 of them threw AIOOBE before the fix). -- 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]
