Repository navigation
docs: explain operator plan identity and exchange reuse - #6352
Conversation
sunchao
left a comment
There was a problem hiding this comment.
Summary
- Prior state and problem: The operator guide omitted identity overrides and exchange-reuse requirements, leaving contributors without guidance on preventing incorrect reuse.
- Design approach: Completes the Filter and Project examples and explains identity, canonicalization, and regression testing.
- Correctness / compatibility analysis: The documented methods match the current implementations. Verified canonicalization,
sameResult,semanticHash, and exchange reuse against Spark sources for 3.4.3, 3.5.9, 4.0.4, 4.1.3, and 4.2.0. Scan and broadcast exceptions match Comet’s code. - Key design decisions: Separates semantic fields from serialization state and explains why equality and canonicalized equality differ. Documents existing conventions without introducing another abstraction.
- Implementation sketch: Updates one contributor-guide file, including examples and guidance linked to existing aggregate regression tests.
- Behavioral changes worth calling out: Documentation only. No execution changes or runtime overhead are introduced.
- Suggested improvements: None meeting the reporting threshold. No introduced P1/P2 issues found within this review.
Reviewed full SHA f5bf0f9fd779fc7242a50603872243f6e2431d17 and the entire PR diff relative to base e1d2c11729c2fc60a5def4e87bb17e5b28df2a29, using their merge base. Confirmed non-draft status. The discussion snapshot contains no reviews, issue comments, inline comments, or threads.
Routed skills: review-comet-pr. No sibling skill applies to this documentation-only change.
Exact-head CI: Upstream CI succeeded, including Preflight, markdown formatting, and Required Checks. CodeQL also succeeded. Runtime suites and site deployment were skipped by documentation filters. The cited fork CI also passed at this SHA.
Validation: Local whitespace checks and source comparisons for both examples passed. No JVM/native tests or full documentation build were run locally. Local Prettier was unavailable; formatting validation relies on exact-head CI.
Which issue does this PR close?
Closes #5832.
Rationale for this change
The operator guide omits plan identity and exchange reuse. Its incomplete examples can lead contributors to include serialization state in equality or omit semantic parameters, allowing incorrect exchange reuse and wrong query results.
What changes are included in this PR?
Complete the Filter and Project examples with their current
stringArgs,equalsandhashCodeoverrides. Explain semantic parameters, serialization state, canonicalization, and the alternativeoriginalPlanconvention used by scan and broadcast operators.Add guidance for exchange-reuse regressions that verify native execution, distinguish different plans and retain reuse for equivalent plans, including optimizer rules that can otherwise hide an omission.
How are these changes tested?
Reviewed the examples against the current operators and canonicalization code, and checked the linked regression examples. Prettier and whitespace checks pass.
Fork CI passed for head
f5bf0f9fd. This changes only the contributor guide; runtime jobs were skipped by the documentation path filters, and no JVM/native runtime suites were run locally for this documentation change.