fix sql_mode parser modes#25438
Conversation
dffadb5 to
c62ab47
Compare
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? |
c62ab47 to
8cf48fb
Compare
57ca47a to
8fe438e
Compare
XuPeng-SH
left a comment
There was a problem hiding this comment.
Requesting changes: the top-level query parser now honors session sql_mode, but the prepared-string path is still not wired through, so this PR does not close the functional loop for SQL that is prepared from a string or user variable.
Blocking issue:
pkg/frontend/mysql_cmd_executor.go:1315 still parses PrepareString.Sql with mysql.Parse(execCtx.reqCtx, st.Sql, v.(int64)), and pkg/sql/plan/build_dcl.go:77 re-parses the same prepared string with mysql.Parse(ctx.GetContext(), pstmt.Sql, v.(int64)). Both calls force empty parser SQL mode. That means cases like these still parse the inner SQL with the old/default behavior instead of the current session mode:
set sql_mode = 'PIPES_AS_CONCAT';
prepare s from "select 'a'||'b'";
execute s;
set sql_mode = 'ANSI_QUOTES';
prepare s from 'select "id" from "t_ansi"';
set sql_mode = 'NO_BACKSLASH_ESCAPES';
prepare s from "select hex('a\\nb')";
set sql_mode = 'REAL_AS_FLOAT';
prepare s from 'create table t_real_float (r real)';The fix should use the same mode derivation added for normal parsing, i.e. resolve sql_mode, pass it through mysql.SessionSQLModeForParser, and call ParseWithSQLMode / ParseOneWithSQLMode in both the frontend pre-parse and the plan builder re-parse. Please add regression coverage for prepared-string and PREPARE ... FROM @var paths, not only direct parser tests.
Also worth fixing while touching this block: in stmt_kind.go, preStmt.Free() is deferred before checking the parse error. If parsing the prepared string fails, preStmt can be nil and the error path can panic. Move the defer after the err != nil check.
Local checks run:
make err-checkgo test -count=1 -timeout 300s ./pkg/sql/parsers/dialect/mysqlwith local CGo/rpath setupgo test -count=1 -timeout 300s ./pkg/sql/parsers -run 'TestParse|TestSplitSqlBySemicolon'with local CGo/rpath setup
CI was still mostly pending when reviewed.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Requesting changes for one blocker:
pkg/frontend/mysql_cmd_executor.go:2468-2511currently appliessql_modeonly once to the full incoming SQL packet before any statement executes.- MatrixOne enables
CLIENT_MULTI_STATEMENTSby default (pkg/frontend/mysql_protocol.go:54-65), so a request likeSET sql_mode=ANSI_QUOTES; SELECT "c";orSET sql_mode=REAL_AS_FLOAT; CREATE TABLE t(r REAL);still parses the second statement under the old mode. That means session-scoped parser behavior is still wrong for later statements in the same request. - Suggested fix: parse multi-statement packets incrementally (or split/reparse after
SET sql_mode) so a leadingSET sql_modecan affect subsequent statements in that same packet.
I would also add a regression test for the exact same-request pattern above once the execution path is fixed.
…er-modes # Conflicts: # pkg/sql/parsers/dialect/mysql/mysql_sql.go
aunjgr
left a comment
There was a problem hiding this comment.
Approach is correct. Wiring sql_mode into the scanner is the right level. Minor suggestions:
-
reparseAfterStaticSQLModeSetsilently returns on fragment count mismatch — a debug log would help catch unexpected mismatches (e.g., CREATE PROCEDURE bodies with semicolons). -
fragmentHasStatementcould be simplified:strings.TrimSpace(fragment) != ""is sufficient since the parser already validates on reparse. The scanner.Scan() call is redundant. -
defer mysql.PutScanner(scanner)in fragmentHasStatement stacks defers in multi-statement batches. Consider manually putting the scanner back to avoid defer accumulation.
…er-modes # Conflicts: # pkg/sql/parsers/dialect/mysql/mysql_sql.go
What type of PR is this?
Which issue(s) this PR fixes:
Fixes #25407
What this PR does / why we need it:
This wires parser-level
sql_modesettings into the MySQL parser and scanner:ANSI_QUOTESmakes double-quoted text parse as identifiers.PIPES_AS_CONCATkeeps||as concat; otherwise||parses as logical OR.NO_BACKSLASH_ESCAPESkeeps backslashes as ordinary string characters.REAL_AS_FLOATmapsREALcolumns to FLOAT instead of DOUBLE.Root Cause
sql_modeaccepted these mode names, but frontend parsing only passed SQL text andlower_case_table_namesinto the parser. The scanner and yacc actions therefore used fixed behavior regardless of sessionsql_mode.