fix variables - #26762
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? |
iamlinjunhong
left a comment
There was a problem hiding this comment.
Reviewed the complete diff from merge-base 6798bd63884c3fb363589565f925fd16f94eccbe to head 2b290fc5f5c05f3be1105316f42a4c82df23b04b, including parser generation, frontend compile/execute paths, prepared reuse, planner binding, and tests. Requesting changes for two P1 correctness issues; one P2 performance issue is also recorded inline. No P0 or P3 findings.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Request changes on exact head 2b290fc5f5c05f3be1105316f42a4c82df23b04b.
I independently traced the parser → planner → frontend/status execution paths and found three blocking correctness gaps:
SELECT ... INTO @varsvalidates expression/variable cardinality only after receiving a non-empty batch.select 1 where false into @a, @btherefore succeeds and leaves the variables untouched, while MySQL 8.4 rejects it with error 1222 regardless of row count. I reproduced the silent success through MatrixOne's embedded SQL path; the structural check must be independent of runtime result cardinality.- The background execution path installs and fills
selectIntoUserVariables, butexecuteStatusStmtInBackonly callsrunner.Runand never callsapply. Stored-procedure SQL uses this path, soSELECT ... INTO @varcan report success without assigning the variable. - Capture keeps only
[]anyand assignment callsSetUserDefinedVar, which hard-codesIsBin=false. Binary-string metadata from the result vector is therefore lost. Later prepared execution (EXECUTE ... USING @v) consultsResolveVariableIsBin, so values assigned by this new syntax can change type/lookup semantics compared with the existingSETpath.
There is also an avoidable unhappy-path cost: the collector detects a second row but does not stop execution, and reports the error only after the entire query has completed. Large inputs continue scanning and transporting rows after the outcome is already known.
Focused parser, planner, frontend, and collector tests pass, but they do not cover these execution/metadata/zero-row boundaries. The zero-row counterexample fails against this head exactly because MatrixOne returns nil error.
aunjgr
left a comment
There was a problem hiding this comment.
Reviewed the exact head. Requesting changes for three P1 correctness gaps. First, pkg/frontend/select_into_user_variables.go:61 returns before validating expression and variable arity for a zero-row result, so SELECT 1 WHERE FALSE INTO @A,@b silently succeeds. Validate structural arity independently of runtime batches. Second, line 91 stores only the extracted value and loses the source vector binary flag; preserve per-column IsBin metadata and use the binary-aware setter because EXECUTE USING depends on it. Third, the background and stored-procedure path installs the collector but executeStatusStmtInBack never calls apply, so SELECT INTO reports success without assigning variables. Also return the too-many-rows error as soon as rowCount exceeds one instead of scanning the remaining result.
|
|
iamlinjunhong
left a comment
There was a problem hiding this comment.
Reviewed the complete diff from merge-base a4b0ce286d182c24efd5a349620ae37016262301 to exact head ce302fc95481492fd2714acb65cdc34251ffc42c, including parser generation, SELECT-INTO normal/background/prepared execution, user-variable type binding and value reconstruction, diagnostics, lifecycle/Q1-Q3 paths, and tests. The author explicitly replied to the previous P2 comments in the PR conversation, and those prior findings are addressed on this head.
This pass confirms three new P1 correctness defects: array-valued user variables can be reconstructed with invalid raw bytes and panic, TIMESTAMP user variables can shift across session/process time zones, and INTO clauses nested in UNION/parenthesized query trees can be silently dropped. No P0, P2, or P3 findings. Requesting changes because P1 blockers remain.
All 26 GitHub checks are terminal with no failures, and git diff --check is clean. A PR-specific targeted-test worktree could not be created because this isolated repository exposes .git/worktrees read-only; no code or worktree files were modified.
aunjgr
left a comment
There was a problem hiding this comment.
Re-reviewed exact head ce302fc after the successful CI rollup. The follow-up closes the earlier blockers: zero-row arity is validated before execution, second rows fail during capture, binary/type metadata is retained, and frontend/background execution both apply the collected variables.
|
What type of PR is this?
Which issue(s) this PR fixes:
issue #25123
#24492
What this PR does / why we need it:
支持未赋值用户变量读取返回 NULL,不再报 “user variable does not exist”。
支持 SELECT ... INTO @var,包括多变量赋值、空结果不覆盖旧值、多行结果报错。
修复用户变量数值表达式:
修复 prepared statement 参数数值上下文:
补充了 planner 单测和 BVT case: