perf(plan): make outer and anti join planning fail-closed - #27934
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
XuPeng-SH
left a comment
There was a problem hiding this comment.
Reviewed exact head 013e579. I did not find another concrete implementation defect in the legality proofs, and the focused planner tests plus additional UPDATE/DELETE/lateral diagnostics pass, but two required gates are still missing:
-
[blocking][correctness evidence] The claimed public positive-path coverage is absent. pkg/tests/issues/left_join_anti_rewrite_test.go only executes coalesce(r.id, 0) IS NULL and json_object(r.id, 1) IS NULL. Both are deliberately fail-closed cases that must remain LEFT joins, so no black-box test actually executes the new LEFT-to-ANTI path. The outer-association tests likewise invoke helpers on hand-built nodes and do not prove that real SQL reaches either rule or preserves results. This suite would stay green if every positive rewrite became unreachable. Please add public SQL/result regressions for the positive ANTI rewrite and both outer-association rules, including matched, unmatched, duplicate, and NULL rows; pair the result oracle with a typed plan-shape assertion or an independently equivalent/reference query. This is also the validation contract stated in related issue #26768.
-
[blocking][performance evidence] This performance PR has no exact-head before/after evidence. It adds an unconditional whole-plan anti-rewrite traversal and recursive stats recalculation to every query, adds two more recursive outer-rule traversals in the normal join-ordering path, changes cardinality for all ANTI joins, and changes shuffle-key selection. Green UT/BVT establishes functional compatibility but not planner CPU/latency, target benefit, or workload no-regression. Please attach reproducible same-environment before/after planner metrics and representative target/control plan/runtime evidence (including the TPC-DS target and TPCH no-regression control required by #26768, or an explicitly agreed smaller gate) to the PR body.
aunjgr
left a comment
There was a problem hiding this comment.
Request changes on exact head 013e579bdb1305bd7e48751c0ff1b4de9d2a24c7.
This implementation still fails the mandatory design-first gate. This split alone changes more than 500 non-generated production lines in outer/anti join rewriting, cardinality, build/probe ordering, and shuffle selection, and it materially affects the optimizer hot path. The design contract explicitly applies to the complete feature/refactor and is not waived by splitting implementation PRs.
The series design PR #27930 was closed unmerged with changes requested; no exact revision has an independent approval or accepted in-progress status. A PR-body description is not a stable versioned design decision record. Restore and approve the complete series design first, including rule composition/order, semantic guards, observability/rollback, plan-quality budgets, and the counterexample matrix, then request implementation review against that accepted revision.
|
Performance evidence update for the TPCH no-regression control:
I added the reproducible run links, exact revisions, and scale timings to the PR body. This addresses the TPCH control portion of the performance feedback; it does not claim that the separate TPC-DS target/planner-overhead or correctness-evidence gates are closed. |
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep re-review of exact head e987af7bd9fde1bb9d903af9f594131a773ce4c7 against current main@c46d897e9645b80178568ef0783dd8e99e527222. This head does close three prior findings: no-op queries no longer trigger full stats recalculation, outerAntiPlanning now owns the complete rollback cohort, and public SQL reaches all three positive rewrites. Substantive CI is green and git diff --check is clean. The current head is still not approvable for the following independent blockers.
-
[P1][performance/cost correctness] Right-ANTI physical swapping makes the new cardinality estimate use the wrong side.
ReCalcNodeStatspasses physicalchild[0]asleftStatstoestimateAntiJoinOutcnt(stats.go:1226-1235), andantiJoinLeftKeysArePrimaryKeylikewise hard-codesChildren[0]as the logical left (stats.go:1491-1524). However,determineBuildAndProbeSidesetsIsRightJoinwhen the logical left is much smaller,swapJoinChildrenthen flips the children, andreCalcNodeStatsAfterSwaprecalculates the swapped tree while correcting only right-SINGLE (stats.go:1457-1481). A focused exact-head regression with a 10-row PK logical-left and a 10,000-row right input takes the normal right-ANTI path and producesOutcnt=5000; an ANTI result cannot exceed its 10-row preserved input. This impossible estimate is present before subsequent shuffle and runtime-filter planning and can poison ancestor costs and distribution decisions. Make post-swap ANTI estimation explicitly use the logical preserved/matching sides, including the PK proof, and add a regression that runsdetermineBuildAndProbeSide -> swapJoinChildren -> reCalcNodeStatsAfterSwapfor enabled and rollback modes. -
[design gate] The exact design record explicitly says this candidate is not at its decisive-review state. The RFC is still
Status: draft(docs/rfcs/00000000_stats_independent_analytic_plan_rewrites.md:1), lines 29-33 require the final exact candidate to advance toin-progress, and the Ready gate at lines 491-498 requires the unique MORPC allocation and all implementation/evidence gates to be closed. The PR body and latest author comment both say the cumulative MORPC allocation against still-open #27756 is intentionally unresolved. That is a real cross-branch compatibility gate, not merely a label mismatch; resolve it on the final rebased candidate and make the committed RFC describe that exact state. -
[performance acceptance] The body does not provide the auditable evidence required by this PR's own RFC. The planner summary omits p95, reachable-node counts, exact commands/raw artifacts, and per-case results, so the 5% control and 15%/25% admitted budgets cannot be independently checked. Q80 has one wall time and digest but no same-environment baseline or rows/bytes, peak memory, and spill bytes; Q78 has a runtime/plan claim but no terminal result/digest, baseline, or resource profile. The TPCH same-runner links are useful and close that control only. Attach the missing exact-head artifacts and target resource evidence rather than treating green UT/BVT as a planner-performance pass.
Validation note: I proved blocker 1 with a temporary package-local regression on the exact head; it failed with 5000 is not less than or equal to 10. The temporary test and staged macOS native library were removed after the run, leaving the review worktree clean.
|
On the MORPC point, I disagree that concurrent use of the next number on unmerged branches is a P1 defect in this PR. MORPC versions are cumulative integration numbers allocated against the merge base, not durable reservations made by open branches. Two unmerged heads cannot coexist in a rolling deployment: only code that has actually reached the target branch can be built into that deployment. The repository-safe rule is therefore straightforward: the first PR merged owns the next number; every later PR rebases and moves its gates/tests/generated artifacts to the next contiguous number before it can merge. GitHub's up-to-date-base/CI requirements enforce that integration point, and the renumbered head receives normal re-review. Blocking semantic review of every open PR until all other open users of the provisional number merge serializes unrelated work without improving mixed-version safety. The unsafe state would be merging a stale later head without rebasing; that is already prohibited. Please evaluate this PR's protocol fallback contract and predecessor test independently of other unmerged branch-local numbers. I will handle the mechanical final allocation on the merge-order rebase, whoever lands first. |
|
Addressed the review findings on exact head Right-ANTI physical swapCommit The regression exercises the real sequence
Focused, full planner/compile, public SQL, scoped lint, and Design stateRFC v6 is Auditable planner corpusBase was taskset -c 15 ./.agents/skills/mo-dev/scripts/mo-cgo-test \
-run=^$ -bench=BenchmarkOuterAntiPlanning -benchmem \
-benchtime=1s -count=5 -cpu=1 ./pkg/sql/planThe fixture SHA-256 is Raw samples below are p95 is nearest-rank (maximum of five samples):
These are within the RFC's fixed control/admitted p50, p95, and allocation budgets; no case gains a reachable node. Exact-head TPC-DS 1 TiB evidenceEnvironment: the same local NVMe dataset Q80 completed twice on exact head with identical results and resources:
Raw Q78 exact-head terminal/resource evidence:
Raw The recorded same-host predecessor completed in The exact-head Q78 plain plan contains three ANTI joins and REUSE on both following joins. No query was run concurrently and automatic merge remained disabled. |
|
RFC-only follow-up |
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep re-review of exact head 8ac5fe82192320c5b0200ff36f1a6d27890c1ba6 against base c46d897e9645b80178568ef0783dd8e99e527222, including the increment after my prior requested-changes head e987af7bd9fd, the real RIGHT-ANTI physical sequence, the complete RFC v6 contract, main-branch drift, planner-cost fields, rollback, and counterexamples.
The prior blockers are closed:
- RIGHT-ANTI now passes explicit logical preserved/matching node IDs after physical child swapping. The PK proof follows that same mapping, output is clamped to the preserved cardinality, and BlockNum comes from the preserved side. The regression executes
determineBuildAndProbeSide -> swapJoinChildren -> reCalcNodeStatsAfterSwapin enabled and rollback modes. - RFC v6 is in-progress and treats the cumulative MORPC number as final-rebase integration metadata with a real predecessor boundary, not as a semantic hard-coded reservation.
- The exact-base/head planner corpus reports raw p50/p95, allocations, and reachable-node counts within the stated budgets. Exact-head Q80/Q78 terminal results, digests, rows/bytes, memory, spill, and HWM are attached, with the TPCH control retained.
I rechecked ordinary LEFT-ANTI, swapped RIGHT-ANTI, complete/partial/right-only PK, rollback, zero/non-finite estimate clamping, join cost/hash-map direction, physical call ordering, and the fail-closed RFC boundaries. I found no additional correctness, performance, lifecycle, or design blocker. Current main has no post-base overlap in the touched planner/RFC paths, and substantive exact-head CI is terminal green.
Validation note: the exact worktree's focused local test invocation was blocked before compilation by the absent cgo/libmo.dylib; I do not count that as a local pass. This approval relies on source inspection, the exact-head regression/CI, and the attached reproducible evidence.
|
Updated exact head to Validation on the merged head:
All passed with the repository CGo test wrapper. |
aunjgr
left a comment
There was a problem hiding this comment.
Reviewed exact head bb34f62 against merge base e0a8738.
The v6 RFC is now in-progress and the implementation matches its ordered, fail-closed contracts. The prior objective gaps are closed: no-op plans skip recursive stats recalculation; outerAntiPlanning owns the complete rollback cohort; public SQL reaches LEFT-to-ANTI and both outer-association rules with duplicate/NULL result oracles; physical right-ANTI cardinality explicitly maps logical preserved/matching sides after child swapping and clamps output to the preserved input; and the planner/target/control evidence is recorded against fixed budgets. A range-diff across the newest-main merge shows all 19 semantic commits unchanged, and exact-head CI is terminal green.
I found no remaining correctness, compatibility, lifecycle, boundedness, or performance gate blocker.
Merge Queue Status
This pull request spent 42 minutes 45 seconds in the queue, with no time running CI. Waiting for
All conditions
ReasonThe merge conditions cannot be satisfied due to failing checks Failing checks:
HintYou may have to fix your CI before adding the pull request to the queue again. Requeued — the merge queue status continues in this comment ↓. |
Merge Queue Status
This pull request spent 10 minutes 9 seconds in the queue, with no time running CI. Waiting for
All conditions
ReasonPull request #27934 has been dequeued Pull request from fork cannot be queued. This pull request comes from a fork, and Mergify needs the author's permission to update its branch.
Failing checks:
HintYou should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it. Tick the box to put this pull request back in the merge queue (same as
|
What type of PR is this?
Which issue(s) this PR fixes:
Related to #26768
What this PR does / why we need it:
Owns the fail-closed outer/ANTI closure and the single RFC v6 design record for #27914, #27915, and #27934. The RFC and implementation are reviewed as one exact-head unit and are now aligned at
in-progress.outerAntiPlanning=1for every rule owned by this PR.Exact-head validation:
./pkg/sql/planand./pkg/sql/compile: PASS.TestOuterAndAntiJoinRewritesPreserveSQLResults: PASS.determineBuildAndProbeSide -> swapJoinChildren -> reCalcNodeStatsAfterSwapin enabled and rollback modes: PASS; estimated output never exceeds the 10-row preserved input.git diff --check: clean.origin/main@c46d897e96: control p50 +0.2% / p95 -0.3%; admitted p50 maximum +8.4% / p95 maximum +5.0%; allocation maximum +0.8%; no reachable-node increase.67cc97acf391bb464285c8e42615cafdd5b1b21a24893bf7e243c73495dcd661; 16,634,768,895 rows read; 580,640,176,980 bytes scanned; zero spill bytes; second fresh-service run hadVmHWM=7,337,188 KiB.aba289da41554fc62aae876cb7ef4b0af4d5650a6f5bec4b9bd0190aa00730fe; 5,544,181,980 rows read; 185,472,581,988 bytes scanned; 9,618,441,088-byte StatsArray memory; 295,955,082,891 spill bytes; fresh-service processVmHWM=17,174,460 KiB.The exact benchmark command, fixture, per-sample raw output, p50/p95 calculations, and current-head TPC-DS statement/resource records are attached in the review response.