Skip to content

fix(frontend): specialize direct prepared result metadata - #27713

Merged
XuPeng-SH merged 15 commits into
matrixorigin:mainfrom
ck89119:issue-27088-metadata-main
Aug 27, 2026
Merged

fix(frontend): specialize direct prepared result metadata#27713
XuPeng-SH merged 15 commits into
matrixorigin:mainfrom
ck89119:issue-27088-metadata-main

Conversation

@ck89119

@ck89119 ck89119 commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

Fixes #27290

Related to #27088.

What this PR does / why we need it:

Root cause

COM_STMT_PREPARE derives result metadata while a direct projected parameter still has its prepare-time TEXT type. COM_STMT_EXECUTE preserved the exact runtime value but neither specialized the visible result type nor refreshed execution metadata from the binary protocol parameter domain. JDBC therefore observed TINYTEXT and 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

  • Discover direct result parameters once at PREPARE/rebuild time, including transparent projection, ORDER BY, and DISTINCT paths.
  • Keep explicit casts, nested consumers, predicates, and set-operation common-type resolution out of direct-result specialization.
  • Derive binary runtime types for signed/unsigned integers, floating-point values, BOOL, BIT, and exact DECIMAL64/128/256 domains.
  • Preserve DECIMAL lexical precision/scale, including fixed-scale zero and exponent notation.
  • Propagate specialized types to visible result expressions while restoring runtime ParamRef evaluation.
  • Generate execution-only column definitions without mutating immutable PREPARE metadata.
  • Reuse the existing bounded single-entry runtime plan/compile cache and release replaced categories without invalidating the new parameter generation.
  • Document the direct-result metadata contract and ownership boundary.

Compatibility and scope

The ordinary prepared fast path remains unchanged when no direct numeric result parameter is present. Explicit CAST, nested expression consumers, and UNION/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 emit COM_STMT_EXECUTE with a MYSQL_TYPE_NEWDECIMAL descriptor. A generated SQL PREPARE 'SELECT ?' control reports LONGVARCHAR for 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 by TestIssue25753PreparedNumericProtocolLifecycle, 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 3f8768aee19c261dee4052660f640ebb3db7b0f0 and design blob 0b49bfa791d1cef115206d2ba34e1d33cc0d1d6a.

This PR's direct-result amendment is pending independent design approval at head 2ca38c1b12ae6580fd6fc495b36e4a40a3baae6f, design blob c907187bbd958ec645cccd477060634755ba1f44, and latest review-fix revision 67eaeb1f8752c535fd33eda9df8f8d060849bea5 (the subsequent head change merges current main). The amended contract explicitly distinguishes common-type normalization from visible direct-result metadata: 9.0 and 9.00 use 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/plan full CGo suite: passed
  • pkg/frontend full CGo suite: passed
  • TestIssue25753PreparedNumericProtocolLifecycle: passed with BIGINT, DECIMAL128/256, fixed-scale/exponent zero, row-producing DISTINCT, negative-scale rejection, NULL, and category-reuse coverage
  • Focused DISTINCT/DECIMAL/frontend review regressions under -race -count=3: passed
  • go vet ./pkg/sql/plan ./pkg/frontend ./pkg/tests/issues: passed
  • Changed production statement-block coverage: 88.5%
  • 1 MiB DECIMAL exact domain scan: 0 B/op, 0 allocs/op; OID-only frontend text-comparison admission: 16 B/op, 1 alloc/op with no payload scan/copy; full scan + plan copy + replacement + typed materialization: 4,234 B/op, 49 allocs/op, independent of lexeme length (review witness baseline: 7,398,093 B/op)
  • Main's TestIssue27443BinaryPreparedDMLAndAggregate and this PR's real protocol lifecycle test both pass after conflict resolution

@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

# 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 aptend left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 aptend left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deep re-review completed at exact head 2ca38c1b12ae6580fd6fc495b36e4a40a3baae6f.

My three prior blockers are closed:

  1. Direct-result discovery and type propagation now follow row-producing DISTINCT through the AGG group expression, with planner and real COM_STMT coverage.
  2. Zero is classified before the nonzero exponent-width bound, so representable zero spellings such as 0e+77 publish DECIMAL(1,0) while unrepresentable negative scale remains rejected.
  3. 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug Something isn't working size/XL Denotes a PR that changes [1000, 1999] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Server-prepared SELECT ? reports TINYTEXT and makes JDBC getBigDecimal lose precision

5 participants