yashmayya commented on code in PR #19264:
URL: https://github.com/apache/pinot/pull/19264#discussion_r4224732664
##########
pinot-core/src/main/java/org/apache/pinot/core/plan/maker/InstancePlanMakerImplV2.java:
##########
@@ -422,6 +427,89 @@ protected CombinePlanNode
createCombinePlanNode(List<PlanNode> planNodes, QueryC
return new CombinePlanNode(planNodes, queryContext, executorService,
streamer);
}
+ /// Only emit BASE records when every local BASE key and every derived
grouping-set key can fit the combine
+ /// table. Summing each segment's dictionary product is conservative even
when keys overlap across segments.
+ /// Unknown, raw, transformed and multi-value columns use the per-row
expansion path. This also makes a larger
+ /// `groupingSetsBaseAggregationMaxGroups` option unable to bypass the
actual `numGroupsLimit` memory guard.
+ @VisibleForTesting
+ static void boundGroupingSetsBaseAggregation(List<SegmentContext>
segmentContexts, QueryContext queryContext) {
+ if (queryContext.isGroupingSetsBaseAggregation()) {
+ queryContext.setGroupingSetsBaseAggregationAllowed(
+ fitsGroupingSetsBaseAggregationLimit(segmentContexts, queryContext));
+ }
+ }
+
+ @VisibleForTesting
+ static boolean fitsGroupingSetsBaseAggregationLimit(List<SegmentContext>
segmentContexts, QueryContext queryContext) {
+ List<ExpressionContext> groupByExpressions =
queryContext.getGroupByExpressions();
+ List<int[]> groupingSets = queryContext.getGroupingSets();
+ int limit = queryContext.getNumGroupsLimit();
+ if (groupByExpressions == null || groupingSets == null || limit <= 0) {
+ return false;
+ }
+
+ long remainingBase = limit;
+ long remainingDerived = limit;
+ for (SegmentContext segmentContext : segmentContexts) {
+ IndexSegment segment = segmentContext.getIndexSegment();
+ if (segment.getSegmentMetadata().getTotalDocs() == 0) {
+ continue;
+ }
+ long[] cardinalities = new long[groupByExpressions.size()];
+ for (int i = 0; i < cardinalities.length; i++) {
+ ExpressionContext expression = groupByExpressions.get(i);
+ if (expression.getType() != ExpressionContext.Type.IDENTIFIER) {
+ return false;
+ }
+ DataSource dataSource =
segment.getDataSourceNullable(expression.getIdentifier());
+ if (dataSource == null ||
!dataSource.getDataSourceMetadata().isSingleValue()) {
+ return false;
+ }
+ Dictionary dictionary = dataSource.getDictionary();
+ ForwardIndexReader<?> forwardIndex = dataSource.getForwardIndex();
+ if (dictionary == null || forwardIndex == null ||
!forwardIndex.isDictionaryEncoded()) {
+ return false;
+ }
+ long cardinality = dictionary.length();
Review Comment:
**Blocker: consuming segments can make opt-in queries fail**
This bound reads `dictionary.length()` while the plan is built. A consuming
segment's doc count is read later, in `FilterPlanNode.run()`. New keys indexed
in between can push the base groups past `numGroupsLimit`, and
`GroupingSetsBaseIndexedTable` then fails the query.
Repro: `numGroupsLimit=100`, one completed segment (day 1, 60 users), one
consuming segment (day 2, 35 users), `ROLLUP(day, userId)`. The bound passes
(95 base, 99 derived). Then 10 new users are indexed between `makeInstancePlan`
and `execute`:
- Opt-in: error 200, `Grouping-set base groups exceeded the planned group
limit`.
- Expansion: grand total 105, which is exact.
Realtime tables near the limit with a steady flow of new keys (for example,
one day per segment with user IDs) can hit this. The bound is tight for that
shape, so a small amount of new data is enough.
Fix idea: use the expansion path when any segment is mutable, or keep
headroom below the limit for consuming segments.
--
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]
