Repository navigation
fix: preserve nested prepared common-value domains - #29528
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.
Deep review of 1f56c9a4eb6569330d5821c9c495f91cc9daed46 against fcd9686e0de5c71b6cd2913f922ad54e7df0a324: APPROVE — no verified blocking finding. No subagents were used.
Design and implementation
Read all seven changed files and traced the propagation into the existing source-binding, numeric-prefix/common-precision, overload-selection, transport-lowering and frontend cache owners. This is an incremental correctness repair, not a new execution/storage/state-machine architecture. The new walker supplies an enclosing common-value domain to unresolved result roles; numeric conversion, Decimal256's 76-digit limit and overflow still belong to the existing binder. IFNULL's SQL result order is restored without treating its CASE condition as a peer.
Resolved children, explicit casts, concrete string/FLOAT boundaries and NULL-order rules retain their own domains. All fixed peers constrain inference jointly; executable ParamRefs survive, and spelling-derived choices are marked value-dependent before frontend cache admission. Compiler context is restored on errors; the immutable prepared template is not replaced by a value-specific cache generation. No new durable format, RPC field, goroutine, lock/wait, or separately owned vector is introduced. Data compatibility is unaffected; mixed-version deployment was not used as a review gate.
Independent QA and evidence
- Worktree-local native dependencies were built with the repository Make workflow.
mo-cgo-test ./pkg/sql/plan -run TestPreparedCommonValue -count=1passed. - All four new public issue-26879 regressions passed locally, exercising SQL PREPARE and COM_STMT, fixed-peer order, heterogeneous marker shapes, precision overflow and recovery, root string boundaries and same-type value changes.
- The existing
TestIssue27088PreparedDecimalCommonTypepassed locally, checking adjacent DECIMAL/source-domain behavior and prepared reuse. - Added reviewer-only planner and real SQL-protocol probes for nested nonnumeric strings, COALESCE/IFNULL, deeper nesting and untyped-NULL operands. Compared the potentially suspicious cases against an isolated MySQL 8.0.45 instance, rather than assuming literal substitution is a prepared-statement oracle. In particular, a marker-only child containing
abccan legitimately inherit the outer numeric domain; a child containing an untyped NULL has the observed string-domain/order boundary. The MO results matched these controls. These probes are not PR source changes. - Verified completed successful CI run 37097432883 belongs to this exact head. Earlier canceled executions are not counted as validation.
Performance and engineering quality
Reviewed implementation +305/-1, tests/fixtures +1143/-1, and documentation 0 separately. Planner tests inspect domain/precision and reuse decisions; public tests establish protocol results; BVT fixtures exercise the delivered SQL path. Their overlap has a distinct layer purpose, rather than independent heavyweight cluster creation: the public tests reuse the existing process-local fixture and close statements/connections/results.
The propagation performs repeated subtree walks, so it is not a performance optimization. A reviewer benchmark of a deliberately deep 128-marker COALESCE chain measured about 24.3 ms / 16.57 MB per binding on the head versus about 19.8 ms / 16.17 MB on the declared base (same host/native inputs; indicative microbenchmark, not a whole-query guarantee). Most deep-tree allocation cost already exists on the base. The incremental planning cost is non-blocking for this correctness repair; no new per-row scan work or I/O is added. Avoid expanding this into additional parallel inference branches or a new cache/state owner.
At inspection there are no actionable PR review threads to leave unresolved. This approval covers the reviewed delta and tested contracts, not a claim that every historical prepared-expression issue is solved. The branch is currently behind main; rebasing/new code should be evaluated for semantic drift before merge.
Merge Queue Status
This pull request spent 40 minutes 4 seconds in the queue, with no time running CI. ReasonPull request #29528 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.
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:
issue #26879
Incremental follow-up to merged #29428.
What this PR does / why we need it:
Prepared common-value expressions still lose type context across unresolved nested results or IFNULL lowering, and a plain NULL before a DECIMAL peer incorrectly inherits the later DECIMAL domain.
Regression coverage includes DECIMAL64/128/256, both SQL PREPARE and COM_STMT, nested/reversed/deep common-value calls, typed/untyped NULL controls, same-type value changes, invalid-text recovery, ABS-derived results, and explicit CHAR/DOUBLE boundaries. Original comparison results were checked against MySQL 8.0.45.
The review follow-up also covers mixed DECIMAL(65,30)/DECIMAL(38,0) peers in both orders, nested versus direct-marker controls, 65-digit overflow (error 1690), the Decimal256 46/47-integral-digit boundary at scale 30, and recovery on the same prepared statement. These precision cases use the existing direct-marker behavior and MatrixOne's 76-digit bound as their oracle.
The second review follow-up covers heterogeneous marker values (65 nines and
1e-30) in nested COALESCE, IFNULL, deep nesting, and separate big/big versus tiny/tiny sibling subtrees. All use the exact DECIMAL(76,11) domain rather than rejecting or falling back to DOUBLE; 77-digit overflow and recovery remain covered in both protocols.The root-string review follow-up covers LEAST/GREATEST with direct, COALESCE, IFNULL, deep and sibling operands. SQL PREPARE root values such as
abc,!and the empty string retain lexical child spellings (0002) and do not spuriously reject 77-digit strings. Same-type string→numeric-prefix→string rebinding is value-dependent; COM_STMT retains its existing numeric-prefix policy. The new public matrix contains 200 executions.Validation
pkg/sql/plantests in normal mode; focused changed-code coverage: 202/229 executable lines (88.2%).go vet, incremental repository lint, matching gofmt, andgit diff --check.Latest verified
mo/main(fcd9686e0d, including the subsequent prepared numeric/temporal, peer-pruning and remote-expression changes) was merged before the review update; the final build and tests were rerun on that baseline. Local CLAUDE documents, logs, and caches are not included in the PR.Validation note: the initial delivery's full-package atomic coverage hit the unchanged trailing-zero allocation assertion (
2allocations versus a limit of1). The complete package passed in normal mode, and the focused changed-code coverage run passed; no allocation assertion was relaxed. Incremental lint passed; the earlier full-package lint report contained only formatting findings in two untouched baseline test files.