morningman opened a new pull request, #70197:
URL: https://github.com/apache/doris/pull/70197
### 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_agg` among them, but not
`enable_local_exchange_before_streaming_agg`,
`enable_streaming_agg_hash_join_force_passthrough`,
`enable_distinct_streaming_agg_force_passthrough` and `force_to_local_shuffle`.
A session-level `SET` of 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: `FEOpExecutor` sends
`SessionVariable#getForwardVariables()`, and `ConnectProcessor` on the master
applies them with `setForwardedSessionVariables()` to a session that starts
from the master's global variables. Only variables whose `@VarAttr` sets
`needForward` (or one of the `affectQueryResult*` attributes) are sent.
**The problem, and what it cost.** These four do not set it:
| Variable | Default | What it decides |
|---|---|---|
| `enable_local_exchange_before_streaming_agg` (#66222) | false | whether a
streaming pre-aggregation asks for a local hash exchange on its grouping keys:
BE `StreamingAggOperatorX`, and the FE planner with #70177 |
| `enable_streaming_agg_hash_join_force_passthrough` | true | whether a
streaming pre-aggregation right above a hash join asks for a passthrough local
exchange: the FE planner (`AggregationNode`) and BE `StreamingAggOperatorX` |
| `enable_distinct_streaming_agg_force_passthrough` | true | the same for a
distinct streaming pre-aggregation: the FE planner and BE
`DistinctStreamingAggOperatorX` |
| `force_to_local_shuffle` | false | makes every scan serial
(`ScanNode#isSerialNode`), so the FE planner pools the scan and adds a local
shuffle above it |
So 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 ... SELECT` or
a CTAS issued through a follower or an observer, is planned with the master's
global value, and `EXPLAIN` on 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 = true` on the four, as #69273
does for the others, and add them to the list in
`SessionVariablesTest#testForwardPlanAndExecutionKnobs`.
**Results.**
| Statement forwarded from a follower after `SET <variable> = <non-default
value>` | Before | After |
|---|---|---|
| Value the master plans it with | the master's global value | the session's
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 <!-- At least one of them must be included. -->
- [ ] Regression test
- [x] Unit Test
- [ ] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
- [ ] This is a refactor/code format and no logic has been changed.
- [ ] Previous test can cover this change.
- [ ] No code files have been changed.
- [ ] Other reason <!-- Add your reason? -->
`SessionVariablesTest#testForwardPlanAndExecutionKnobs` checks that the
four are in `getForwardVariables()`. With `SessionVariable.java` reverted 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 call
`getForwardVariables()` or `setForwardedSessionVariables()` pass (89 tests),
and fe-core Checkstyle reports nothing. Not run on a cluster with two FEs.
- Behavior changed:
- [ ] No.
- [x] Yes. <!-- Explain the behavior change -->
A forwarded statement uses the session's value of these four variables
instead of the master's global value.
- Does this need documentation?
- [x] No.
- [ ] Yes. <!-- Add document PR link here. eg:
https://github.com/apache/doris-website/pull/1214 -->
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label <!-- Add branch pick label that this PR should
merge into -->
--
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]
