Repository navigation
fix(parser): reject empty CTE bodies to avoid Format panic - #30151
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? |
|
mo-bot review · Commit: Review completed. Read the assessment. |
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
mo-bot review · mo-bot-local · review
Commit: f149b512a6d24b0f1af22aa86f781833221c6608
Assessment: request_changes
Reviewed the complete three-file delta and parser, CTAS, and PREPARE consumers. The shared parser guard is a coherent fix with negligible cost and no confirmed implementation defect. Required public SQL validation remains unverified.
Model: gpt-6-astra / xhigh
Session: 01a12d8f-7dea-77c2-a565-38d45b5564e4 · slot 0 · generation 0
- P2
pkg/sql/parsers/dialect/mysql/empty_cte_body_test.go:37: Validation requirement: empty CTEs submitted through CTAS or PREPARE, followed by another statement on the same connection.
Impact: The tests establish parser rejection but do not verify frontend error handling, absence of internal-panic responses, or session recovery—the public behavior this fix promises.
Evidence: The repository testing contract requires public-path proof for SQL-visible errors. No empty-CTE BVT is added; local_validation is empty, and supplied CI does not cover this behavior. The author-reported E2E logs are unavailable and lack candidate build/exit receipts. Add a minimal public SQL regression or provide traceable equivalent execution evidence covering empty-body SELECT/CTAS/PREPARE, valid controls, and SELECT 1 after rejection.
Validation:
-
Isolation preflight passed; native prerequisites verified against 192 matching source inputs, 62 artifact hashes, and the original build-owner receipt/log.
-
Uncached mysql parser and public parsers package tests passed, including all 11 new cases; parser-tests.log and receipt 1791961025162944634-3.json.
-
Additional parser-entrypoint, SQL-mode, nested/comment-only body, PREPARE parsing, and reuse checks passed; cte-boundary.log.
-
Base-parser overlay rejected the new assertions in all six negative cases as expected; base-negative-control.log. This was a controlled baseline failure.
-
Generated executable code matched the grammar; explicit go vet passed.
-
Commands, clean-head fingerprints, terminal exits, and log hashes reconciled in /home/xupeng/.local/state/mo-bot/jobs/1f47f6dc93ce15eaf163ba7b/attempt-2/validation-manifest.json.
Limitations:
-
No live SQL service/BVT run was performed; author-reported service results remain unverified.
-
The linked issue's full text was not supplied; its reproduction and intended behavior were reviewed from the PR description.
-
Performance assessment is structural: one nil comparison per CTE reduction, with no added work on non-CTE paths.
iamlinjunhong
left a comment
iamlinjunhong
left a comment
There was a problem hiding this comment.
已审查 head f149b512a6d24b0f1af22aa86f781833221c6608，同步后的 origin/main 为 a53390c57de48ac280249ff0e88831e502e20fb0。完整 Git diff 与 Loom 提供的 diff 一致：3 个文件，71 行新增。
结论：COMMENT。确认 1 个 P2 非阻塞测试覆盖缺口；未发现 P0/P1 或其他实现缺陷。
- [P2] 补充 PREPARE 入口及错误后的连接恢复回归测试 —
pkg/sql/parsers/dialect/mysql/empty_cte_body_test.go:37。触发场景：同一连接执行PREPARE s FROM 'WITH a AS () SELECT 1'，随后执行SELECT 1。现有负例都直接调用mysql.ParseOne，覆盖 SELECT/CTAS 的 parser 拒绝，却没有覆盖 PREPARE 字符串内部 SQL 的再次解析和 frontend 错误处理。grammar 的FROM STRING分支只构造PrepareString；内部 SQL 在parsePreparedStatement或buildPrepare中重新解析，成功后才会进入ParameterCount的 AST 格式化。这正是本修复所涉及的另一条原始 panic 路径，现有测试无法验证客户端收到正常错误以及连接仍可使用。建议增加 frontend 或公共 SQL 自动回归：上述非法 PREPARE 返回语法错误，之后同一连接的SELECT 1成功，并加入合法非空 CTE 的 PREPARE 对照。PR 描述中的手工 E2E 结果尚未形成这条路径的自动回归覆盖。
实现检查：grammar 与生成的 parser 均在构造 CTE 前检查 nil body，错误记录和 ret1 退出方式与现有 parser 一致；各解析入口会返回该错误，scanner/lexer 的复用会重置状态。已核对 CTAS 的 buildTableDefs 格式化、PREPARE 字符串解析、buildPrepare/ParameterCount 及原生 COM_STMT_PREPARE 调用链。新增成本为每个 CTE 一次 nil 判断，未发现新增共享状态、生命周期问题或合法 SQL 兼容性变化。
验证范围：完整 diff 和 11 个新增测试用例已逐项检查；git diff --check 通过。本次未创建 worktree，未运行 Go 测试或 SQL 服务，未独立复验 PR 描述中的 E2E/BVT 结果。
Loom 快照中的 5 个检查均已结束：2 success、2 neutral、1 skipped；其中 Iceberg E2E 被跳过。PR 的 mergeable=true，mergeable_state=blocked。这些检查未提供本修复所需的 PREPARE/CTAS 公共 SQL 回归证据。
历史 review、inline comments 和 PR conversation 已读取：没有 iamlinjunhong 的上轮 review，也没有作者对已有 P2 的对应回复；已有其他审查者的 review 不改变本次首次 review 的事件规则。仅存在 P2，因此选择 COMMENT。
|
Reply to the mo-bot review — P2 "public SQL path verification". Status, stated explicitly in three parts: 1. In-PR evidence (verified). The parser fix is exercised by unit tests ( 2. Not evidenced in this PR (as the review notes, and I agree). A traceable public-path trace — empty-CTE CTAS/PREPARE returning an error and the same connection continuing with 3. Proposed resolution, maintainer's call:
If there is no objection I will deliver (a) in the next scheduled round. Per repo routing, @XuPeng-SH is being added as requested reviewer. CI note at time of writing: 本操作由LW授权的MOI智能体提交 |
|
回访更新（2026-10-14 08:06Z 读数，head
本操作由LW授权的MOI智能体提交 |
|
由LW授权的MOI智能体提交。 回复 head
本轮仅回复评审意见，不改动本 PR 代码；不合并、不关闭。跟进 PR 建立后在本线程回链。 |
|
Replying to @XuPeng-SH (mo-bot review) and @iamlinjunhong — we agree with both P2s; they are legitimate gaps and must be closed by public-SQL regression evidence, not by this PR's parser-level tests alone. Scope rationale: this PR intentionally stays minimal (grammar + regenerated parser + 11 in-process negative cases, 3 files) and proves only parse-time rejection. What it does not prove is the public behavior in #30146: CTAS/PREPARE entrypoints, frontend error delivery (1064 instead of internal panic), and same-connection recovery — and as mo-bot notes, the E2E output in the PR description was manual and untraceable, so it cannot serve as CI evidence. Plan to close the gap in a follow-up regression PR, kept independent of this fix:
CC @XuPeng-SH @iamlinjunhong — please confirm this scope split is acceptable; the follow-up will link back here. 由LW授权的MOI智能体提交 |
|
BVT-only 跟进 PR 用例草案（状态更新：本 PR 自 2026-10-14 10:33:35Z 后无任何新活动/回复，对既定跟进方案无维护者反对意见；按既定方案继续。由LW授权的MOI智能体提交） 两位评审要求的公共 SQL 路径回归，拟作为独立跟进 PR 交付，新增 2 个文件（不改动本 PR）：
-- empty CTE body must be rejected as a syntax error on public SQL paths,
-- without internal panic, and the session must stay usable after the error.
prepare s from 'with a as () select 1';
select 1;
prepare s2 from 'with a as (select 1) select * from a';
execute s2;
deallocate prepare s2;
create table t_ctas_empty as with a as () select 1;
create table t_ctas_ok as with a as (select 1) select * from a;
select * from t_ctas_ok;
drop table t_ctas_ok;
select 1;覆盖点（与评审意见一一对应）：
证据计划（RED→GREEN）：
执行通道说明：本次执行窗口无提交通道（沙箱未绑定、无 commit/push 能力，fork 上仅有验证型 workflow），故本轮先固化草案供预审，提交与 RED→GREEN 记录在具备通道的下一窗口完成；如有路径/命名/用例集调整意见，可直接在本线程指出。 本轮无代码改动，未合并/关闭任何 Issue，未触发/重跑/取消 CI。 由LW授权的MOI智能体提交 |
XuPeng-SH
left a comment
XuPeng-SH
left a comment
There was a problem hiding this comment.
APPROVE — independently verified public SQL behavior
Reviewed head f149b512a6d24b0f1af22aa86f781833221c6608 against merge-base a53390c57de48ac280249ff0e88831e502e20fb0 (all three changed files, +71/-0).
The shared grammar boundary is the correct owner: reject a nil CTE statement before constructing the AST, covering all parser consumers without duplicating checks in formatting or execution. Grammar and generated action agree. No blocking implementation, lifecycle or compatibility defect found. Cost is one nil comparison per CTE reduction, with no added non-CTE execution work; this is structural analysis, not a throughput benchmark.
Previous P2: public-path validation now supplied
I built this revision's native prerequisites and ran a temporary reviewer-owned test in pkg/embed through a real single-CN cluster and the MySQL TCP frontend, using one pinned sql.Conn (no transparent connection replacement).
| Behavior | Observed / asserted |
|---|---|
| Empty-body SELECT, comment-only body, recursive and multi-CTE cases | 1064 / 42000 syntax error |
| Empty-body CTAS | 1064 / 42000; no table created |
SQL PREPARE bad_stmt FROM 'WITH a AS () SELECT 1' |
1105 / HY000 with syntax-error message, no panic |
Binary COM_STMT_PREPARE of empty-body CTE |
1064 / 42000 |
SELECT 1 immediately after every rejection |
Success on the same physical connection |
| Valid CTE SELECT, SQL PREPARE/EXECUTE, binary prepare/execute, CTAS/readback | Correct value 1 |
Important correction to the proposed follow-up expectation: SQL PREPARE does not currently return 1064 here. The unchanged inner-parser/frontend error mapping produces 1105. The unrelated control PREPARE unrelated_syntax FROM 'SELECT FROM' also returns 1105, and the mapping/buildPrepare/frontend files are unchanged from base. This is not a new error-mapping branch introduced by this fix. The panic is eliminated. A future regression must record actual behavior rather than blindly expecting 1064 for every entrypoint.
Commands (Go 1.27.1 / Linux amd64, disk-backed TMPDIR and GOTMPDIR, candidate-native build):
make cgo
.agents/skills/mo-dev/scripts/mo-cgo-test -p=2 -count=1 -timeout=180s ./pkg/sql/parsers/dialect/mysql ./pkg/sql/parsers
.agents/skills/mo-dev/scripts/mo-cgo-test -p=2 -v -count=1 -timeout=300s -run '^TestPR30151PublicSQLReview$' ./pkg/embedTerminal results: native build exit 0; mysql parser package PASS (0.089s); public parser package PASS (0.032s); public SQL probe PASS (test 5.46s, package 5.516s), exit 0. The first strict probe failed specifically on expecting 1064 for SQL PREPARE; it was investigated with the unrelated malformed-PREPARE control before rerunning with explicit entrypoint-specific error assertions.
Exact-head CI run 38524474551 completed successfully, including UT, SCA and pessimistic/proxy multi-CN BVT. Skipped checks are not counted as passes. I did not rerun full BVT or benchmark the workload locally.
This supplies the traceable equivalent execution evidence requested by my earlier P2; that validation blocker is closed. A small committed public-SQL regression remains worthwhile for lasting coverage, but is non-blocking with this execution proof and the existing parser regressions. No PR commits were changed.
Local evidence: /home/xupeng/.cache/pr30151-parser.log, /home/xupeng/.cache/pr30151-public-sql-strict.log, /home/xupeng/.cache/pr30151-public-sql2.log, /home/xupeng/.cache/pr30151-review-probe.go.
Final SQL log SHA256: 26292008308859fb9ba9df94cccfae473f478aeff4f4e4e87f0a1ec5476555db.
Reproducible reviewer probe (temporary, not committed)
package embed
import (
"context"
"database/sql"
"errors"
"fmt"
mysqlclient "github.com/go-sql-driver/mysql"
"github.com/stretchr/testify/require"
"strings"
"testing"
"time"
)
func TestPR30151PublicSQLReview(t *testing.T) {
RunSingleCNBaseClusterTests(t, func(c Cluster) {
ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute)
defer cancel()
cn, err := c.GetCNService(0)
require.NoError(t, err)
db, err := sql.Open("mysql", fmt.Sprintf("dump:111@tcp(127.0.0.1:%d)/?interpolateParams=false", cn.GetServiceConfig().CN.Frontend.Port))
require.NoError(t, err)
defer db.Close()
conn, err := db.Conn(ctx)
require.NoError(t, err)
defer conn.Close()
exec := func(q string) { t.Helper(); _, err := conn.ExecContext(ctx, q); require.NoError(t, err, q) }
check := func(q string, want int) {
t.Helper()
var got int
require.NoError(t, conn.QueryRowContext(ctx, q).Scan(&got), q)
require.Equal(t, want, got, q)
}
reject := func(q string) {
t.Helper()
_, err := conn.ExecContext(ctx, q)
require.Error(t, err, q)
var me *mysqlclient.MySQLError
require.True(t, errors.As(err, &me), "%v", err)
want := uint16(1064)
if strings.HasPrefix(q, "prepare ") {
want = 1105
}
require.Equal(t, want, me.Number, "%v", err)
require.NotContains(t, me.Message, "panic")
t.Logf("REJECT %s => %v", q, err)
check("select 1", 1)
}
exec("create database pr30151_review")
defer conn.ExecContext(context.Background(), "drop database if exists pr30151_review")
exec("use pr30151_review")
for _, q := range []string{"with a as () select 1", "with a as (/* empty */) select 1", "with recursive a as () select 1", "with a as (select 1), b as () select * from a", "create table bad_ctas as with a as () select 1", "prepare bad_stmt from 'with a as () select 1'", "prepare unrelated_syntax from 'select from'"} {
reject(q)
}
stmt, err := conn.PrepareContext(ctx, "with a as () select 1")
require.Error(t, err)
if stmt != nil {
stmt.Close()
}
var me *mysqlclient.MySQLError
require.True(t, errors.As(err, &me))
require.Equal(t, uint16(1064), me.Number)
t.Logf("COM_STMT_PREPARE => %v", err)
check("select 1", 1)
check("with a as (select 1 as x) select x from a", 1)
exec("prepare good_stmt from 'with a as (select 1 as x) select x from a'")
check("execute good_stmt", 1)
exec("deallocate prepare good_stmt")
exec("create table good_ctas as with a as (select 1 as x) select x from a")
check("select x from good_ctas", 1)
check("select count(*) from information_schema.tables where table_schema='pr30151_review' and table_name='bad_ctas'", 0)
good, err := conn.PrepareContext(ctx, "with a as (select 1 as x) select x from a")
require.NoError(t, err)
defer good.Close()
var got int
require.NoError(t, good.QueryRowContext(ctx).Scan(&got))
require.Equal(t, 1, got)
check("select 1", 1)
t.Log("PUBLIC_SQL_ALL_PASS: SELECT/CTAS/SQL PREPARE/binary PREPARE, errors 1064, same physical connection recovery, valid controls, no invalid table side effect")
})
}
|
Queued — the merge queue status continues in this comment ↓. |
Merge Queue Status
This pull request spent 53 minutes 17 seconds in the queue, with no time running CI. ReasonThe pull request can't be updated
HintYou should update or rebase your pull request manually. If you do, this pull request will automatically be requeued once the queue conditions match again. Requeued — the merge queue status continues in this comment ↓. |
matrix-meow
added
size/XXL
|
CI readback / CI 跟进（由 LW 授权的 MOI 智能体提交，identity: MatrixAdventurer / MOI Agent）： 本轮回访结论（2026-10-18 22:09 前后）
已执行操作
供 @XuPeng-SH 参考判断；如 attempt 2 仍以同类形态失败，建议由有权限者从 Actions UI 拉取失败 job 日志（接口侧无法取回）。 |
Merge Queue Status
This pull request spent 30 seconds in the queue, with no time running CI. ReasonThe pull request can't be updated
HintYou should update or rebase your pull request manually. If you do, this pull request will automatically be requeued once the queue conditions match again. Tick the box to put this pull request back in the merge queue (same as
|
What type of PR is this?
Which issue(s) this PR fixes:
Fixes #30146
What this PR does / why we need it:
stmtallows an EMPTY alternative for bare trailing semicolons. The same production is used insidecommon_table_expr(AS '(' stmt ')'), soWITH a AS () …built a CTE withStmt == nil. Paths that format the AST (CTASbuildTableDefs, PREPAREParameterCount) panic. MySQL rejects the same text as a syntax error.Fix: in
common_table_expr, if the body statement is nil,yylex.Error("empty common table expression body")andgoto ret1.Files:
mysql_sql.y+ matching action inmysql_sql.go(+7/+7); regression testempty_cte_body_test.go.Fixes #30146
Evidence
Unit (RED → GREEN)
go test ./pkg/sql/parsers/dialect/mysql/ -run TestEmptyCTEBodyRejectedTestNonEmptyCTEBodyStillParsesPASS; full./pkg/sql/parsers/dialect/mysql/PASSE2E same commit
a53390c5, binariesmo-main30146/mo-emptycte, single-nodeetc/launch:with a as () select 1(1,)prepare s from 'with a as () select 1'CTE.Formatcreate table … as with a as () select 1create table … as with a as (select 42 as id) select * from a42select 1after failuresArtifacts:
/workspace/mo/e2e/30146_before_main.txt,30146_after.txt.BVT (fix binary)
Merge Danger