fix(function): align CONV and BIN typed dispatch - #28650
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? |
aptend
left a comment
There was a problem hiding this comment.
Deep re-review of exact head 219e8a6 covered every prior review, inline comment, reply/thread state, the delta from aptend previous reviewed head 5dcf95c, and the complete diff. The earlier temporal, integer-from_base, float-formatting, literal-width, and sender/receiver fencing findings are addressed, but the protocol-version collision, binary-protocol BIT dispatch, and checked-in BVT oracle still block this head.
Validation: make thirdparties and make cgo passed; owning suites for ./pkg/sql/plan/function, ./pkg/sql/plan, ./pkg/frontend, ./pkg/sql/compile, and ./pkg/pb/plan passed; focused race runs passed; git diff --check passed. A review-only COM_STMT_EXECUTE counterexample failed with expected 15 / actual 1 and was removed afterward. The current GitHub Compose + Pessimistic BVT also fails deterministically because func_string_bin.test cannot be matched to its result file and all 62 commands are marked abnormal.
| MORPCVersion62 int64 = 62 // VARCHAR OCT overload identities | ||
| MORPCVersion63 int64 = 63 // INSERT IGNORE CHECK warning diagnostics | ||
| MORPCLatestVersion = MORPCVersion63 | ||
| MORPCVersion64 int64 = 64 // typed BIN/CONV execution contracts |
There was a problem hiding this comment.
[P1] This protocol number is not uniquely owned. Current main ends at v63, but #28528 exact head ee2ddcb independently defines MORPCVersion64 for AUTO_ID_CACHE/PRE_INSERT, and #28272 exact head 3ac65f4 defines it for persistent data-branch identity. In a mixed cluster, a #28650 coordinator sees a #28528 node report 64 and dispatches typed BIN/CONV even though that receiver lacks these overloads; conversely, #28528 can send its auto-ID marker to this build, which lacks that contract. The >=64 check therefore does not prove either capability. Please coordinate a unique version after the real predecessor lands (or negotiate separate capabilities), rebase, and test against that predecessor.
| prepareControl.Plan), | ||
| bitCountOverloadParamPositions: plan2.PreparedPlanBitCountFallbackParamPositions( | ||
| prepareControl.Plan), | ||
| conversionParamPositions: plan2.PreparedPlanConversionParamPositions( |
There was a problem hiding this comment.
[P1] Enabling conversion specialization for binary prepares exposes a BIT-domain regression when a client sets the protocol unsigned flag. ParseExecuteData decodes MYSQL_TYPE_BIT as uint64, while binaryProtocolPrepareParamDomains currently maps (MYSQL_TYPE_BIT, true) to T_uint64 instead of T_bit. Reproducer: prepare select conv(?, 2, 10), then COM_STMT_EXECUTE value 15 with type BIT and flag 0x80. This head chooses the uint64 overload, decimal-spells 15, parses it as base 2, and returns 1; the BIT contract (and SELECT conv(0b1111,2,10)) returns 15. Preserve T_bit regardless of this flag, as binaryProtocolPrepareParamConcreteType already does, and add signed/unsigned BIT packet tests.
| conv_bit_value | ||
| 15 | ||
| SELECT BIN(X'') AS bin_empty_hex, CONV(X'', 2, 10) AS conv_empty_hex, | ||
| BIN(X'FFFFFFFFFFFFFFFFFF') AS bin_wide_hex, |
There was a problem hiding this comment.
[P1] The checked-in oracle makes the current BVT fail before any query in this file executes. mo-tester normalizes continuation lines in recorded SQL without their leading indentation (see existing generated results such as func_string_oct.result), but this result retains seven spaces here and again on lines 61-63. The exact-head Compose + Pessimistic BVT reports that the command ending at func_string_bin.test:25 does not exist in the result file, then marks all 62 commands abnormal and exits 1. Please regenerate or normalize these multiline result commands so the added coverage actually runs.
aptend
left a comment
There was a problem hiding this comment.
Deep re-review of exact head a1b717fb3f117672964c8b8daff4c6a83f124832 against base/merge-base b349fea56f1c16e3c947ac730cc114fca3c3f241. I read the complete review, inline-comment, reply, and thread history; used range-diff to compare the rebased series with my previous reviewed head 219e8a6fe4abc494c776678011ee7af63895bbd5; and rechecked the full 22-file diff. The BVT oracle blocker is fixed, and the four incremental commits correctly add and normalize the float cases. The earlier temporal/integer source-base findings also remain closed. Two blocking issues from the previous review are still present:
-
pkg/defines/const.go:102still assigns typed BIN/CONV to MORPC v64. Two independent open exact heads currently assign the same capability number to incompatible contracts: #28528 at089c2e68cb712116cc584e6a3456dd782b321ef0uses v64 for AUTO_ID_CACHE/PRE_INSERT, and #28624 atbfe9669e50982e9bc88576ee57cb295f0aed5e55uses v64 for widened DECIMAL SUM state. Trigger: a mixed cluster combines either head with this one. Impact:version >= 64passes even though the peer does not implement the advertised feature, so new overloads/physical vectors or unrelated v64 payloads can be sent to an incompatible receiver. Please coordinate a unique version after the actual predecessor or negotiate separate capabilities. -
pkg/frontend/computation_wrapper.go:1222still maps(MYSQL_TYPE_BIT, unsigned=true)toT_uint64, unlikebinaryProtocolPrepareParamConcreteType, which preservesT_bit. Trigger: prepareSELECT CONV(?, 2, 10)and COM_STMT_EXECUTE value 15 as MYSQL_TYPE_BIT with flag 0x80. Impact: runtime specialization selects the UINT64 overload, decimal-spells 15, reparses it in base 2, and returns 1 instead of the BIT result 15. A review-only exact-head test ofbinaryProtocolPrepareParamDomains(MYSQL_TYPE_BIT, true, "15")expectedT_bitand deterministically gotT_uint64; it was removed afterward. Please preserve BIT regardless of the unsigned flag and cover both packet flags through conversion execution.
Validation on this exact head: make thirdparties, make cgo, git diff --check, focused BIN/CONV/float tests, frontend protocol-domain tests, and remote-expression compatibility tests in pkg/sql/compile and pkg/pb/plan. The review-only BIT invariant test is the only intentional failure. GitHub Compose/Proxy BVT, coverage, and SCA are green; Ubuntu UT was still queued at the final check.
|
Follow-up pushed at f8f0e86, rebased onto main 438353c. Addressed the remaining BIT protocol finding:
The v64 fence remains intentionally cumulative: current main is v63, so this change takes v64 in the formal merge order. Concurrent branches that provisionally use v64 must rebase and allocate the next value before merging; this PR does not claim interoperability between independently built sibling branches. Validation on the rebased head: frontend full UT; function/pb-plan race suite; compile/plan protocol and prepared-runtime race tests; BIT packet race x5; CGo-aware go vet; incremental golangci-lint (0 issues); and git diff --check all pass. The packet test proves parser -> execute binder -> expression executor; existing cache lifecycle tests remain the cache reuse/replacement evidence. Fresh CI/BVT on this head is intentionally not awaited. |
|
Final rebase follow-up: the feature branch is now at c668423, rebased onto main c4053fd (including #28671). The same BIT fix and packet-path regression are unchanged; the final-head owning suites were rerun successfully:
The PR body now records the final base/head and the v64 merge-order constraint. Fresh CI/BVT is left to the normal PR workflow. |
What type of PR is this?
Which issue(s) this PR fixes:
#28392, #28460, #28461
What this PR does / why we need it
CONV/BIN accepted several typed domains at bind time but dispatched through
string-only or incompatible accessors at execution time. Prepared markers made
the gap observable because the runtime wire domain could change between
executions. Numeric prefixes and MySQL's FLOAT/DOUBLE textual representation
also had boundary mismatches.
The change keeps the existing integer/string fast paths and closes the common
typed dispatch boundary:
values through type-correct overloads;
decimal, float, and temporal prefixes;
while keeping empty/over-wide literals bounded and deterministic;
varlena-accessor panic;
reuse the existing bounded runtime-plan cache without stale parameter
references.
Review follow-up: binary protocol BIT domain
MYSQL_TYPE_BIT is an opaque bit-domain value. Its binary-protocol unsigned flag
describes wire encoding, not a semantic conversion to UINT64. The prepared
binary path now preserves T_bit for both flag variants. The packet-path
regression test exercises protocol parsing, execute-time binding, expression
execution, signed/unsigned BIT values, ordinary signed/unsigned LONGLONG, and
BIT rebinding after an integer execution.
Compatibility boundary
The final branch is rebased onto main at c4053fd (main protocol version 63)
and allocates the next cumulative version, MORPCVersion64, for the changed
typed BIN/CONV execution contract. MORPC versions are cumulative and must be
allocated in actual merge order: concurrent feature branches that provisionally
use v64 must rebase and take the next value before merging. This PR does not
claim interoperability between independently built sibling branches; sender
and receiver continue to fail closed below v64.
Validation / test plan
All commands use the repository CGo wrapper where required:
mo-cgo-test -race -count=1 -timeout=600s ./pkg/sql/plan/function ./pkg/pb/plan — PASS;
closure — 0 issues;
The packet regression proves the protocol-parser -> execute binder ->
expression-executor contract; the retained prepared-cache tests separately
cover same-domain reuse, type replacement, parameter-reference restoration,
and cleanup. No new unbounded cache, wait, goroutine, or per-row scan was
introduced.
The final rebase also included main's #28671 test-infrastructure change; the
owning package suites were rerun on the final head. Fresh CI/BVT on the pushed
head remains the authoritative clean Linux and distributed validation and is
intentionally not awaited here.