github-actions[bot] commented on code in PR #66104:
URL: https://github.com/apache/doris/pull/66104#discussion_r3662305710
##########
be/src/exec/operator/distinct_streaming_aggregation_operator.h:
##########
@@ -118,6 +118,11 @@ class DistinctStreamingAggOperatorX final
if (_needs_finalize && _probe_expr_ctxs.empty()) {
return {ExchangeType::NOOP};
}
+ if (!_needs_finalize && !state->enable_local_exchange_before_agg() &&
Review Comment:
[P1] Do not bypass HASH for non-streaming DISTINCT dedup
When the knob is false, `_needs_finalize` is false, `_is_streaming_preagg`
is false, and the child is non-serial/NOOP, this new branch returns NOOP before
the existing non-streaming-dedup branch below can require HASH. That stage must
co-locate equal keys for correctness; only streaming preaggregation is safe to
opt out. PR #63366 encodes the same distinction with a `useStreamingPreagg`
guard. Please require `_is_streaming_preagg` here and add a safe-child
non-streaming-dedup test.
##########
be/src/exec/operator/aggregation_sink_operator.h:
##########
@@ -160,6 +160,11 @@ class AggSinkOperatorX MOCK_REMOVE(final) : public
DataSinkOperatorX<AggSinkLoca
:
DataSinkOperatorX<AggSinkLocalState>::required_data_distribution(
state);
}
+ const bool child_breaks_distribution =
child_breaks_local_key_distribution(state);
+ if (!_needs_finalize && !state->enable_local_exchange_before_agg() &&
Review Comment:
[P1] Keep the correctness shuffle for FIRST_MERGE
This early return treats every non-finalizing aggregation as an optional
local preaggregation. On this branch, however, `AggPhase.GLOBAL` maps to
`FIRST_MERGE` and can have `need_finalize=false`; with a non-serial/NOOP child,
`child_breaks_distribution` is false and this returns NOOP. Equal serialized
keys can then be merged by different local tasks and produce wrong
aggregate/DISTINCT results. Merged PR #63366 documents this exact phase
distinction. Please restrict the opt-out to non-merge phases and make
merge-phase metadata available before local-exchange planning (the current
`_is_merge` is populated only in `prepare()`), rather than relying only on the
child-break check.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.java:
##########
@@ -1373,6 +1374,9 @@ public void checkQuerySlotCount(String slotCnt) {
@VariableMgr.VarAttr(name =
ENABLE_STREAMING_AGG_HASH_JOIN_FORCE_PASSTHROUGH, fuzzy = true)
public boolean enableStreamingAggHashJoinForcePassthrough = true;
+ @VariableMgr.VarAttr(name = ENABLE_LOCAL_EXCHANGE_BEFORE_AGG, fuzzy = true)
Review Comment:
[P2] Forward this session setting to the executing FE
`getForwardVariables()` only includes fields marked `needForward` (or one of
the affect-result flags), and `FEOpExecutor` sends only that filtered map when
an observer forwards a query to the master. A client that ran `SET
enable_local_exchange_before_agg=false` on the observer therefore executes the
forwarded query with the master's default `true`, silently ignoring the
requested opt-out. Please add `needForward = true` and cover the false value
through `getForwardVariables()`/`setForwardedSessionVariables()`.
--
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]
