fix: preserve binary prepared DML and aggregate types - #27493
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? |
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
Request changes on exact head fcdff5af7e8973fbdb0cd62ffef87c2f3658c2c4.
- [P1 correctness] Skipping runtime specialization for the entire DML plan preserves stale prepare-time semantics in domain-sensitive DML expressions.
initExecuteStmtParamWithResolverInSession now excludes every INSERT/UPDATE/DELETE/MERGE plan from FillValuesOfParamsInPlanWithSpecialization (pkg/frontend/computation_wrapper.go, the new preparedDML gate). That protects positional write projections, but it also excludes predicates and computed expressions that still require execute-time type binding.
I reproduced this end to end on this exact head through the MySQL binary prepared-statement protocol:
create table predicate_dst (id int primary key, status int);
insert into predicate_dst values (1, 0);
prepare: update predicate_dst set status = 1 where ? = ?;
execute args: int64(1), "1.00";EXECUTE succeeds, but readback remains status = 0. The comparison retains the prepared TEXT/TEXT overload and evaluates false; the runtime-specialized comparison is numeric and should evaluate true. This is the same semantic class as the mixed-domain prepared comparison regression. Please preserve the write operator’s positional contract without disabling specialization for domain-sensitive DML predicates/expressions, and add this protocol-level regression.
- [P1 type consistency] Runtime aggregate type propagation is still incomplete for window aggregates.
For binary prepared select sum(?) over () from nation with an INT64 value, FillValuesOfParamsInPlanWithSpecialization correctly rebinds WINDOW.WinSpecList[0] to DECIMAL128, but refreshPreparedPlanProjectionTypes only handles Node_AGG and leaves the WINDOW synthetic result in Node_WINDOW.ProjectList as FLOAT64. The final projected/result-column type therefore remains stale, preserving the same executor-vector versus projection/protocol type mismatch this PR fixes for plain SUM(?).
A focused exact-head assertion observes WinSpecList type DECIMAL128 (33) and synthetic WINDOW projection type FLOAT64 (31). Please refresh Node_WINDOW result projections (and downstream positional projections) and cover SUM(?) OVER () with a binary integer parameter.
matrix-meow
added
size/XL
aunjgr
left a comment
aunjgr
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 7964fb32eb7b020f4a8109666aa2cdda0fd7c25d against merge base ae12701a888fac102e9641cc7cbb8706527366d8.
The marker/literal and marker/marker coercion blocker is closed, including nested numeric expressions, IN/BETWEEN families, ASCII-whitespace warnings, and engine-owned DOUBLE conversion. One public-path hole remains.
[P1] Do not exclude numeric-column comparisons from text coercion
preparedRuntimeTextComparisonScanRule.scanExpr only collects text parameters when !preparedExprsContainColumn(function.Args) (pkg/sql/plan/utils.go:3450-3460). Thus an indexed predicate such as int_col = ? is deliberately left on its cached prepare-time implicit integer cast; the new test at prepared_aggregate_params_test.go:437-456 codifies that exclusion.
That cached cast is not semantically equivalent for COM_STMT text packets. The implicit string-to-integer executor calls strict parseSignedCastString (pkg/sql/plan/function/func_cast.go:6293-6307), so "1abc" and "foo" return cast errors before the numeric-prefix/zero conversion and warning path can run. Ordinary MySQL numeric comparison instead consumes prefix 1 from "1abc", or converts a missing prefix to zero, with warning 1292.
Concrete binary-protocol witness:
create table t(id int primary key, status int);
insert into t values (1, 0);
prepare: update t set status=1 where id = ?;
execute parameter: VAR_STRING "1abc"The comparison should match id=1, update the row, and report a truncation warning; this head retains the strict integer cast and errors. The same failure applies to column-owned inequalities and IN/BETWEEN shapes. This is correctness on a common indexed prepared predicate, not merely a missed optimization.
Preserve the indexable column side, but give the text parameter an engine-owned MySQL numeric-prefix conversion into the column/common numeric domain instead of skipping specialization. Add public COM_STMT regressions for heap and indexed columns with complete numbers, prefixes, missing prefixes, range overflow, NULL, and the comparison/IN/BETWEEN families. Explicit CAST must remain authoritative.
The earlier DML write-root, derived/scalar projection, aggregate/window metadata, polymorphic aggregate, finite exponent, and marker-only coercion gaps are otherwise closed. I found no additional confirmed blocker.
aptend
left a comment
aptend
left a comment
There was a problem hiding this comment.
Deep re-review completed on exact head 3445fdcbc567308aa997f3ffae77f06412436a44. I read the complete review, inline-comment, reply, and resolved/unresolved-thread history; compared the increment from my previous reviewed head d82bdee81d79711401287ae434ed1ef74058e49b and the later reviewer head 7964fb32eb7b020f4a8109666aa2cdda0fd7c25d; and rechecked the full PR diff. The earlier nested-write, aggregate/window, numeric-prefix, missing-prefix, fractional, and range-overflow blockers are closed. Two blocking precision holes remain when the new optimization casts an already-rounded DOUBLE parameter back into the column domain.
Validation on this exact head:
make thirdpartiesand the required CGO environmentgo test ./pkg/sql/plan ./pkg/frontend ./pkg/sql/colexec -count=1: passgo test ./pkg/tests/issues -run '^TestIssue27443BinaryPreparedDMLAndAggregate$' -count=1: pass- focused prepared/binary plan/frontend tests at count 10 and under
-race: pass go vet ./pkg/sql/plan ./pkg/frontend ./pkg/sql/colexec: passgit diff --check refs/codex/review/pr27493-base...HEAD: pass- temporary public COM_STMT BIGINT precision counterexample: expected 2 matching rows, got 1
- temporary public COM_STMT DECIMAL precision counterexample: expected the exact decimal row to update, status remained 0
The temporary counterexample tests were removed and the review worktree is clean.
…pared-dml-review # Conflicts: # pkg/tests/issues/issue_26095_test.go # pkg/tests/issues/issue_26111_test.go
|
@aptend The two latest precision blockers are fixed in ed0c2f2: exact BIGINT precision-loss detection, DECIMAL/text filters kept in the common DOUBLE domain, and separate typed lock-key handling. Public binary-protocol regressions cover both cases. The branch is merged with current main and the new CI run is in progress. Please re-review when convenient. |
|
The failing checks were not flaky. In run 32937384412, both independent multi-CN BVT jobs failed deterministically at I merged the latest |
aptend
left a comment
aptend
left a comment
There was a problem hiding this comment.
The previous integral-BIGINT and DECIMAL blockers are addressed, but the precision fallback still misses fractional text prefixes that round to an integral DOUBLE. That leaves a silent wrong-row DML case in the same public binary-protocol path, so this head still needs changes.
aunjgr
left a comment
aunjgr
left a comment
There was a problem hiding this comment.
Approved at exact head b455c356e46b5fd066986c3c1fbaf2ab4bd675f0 against base 7792fcaa09cfd24abd6d1070ec7213c229b6e02b.
The execute-time design now preserves only the positional outer DML assignment contract while recursively specializing domain-sensitive predicates, CASE/arithmetic expressions, derived/scalar subqueries, aggregate/window results, and result metadata on an isolated plan copy. Numeric text comparisons use the engine DOUBLE conversion path, including prefix/missing-prefix/range behavior, while exact column-domain casts remain available when safe and LOCK_OP receives a typed non-key fallback rather than an approximate key.
The latest precision check compares the complete numeric prefix as an exact rational with the exact rational represented by the parsed float64. That catches integral and fractional prefixes rounded by DOUBLE—including 9007199254740992.5—before narrowing them into BIGINT, while exactly representable prefixes retain the indexable column path. Explicit DECIMAL protocol domains remain separate. Public COM_STMT regressions cover the original silent DML loss, SUM/MAX_BY metadata, nested writes, numeric coercion, BIGINT/DECIMAL precision, and the final fractional boundary. I found no remaining blocker.
# Conflicts: # pkg/frontend/computation_wrapper.go # pkg/frontend/computation_wrapper_test.go # pkg/frontend/prepared_explain_test.go # pkg/sql/plan/utils.go # pkg/sql/plan/utils_test.go # pkg/sql/plan/visit_plan_rule.go # pkg/sql/plan/visit_plan_rule_test.go
aptend
left a comment
aptend
left a comment
There was a problem hiding this comment.
Deep re-review of exact head 54ddca9 found one blocking COM_STMT regression below. I independently verified that the previous fractional numeric-prefix blocker is closed for the original case plus exponent-form, trailing-suffix, and negative tie-to-even variants. The relevant plan/frontend/colexec suites, focused frontend race run, and vet pass.
aptend
left a comment
aptend
left a comment
There was a problem hiding this comment.
Deep re-review completed on exact head 5a7062c13b98c560fd79382e07deb3f32ef2eeaa against base 6b8987741df912f38cece5bd026349d79672faf9. I read the complete review, inline-comment, reply, and resolved/unresolved-thread history; compared the increment from my previous reviewed head 54ddca9e49a825c510f75fc221f258c74653794b; and rechecked the full PR diff and the merged-main result.
The last direct-result invalidation blocker is closed: the later specialization pass now contributes to, rather than overwrites, the accumulated invalidation state, while runtime-cache installation still depends on the later pass itself. The focused unit and public COM_STMT regressions cover the former SELECT ?, ? = ? cached-TEXT-compile crash. I also independently exercised repeated execution of that same server-side statement while switching the direct result across INT64, FLOAT64, TEXT, and back to INT64, with both no-op and numeric trailing comparison scans; result vectors and metadata remained consistent.
The earlier DML write-root, mixed predicate, derived/scalar projection, aggregate/window metadata, polymorphic aggregate, numeric-prefix, BIGINT/DECIMAL precision, fractional rounding, and lock-key blockers remain closed on the complete current tree. The merged-main tree exactly matches Git’s automatic merge result.
Validation on this exact head:
make thirdpartieswith the required CGO environmentgo test ./pkg/sql/plan ./pkg/frontend ./pkg/sql/colexec -count=1: passTestIssue27443BinaryPreparedDMLAndAggregatethrough COM_STMT_PREPARE/EXECUTE: pass- focused planner/frontend race tests at count 5: pass
- affected-package
go vet: pass git diff --check: pass- current required checks observed so far are passing; remaining GitHub jobs were still running at review time
No blocking issue found.
|
@mergify refresh |
Merge Queue Status
This pull request spent 15 seconds in the queue, including 2 seconds running CI. Required conditions to merge
|
✅ Pull request refreshed |
5 participants
What type of PR is this?
Which issue(s) this PR fixes:
issue #27443
What this PR does / why we need it:
Binary COM_STMT_EXECUTE currently specializes DML plans after prepare-time binding. Replacing parameter expressions on that execute-time copy can break the positional projections consumed by INSERT/UPDATE operators, causing successful statements to lose their writes. Keep INSERT/UPDATE/DELETE/MERGE on the cached parameterized compile path while retaining runtime specialization for read plans.
Also propagate execute-time aggregate result types through synthetic aggregate/project column references, so
SUM(?)with an integer binary parameter keeps its DECIMAL128 vector and result metadata in sync.The regression test uses the MySQL binary protocol (
interpolateParams=false) and covers REPLACE ... SELECT, a mixed-type UPDATE, andSELECT SUM(?).