englefly opened a new pull request, #67788:
URL: https://github.com/apache/doris/pull/67788
### What problem does this solve?
Issue Number: N/A
Related PR: None
Problem Summary:
`PhysicalPlanTranslator.visitPhysicalRecursiveUnion` absorbs its children's
fragments: it calls `PlanTranslatorContext#mergePlanFragment` and then
`setPlanRoot`, which rewrites the fragment ownership of the child plan trees
and stops only at an exchange. That makes the recursive union a
fragment-merging node exactly like `visitPhysicalHashJoin`,
`visitPhysicalNestedLoopJoin` and `visitPhysicalSetOperation`, but unlike those
three it did not declare the merge context
(`PlanTranslatorContext#enterFragmentMergeChild`) while translating its
children.
Bucketed aggregation fusion deletes the exchange between a one-phase GLOBAL
aggregate and its `distribute -> olap scan` child, and that exchange is the
only thing that keeps the olap scan in a fragment of its own. Without the merge
context, the base case of a recursive CTE was translated as `VBUCKETED
AGGREGATE -> VOlapScanNode`:
WITH RECURSIVE cte AS (
SELECT k, SUM(v) AS sv, CAST(1 AS INT) AS lvl FROM t GROUP BY k
UNION ALL
SELECT k, sv, CAST(lvl + 1 AS INT) AS lvl FROM cte WHERE lvl < 3)
SELECT k, MAX(sv) AS msv FROM cte GROUP BY k
with `t` distributed by a column other than `k` and `agg_phase=1,
enable_bucketed_hash_agg=true, be_number_for_test=1,
bucketed_agg_min_input_rows=0, bucketed_agg_high_card_threshold=1`. Before the
fix that plan contains `1:VBUCKETED AGGREGATE -> 0:VOlapScanNode` in the base
case fragment.
The query still returns correct rows today, because
`RequestPropertyDeriver.visitPhysicalRecursiveUnion` requires GATHER from both
children and the gather exchange the enforcer inserts above the base case
happens to keep the scan in its own fragment. The legality of the merged
fragment therefore depended on that non-local fact about the current property
derivation instead of on the structure of the plan itself. As soon as a child
no longer needs such an exchange, the same plan leaves two olap scans in the
recursive union fragment and scan assignment rejects it with "Not supported
multiple scan multiple OlapTable but not contains colocate join or bucket
shuffle join" (EXPLAIN still succeeds, because scan assignment only happens
when the SELECT runs).
Fix: bracket the child visits of `visitPhysicalRecursiveUnion` with
`enterFragmentMergeChild`/`exitFragmentMergeChild`, exactly like
`visitPhysicalSetOperation`.
After the fix the base case aggregate falls back to a regular `VAGGREGATE`
over its exchange and the olap scan keeps a fragment of its own (one extra
fragment on a single-BE deployment). Only the plan shape changes; query results
are unaffected.
### Release note
None
### Check List (For Author)
- Test: FE unit test
-
`fe/fe-core/src/test/java/org/apache/doris/nereids/glue/translator/RecursiveUnionFragmentMergeContextTest.java`
(new) enables bucketed aggregation, then asserts that a plain single table
aggregate is still fused into a `BucketedAggregationNode` (positive control, so
the case cannot pass vacuously) while the base case of the recursive CTE above
is not. Before the fix it fails with the base case translated as `VBUCKETED
AGGREGATE`; after the fix it passes. Ran with `sh run-fe-ut.sh --run
org.apache.doris.nereids.glue.translator.RecursiveUnionFragmentMergeContextTest`.
-
`org.apache.doris.nereids.glue.translator.BucketedAggregateTranslatorTest`
still passes.
- FE checkstyle: 0 violations.
- Regression suites were not run: this workspace has no running FE/BE
cluster.
- Behavior changed: No. The returned rows are unchanged; only the fragment
shape of recursive CTE queries whose base case or recursive term carries a
one-phase GLOBAL aggregate changes (that aggregate keeps its exchange instead
of being fused into the scan).
- Does this need documentation: No
### What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
### Release note
None
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [ ] Regression test
- [ ] 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? -->
- Behavior changed:
- [ ] No.
- [ ] Yes. <!-- Explain the behavior change -->
- Does this need documentation?
- [ ] 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]
