mrhhsg commented on code in PR #68651:
URL: https://github.com/apache/doris/pull/68651#discussion_r4151661098
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/properties/ChildrenPropertiesRegulator.java:
##########
@@ -151,6 +151,19 @@ private boolean
shouldBanOnePhaseAgg(PhysicalHashAggregate<? extends Plan> aggre
// group by key is skew
return skewOnShuffleExpr(aggregate);
} else {
+ // Bucketed hash agg exception: allow one-phase GLOBAL + distribute
+ // pattern so the translator can fuse it into
BucketedAggregationNode.
+ // Only aggregates with the shape the translator actually fuses
qualify;
+ // the others would keep their exchange and stay banned as before.
That
+ // includes a distribute on the parent's keys
(agg_shuffle_use_parent_key),
+ // a strict subset of the GROUP BY keys, which the translator
never fuses.
+ // Gate with data-volume checks using group-level statistics to
avoid
+ // generating this pattern when bucketed agg is unsuitable.
+ DistributionSpec childDistribution
+ = ((PhysicalDistribute<?>)
children.get(0).getPlan()).getDistributionSpec();
+ if (AggregateUtils.isBucketedHashAggFusible(aggregate,
childDistribution)) {
Review Comment:
Valid, fixed in f6519987a42.
I reproduced both shapes on 8b5360adb91 with FE unit tests (default
optimizer choice, `agg_phase=0`):
- nested aggregate: `OlapScan -> BUCKETED AGGREGATE (inner) -> EXCHANGE(HASH
k) -> AGGREGATE (outer, one-phase)`, so every inner row was exchanged;
- projected CTE consumer: a single `AGGREGATE` reading the
`HASH_PARTITIONED` exchange of the consumer, no local phase.
What changed:
- `AggregateUtils.isBucketedHashAggFusible(GroupExpression,
PhysicalProperties)` evaluates the translator's child condition on the memo:
the child chosen for the aggregate must be a distribute on exactly the GROUP BY
keys, and following the lowest cost plan of each group below it must reach an
olap scan through unary nodes only. It rejects the same nodes as the translator
(join, set operation, CTE consumer/anchor, nested aggregate, storage layer
aggregate); `isSingleOlapScanPipeline` moved from `PhysicalPlanTranslator` into
`AggregateUtils` so the plan-tree and memo versions share one list.
- `ChildrenPropertiesRegulator` uses it for the exemption, so both shapes
are banned again as they were before bucketed aggregation existed.
- The discount had the same gap for shapes the regulator does not ban, e.g.
a one-phase aggregate reading a CTE consumer directly (allowed by the
pre-existing CTE branch) or one without any distribute. `CostCalculator` now
evaluates the same gate and `CostModel` only discounts an aggregate that is
fused, so an unfused aggregate costs the same with bucketed aggregation enabled
or disabled.
Tests:
-
`BucketedAggregateTranslatorTest.testAggregateOverNestedAggregateIsNotExemptedAsBucketed`
and `testAggregateOverProjectedCteConsumerIsNotExemptedAsBucketed` assert the
default choice for the two shapes; both fail on 8b5360adb91 with the plans
above. The nested case also asserts that the inner aggregate is still fused.
- `testOnlyFusedAggregateGetsBucketedCostDiscount` compares the best plan
cost with bucketed aggregation on and off; it fails if only the regulator part
is applied (the CTE consumer plan is still discounted).
- `bucketed_hash_agg` Test 12 compares the results of both shapes with
bucketed aggregation on and off.
- On a single-BE cluster with a 300k-row analyzed table and default session
variables, the outer aggregate of the nested query now pre-aggregates before
its exchange (the inner one is fused), and the projected-CTE query has the same
shape as with `enable_bucketed_hash_agg=false`.
Not covered: the translator also keeps an aggregate unfused when a
fragment-merging parent (join / set operation) consumes it without an exchange.
That depends on the parent, which is not visible when the aggregate is costed,
so it is still decided only in the translator.
--
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]
