fix: protocol-level prepared select ? + ? fails at prepare and panics at execute (#25423)#25452
Draft
ck89119 wants to merge 5 commits into
Draft
fix: protocol-level prepared select ? + ? fails at prepare and panics at execute (#25423)#25452ck89119 wants to merge 5 commits into
select ? + ? fails at prepare and panics at execute (#25423)#25452ck89119 wants to merge 5 commits into
Conversation
…atrixorigin#25423) Protocol-level prepared statements such as `select ? + ?` failed during COM_STMT_PREPARE with `invalid argument operator +, bad value [TEXT TEXT]`. `baseBindParam` unconditionally bound every unknown parameter as `T_text`, so the `+` resolver saw two concrete TEXT operands and rejected the arithmetic before the real values arrive with COM_STMT_EXECUTE. Bind the parameter as `T_any` instead, letting the existing function type deduction promote both unknown params to a numeric type (`? + ?` -> int64). `T_any` alone is not enough: parameter values are always stored by the frontend as text bytes, and `ResetPreparePlan` keeps the `ParamRef` in the compiled plan rather than substituting typed constants. At runtime `ParamExpressionExecutor` materializes the value via `NewConstBytes(paramRef.Typ, textBytes)`; a `T_any` type has element size 0 and panics in NewConstBytes/SetConstBytes (and `cast(T_any -> int64)` would otherwise be treated as scalar NULL). Materialize an unresolved `T_any` parameter as `T_text` so the const has a real element size and the planner-inserted cast converts the text to the wanted type. Real text columns keep their existing semantics: `text_col + text_col` still errors with `[TEXT TEXT]`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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? |
…ID cast (matrixorigin#25423) Three CI failures when prepared-statement params are T_any instead of T_text: 1. "?"/"%" with T_any + BIGINT (/ "%"): fixedTypeCastRule2 had a single-T_any special case that overrode the initFixed2 table. For `10/?`, the override returned {BIGINT, BIGINT} instead of the correct {FLOAT64, FLOAT64} that the table specifies, and divOperatorSupports rejects BIGINT. Remove the override; let the comprehensive initFixed2 rules handle the cast. 2. "0.0 + ?" with a large parameter: initFixed1 mapped {DECIMAL64, ANY} to {DECIMAL64, DECIMAL64} whereas {DECIMAL64, TEXT} mapped to {FLOAT64, FLOAT64}. The narrower decimal type caused overflow at EXECUTE. Align the T_any rules with the T_text rules (to float64) so that any parameter can carry values beyond the decimal range. 3. "meta_scan(?)": the cast from T_any to UUID was missing from supportedTypeCast[T_any], though T_text had it. Add T_uuid to the supported targets for T_any. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…matrixorigin#25423) Replace the T_any approach (which conflated unresolved params with SQL NULL) with a targeted binder rewrite: when both arguments of an arithmetic operator (+, -, *, /, %) are T_text prepared-statement parameter references (Expr_P), promote each to float64 via an implicit cast before the function type check. This avoids three P1 regressions that the T_any approach introduced: - ? = ? remained text comparison ("01" != "1") rather than silently switching to integer comparison. - DECIMAL + NULL kept its output schema (DECIMAL) since T_any is unchanged. - PREPARE metadata stayed T_text/MYSQL_TYPE_VARCHAR, matching EXECUTE data. Real TEXT column arithmetic is untouched (column refs are not Expr_P, so the rewrite does not fire), preserving the existing TEXT + TEXT error semantics. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Addresses SCA ineffassign lint finding — the error from casting the second prepared-statement parameter to float64 was not immediately checked, though it was caught by the enclosing err check. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
Which issue(s) this PR fixes:
issue #25423
What this PR does / why we need it:
Protocol-level prepared statements such as
select ? + ?(e.g. viamysql-connector-python'scursor(prepared=True)) failed duringCOM_STMT_PREPAREwith:Root cause
baseBindParambound every unknown prepared-statement parameter asT_text.For
? + ?the+resolver therefore saw two concreteTEXToperands andrejected the arithmetic at PREPARE time, before the real values arrive with
COM_STMT_EXECUTE. The function type system already knows how to deduceT_any + T_anyinto a numeric type, and string-context binders (LIKE, etc.)already handle
T_anyexplicitly.Fix (two parts)
pkg/sql/plan/base_binder.go: bind an unresolved parameter asT_anyinstead of
T_text, so the existing type deduction promotes? + ?to anumeric type (
int64).pkg/sql/colexec/evalExpression.go:T_anyalone is not enough. Parametervalues are always stored by the frontend as text bytes, and
ResetPreparePlankeeps theParamRefin the compiled plan instead ofsubstituting typed constants. At runtime
ParamExpressionExecutormaterializes the value via
NewConstBytes(paramRef.Typ, textBytes); aT_anytype has element size 0 and panics inNewConstBytes/SetConstBytesatCOM_STMT_EXECUTE(andcast(T_any -> int64)wouldotherwise be treated as scalar
NULL). We materialize an unresolvedT_anyparameter as
T_text, so the const has a real element size and theplanner-inserted cast converts the text bytes to the wanted type. This keeps
the pre-existing "params materialize as text" behavior while decoupling the
planning-time type from the runtime materialization type.
Real
TEXTcolumns keep their existing semantics:text_col + text_colstillerrors with
[TEXT TEXT]; this PR does not relax arithmetic for real textvalues.
Verification
mysql-connector-python9.7.0cursor(prepared=True)select ? + ?with(1, 2)returns3on both the C-extension and purePython paths.
PREPARE ... EXECUTE USINGreturns3.> ?int comparison,= ?stringequality,
LIKE ?,UPDATE ... = ?,INSERT (?,?,?).TestPrepareArithmeticParams(plan) andTestParamExpressionExecutorAnyType(colexec).pkg/sql/planandpkg/sql/colexecpass;go vetandgit diff --checkclean.Known boundary (pre-existing limitation, out of scope): because params are
stored as text and
any + anydeduces toint64at PREPARE, passing anon-integer such as
2.5to? + ?errors atcast to int. Before this fixthe statement could not even be prepared, so this is a strict improvement.
PR Checklist