fix: preserve prepared arithmetic and exact integer semantics - #27795
Conversation
# Conflicts: # pkg/frontend/computation_wrapper.go # pkg/sql/plan/utils.go
XuPeng-SH
left a comment
There was a problem hiding this comment.
[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, andNULLIFremain VARCHAR/TEXT (specialized=falsefor several cases).SUM(?)andAVG(?)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
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
[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
left a comment
There was a problem hiding this comment.
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.
XuPeng-SH
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.
IFFis canonicalized toIFbefore 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 andDatabaseTypeName, 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.
What type of PR is this?
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 ... USINGarithmetic could retain an earlier integer cast when a later execution supplied DECIMAL;2^53+1through DOUBLE, merging adjacent BIGINT values or raising a BIT range error.This PR keeps the two ownership rules separate while validating them together:
The implementation is based on current
mainafter #27483. No code was cherry-picked from the older broad implementation in #27466.Testing
pkg/sql/planandpkg/frontendtest packagesTestIssue27088PreparedDecimalCommonType, including SQL PREPARE and COM_STMT BIGINT UNSIGNED/BIT(64) string, integer, NULL, and reuse transitionsvet=allforpkg/sql/plan,pkg/frontend, andpkg/tests/issuesmake buildprepare_all.sqlin normal mo-tester comparison mode twice on the same final binary:438 total / 437 success / 0 failed / 1 ignored / 0 abnormalin both runs