Repository navigation
[fix](fe) Honor enable_local_exchange_before_streaming_agg in the FE local shuffle planner - #70177
Conversation
…local shuffle planner ### What problem does this PR solve? Issue Number: None Related PR: apache#66216, apache#66222, apache#63766 Problem Summary: apache#66222 (branch-4.1 / 4.2: apache#66216) added enable_local_exchange_before_streaming_agg (default false) on the BE side: StreamingAggOperatorX::required_data_distribution() keeps the distribution a streaming pre-aggregation inherits, and asks for a local hash exchange on its partition expressions only when the variable is on or when OperatorBase::child_breaks_local_key_distribution() (apache#63766) says the child is serial or itself requires a non-hash local exchange. The variable replaced enable_local_exchange_before_agg for streaming aggregation. On master the FE plans local exchanges by default (enable_local_shuffle_planner) and BE does not. The streaming aggregation branch of AggregationNode.enforceAndDeriveLocalExchange() still had the rule from before apache#66222: it never read the new variable and, with enable_local_exchange_before_agg at its default, required a hash distribution for every streaming pre-aggregation with grouping keys. So the default master plan hashed the input of a streaming pre-aggregation again above a child that keeps the key distribution, such as a UNION ALL or an external table scan that is not serial, where the BE planner and branch-4.x add no local exchange. Setting the variable had no effect, and enable_local_exchange_before_agg=false dropped the hash exchange even after a child that breaks the key distribution. Results were not affected: the operators that need hash-distributed input for correctness ask for it themselves. The FE now follows BE: after the hash join passthrough rule, a streaming pre-aggregation keeps the base class requirement unless the variable is on or the child breaks the key distribution, and enable_local_exchange_before_agg no longer applies to it. The new childBreaksLocalKeyDistribution() mirrors the BE check with each operator's required distribution: a child that is serial on BE, a nested loop join (ADAPTIVE_PASSTHROUGH, except null-aware anti, right outer / semi / anti and full outer joins), a broadcast hash join that is forced to passthrough or has a serial probe child, a table function and assert num rows (PASSTHROUGH). On a one-BE cluster with non-pooling scans, the profiles of GROUP BY over UNION ALL and over an Iceberg table with 6 data files had a LOCAL_HASH_SHUFFLE with the FE planner and none with the BE planner; they now have none with both. GROUP BY over a nested loop join keeps ADAPTIVE_PASSTHROUGH and LOCAL_HASH_SHUFFLE with both planners. ### Release note With the FE local shuffle planner (the default), a streaming pre-aggregation no longer adds a local hash exchange on its grouping keys above a child that keeps the key distribution, such as a UNION ALL or an external table scan, as with the BE planner and branch-4.x. Set enable_local_exchange_before_streaming_agg=true to add it. ### Check List (For Author) - Test: Unit Test, Regression test, Manual test - LocalShuffleNodeCoverageTest: 4 new tests (3 fail without the fix). LocalExchangePlannerTest: 2 new tests (1 fails without the fix); testSetOperationUnderAggHasHashShuffle now turns the variable on. 141 tests in the 9 related FE test classes pass. Each of 6 mutations of the new check fails only its target tests. - New regression nereids_p0/local_shuffle/ test_local_exchange_before_streaming_agg passes on a local one-BE cluster; its default-plan check fails without the fix. - Manual: the profile comparison above. - Behavior changed: Yes. With the FE planner, streaming pre-aggregation follows enable_local_exchange_before_streaming_agg, and enable_local_exchange_before_agg no longer affects it. Plans change, results do not. - Does this need documentation: No Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
There was a problem hiding this comment.
Static review of PR #70177 at 9231589e704d2a4de53576c7c052f98195b69714: no substantiated new findings. No existing inline findings require retention. Review converged in one round, with two complete-scope reviewers and a separate risk-focused reviewer returning NO_NEW_VALUABLE_FINDINGS.
The FE streaming-preaggregation branch now follows StreamingAggOperatorX: it keeps inherited distribution with enable_local_exchange_before_streaming_agg=false, retains HASH after serial/non-hash-exchange children, and preserves the existing hash-join passthrough override. Traced flow: physical LOCAL/INPUT_TO_BUFFER aggregate -> distribute translation -> per-BE instance assignment -> FE exchange enforcement -> existing Thrift plan -> BE operators. Final/merge aggregation continues to enforce its own correctness requirements.
Critical checkpoint conclusions:
- Goal and test linkage: the change addresses the FE/BE setting mismatch. Unit cases cover both switch values and positive/negative child classifications; real-planner tests pin exact exchange placement; regression cases compare FE/BE planners across both values.
- Scope and clarity: one production class and four directly related test/output files; no unrelated production change.
- Concurrency and locks: new checks run during query-local planning, introduce no threads/shared mutable state, and add no lock, IO or journal wait. No new lock-order concern.
- Lifecycle and initialization: existing query-owned plan nodes and session context are reused; no new resource owner, retained cache or static initializer.
- Configuration: no setting added. The existing streaming switch is read for each plan; its existing Thrift forwarding is present. The general aggregation switch retains its separate non-streaming/distinct roles.
- Compatibility: no ABI, persistent format, node type or wire-layout change; emitted exchanges already exist.
- Thrift/protobuf IDs: no definition changes; branch field-ID comparison is inapplicable.
- Parallel paths: non-streaming final/merge and distinct-streaming branches are unchanged. Relevant serial/nonserial, broadcast/partitioned join, null-aware and right/full join exceptions, wrappers, scans, exchanges and spill delegates were checked.
- Special conditions: NL-join raw serial checks classify precisely the BE exception enums; runtime serial checks handle actual task mode. Nested-streaming and recursive-CTE concerns were dismissed using translator exchange boundaries and single-instance assignment evidence.
- Test coverage: added positive/negative unit and real-plan cases complement the result matrix. Shared test context resets the newly toggled flag; regression hints control relevant fuzzy variables and explain parsing matches actual formatting.
- Expected results: independently recalculated all 12 new result blocks; counts/sums and lexical
order_qtordering match. Static inspection cannot establish file-generation provenance. - Observability: existing distributed explain includes streaming markers and exchange types, sufficient for this planner-only change.
- Transactions and persistence: no EditLog, replay, visible-version or commit/failover behavior changes.
- Data writes and atomicity: no production write path, rowset/delete-bitmap or crash-recovery change.
- FE/BE variables: no new transmitted field; the existing streaming flag is already serialized consistently.
- Performance and memory: intended redundant hash exchange is removed; retained serial redistribution and downstream correctness checks were traced. No new per-row work, large allocation or native memory-accounting change. Performance was not measured.
- Other invariants and issues: no newly swallowed error or reachable wrong-result/task-count path was substantiated. Committed-data visibility and storage invariants are outside this planner delta; aggregation correctness remains guarded by downstream requirements.
Coverage: all five changed files and relevant upstream/downstream mechanisms reviewed. No additional user focus was supplied, and no unresolved static candidate or coverage gap remains. This was static inspection only: no builds, Doris tests, cluster execution or benchmarks were run; author-reported runtime results were not independently reproduced.
Review converged after 1 round(s).
TPC-H: Total hot run time: 28845 ms |
TPC-DS: Total hot run time: 153926 ms |
ClickBench: Total hot run time: 24.29 s |
…r key-breaking children (#70296) ### Background On master the FE plans the local exchanges that redistribute rows among the pipeline tasks of one BE (`enable_local_shuffle_planner`, on by default). Each plan node follows the rule of the BE operator it becomes, `required_data_distribution()`, which BE applies itself when the FE planner is off. `enable_local_exchange_before_agg` (default true) controls the exchange before a LOCAL (first-phase) aggregation. Since #63766, BE skips it with the switch off only while the child keeps the grouping-key distribution; a child breaks it when it is serial or asks for a non-hash local exchange itself, such as the probe side of a nested loop join or a table function. ### What problem does this PR solve? Issue Number: None Related PR: #63366 (introduced the FE rule without the child check), #63766 (the BE rule), #70177 (the same fix for streaming aggregation; this PR reuses its `childBreaksLocalKeyDistribution()`), #62438 (added the switch) PR Type: Bugfix Problem Summary: With `enable_local_exchange_before_agg=false`, the FE planner dropped the local exchange below a LOCAL `AggSink` and below a streaming distinct pre-aggregation whatever their child was, while BE keeps it after a child that breaks the key distribution. Queries planned by the FE therefore got fewer local exchanges than BE planning gives, for example no hash exchange between a nested loop join and the pre-aggregation above it. This PR adds BE's child check to both cases. Results do not change; most of the diff is tests. ### Details - **Change map:** production code +15 / -7 in `AggregationNode` (two conditions and their comments) and a `PlanNode` javadoc; tests +345 / -34: cases in `LocalShuffleNodeCoverageTest` and `LocalExchangePlannerTest`, and the new regression suite `nereids_p0/local_shuffle/test_local_exchange_before_agg_breaking_child` with its generated `.out` (+169). - **Frequency:** Always, when `enable_local_exchange_before_agg=false` (not the default; P0 randomizes it) and a LOCAL aggregation sits on a child that breaks the key distribution. Only plans and speed change: the FE always requires hash input at FIRST_MERGE and at the non-streaming DISTINCT dedup (a deliberate divergence from BE), so results stay right. - **Root cause:** `AggregationNode.enforceAndDeriveLocalExchange()` mirrors the BE operators, but its DistinctStreamingAgg and AggSink LOCAL branches fall back to the base class on `!enableLeBeforeAgg` alone, without BE's `child_breaks_local_key_distribution()`. #63366 wrote them after #63766 had merged, from the rule before it. - **Why existing tests missed it:** Unit tests: the two "LE disabled" cases in `LocalShuffleNodeCoverageTest` use a child that keeps the key distribution, no case reached the DistinctStreamingAgg branch, and `LocalExchangePlannerTest` had no plan with the switch off. Regression: `query_p0/join/test_agg_after_nested_loop_join_local_exchange` (#63766's suite) and `nereids_p0/local_shuffle/test_enable_local_exchange_before_agg` compare only results, which were right, and no `.out` shows local exchanges. - **Fix:** Both branches fall back to the base class only when `childBreaksLocalKeyDistribution()` is false. After a key-breaking child, the AggSink LOCAL phase goes on to its hash requirement, and the streaming distinct pre-aggregation to `enable_distinct_streaming_agg_force_passthrough` (PASSTHROUGH, else the base class), as on BE; it never asks for hash. - **Other paths and branches:** Streaming aggregation got the check in #70177; BE needs no change. One known difference from BE planning remains, with the switch on or off: the FE lets an ADAPTIVE_PASSTHROUGH or PASSTHROUGH child output satisfy a PASSTHROUGH requirement, where BE `Pipeline::need_to_local_exchange()` always adds one (last row of the table in Testing). branch-4.1 and branch-4.2 have no FE local shuffle planner and are not affected. ### Testing - `LocalShuffleNodeCoverageTest` and `LocalExchangePlannerTest`, two new cases each: all four fail without the fix (for example, a serial child got `PASSTHROUGH` instead of `LOCAL_EXECUTION_HASH_SHUFFLE`) and pass with it. The 12 fe-core test classes that plan local exchanges or print distributed plans pass (168 tests). Run locally. - A planner probe over 288 plans (18 query shapes × pooling on and off × `agg_phase` 0 and 2 × `disable_streaming_preaggregations` on and off × the switch): only 32 with the switch off change. 30 of them now equal the plan with the switch on; the other 2 keep the distribution of a `UNION ALL`, as the switch asks. - Local exchanges in the query profiles on a local 1 FE + 1 BE cluster (master `4530f761fa1`; `doris-fe.jar` from `7c4b4462dcc` with #70177, and for "after" with only `AggregationNode.class` replaced), switch off, `parallel_pipeline_task_num=4`, `agg_phase=2`. The `AggSink` rows set `disable_streaming_preaggregations=true`; the distinct rows are a `GROUP BY` without aggregate functions: | Query | BE planner | FE planner before | FE planner after | |---|---|---|---| | LOCAL `AggSink` over a nested loop join | ADAPTIVE_PASSTHROUGH, LOCAL_HASH_SHUFFLE | ADAPTIVE_PASSTHROUGH | ADAPTIVE_PASSTHROUGH, LOCAL_HASH_SHUFFLE | | LOCAL `AggSink` over `explode` | PASSTHROUGH, LOCAL_HASH_SHUFFLE | PASSTHROUGH | PASSTHROUGH, LOCAL_HASH_SHUFFLE | | LOCAL `AggSink` over a one-tablet scan (serial) | PASSTHROUGH, LOCAL_HASH_SHUFFLE | PASSTHROUGH | PASSTHROUGH, LOCAL_HASH_SHUFFLE | | LOCAL `AggSink` over `UNION ALL` | none | none | none | | distinct pre-aggregation over `explode` | PASSTHROUGH ×2 | PASSTHROUGH | PASSTHROUGH ×2 | | distinct pre-aggregation over a nested loop join | ADAPTIVE_PASSTHROUGH, PASSTHROUGH | ADAPTIVE_PASSTHROUGH | ADAPTIVE_PASSTHROUGH | - The new regression suite fails on the FE without the fix at its first plan check (`nlj: [VNESTED]`) and passes with it; its 16 result blocks (4 queries × both planners × the switch on and off) match results computed independently from the inserted rows. `nereids_p0/local_shuffle` (11 suites), `query_p0/join/test_agg_after_nested_loop_join_local_exchange` and `nereids_syntax_p0/agg_4_phase` pass on the same cluster with the fix. ### Notes for Reviewers - `baseClassRequire()` stays in both branches although only a non-serial child reaches them now, since a serial child always breaks the key distribution. It keeps the branches shaped like BE's call to the base class, as #70177 does for streaming aggregation. - In `LocalShuffleNodeCoverageTest`, #70177's five key-breaking children moved into `childrenThatBreakKeyDistribution()` and its aggregate builder into `buildLocalPhaseAgg()`, so the new cases reuse them; #70177's case is otherwise unchanged. - The `PlanNode#requiresShuffleForCorrectness` javadoc still named `enable_local_exchange_before_agg` for streaming aggregation, which reads `enable_local_exchange_before_streaming_agg` since #70177.
…#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.
What problem does this PR solve?
Issue Number: None
Related PR: #66216, #66222, #63766
Problem Summary:
In short. This brings the FE local shuffle planner in line with
enable_local_exchange_before_streaming_agg, which branch-4.1 / 4.2 #66216 and master #66222 added on the BE side only. With the FE planner, which master uses by default, a streaming pre-aggregation still asked for a local hash exchange on its grouping keys whatever the variable said. Master plans therefore had aLOCAL_EXECUTION_HASH_SHUFFLElocal exchange that the BE planner, and branch-4.x, do not add, for example right above aUNION ALLor an external table scan. The FE now follows the same rule as BEStreamingAggOperatorX.Background. #66222 added
enable_local_exchange_before_streaming_agg(default false) so that a streaming pre-aggregation keeps the distribution it inherits. It replacedenable_local_exchange_before_aggfor streaming aggregation. InStreamingAggOperatorX::required_data_distribution(), the operator asks for a hash local exchange on its partition expressions only when the variable is on, or whenOperatorBase::child_breaks_local_key_distribution()(#63766) says the child spreads rows without regard to any key: the child is serial, or the child's own required distribution is a non-hash local exchange (PASSTHROUGH, ADAPTIVE_PASSTHROUGH, ...).The problem, and what it cost. On master the FE plans local exchanges (
enable_local_shuffle_planner, default true) and BE does not.AggregationNode.enforceAndDeriveLocalExchange()mirrors the BE operators, but its streaming aggregation branch still had the rule from before #66222. It never read the new variable. It skipped the hash requirement only whenenable_local_exchange_before_aggwas off, and then without the child check. With the default settings it required a hash distribution for every streaming pre-aggregation with grouping keys, so:LOCAL_EXECUTION_HASH_SHUFFLEthat BE planning does not. Examples are aUNION ALLand an external table scan that is not serial. Every row was hashed and moved once more before the pre-aggregation.enable_local_exchange_before_streaming_agg=truehad no effect with the FE planner.enable_local_exchange_before_agg=false, the FE dropped the hash requirement even after a child that breaks the key distribution, where BE keeps it.None of this changes query results: a streaming pre-aggregation only affects performance, and the operators that need hash-distributed input for correctness ask for it themselves.
How this PR fixes it. The streaming aggregation branch of
AggregationNode.enforceAndDeriveLocalExchange()now follows BE: after the existing hash join passthrough rule, it keeps the base class requirement (no exchange, or PASSTHROUGH over a serial child) unlessenable_local_exchange_before_streaming_aggis on or the child breaks the key distribution.enable_local_exchange_before_aggno longer affects streaming aggregation. The newchildBreaksLocalKeyDistribution()mirrorschild_breaks_local_key_distribution()with each BE operator'srequired_data_distribution():enable_broadcast_join_force_passthrough) or when the probe child is serialSelectNodeandRepeatNodetake the serial flag of their child, so the serial check already covers the base class rule (PASSTHROUGH over a serial child) for them.Results. Local exchanges in the plans of three queries on a one-BE cluster,
parallel_pipeline_task_num=4, non-pooling scans, taken from the query profiles:GROUP BYoverUNION ALLof two tablesGROUP BYover a nested loop joinGROUP BYover an Iceberg table with 6 data filesA planner probe over 72 plans (8 query shapes × pooling / non-pooling scans ×
agg_phase0 / 2 × the variable off / on) found only theUNION ALLplans changed with the variable off. A scan of an OLAP table, a nested loop join,explode, broadcast and shuffle joins, aLIMITsubquery andGROUPING SETSgave the same plans as before.Release note
With the FE local shuffle planner (the default), a streaming pre-aggregation no longer adds a local hash exchange on its grouping keys above a child that keeps the key distribution, such as a UNION ALL or an external table scan. This matches the BE planner and branch-4.x. Set
enable_local_exchange_before_streaming_agg=trueto add the exchange.Check List (For Author)
Test
New tests: 4 in
LocalShuffleNodeCoverageTest(3 fail without the fix; the fourth pins the child check), 2 inLocalExchangePlannerTest(1 fails without the fix), andtestSetOperationUnderAggHasHashShufflenow turns the variable on. The 9 FE test classes that plan local exchanges or print distributed plans pass (141 tests). Each of 6 mutations of the new code fails only the tests aimed at it: dropping the serial check, the nested loop join case, the hash join case, the table function / assert num rows case, or the nested loop join's exception for the serial joins, or ignoring the variable. The new regressionnereids_p0/local_shuffle/test_local_exchange_before_streaming_aggpasses on a local one-BE cluster, and its first plan check fails on the FE without this fix. The profile comparison in Results was taken on the same cluster.Behavior changed:
With the FE local shuffle planner, a streaming pre-aggregation follows
enable_local_exchange_before_streaming_aggas described above, andenable_local_exchange_before_aggno longer affects it. Plans change, results do not.Does this need documentation?
Check List (For Reviewer who merge this PR)