fix: specialize prepared decimal common types - #27483
Conversation
matrix-meow
added
size/XXL
# Conflicts: # pkg/defines/const.go # pkg/sql/compile/compile.go
aptend
left a comment
aptend
left a comment
There was a problem hiding this comment.
Deep re-review on exact head 8e4e357025b8bbf199e0f636fd7d9e04e3966627. I read the prior review, reviewed the complete 21d9a06...8e4e357 increment and the full PR diff, and independently reran the affected suite. The new delta closes the earlier COM_STMT exact-integer admission and DECIMAL(76) inference gaps, and its single-DECIMAL cache case works. Three blockers remain:
- Row-dependent BETWEEN violates SQL three-valued logic when one bound is NULL and the other comparison is already false. This changes NOT BETWEEN and WHERE results.
- A prepared parameter on the left of a mixed DECIMAL/DOUBLE IN list is provisionally locked to DECIMAL, so execution never forms the one FLOAT64 domain required by the PRs own common-type invariant.
- The new runtime cache is disabled by every unrelated non-DECIMAL parameter and by the supported binary string numeric-prefix path, so common multi-parameter statements still deep-copy and recompile on every execute.
Each inline comment includes an exact trigger, impact, and reproducer.
Validation on this exact head: make thirdparties, make -C cgo; full CGo-backed tests for defines, pb/plan, sql/plan/function, sql/plan/rule, sql/plan, frontend, sql/compile, vm/process; the full issue #27088 embedded regression; go vet; and git diff --check. All repository tests passed. Three temporary counterexample tests/benchmarks failed or exposed the misses as described below and were removed afterward.
aptend
left a comment
aptend
left a comment
There was a problem hiding this comment.
Deep re-review completed at exact head 98f74c1. I reread the complete review history and all three prior inline threads, compared the semantic delta from the previously reviewed head 8e4e357, and rechecked the full PR diff. The BETWEEN three-valued-logic fix now makes FALSE dominate NULL correctly; mixed DECIMAL/approximate IN specialization selects FLOAT64; and runtime specialization caching retains/restores every unrelated parameter while keying the relevant runtime types. I also independently exercised reuse with two parameters whose values both changed, and the cached plan retained the unrelated parameter position. Validation passed: the issue 27088 COM_STMT integration suite; all affected package tests; the four focused regressions under race 20 times; go vet; git diff --check; and a clean merge-tree against current base.
aunjgr
left a comment
aunjgr
left a comment
There was a problem hiding this comment.
Reviewed exact head f8da136e8d43bcdbac4acbe67bd6a41a744f36fa against merge base 22b91fe986b269d24a6b8816eafdd1fa6327c8e1.
I checked the current full production diff and the post-approval main-merge resolution independently. Numeric-prefix inference follows the execution-layer prefix contract (including non-numeric-to-zero), exact DECIMAL domains remain distinct from approximate common domains, BETWEEN preserves FALSE-dominates-NULL three-valued logic, and the one-entry specialization cache restores parameter references instead of retaining prior literal values. Cache invalidation/cleanup, compile retry, SET/DDL materialization, and the MORPC v29 capability boundary are closed on both ends. The only merge conflict advanced the capability number after main's v27/v28 additions; the final staticcheck change is semantic-preserving. I found no blocking issue.
# Conflicts: # pkg/defines/const.go # pkg/frontend/computation_wrapper.go # pkg/sql/plan/utils.go
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep re-review on exact head 8a2d958413acc79106b2fef877a433bd7edc6688. The earlier COM_STMT exact-integer admission, Decimal256 inference, BETWEEN 3VL, mixed approximate-domain, and multi-parameter cache findings are fixed; the focused frontend/plan/remote-protocol tests and the full issue #27088 embedded regression pass. One prepared-path performance blocker remains.
- [P1 performance] SQL
PREPARE/EXECUTEstill deep-copies and recompiles for every equivalent execution. The runtime cache is gated bybinaryExecuteatpkg/frontend/computation_wrapper.go:1191, so SQLEXECUTEcan never read or populateruntimePlan/runtimeCompile. Every decimal-aware SQL execution therefore callsspecializePreparedExecutionPlanagain, gets a new deep-copied plan, and forcesretComp = nilat lines 1248-1252. This is the same lifecycle gap raised in the earlier review, only the COM_STMT half has been cached.
I reproduced this with the PR's own coalesce(?, DECIMAL) SQL-EXECUTE shape and the same user-variable value/category on two consecutive executions. Both calls returned a nil compile and distinct runtime-plan pointers. Five 1-second benchmark runs on the same Apple M4 measured:
- cached COM_STMT same-category path: 3.122-3.126 us/op, 488 B/op, 15 allocs/op;
- SQL EXECUTE same-category path: 9.340-9.479 us/op, 10,664 B/op, 123 allocs/op.
Those SQL numbers cover only frontend initialization/specialization; the full createCompile forced by the nil result is not included, so the end-to-end regression is larger. Issue #27088 explicitly includes SQL PREPARE/EXECUTE and requires stable-category reuse without churn. Please extend the bounded semantic-category cache (or an equivalent safe design) to eligible SQL EXECUTE queries, and add a repeated-execute gate that proves both runtime plan and compile reuse. 9.0 -> 9.00 already normalizes to the same current binary cache key in my counterexample, so trailing-zero normalization itself is not the problem.
- [P2 scope/hygiene] The production PR still contains unrelated working notes and deletes an unrelated tracked design document. The diff adds 646 lines in
CLAUDE_TODO_2026-08-22.mdandCLAUDE_TODO_2026-08-23.md, while deletingCLAUDE_BINARY_STRING_SEMANTIC_MODEL.mdfrom main. These are implementation/review scratch plans and are not part of the bug fix or public documentation contract. Please remove the TODO artifacts and restore the unrelated main document so this already-XXL change does not carry private process state or overwrite another PR's documentation.
Validation: git diff --check; focused CGo-backed frontend, sql/plan, sql/compile, and issue tests; exact repeated-execution counterexamples; five benchmark runs; cache/lifecycle/unhappy-path review; and a clean conflict-free merge-tree against latest main 728b7e488122ac1f228dd185759f013b252e523e.
|
Thanks for the deep re-review. Both requested changes are now addressed.
Validation passed for the full affected frontend/plan/compile/pb/process packages, the focused issue regressions, SCA, build, vet, and diff checks. Latest head: |
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep re-review of exact head de77194.
The previous SQL EXECUTE cache blocker is fixed. The one-entry semantic-category cache restores ParamRefs before storing the specialized plan, reuses both plan and compile, replaces and releases the old compile on a category change, and is cleared on rebuild and Close. The focused SQL cache test passes. I also measured an exact-head SQL EXECUTE cache-hit benchmark on the same Apple M4 with five 1-second samples: 3.30-3.50 us/op, 696 B/op, 14 allocs/op, versus the prior reviewed 9.34-9.48 us/op, 10,664 B/op, 123 allocs/op. The prior COM_STMT admission, Decimal256 inference, BETWEEN 3VL, mixed-domain, and unrelated-parameter findings remain closed. Latest CI is green.
Two blockers remain:
- [P1 design gate] Provide an approved design record for the complete prepared common-type specialization architecture.
This is not an ordinary local bug fix in its final scope: it changes about 5.3k lines across frontend, planner, compile, process, and MORPC; introduces execute-time plan rewriting, a new one-entry Compile lifecycle/cache, retry behavior, and MORPC v30 compatibility. It triggers the current design-first gate by size, component count, wire compatibility, lifecycle/cache, and hot-path criteria. There is no stable reviewed design linked from issue #27088 or this PR. The PR body and accumulated implementation review notes do not close the required alternatives, cache ownership/state transitions, invalidation and retry matrix, mixed-version rollout/rollback, resource bound, and measurable end-to-end performance contract.
Please add the design under docs/rfcs/, docs/design/, or another stable versioned location, link issue #27088 and this PR/revision, obtain a distinct design approval, and then verify this implementation against it.
- [P2 scope/documentation] Do not silently delete the existing binary-string semantic contract owned by another issue/PR series.
CLAUDE_BINARY_STRING_SEMANTIC_MODEL.md is present on current main and was introduced by #27224 as the contract for #27214 and follow-up issues #27215-#27218. This PR still deletes it, despite its scope being issue #27088, and de77194 explicitly preserves that deletion without a reviewed successor. If parts of that contract are obsolete, replace or update them through the owning design and identify the superseding approved revision; otherwise restore the file. Removing another series contract leaves current runtime-domain, source, literal-form, conversion, and NULL invariants undocumented.
Change scope: whole prepared common-type specialization feature/refactor
Trigger: size plus cross-component cache/lifecycle, wire compatibility, retry, and hot-path triggers
Design: missing approved artifact
Decision: REQUEST_CHANGES
|
Thanks for the design-gate review. I addressed both remaining blockers in
Could you please review the design artifact separately and record approval or any required design changes? After design approval, I will verify the implementation against the approved document revision as the final conformance gate. |
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
[P1 lifecycle/design] Replace the one-entry runtime cache only after the candidate Compile succeeds.
The new design says in sections 5.4 and 8 that a replacement is installed only after successful compilation and that a failed candidate must not prematurely displace the last valid cache. The implementation does the opposite: initExecuteStmtParam calls clearRuntimeSpecializationCache at computation_wrapper.go:1216 before storing the candidate plan/key, while createCompile happens later in the execution path. The old runtime Compile is therefore released before the replacement Compile even starts.
I reproduced the state transition on this exact head using the existing binary DECIMAL cache fixture: cache category 9.0 with a valid runtime Compile, then execute category 99.0. initExecuteStmtParam returns nil to request a new compile, but prepareStmt.runtimeCompile is already nil and the old Compile has been released. Any candidate compile failure now leaves the statement with the last valid cache destroyed and a new plan/key without a Compile. The existing same-category tests pass because they never exercise replacement failure.
The design is also internally inconsistent here: the state-machine row says release old runtime compile, then build replacement, while sections 5.4/8 require build-success-before-replace. Please choose the fail-safe two-phase invariant, update the table, stage the candidate plan/key/Compile outside the live cache, and swap/release the old owner only after successful compilation. On specialization/compile/retry failure, discard the candidate and leave the previous cache unchanged. Add deterministic category-replacement success and injected compile-failure tests that assert exact release ownership.
The prior blockers are otherwise closed: the design artifact now covers the full architecture, the unrelated binary-string semantic contract is restored, and the SQL EXECUTE reuse/performance fix remains intact.
|
Thanks — the cache lifecycle now follows the fail-safe two-phase invariant in
Added deterministic coverage for both paths:
Validation passed: affected full packages and issue regressions, focused lifecycle tests under race and count=20, SCA, build, vet, and diff checks. |
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep re-review completed at exact head 70fa891ed07d9b5c1770674e0f6c746c61bb19f8.
Design gate: PASS. I explicitly approve docs/design/CLAUDE_PREPARED_COMMON_TYPE_SPECIALIZATION.md as the governing design for this revision. Its narrow numeric-prefix admission, immutable-plan specialization, one-entry semantic-category cache, protocol boundary, invalidation rules, and ownership model are consistent with the implementation and close the previously missing design requirement.
The last blocking lifecycle issue is fixed. A category miss now retains the live cache and stages the candidate plan/key in TxnComputationWrapper; a compile failure discards only that candidate, while a successful compile publishes the new plan/key/compile together and then releases the old compile. Rebuild and statement-close paths clear the runtime cache with exactly-once ownership, and ordinary prepared executions remain on the unchanged cached fast path.
I rechecked the full change map and prior semantic fixes for SQL EXECUTE and COM_STMT, exact versus approximate numeric domains, NULL/BETWEEN behavior, retries, SET/DDL/subquery paths, cache key bounds, mixed-version capability handling, and stale-literal prevention. I found no remaining correctness, performance, unhappy-path, or resource-lifecycle blocker. Focused exact-head tests for candidate staging, semantic-category reuse/replacement, and compile-failure retention passed; exact-head CI is green.
Merge Queue Status
This pull request spent 29 minutes 59 seconds in the queue, with no time running CI. Waiting for
All conditions
ReasonPull request #27483 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
|
5 participants
What type of PR is this?
Which issue(s) this PR fixes:
issue #27088
What this PR does / why we need it:
This PR implements the common-type planner split after #27440:
Testing: