Copilot commented on code in PR #60045:
URL: https://github.com/apache/doris/pull/60045#discussion_r2710692691
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/glue/translator/PhysicalPlanTranslator.java:
##########
@@ -2539,36 +2539,39 @@ public PlanFragment
visitPhysicalRepeat(PhysicalRepeat<? extends Plan> repeat, P
PlanFragment inputPlanFragment = repeat.child(0).accept(this, context);
List<List<Expr>> distributeExprLists =
getDistributeExprs(repeat.child(0));
- ImmutableSet<Expression> flattenGroupingSetExprs = ImmutableSet.copyOf(
- ExpressionUtils.flatExpressions(repeat.getGroupingSets()));
+ Set<NamedExpression> outputSets =
ImmutableSet.copyOf(repeat.getOutputExpressions());
+ List<Expression> flattenGroupingExpressions =
repeat.getGroupByExpressions();
+ Set<Slot> preRepeatExpressions = Sets.newLinkedHashSet();
+ // keep group by expression comes first
Review Comment:
The comment has a grammatical error. It should be "keep group by expressions
coming first" or "keep group by expression slots first" to be grammatically
correct.
```suggestion
// keep group by expressions coming first
```
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/glue/translator/PhysicalPlanTranslator.java:
##########
@@ -2539,36 +2539,39 @@ public PlanFragment
visitPhysicalRepeat(PhysicalRepeat<? extends Plan> repeat, P
PlanFragment inputPlanFragment = repeat.child(0).accept(this, context);
List<List<Expr>> distributeExprLists =
getDistributeExprs(repeat.child(0));
- ImmutableSet<Expression> flattenGroupingSetExprs = ImmutableSet.copyOf(
- ExpressionUtils.flatExpressions(repeat.getGroupingSets()));
+ Set<NamedExpression> outputSets =
ImmutableSet.copyOf(repeat.getOutputExpressions());
+ List<Expression> flattenGroupingExpressions =
repeat.getGroupByExpressions();
+ Set<Slot> preRepeatExpressions = Sets.newLinkedHashSet();
+ // keep group by expression comes first
+ for (Expression groupByExpr : flattenGroupingExpressions) {
Review Comment:
The unsafe cast from Expression to Slot lacks validation or documentation.
While the grouping sets should be normalized to slots during analysis, an
explicit check or comment explaining this assumption would improve code safety
and maintainability. Consider adding a check like
'Preconditions.checkArgument(groupByExpr instanceof Slot)' or a comment
explaining why this cast is safe.
```suggestion
for (Expression groupByExpr : flattenGroupingExpressions) {
Preconditions.checkArgument(groupByExpr instanceof Slot,
"Group by expressions in PhysicalRepeat must be Slot,
but got: %s", groupByExpr);
```
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/glue/translator/PhysicalPlanTranslator.java:
##########
@@ -2539,36 +2539,39 @@ public PlanFragment
visitPhysicalRepeat(PhysicalRepeat<? extends Plan> repeat, P
PlanFragment inputPlanFragment = repeat.child(0).accept(this, context);
List<List<Expr>> distributeExprLists =
getDistributeExprs(repeat.child(0));
- ImmutableSet<Expression> flattenGroupingSetExprs = ImmutableSet.copyOf(
- ExpressionUtils.flatExpressions(repeat.getGroupingSets()));
+ Set<NamedExpression> outputSets =
ImmutableSet.copyOf(repeat.getOutputExpressions());
+ List<Expression> flattenGroupingExpressions =
repeat.getGroupByExpressions();
+ Set<Slot> preRepeatExpressions = Sets.newLinkedHashSet();
+ // keep group by expression comes first
+ for (Expression groupByExpr : flattenGroupingExpressions) {
+ Slot groupBySlot = (Slot) groupByExpr;
+ Preconditions.checkState(outputSets.contains(groupBySlot));
+ preRepeatExpressions.add(groupBySlot);
+ }
- List<Slot> aggregateFunctionUsedSlots = repeat.getOutputExpressions()
- .stream()
- .filter(output -> !flattenGroupingSetExprs.contains(output))
- .filter(output ->
!output.containsType(GroupingScalarFunction.class))
- .distinct()
- .map(NamedExpression::toSlot)
+ // add aggregate function used expressions
+ for (NamedExpression outputExpr : repeat.getOutputExpressions()) {
+ if (!outputExpr.containsType(GroupingScalarFunction.class)) {
+ preRepeatExpressions.add(outputExpr.toSlot());
+ }
+ }
+
+ List<Expr> preRepeatExprs = preRepeatExpressions.stream()
+ .map(expr -> ExpressionTranslator.translate(expr, context))
.collect(ImmutableList.toImmutableList());
- // keep flattenGroupingSetExprs comes first
- List<Expr> preRepeatExprs =
Stream.concat(flattenGroupingSetExprs.stream(),
aggregateFunctionUsedSlots.stream())
- .map(expr -> ExpressionTranslator.translate(expr,
context)).collect(ImmutableList.toImmutableList());
-
- // outputSlots's order need same with preRepeatExprs
- List<Slot> outputSlots = Stream.concat(Stream
- .concat(repeat.getOutputExpressions().stream()
- .filter(output ->
flattenGroupingSetExprs.contains(output)),
- repeat.getOutputExpressions().stream()
- .filter(output ->
!flattenGroupingSetExprs.contains(output))
- .filter(output ->
!output.containsType(GroupingScalarFunction.class))
- .distinct()
- ),
-
Stream.concat(Stream.of(repeat.getGroupingId().toSlot()),
- repeat.getOutputExpressions().stream()
- .filter(output ->
output.containsType(GroupingScalarFunction.class)))
- )
-
.map(NamedExpression::toSlot).collect(ImmutableList.toImmutableList());
+ // outputSlots's order need same with preRepeatExprs, then grouping
id, then grouping function slots
Review Comment:
The comment has a grammatical error. It should be "outputSlots's order needs
to be the same as preRepeatExprs" or "outputSlots's order must match
preRepeatExprs" to be grammatically correct.
```suggestion
// outputSlots's order must match preRepeatExprs, then grouping id,
then grouping function slots
```
--
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]
