xiangfu0 commented on PR #19264: URL: https://github.com/apache/pinot/pull/19264#issuecomment-5306251738
Thanks for the review! Addressed all three inline comments in fc597a0: **1. `GroupingSetsGroupKeyGenerator` — shift-by-64 (`$groupingId`) bug.** Long-packing is now disabled when the union columns consume all 64 bits (the discriminator shift would be `>= 64`), so the shift stays strictly below 64 and Java's mod-64 shift masking can't turn a shift of 64 into 0. Those queries fall back to the correct `Object2IntOpenHashMap` path. Good catch on the full-width / single-set boundary. **2. `GroupByUtils` — derived table unbounded by `numGroupsLimit`.** The derived map is now bounded by `numGroupsLimit` *while it is built*: once the limit is reached, brand-new derived keys are skipped while existing groups keep accumulating — mirroring the generator's per-segment cap so the per-set fan-out (up to `numSets` × base groups) can't exhaust heap before the trim runs. The derived fan-out is also folded into the `numGroupsLimitReached` / `numGroupsWarningLimitReached` flags. **3. `BenchmarkGroupingSetsQueriesSSE` — static executor shut down in `@TearDown`.** Replaced with a per-trial executor created in `@Setup` and shut down in `@TearDown`, so its lifecycle matches each JMH trial and later `@Param` combinations don't submit to a shut-down pool. On the suppressed comment about defaulting `groupingSetsBaseAggregation` **off**: it's currently default-on. I'm happy to flip it to default-off for the first release if the maintainers prefer the standard opt-in-then-flip rollout — let me know your preference and I'll update the option default plus the doc/benchmark descriptions. Note: the failing `SegmentDeletionManagerTest.testRemoveDeletedSegments` in "Unit Test Set 2" is a pre-existing flake (120s async-filesystem-deletion timeout in pinot-controller) unrelated to this PR, which touches only pinot-core / pinot-spi / pinot-perf / integration-tests. I re-ran the failed job. -- 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]
