docs(rfc): define analytic optimizer rewrite contracts - #27930
Conversation
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require reviewTip of the day💡 Did you know, you can describe a rule in plain language on the Rules page and Qodo drafts it for you |
PR Summary by QodoDefine contracts for analytic optimizer rewrites
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
XuPeng-SH
left a comment
There was a problem hiding this comment.
I am requesting changes on design revision v1 at 82f38af for four blocking contract gaps:
-
Protocol v42 is already allocated on current
main, and reusing it can produce wrong results during a rolling upgrade. This RFC assigns v42 to vector grouping expansion, while currentmainassigns v42 to transactional SQL-task child cleanup (MORPCVersion42, latest=42). A cluster already advertising deployment protocol 42 would therefore enable the new grouping plan before all v42 receivers understandProjectionGroupingFlags,ProjectionGroupingSetCount, andDynamicGrouping; old receivers can ignore the append-only fields and execute different grouping semantics. Rebase on current main, allocate the next unique version, and cover old/new sender-receiver plus rollback boundaries around the actual predecessor. -
The partial-SUM legality contract does not define a merge-safe numeric domain. “Supported SUM states/types” is not a proof. The linked implementation currently admits every non-DISTINCT function named
sumwithout a type/state guard. Floating SUM is non-associative: if two dimension PKs map to one output group, introducing a per-PK partial aggregate can regroup values such as1e16,-1e16, and1and change the result. Decimal/integer overflow and error timing also need an explicit contract. This contradicts the RFC invariant that aggregate values and the evaluation/error domain are unchanged. Enumerate the exact admitted input/result/state types and NULL/empty/overflow semantics, reject unsafe states, and add positive plus negative black-box counterexamples. -
The RFC cannot satisfy its own acceptance gate yet. It says the design is accepted only after complete TPCH/TPCDS planner measurements, while #26768 states that M0 frozen artifacts, checksums, collection commands, typed optimizer decisions, and before/after distributions are the acceptance foundation. This PR links no reproducible corpus or report, and green documentation CI cannot establish the p50/p95 and unchanged-control claims. Either attach the exact M0 evidence required by the gate, or clearly make these implementation-approval gates and sequence #27914/#27915 after a reproducible M0 artifact.
-
Failure containment is source-revert-only and has no detection/rollout contract. These two implementation PRs add roughly 6.8k lines across planner, protocol, compiler, and executor paths and enable several wrong-result-capable rules. A targeted code revert still requires diagnosis, rebuild, and deployment, but the RFC defines neither per-rule observability nor an operational disable/rollback boundary. Define how each rule is identified in plan/decision telemetry, how rollout is staged, and how one rule can be disabled or contained without query/table special cases; if an existing optimizer/deployment gate is the intended mechanism, name its owner, default, version behavior, and rollback test.
Also align the RFC lifecycle metadata during the revision: repository statuses are draft/in-progress/completed/rejected/obsolete/postponed; proposed is not a recognized state. Because approval is tied to the exact revision, the accepted revision should land as in-progress, not require an unreviewed semantic/status follow-up.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Counterexample-driven follow-up on the same exact head found two additional correctness cases that the revised contract and implementation tests must close:
-
Partial SUM can expand the evaluation domain even for deterministic expressions. Consider a fact row whose dimension key has no matching PK row and whose SUM argument (or fact-side group expression) is deterministic but fallible. The historical plan performs the INNER JOIN first, drops that fact row, and never evaluates the expression. The proposed partial aggregate runs before the join and evaluates it, so the rewritten query can fail while the old query succeeds. #27914 currently checks volatility and side ownership, but not totality/fallibility, and no trusted FK-completeness proof exists. Require bare/structurally-total expressions or a stronger proof, and add an orphan-key + failing-expression negative SQL test.
-
Shared grouping sets need an explicit empty-input contract. For
GROUPING SETS ((k), ())(or ROLLUP) with an actually empty input but stale positive estimates, the empty grouping set must still produce its global aggregate row (COUNT(*) = 0,SUM(...) IS NULL, correctGROUPING()bits). The new expansion projection emits sets only after receiving a non-empty child batch, while the dynamic aggregate has non-empty group keys and therefore no ordinary H0 dummy group. Define how the empty-set row is synthesized and test empty runtime input independently of planner estimates; stats cannot be used as a semantic proof.
The negative matrix should also include: right-side-PK-only ANTI joins as a control for the left-PK lower bound; SQL NULL versus rollup-sentinel collisions; duplicate grouping sets; CTE producer error/cancel with another consumer draining; disk/FD exhaustion during spill; and cross-rule compositions at the declared boundaries (CTE→grouping, outer reassociation→partial SUM, DNF→shuffle, MARK→build-side selection). Each needs an exact result/error/resource oracle, not only a category name in the table.
What type of PR is this?
Which issue(s) this PR fixes:
issue #26768
What this PR does / why we need it:
Defines the versioned design contract for the analytic optimizer rewrite series in #27914 and #27915 before implementation review continues.
The RFC fixes rule order and non-fixpoint behavior; NULL, correlation, volatility, lineage, uniqueness, evaluation-domain, and fail-closed invariants; resource, compile-time, and plan-quality budgets; MORPC v42 compatibility; per-rule positive and counterexample validation; and rollback boundaries.
This is a design-only PR. It does not change runtime behavior.