xiangfu0 commented on code in PR #18817:
URL: https://github.com/apache/pinot/pull/18817#discussion_r3476186439
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/planner/physical/v2/opt/rules/AggregatePushdownRule.java:
##########
@@ -94,6 +98,33 @@ public PRelNode onMatch(PRelOptRuleCall call) {
hintOptions = Map.of();
}
boolean isInputExchange = call._currentNode.unwrap().getInput(0)
instanceof Exchange;
+ if (aggRel.getGroupType() != Aggregate.Group.SIMPLE) {
Review Comment:
Good catch — the v2 grouping-set branch now checks `withinGroupCollation`
before the split, matching the default planner's `createPlan`. Since the DIRECT
fallback does not preserve the ORDER BY across the per-set expansion either,
the v2 planner now rejects `WITHIN GROUP` ordered aggregates under `GROUPING
SETS` / `ROLLUP` / `CUBE` with an explicit error (run on the single-stage
engine) instead of silently producing a wrong list. Added
`testV2PhysicalPlannerRejectsWithinGroupRollup` (4091cff).
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
pinot-query-planner/src/main/java/org/apache/pinot/calcite/rel/logical/PinotLogicalAggregate.java:
##########
@@ -92,6 +97,35 @@ public PinotLogicalAggregate copy(RelTraitSet traitSet,
RelNode input, Immutable
_leafReturnFinalResult, _collations, _limit);
}
+ /// Whether this LEAF aggregate must synthesize the {@code $groupingId}
discriminator column. Only a LEAF
+ /// grouping-set aggregate does: it is converted to a single-stage query
that expands each row across the
+ /// grouping sets and appends {@code $groupingId}; the multi-stage final
stage then groups on it.
+ public boolean emitsGroupingId() {
+ return _aggType == AggType.LEAF && getGroupType() != Group.SIMPLE;
+ }
+
+ @Override
+ protected RelDataType deriveRowType() {
+ RelDataType rowType = super.deriveRowType();
+ if (!emitsGroupingId()) {
+ return rowType;
+ }
+ // Insert the synthetic $groupingId INT column right after the union
group-by columns (before the aggregate
+ // columns), matching the single-stage leaf output layout: [group keys...,
$groupingId, aggregates...].
+ RelDataTypeFactory typeFactory = getCluster().getTypeFactory();
+ RelDataTypeFactory.Builder builder = typeFactory.builder();
+ List<RelDataTypeField> fields = rowType.getFieldList();
+ int groupCount = getGroupCount();
+ for (int i = 0; i < groupCount; i++) {
+ builder.add(fields.get(i));
+ }
+ builder.add(GroupingSets.GROUPING_ID_COLUMN,
typeFactory.createSqlType(SqlTypeName.INTEGER));
Review Comment:
The grouping-set feature ships entirely within a single release, so brokers
and servers are upgraded together — there is no mixed-version window where an
older server would see `AggregateNode.groupingSets` from a newer broker. MSE
has no per-feature capability gate today and relies on lockstep upgrades for
plan-shape changes (e.g. unknown plan-node types fail fast at deserialization);
the upgrade ordering is also noted on the proto field as a backstop.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
--
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]
