Skip to content

Fix prepared parameter runtime type handling - #26622

Draft
ck89119 wants to merge 18 commits into
matrixorigin:mainfrom
ck89119:issue-25408-main
Draft

Fix prepared parameter runtime type handling#26622
ck89119 wants to merge 18 commits into
matrixorigin:mainfrom
ck89119:issue-25408-main

Conversation

@ck89119

@ck89119 ck89119 commented Aug 3, 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:

issue #25408

What this PR does / why we need it:

  • Preserve fractional values when the same arithmetic prepared statement is executed after an integer value.
  • Validate the runtime types of prepared LIMIT and OFFSET parameters for both text and binary protocols.
  • Keep failed executions from contaminating subsequent executions and add planner, frontend, and distributed regressions.

@ck89119

ck89119 commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

Deep review result

Reviewed exact head 32661263e7b4ddb0103ecd504ec0271895348949 against base c726a6199094184a2e6ebc7504c3e011982b8976.

Verdict: changes requested. The full regression matrix found four correctness blockers and one test-closure blocker.

Blocking findings

  1. [P0] Dynamic numeric specialization corrupts optimized plans and can panic Hash Join.

    specializePreparedNumericPlan calls FillValuesOfParamsInPlan on an already optimized prepared plan. That path uses DeepCopyPlan / DeepCopyNode, but DeepCopyNode does not preserve SendMsgList or RecvMsgList (and also omits other execution/DML metadata).

    Existing BVT reproducer:

    prepare stmt1 from 'select count(*) from (t1 join t2 on t1.a=t2.a%?)';
    set @value1=2;
    execute stmt1 using @value1;

    Actual result:

    internal error: panic wrong joinmap tag!
    

    constructHashJoin requires the join-map tag that was dropped by the copy. This makes the specialization path unsafe for arbitrary optimized query/DML plans.

  2. [P0] UINT64 mixed numeric coercion is incorrect across commuted and nested expressions.

    The literal restoration and UINT64 special handling in ResetParamRefRule do not preserve the original numeric domain. Existing BVTs show all of the following:

    • 0.0 + uint64_max casts 0.0 to UINT64 and fails with bad value 0.0;
    • 0 + uint64_max changes the previous overflow behavior into a successful UINT64 result;
    • (? + 0.5) + bit casts 0.5 to UINT64 and fails;
    • bit + (0.5 + ?) fails in the same way.

    The fix needs a complete signed/unsigned/decimal/float/BIT type lattice, including operand order and nesting, rather than another literal-specific branch.

  3. [P1] Rebinding a type-sensitive parent changes a BOOL result into an integer/bit result.

    The PR's own distributed regression fails:

    PREPARE numeric_compare FROM 'SELECT (? + 1) > 0 AS r';
    SET @compare_param = 2;
    EXECUTE numeric_compare USING @compare_param;

    Expected BOOL metadata/value: [10,1,0] / true.

    Actual metadata/value: [-7,1,0] / 1.

    The planner tests only assert that specialization returns no error. They do not assert the final type/value. The regression matrix should also cover BETWEEN, IN, CASE, WHERE, HAVING, and JOIN predicates.

  4. [P1] Ordinary BOOL user variables are globally converted to INT64.

    normalizeUserVariableValue converts BOOL values to int64(0/1), and the ordinary SET @var path now calls it unconditionally. This is outside prepared-statement specialization and breaks existing behavior:

    • SET @val = CAST(0 AS BOOL): false becomes 0;
    • boolean expressions stored in user variables: false/true become 0/1.

    If prepared numeric parameters need a numeric runtime type, that conversion should occur at the prepared-parameter extraction boundary without changing global user-variable storage/display semantics.

  5. [P1, test closure] The new LIMIT/OFFSET contract conflicts with an unchanged existing BVT.

    Issue [Bug]: Prepared statement parameter type re-execution and LIMIT validation are not MySQL-compatible #25408 explicitly requires a string LIMIT ? parameter to be rejected, so the new validation behavior is intentional. However, the existing prepare.test still expects SET @b_var = '1'; EXECUTE ... LIMIT ? to return a row, while the new test expects Incorrect arguments to EXECUTE.

    The old regression/result must be migrated to the new contract; as submitted, the repository contains contradictory expectations and CI is guaranteed to fail.

Validation and CI

Local tests on the exact head passed:

  • pkg/common/moerr
  • pkg/sql/parsers/tree
  • pkg/sql/plan/function
  • pkg/sql/plan
  • pkg/frontend
  • git diff --check

This confirms that the current unit tests do not cover the end-to-end result types, optimized-plan metadata, or existing SQL compatibility contracts above.

Current failed checks:

  • Ubuntu UT: TestBasicCluster times out during txn-trace shutdown while waiting in GlobalStats.Get. The current evidence does not attribute this failure to the PR, but the check remains a readiness blocker.
  • Standalone pessimistic BVT: reproduces the Join, UINT64, comparison, BOOL, and LIMIT-contract failures.
  • Compose proxy BVT: reproduces the ordinary BOOL user-variable regression.
  • Coverage: downstream failure because required UT/BVT producers failed.

The later account/snapshot failures in the standalone run occur after the Join panic, server reconnect, and account cleanup timeout; they appear to be cascading contamination rather than additional independent defects.

ck89119 added 2 commits August 4, 2026 18:23
# Conflicts:
#	pkg/frontend/authenticate.go
#	pkg/frontend/plsql_interpreter.go
#	pkg/frontend/plsql_interpreter_test.go
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/XXL Denotes a PR that changes 2000+ lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants