xiangfu0 commented on PR #19264: URL: https://github.com/apache/pinot/pull/19264#issuecomment-6091196215
Thanks for the thorough follow-up — addressed all four points: 1. **Mixed layouts + per-set trim**: the trim now runs in `GroupByCombineOperator.mergeResults` AFTER the full-layout (expansion-path) records are merged into the derived table, exactly as you suggested, so every retained group carries its complete merged value. `deriveGroupingSetsFromMergedBaseTable` no longer trims. Added `testPerSetTrimRunsAfterFullLayoutMerge` reproducing your shape (a group whose base-path share is small enough to be trimmed but whose expansion-path share would resurrect it with a partial total). 2. **Bound too conservative**: added your per-column-union bound as a second chance when the per-segment product sum fails: `fitsByColumnUnionCardinalities` unions each column's dictionary values across segments (via `Dictionary#getInternal`, so BYTES hash correctly) and bounds base/derived groups by products of union cardinalities — segments sharing a value space no longer count the same key once per segment. Guards: it bails per column once the union alone exceeds `numGroupsLimit` (the base product can then never fit), and refuses to scan columns whose total dictionary entries exceed `max(4 * numGroupsLimit, 64K)` so plan-time cost stays bounded (those unions almost certainly exceed the limit anyway). Both proofs remain sound upper bounds; either passing admits the base path. 3. **Observability**: v1 EXPLAIN's `GROUP_BY` node now prints `groupingSetsBaseAggregation:true/false` for grouping-set queries (the plan-level decision: opted in + bound proof passed), and a new server meter `GROUPING_SETS_BASE_AGGREGATION` counts combines that actually derived from base groups (per-segment carve-outs can still fall back individually, which the meter reflects). 4. **Dead code**: removed `deriveCappedGroupingSets` and the `areGroupingSetsDerivedGroupsBounded` plumbing; the derive now relies on the planner's proof outright. -- 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]
