github-actions[bot] commented on code in PR #63366:
URL: https://github.com/apache/doris/pull/63366#discussion_r3450954662
##########
fe/fe-core/src/main/java/org/apache/doris/planner/RepeatNode.java:
##########
@@ -86,11 +90,30 @@ public String getNodeExplainString(String detailPrefix,
TExplainLevel detailLeve
// Determined by its child.
@Override
- public boolean isSerialOperator() {
- return children.get(0).isSerialOperator();
+ public boolean isSerialNode() {
+ return children.get(0).isSerialNode();
}
public GroupingInfo getGroupingInfo() {
return groupingInfo;
}
+
+ @Override
+ public Pair<PlanNode, LocalExchangeType> enforceAndDeriveLocalExchange(
+ PlanTranslatorContext translatorContext, PlanNode parent,
LocalExchangeTypeRequire parentRequire) {
+ // REPEAT (rollup/grouping sets) is NOT distribution-preserving: it
NULLs grouping
+ // columns per set and produces GROUPING_ID, which is part of the
downstream agg hash
+ // key but does not exist below the repeat. Forwarding the parent HASH
require down
+ // would push the local exchange before the row expansion AND hash by
the child
+ // distribution (a single upstream shuffle key) instead of the agg
grouping_exprs,
+ // collapsing rows onto one instance (tpcds q67, +73%). Recurse with
noRequire so the
+ // parent inserts its hash local exchange ABOVE the repeat using its
own grouping_exprs
+ // (mirrors BE, whose RepeatOperatorX has a NOOP
required_data_distribution).
+ Pair<PlanNode, LocalExchangeType> enforceResult
+ = enforceRequire(translatorContext, children.get(0), 0,
+ LocalExchangeTypeRequire.noRequire());
+ children = new java.util.ArrayList<>();
+ children.add(enforceResult.first);
Review Comment:
Repeat is not distribution-preserving here, but this return value forwards
the child's distribution to the parent. A reduced failing shape is:
```text
Aggregation(requireHash by post-repeat grouping exprs/grouping id)
Repeat
child(output already BUCKET_HASH_SHUFFLE or
GLOBAL_EXECUTION_HASH_SHUFFLE)
```
The method intentionally recurses with `noRequire()` so the parent hash
requirement is not pushed below the row-expanding `RepeatNode`. However, if the
child already reports a hash distribution, `enforceResult.second` is hash, and
the parent `enforceRequire(..., requireHash())` can treat the Repeat output as
satisfying its requirement and skip the local exchange above Repeat. Rows are
then partitioned by pre-repeat child keys instead of the post-repeat grouping
keys/grouping id. Please make Repeat report `NOOP` upward so any parent hash
requirement is enforced above it, and add a hash-child unit case because the
current test only covers a NOOP child.
```suggestion
return Pair.of(this, LocalExchangeType.NOOP);
```
--
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]
