fix(frontend): specialize direct prepared result metadata - #27713
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? |
# Conflicts: # pkg/frontend/types.go
# Conflicts: # pkg/frontend/computation_wrapper.go # pkg/frontend/computation_wrapper_test.go # pkg/frontend/mysql_cmd_executor.go # pkg/frontend/types.go # pkg/sql/plan/utils.go # pkg/tests/issues/issue_25753_test.go
aptend
left a comment
There was a problem hiding this comment.
Deep re-review completed at exact head 2ca38c1b12ae6580fd6fc495b36e4a40a3baae6f against base a73f76d109bd528ae2e53a4b8ab294422fb13e21. I read all historical reviews, inline comments, author discussion, resolved/outdated threads, issues #27290/#27088, the complete PR diff, and the increments since my prior reviewed head c99eead69973263e81b96bf6a851213e0fc30440, including the final 67eaeb1f8752c535fd33eda9df8f8d060849bea5 packet-scan fix and the subsequent main merge.
The prior blockers are closed: row-producing DISTINCT now traces and propagates its AGG group expression; zero with large positive exponents is handled before the nonzero width bound; the single scan carries a bounded canonical value so long leading-zero DECIMAL packets materialize successfully without raw-value diagnostics; and OID-only text-comparison admission no longer duplicates the exact DECIMAL payload scan. The real TCP/COM_STMT test is the correct public regression boundary here; a SQL BVT cannot send the failing NEWDECIMAL descriptor.
I explicitly approve the direct-result design amendment at design blob c907187bbd958ec645cccd477060634755ba1f44 as implemented by exact head 2ca38c1b12ae6580fd6fc495b36e4a40a3baae6f.
Validation on this exact head: full go test ./pkg/sql/plan and go test ./pkg/frontend; real protocol TestIssue25753PreparedNumericProtocolLifecycle; focused direct-result/DISTINCT/DECIMAL/cache lifecycle tests under -race -count=3; go vet ./pkg/sql/plan ./pkg/frontend ./pkg/tests/issues; git diff --check; and the 1 MiB scanner/materialization benchmarks. All passed, and the isolated worktree is clean.
aptend
left a comment
There was a problem hiding this comment.
Re-review of the post-approval re-request completed at exact head 2ca38c1b12ae6580fd6fc495b36e4a40a3baae6f. The re-request was created at 2026-08-27 16:48:12Z after my 16:45:21Z approval, but the head, base, design blob (c907187bbd958ec645cccd477060634755ba1f44), full diff, review threads, comments, and blocker closure have not changed. There is therefore no incremental code or discussion to invalidate the prior deep review. I rechecked all review/thread history, the exact-head/base identity, design hash, git diff --check, and clean isolated worktree; my explicit design approval and APPROVE conclusion remain unchanged. The full plan/frontend/protocol/race/vet validation recorded in review 5043321096 was already run on this identical exact commit.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep re-review completed at exact head 2ca38c1b12ae6580fd6fc495b36e4a40a3baae6f.
My three prior blockers are closed:
- Direct-result discovery and type propagation now follow row-producing
DISTINCTthrough the AGG group expression, with planner and real COM_STMT coverage. - Zero is classified before the nonzero exponent-width bound, so representable zero spellings such as
0e+77publishDECIMAL(1,0)while unrepresentable negative scale remains rejected. - The packet path now performs one allocation-bounded scan, carries both visible/normalized domains plus a bounded canonical materialization value, and no longer reparses or echoes an unbounded raw payload. Long leading-zero DECIMAL values therefore materialize correctly without input-sized diagnostics or the former transient allocation amplification.
I also checked the bounded one-entry specialization cache, NULL fallback, semantic cache keying, explicit-cast/set-operation exclusions, and error paths. No new goroutine, wait edge, resource owner, or unbounded retained state is introduced. The real TCP/COM_STMT test is the appropriate public regression boundary because SQL BVT cannot construct the failing NEWDECIMAL descriptor.
The direct-result design amendment at blob c907187bbd958ec645cccd477060634755ba1f44 has an independent exact-head approval, and the implementation matches it. Exact-head CI is green; the focused plan/frontend/protocol/race/vet and 1 MiB benchmark evidence recorded on this same commit is sufficient. No remaining blocking correctness, performance, or unhappy-path issue found.
What type of PR is this?
Which issue(s) this PR fixes:
Fixes #27290
Related to #27088.
What this PR does / why we need it:
Root cause
COM_STMT_PREPAREderives result metadata while a direct projected parameter still has its prepare-timeTEXTtype.COM_STMT_EXECUTEpreserved the exact runtime value but neither specialized the visible result type nor refreshed execution metadata from the binary protocol parameter domain. JDBC therefore observedTINYTEXTand converted exact DECIMAL text through an approximate path.The prepared runtime cache also covered common-type consumers only. Extending it to direct results required preserving runtime parameter references and making compile-category replacement safe for the currently borrowed parameter vector.
Fix
ORDER BY, andDISTINCTpaths.ParamRefevaluation.Compatibility and scope
The ordinary prepared fast path remains unchanged when no direct numeric result parameter is present. Explicit
CAST, nested expression consumers, andUNION/other set operations continue to use their existing result-type owners.A SQL BVT cannot reproduce this protocol contract: mo-tester executes script text with JDBC
Connection.createStatement()and has no client parameter-binding surface, so it cannot emitCOM_STMT_EXECUTEwith aMYSQL_TYPE_NEWDECIMALdescriptor. A generated SQLPREPARE 'SELECT ?'control reportsLONGVARCHARfor all values and would also pass before this fix, so retaining it would not be a valid regression. The exact public boundary is instead covered byTestIssue25753PreparedNumericProtocolLifecycle, which opens a real frontend TCP/COM_STMT connection, emits the DECIMAL descriptor, and asserts value plus wire column type/precision/scale across reuse.Design review gate
PR #27483's common-type baseline was independently approved at implementation head
3f8768aee19c261dee4052660f640ebb3db7b0f0and design blob0b49bfa791d1cef115206d2ba34e1d33cc0d1d6a.This PR's direct-result amendment is pending independent design approval at head
2ca38c1b12ae6580fd6fc495b36e4a40a3baae6f, design blobc907187bbd958ec645cccd477060634755ba1f44, and latest review-fix revision67eaeb1f8752c535fd33eda9df8f8d060849bea5(the subsequent head change merges current main). The amended contract explicitly distinguishes common-type normalization from visible direct-result metadata:9.0and9.00use different direct-result cache categories because scale is public metadata, while equal(OID, width, scale)domains reuse the bounded single cache entry.The PR remains draft until that exact amended design receives approval from a reviewer distinct from the implementation author.
Validation
pkg/sql/planfull CGo suite: passedpkg/frontendfull CGo suite: passedTestIssue25753PreparedNumericProtocolLifecycle: passed with BIGINT, DECIMAL128/256, fixed-scale/exponent zero, row-producing DISTINCT, negative-scale rejection, NULL, and category-reuse coverage-race -count=3: passedgo vet ./pkg/sql/plan ./pkg/frontend ./pkg/tests/issues: passedTestIssue27443BinaryPreparedDMLAndAggregateand this PR's real protocol lifecycle test both pass after conflict resolution