Repository navigation
[fix](fe) Forward four local exchange session variables to the master - #70197
Conversation
### What problem does this PR solve? Issue Number: None Related PR: apache#69273, apache#70177 Problem Summary: A statement that has to run on the master FE (INSERT INTO ... SELECT, CTAS, ...) and arrives at a follower or an observer is forwarded with the session variables that `SessionVariable#getForwardVariables()` sends, and the master applies them to a session that starts from its own global variables. Only variables whose `@VarAttr` sets `needForward` are sent. apache#69273 flagged the planner and executor variables, `enable_local_exchange_before_agg` among them, but not four that also decide the local exchanges of a plan: `enable_local_exchange_before_streaming_agg` (a local hash exchange before a streaming pre-aggregation, BE `StreamingAggOperatorX` and the FE planner with apache#70177), `enable_streaming_agg_hash_join_force_passthrough` and `enable_distinct_streaming_agg_force_passthrough` (a passthrough local exchange below a streaming pre-aggregation, FE `AggregationNode` and the BE operators), and `force_to_local_shuffle` (every scan serial, `ScanNode#isSerialNode`). A session-level SET of one of them was dropped when the statement was forwarded, so the master planned it with its global value and EXPLAIN on the follower showed a plan the master did not run. Results do not change, only the plan and its speed. The four now set `needForward = true`, as apache#69273 does for the others. ### Release note The session variables enable_local_exchange_before_streaming_agg, enable_streaming_agg_hash_join_force_passthrough, enable_distinct_streaming_agg_force_passthrough and force_to_local_shuffle now apply to statements that a follower or observer FE forwards to the master. ### Check List (For Author) - Test: Unit Test - `SessionVariablesTest#testForwardPlanAndExecutionKnobs` lists the four; with `SessionVariable.java` reverted it fails. The test classes that call `getForwardVariables()` or `setForwardedSessionVariables()` pass, and fe-core Checkstyle reports nothing. Not run on a cluster with two FEs. - Behavior changed: Yes. A forwarded statement uses the session's value of these four variables instead of the master's global value. - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
TPC-H: Total hot run time: 28659 ms |
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
TPC-DS: Total hot run time: 151502 ms |
ClickBench: Total hot run time: 24.32 s |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Static review of PR 70197 at head 097b15b found no verified new findings. The four needForward annotations consistently add the existing FE statement forwarding path: getForwardVariables() sends them in TMasterOpRequest, proxy execution applies them to a fresh context, and the three aggregation values continue to flow through the existing TQueryOptions assignments. force_to_local_shuffle is an FE planner control consumed by ScanNode and now survives follower-to-master planning. Rolling-version handling remains compatible because forwarded values are looked up only for fields known by the receiving FE. The added test constants cover all four forwarding keys in the existing forwarding-map check. No existing P0/P1 inline finding applies, and there were no prior inline threads. FE Core checkpoints are unaffected: the patch changes no locking, exception/RPC boundary, or visible-version logic. The review focus file contains no additional user focus. No builds or tests were executed; this was a static review as required. Convergence is false because the dedicated risk-focused child review could not start: the original child and all replacement attempts returned model-capacity errors before writing results. Main-agent rechecks covered that risk scope, but the delegated convergence condition was not met within the three-round bound.
Review did not converge after 3 rounds. This automated review cannot pass the review check.
The dedicated risk-focused subagent required for strict convergence was unavailable: the original and replacement child attempts all failed with model-capacity errors before producing ledger results. Main-agent verification found no candidate, and the complete-file reviewer found no candidate, but delegated convergence could not be proven by round 3.
|
/review |
|
Codex automated review failed and did not complete. Error: Selected model is at capacity. Please try a different model. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
|
/review |
There was a problem hiding this comment.
Reviewed apache/doris PR 70197 at the verified exact head 097b15b against base 0eb5131. The patch adds needForward=true to enable_streaming_agg_hash_join_force_passthrough, enable_local_exchange_before_streaming_agg, enable_distinct_streaming_agg_force_passthrough, and force_to_local_shuffle, and adds all four names to SessionVariablesTest.
The forwarding path was traced from SessionVariable.getForwardVariables() through FEOpExecutor.buildStmtForwardParams() and ConnectProcessor.proxyExecute() into the master session. The three aggregation/local-exchange flags already populate TQueryOptions and have matching BE RuntimeState consumers; force_to_local_shuffle affects ScanNode.isSerialNode(), and its enableLocalShuffle and enableNereidsPlanner companion inputs are already forwarded. Private-field reflection and mixed-version name-map handling are safe. No accepted, duplicate, or unresolved findings remain, and there are no existing inline review threads or P0/P1 findings to carry forward.
Critical review checkpoints: the change is focused and meets its stated forwarding goal; no new concurrency, lifecycle, persistence, security, Thrift/Protobuf, or FE-BE schema behavior is introduced; direct and forwarded statement paths were inspected; the annotation-driven unit-test coverage was inspected. This was a static review under the contract, so no build or runtime tests were executed. The user focus file contains no additional focus points. Both independent review passes returned NO_NEW_VALUABLE_FINDINGS, so this review converged in round 1.
Review converged after 1 round(s).
What problem does this PR solve?
Issue Number: None
Related PR: #69273, #70177
Problem Summary:
In short. Found while bringing branch-4.2 #66216 to the FE local shuffle planner (#70177): four session variables that decide the local exchanges of a plan are not forwarded to the master. #69273 made the planner and executor variables travel with a statement that a follower or an observer forwards to the master,
enable_local_exchange_before_aggamong them, but notenable_local_exchange_before_streaming_agg,enable_streaming_agg_hash_join_force_passthrough,enable_distinct_streaming_agg_force_passthroughandforce_to_local_shuffle. A session-levelSETof one of them is dropped on that path, and the master plans the statement with its own global value. This PR forwards the four as well.Background. A statement that has to run on the master FE (
INSERT INTO ... SELECT,CREATE TABLE ... AS SELECT, ...) and arrives at a follower or an observer is forwarded:FEOpExecutorsendsSessionVariable#getForwardVariables(), andConnectProcessoron the master applies them withsetForwardedSessionVariables()to a session that starts from the master's global variables. Only variables whose@VarAttrsetsneedForward(or one of theaffectQueryResult*attributes) are sent.The problem, and what it cost. These four do not set it:
enable_local_exchange_before_streaming_agg(#66222)StreamingAggOperatorX, and the FE planner with #70177enable_streaming_agg_hash_join_force_passthroughAggregationNode) and BEStreamingAggOperatorXenable_distinct_streaming_agg_force_passthroughDistinctStreamingAggOperatorXforce_to_local_shuffleScanNode#isSerialNode), so the FE planner pools the scan and adds a local shuffle above itSo a session that sets one of them gets that plan only for the statements its own FE runs. A forwarded statement, such as an
INSERT INTO ... SELECTor a CTAS issued through a follower or an observer, is planned with the master's global value, andEXPLAINon the follower shows a plan that the master does not run. Query results stay the same, since these variables only decide how rows move between the pipeline tasks of a BE; what differs is the plan, and with it the speed of the statement.How this PR fixes it. Set
needForward = trueon the four, as #69273 does for the others, and add them to the list inSessionVariablesTest#testForwardPlanAndExecutionKnobs.Results.
SET <variable> = <non-default value>branch-4.1 and branch-4.2 have the same gap, and #69273 is not on them either.
Release note
The session variables enable_local_exchange_before_streaming_agg, enable_streaming_agg_hash_join_force_passthrough, enable_distinct_streaming_agg_force_passthrough and force_to_local_shuffle now apply to statements that a follower or observer FE forwards to the master, as they do to statements an FE runs itself.
Check List (For Author)
Test
SessionVariablesTest#testForwardPlanAndExecutionKnobschecks that the four are ingetForwardVariables(). WithSessionVariable.javareverted to master it fails at the first of the four in the list (enable_distinct_streaming_agg_force_passthrough must be forwarded to the master), and no other test fails. The 6 test classes that callgetForwardVariables()orsetForwardedSessionVariables()pass (89 tests), and fe-core Checkstyle reports nothing. Not run on a cluster with two FEs.Behavior changed:
A forwarded statement uses the session's value of these four variables instead of the master's global value.
Does this need documentation?
Check List (For Reviewer who merge this PR)