morrySnow commented on PR #64793:
URL: https://github.com/apache/doris/pull/64793#issuecomment-4806296983
## Review Summary
Thanks for this PR! The bucket-shuffle dest spreading + bucket-to-hash
parallelism upgrade is a well-designed optimization. I reviewed all files and
here is a summary of findings:
### ⚠️ Items to address
1. **Data race in `vdata_stream_recvr.h:set_dependency`**:
`_source_dependency` is assigned before acquiring `_lock`, while
`set_source_ready()` reads it under the lock. Move the assignment inside the
lock scope.
→
[Comment](https://github.com/apache/doris/pull/64793#discussion_r3479084010)
2. **Session variable default discrepancy**: Code defaults
`local_shuffle_bucket_upgrade_ratio` to `1.5`, but the PR description says
`1.0`. Since `<= 1` disables the feature, a default of `1.0` would mean the
upgrade is off by default — a significant behavioral difference. Please
reconcile.
→
[Comment](https://github.com/apache/doris/pull/64793#discussion_r3479088724)
3. **Typo "RQG RQG testing"** (3 occurrences): Find-and-replace artifact
producing duplicated "RQG" in `test_local_shuffle_rqg_bugs.groovy` (Bug 13, Bug
14, and analytic bugs comments).
→
[Comment](https://github.com/apache/doris/pull/64793#discussion_r3479087379)
### 🟡 Suggestions
4. **New test section missing bug number**: The `count(distinct)+std`
regression test inserted between Bug 20 and Bug 21 in
`test_local_shuffle_rqg_bugs.groovy` lacks a bug label inconsistent with the
rest of the file.
→
[Comment](https://github.com/apache/doris/pull/64793#discussion_r3479089608)
5. **`pathCrossesLocalExchange` memoization**: The DFS runs
per-local-RF-target, potentially O(N × depth). Consider caching the result per
target node.
→
[Comment](https://github.com/apache/doris/pull/64793#discussion_r3479092689)
### ✅ What looks good
- BE orphan instance fix (zero-sender receiver → immediate EOS) — correct
and well-commented
- `ExchangeNode.isSerialOperatorOnBe` decoupling — correctly scoped behind
`isEnableLocalShufflePlanner()`
- `force_local_merge` Thrift field is `optional` — backward compatible
- Bucket dest spreading in
`DistributePlanner.sortDestinationInstancesByJoinBuckets` — correct instance
sizing
- Stacked bucket join whole-chain upgrade logic — the NOOP output for lower
joins is the right approach to keep the re-align LE
- Unit tests in `LocalShuffleNodeCoverageTest` cover all major branches
(eligible, ineligible, parent-bucket-require, stacked, colocate, missing-exprs)
- Regression tests gracefully skip when bucket-shuffle join doesn't form in
the environment
### Unrelated note
`Backend.getCputCores()` has a pre-existing typo ("Cput" vs "Cpu") — not
introduced by this PR, but worth fixing in a follow-up.
---
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
