github-actions[bot] commented on code in PR #66222:
URL: https://github.com/apache/doris/pull/66222#discussion_r3673393844
##########
fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.java:
##########
@@ -1408,6 +1410,9 @@ public void checkQuerySlotCount(String slotCnt) {
@VarAttrDef.VarAttr(name = ENABLE_LOCAL_EXCHANGE_BEFORE_AGG, fuzzy = true)
public boolean enableLocalExchangeBeforeAgg = true;
+ @VarAttrDef.VarAttr(name = ENABLE_LOCAL_EXCHANGE_BEFORE_STREAMING_AGG,
fuzzy = true)
Review Comment:
[P1] Forward this query-affecting variable to the master
`getForwardVariables()` only includes fields marked `needForward` or one of
the query-result-affect flags, and observer queries sent by `FEOpExecutor` are
planned under a fresh master context. Because this annotation sets none of
them, `SET enable_local_exchange_before_streaming_agg=true` on a follower is
omitted and the master keeps false, so the knob silently changes behavior
depending on the connected FE. Please add `needForward=true` and cover
follower-to-master restoration.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.java:
##########
@@ -5719,6 +5725,7 @@ public TQueryOptions toThrift() {
tResult.setEnableDistinctStreamingAggregation(enableDistinctStreamingAggregation);
tResult.setEnableStreamingAggHashJoinForcePassthrough(enableStreamingAggHashJoinForcePassthrough);
tResult.setEnableLocalExchangeBeforeAgg(enableLocalExchangeBeforeAgg);
+
tResult.setEnableLocalExchangeBeforeStreamingAgg(enableLocalExchangeBeforeStreamingAgg);
Review Comment:
[P1] Make the FE planner honor this dedicated switch
With `enable_local_shuffle_planner=true` (the default), `NereidsPlanner`
inserts local exchanges in FE and `RuntimeState::plan_local_shuffle()` prevents
BE from replanning them. `AggregationNode.enforceAndDeriveLocalExchange()`
still gates the `useStreamingPreagg` branch on `enableLocalExchangeBeforeAgg`,
so new=false/old=true still inserts a hash exchange and new=true/old=false
suppresses it—the new variable has no effect on the default path. Please use
`enableLocalExchangeBeforeStreamingAgg` in that branch, retain the old flag for
the other aggregation operators, preserve the non-hash-child reshuffle
invariant, and add opposite-value FE/BE planner tests plus direct BE branch
coverage.
--
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]
