github-actions[bot] commented on code in PR #65968:
URL: https://github.com/apache/doris/pull/65968#discussion_r3643809461
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/NormalizeRepeat.java:
##########
@@ -213,7 +213,7 @@ private static LogicalAggregate<Plan>
normalizeRepeat(LogicalRepeat<Plan> repeat
normalizedRepeatOutput = normalizedRepeatOutput.stream()
.filter(expr -> !expr.equals(groupingId))
.collect(Collectors.toList());
- LogicalRepeat<Plan> normalizedRepeat = repeat.withNormalizedExpr(
+ LogicalRepeat<Plan> normalizedRepeat = repeat.withGroupingIdValues(
Review Comment:
[P1] Preserve the precomputed IDs during MV normalization
When the generated two-consumer CTE is inlined
(`inline_cte_referenced_threshold >= 2` or `cte_inline_mode >= 1`), a CBO
materialized-view rewrite can target this residual Aggregate because it retains
the Repeat in `sourceRepeat`. `AbstractMaterializedViewAggregateRule` rebuilds
that Repeat and calls `doNormalize`, but this no-values overload discards
`groupingIdValues`. For original sets `[(a,b),(a,b,c),(a,b,c,d),()]`, retained
IDs `[3,1,15]` are recomputed as `[1,0,7]`, so an upper consumer sees different
`GROUPING_ID` values and `0` collides with the direct max-set branch. Please
carry the optional list through both MV reconstruction and this normalization
step.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/rewrite/DecomposeRepeatWithPreAggregation.java:
##########
@@ -151,45 +151,48 @@ public Plan visitLogicalAggregate(LogicalAggregate<?
extends Plan> aggregate, Di
LogicalRepeat<Plan> repeat = (LogicalRepeat<Plan>) aggregate.child();
List<List<Expression>> newGroupingSets = new ArrayList<>();
+ List<Long> originalGroupingIdValues =
repeat.toShapes().computeGroupingIdValue();
Review Comment:
[P1] Reuse existing IDs when a residual Repeat is decomposed again
With the generated CTE inlined and successfully rewritten by CBO MV,
`AbstractMaterializedViewRule` immediately runs the whole-tree rewriter again,
so this rule can see a Repeat that already has precomputed IDs. For
`ROLLUP(a,b,c,d,e)`, the first split retains `[1,3,7,15,31]`; recomputing from
the reduced four-column shapes here produces `[0,1,3,7,15]`, so the next direct
branch emits `0` instead of `1`. Initialize from `repeat.getGroupingIdValues()`
when present and only fall back to `toShapes().computeGroupingIdValue()` for an
ordinary Repeat.
##########
fe/fe-core/src/test/java/org/apache/doris/nereids/rules/rewrite/DecomposeRepeatWithPreAggregationTest.java:
##########
@@ -484,11 +487,14 @@ public void testConstructRepeat() throws Exception {
new CTEId(1), "", new LogicalCTEProducer<>(new CTEId(1),
emptyRelation));
List<NamedExpression> groupingFunctionSlots = new ArrayList<>();
LogicalRepeat<Plan> result = (LogicalRepeat<Plan>) method.invoke(rule,
- originalRepeat, consumer, newGroupingSets,
producerToConsumerSlotMap, groupingFunctionSlots);
+ originalRepeat, consumer, newGroupingSets,
ImmutableList.of(1L, 3L),
+ producerToConsumerSlotMap, groupingFunctionSlots);
Assertions.assertNotNull(result);
Assertions.assertEquals(2, result.getGroupingSets().size());
Assertions.assertTrue(groupingFunctionSlots.isEmpty());
+ Assertions.assertEquals(ImmutableList.of(1L, 3L),
result.getGroupingIdValues().get());
Review Comment:
[P2] Add an enabled end-to-end regression for the fixed path
These assertions only inject an arbitrary ID list into the private helper
and verify that it was stored. They do not run the decomposition through the
residual/direct Projects, positional Union mapping, physical Repeat
translation, and an upper consumer. The dedicated `decompose_repeat` regression
suite disables `DECOMPOSE_REPEAT` for its nested/grouping-function result
cases, so it does not cover this result-producing path either. Please add an
enabled result test covering a max-set-not-first or duplicate-set case with a
nested upper plan, and compare the decomposed result with the rule disabled.
--
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]
