Skip to content

fix: preserve prepared arithmetic and exact integer semantics - #27795

Merged
XuPeng-SH merged 25 commits into
matrixorigin:mainfrom
ck89119:issue-25408-runtime-v2-main
Aug 31, 2026
Merged

fix: preserve prepared arithmetic and exact integer semantics#27795
XuPeng-SH merged 25 commits into
matrixorigin:mainfrom
ck89119:issue-25408-runtime-v2-main

Conversation

@ck89119

@ck89119 ck89119 commented Aug 28, 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
issue #27492

What this PR does / why we need it:

Prepared executions transport SQL user-variable values and binary-protocol string values through text-backed parameter vectors. Two related runtime-specialization gaps could therefore reuse or select an approximate domain that did not match the current execution:

  • EXECUTE ... USING arithmetic could retain an earlier integer cast when a later execution supplied DECIMAL;
  • text-backed BIGINT UNSIGNED and BIT(64) comparisons could pass 2^53+1 through DOUBLE, merging adjacent BIGINT values or raising a BIT range error.

This PR keeps the two ownership rules separate while validating them together:

  • SQL arithmetic rebinding uses the current user variable's logical source type and includes it in the specialization-cache identity;
  • comparison rebinding recognizes text operands against exact integer/BIT peers and materializes an in-range integral numeric prefix directly in the peer domain;
  • fractional, out-of-range, nonnumeric, and DECIMAL string comparisons retain the existing MySQL-compatible DOUBLE fallback;
  • BIT comparisons remain in the BIT domain, preserving the column-side comparison/index contract;
  • specialized literals retain parameter provenance so same-category cached plans read each execution's current value.

The implementation is based on current main after #27483. No code was cherry-picked from the older broad implementation in #27466.

Testing

  • complete pkg/sql/plan and pkg/frontend test packages
  • complete TestIssue27088PreparedDecimalCommonType, including SQL PREPARE and COM_STMT BIGINT UNSIGNED/BIT(64) string, integer, NULL, and reuse transitions
  • CGo-wrapper vet=all for pkg/sql/plan, pkg/frontend, and pkg/tests/issues
  • make build
  • prepare_all.sql in normal mo-tester comparison mode twice on the same final binary: 438 total / 437 success / 0 failed / 1 ignored / 0 abnormal in both runs
  • package coverage runs: planner 79.1%, frontend 64.1%, with focused tests for every new specialization branch

@ck89119 ck89119 changed the title fix: rebind SQL EXECUTE arithmetic parameter types fix: preserve prepared arithmetic and exact integer semantics Aug 28, 2026
@matrix-meow matrix-meow added size/L Denotes a PR that changes [500,999] lines and removed size/M Denotes a PR that changes [100,499] lines labels Aug 28, 2026

@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.

[P1] Complete SQL EXECUTE source-domain propagation before merging

At exact head d36dd00, preparedFunctionArgUsesSQLExecuteNumericSource only admits parents that are already numeric and only the arithmetic/generic-function allowlists. That misses valid prepared numeric result consumers whose prepare-time marker caused the parent to bind as TEXT/DOUBLE.

Concrete exact-head counterexamples using ParamValue{Value: "9007199254740993.5", SourceType: DECIMAL128(17,1), HasSourceType: true}:

  • CASE WHEN 1=1 THEN ? ELSE 1 END, IF, COALESCE, IFNULL, and NULLIF remain VARCHAR/TEXT (specialized=false for several cases).
  • SUM(?) and AVG(?) remain on the FLOAT64 aggregate overload (specialized=false).

A temporary focused test that asserts the SUM(?) result domain is DECIMAL fails on this head: the aggregate and its argument both have type id 31 (FLOAT64). The same test for CASE observes type id 61 (VARCHAR). With this value, the FLOAT64 path cannot preserve the DECIMAL value above 2^53, and the conditional path also exposes wrong result metadata. These are the same re-execution/source-domain contract this PR is fixing, and the binder already explicitly tracks prepared provenance for IF/IFF/CASE and has dedicated SUM/AVG fallback handling, so these are reachable omissions rather than unrelated feature expansion.

Please route SQL user-variable SourceType through every prepared numeric value consumer (including selective result arguments and SUM/AVG), then add execute/reuse regressions that compare value and DatabaseTypeName against the direct user-variable form.

Evidence: changed focused tests pass on exact head; the two counterexample tests fail deterministically in pkg/sql/plan. Full exact-head CI is otherwise green.

@aunjgr aunjgr 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.

Reviewed exact head d36dd00 against main. One blocking source-domain gap remains. preparedFunctionArgUsesSQLExecuteNumericSource first requires the prepare-time parent to be numeric and then admits only the arithmetic/generic numeric allowlists. A marker can make a valid runtime numeric result consumer bind as TEXT/DOUBLE at PREPARE, so CASE result arms are rejected by the parent-type guard; SUM(?) and AVG(?) are excluded as aggregate families. Executing with ParamValue DECIMAL128(17,1) value 9007199254740993.5 therefore leaves CASE/IF/COALESCE-family results in a text domain and SUM/AVG on FLOAT64, losing exact value above 2^53 and/or exposing wrong result metadata. Propagate the current SQL source type through every prepared numeric value/result consumer, including selective CASE/IF/COALESCE/IFNULL/NULLIF arms and SUM/AVG, while retaining non-value/control-argument boundaries. Add execute/reuse tests comparing values and DatabaseTypeName with the direct user-variable form. Existing green CI does not exercise these counterexamples.

@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.

[P1] Complete SQL EXECUTE source-domain propagation with one result-consumer contract

At exact head 150a7e7, the previous CASE/SUM counterexamples are fixed, but the same source-domain invariant still stops at another hand-maintained allowlist. preparedSQLExecuteNumericResultConsumer, preparedFunctionArgUsesSQLExecuteNumericSource, and preparedResultParamPosition admit CASE/IF/COALESCE/IFNULL/NULLIF plus SUM/AVG, while the existing runtime-specialization contract already identifies other result-polymorphic consumers such as GREATEST/LEAST/MIN/MAX/ANY_VALUE.

Concrete exact-head execution counterexample:

set @p = cast(9007199254740993.5 as decimal(17,1));
select greatest(@p, 1);                         -- DatabaseTypeName: DECIMAL
prepare s from 'select greatest(?, 1)';
execute s using @p;                            -- DatabaseTypeName: TEXT

A temporary embedded-engine parity test fails deterministically with expected DECIMAL / actual TEXT. Focused planner counterexamples also leave GREATEST, LEAST, MIN, MAX, and ANY_VALUE unspecialized with TEXT arguments/results. This is observable wire metadata drift and can change downstream common-type, ordering, and exact-value behavior; it is not only a cosmetic type difference.

Please derive SQL EXECUTE value-argument/source-domain ownership from one shared result-polymorphism contract instead of extending another function-name allowlist. Preserve explicit CAST and condition/control-argument boundaries, and add direct-vs-prepared reuse tests that compare both value and DatabaseTypeName for GREATEST/LEAST and type-preserving aggregates.

Performance follow-up for the same revision: the new text-comparison detector performs an O(plan expressions) walk at computation_wrapper.go:1481, then an uncached SQL EXECUTE repeats the same walk in specializePreparedExecutionPlan. Cache or pass the first result, and skip the walk when no runtime parameter is string-backed, so this correctness fix does not add an unconditional prepared-execution hot-path traversal.

Evidence: exact-head CI is green; focused planner test failed for all five omitted consumers; embedded direct/prepared parity failed on GREATEST with DECIMAL vs TEXT.

@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-reviewed the complete diff and all review history at exact head 02af1550b03fccd5dbab374a58aff7e4a8126c7e. The earlier CASE/SUM and GREATEST/LEAST/type-preserving aggregate gaps are closed, and the text-comparison scan is now gated and reused. One blocking alias hole remains in the new shared result-domain contract; details are inline. Exact-head frontend/planner/function tests, focused issue regressions, go vet, and git diff --check pass.

Comment thread pkg/sql/plan/utils.go

@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.

Reviewed the complete base-to-head diff, prior review history, hot-path/cache behavior, protobuf metadata changes, DML lock normalization, unhappy paths, and exact-head tests at 02af1550b03fccd5dbab374a58aff7e4a8126c7e.

[P1] Complete the shared result-domain contract for the IFF alias before merging.

preparedNumericResultPolymorphicFunction includes if but omits iff. This is a real reachable alias, not a spelling-only edge case: the function registry maps both names to the same IFF implementation, while base_binder.go already special-cases name == "iff" when recording prepared numeric provenance. The execution-time source-domain path therefore records the provenance and then rejects the same expression from specialization.

I reproduced this deterministically on the exact head with:

SET @p = CAST(9007199254740993.5 AS DECIMAL(17,1));
SELECT IFF(TRUE, @p, 1);
PREPARE s FROM 'SELECT IFF(TRUE, ?, 1)';
EXECUTE s USING @p;

The focused planner execution reports specialized=false and leaves the prepared iff result at type id 61 (VARCHAR/TEXT), whereas the direct expression has a DECIMAL result domain. This is observable result-metadata/common-type drift and breaks direct-vs-prepared exact numeric semantics above 2^53. Please classify by canonical function identity (or canonicalize aliases before consulting the shared contract), then add direct/prepared and repeated-execution coverage for IFF that compares both value and DatabaseTypeName.

[Design gate] This change has grown beyond an ordinary local bug fix: it adds more than 900 non-generated production lines across frontend parameter acquisition/cache identity, binder provenance, runtime plan rewriting, DML lock normalization, and five new plan-protobuf fields. There is no stable versioned design artifact for the resulting specialization protocol. Please add one that fixes the invariants and ownership boundaries: direct/fresh-prepared/reused-prepared parity; source/provenance ownership; canonical result-consumer classification; explicit-cast, control-argument, and assignment boundaries; cache equivalence/invalidation; LockRows ABI; mixed-version protobuf behavior; execute hot-path budget; and the validation matrix. The repeated missed function families and now an alias are evidence that this contract should not remain distributed across hand-maintained name checks.

The previous CASE/SUM and GREATEST/LEAST/type-preserving aggregate gaps are otherwise closed, and the duplicate unconditional plan walk has been removed. Exact-head CI is green. The branch is now behind main's #27882 change in the same visitor; after resolving the blockers, please rebase and rerun the relevant evidence on the combined code.

@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.

Reviewed exact head b3fff6ad4dbfd62fa3c23b377328e96b4d42e690 against current base 58d26091e0ec0eaab92f6ec0239e934a2c861c6b, including the complete effective diff, prior review history, the merged #27882 temporal-specialization changes, generated protobuf output, hot-path/cache behavior, DML/LockRows boundaries, and SQL/COM_STMT public regressions.

The previous blockers are closed:

  • CASE/common-value, GREATEST/LEAST, type-preserving aggregate/window, and pagination consumers now share one occurrence-level source-domain contract rather than independent execution-time allowlists.
  • IFF is canonicalized to IF before both result-consumer classification and value-argument ownership. The focused planner test and embedded direct/first/reused execution test cover exact DECIMAL values above 2^53 and DatabaseTypeName, so the alias no longer remains in the prepare-time TEXT domain.
  • String-backed comparison detection remains gated and its first result is reused; ordinary prepared executions do not gain an unconditional second plan walk.
  • Explicit CAST, control arguments, assignment roots, pagination, and DML lock expressions retain their fixed consumer domains.

Design review: PASS for docs/design/CLAUDE_prepared_runtime_specialization.md at this head. It records the transport/source/consumer ownership split, canonical result-consumer contract, cache equivalence, LockRows ABI, additive/absent-field protobuf behavior, hot-path bound, and direct/first/reused validation matrix that were missing from the earlier revisions.

Exact-head substantive CI is green (build, Linux UT, SCA, coverage, proxy BVT, and pessimistic multi-CN BVT). I found no remaining merge-blocking correctness, compatibility, performance, liveness, or test-contract issue.

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.

5 participants