fix(function): implement MySQL STATEMENT_DIGEST - #27988
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? |
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? |
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
There was a problem hiding this comment.
Re-review of exact head cbbbfe2: REQUEST_CHANGES for four concrete token-compatibility defects reproduced in the actual digest package.
The previous comment-only classification, parameter rejection, NCHAR escapes, unterminated-dollar error, SQL-mode propagation, hint-adjacency and remote capability findings have corresponding repairs. Further counterexamples remain:
P1 — Executable-comment termination consumes a SQL byte
internal/lexer_handlers.go::handleAsterisks is entered after * has already been consumed. Its two skip() calls consume / and then the next SQL byte. Exact-head Compute("SELECT /*!80000 1 */+2") returns text SELECT ? ?, hash 330b0454d6423fbb864f3707655da47d815faa8923bdaa068e8e9365e6c97a55. Adding a space after the comment, or using SELECT 1+2, returns SELECT ? + ?, hash 18c24a99168954090331d4686d78ade5498aa5cfc6b125c260cd8183fa150bd5.
These inputs have the same executed token sequence; whitespace after an executable comment must not decide whether an operator is hashed. Consume only the remaining slash, and test immediately adjacent operators, identifiers, punctuation, and EOF. MySQL's corresponding end-comment code first ungets * before skipping two bytes; this copy does not.
P1 — Function-only names are still classified as functions without (
adjustKeywordForSQLMode only demotes a function keyword when whitespace is followed by (. It leaves the keyword classification intact when no opening parenthesis follows at all. In default mode, exact-head SELECT COUNT FROM t produces SELECT COUNT FROM t / `1930d3af5bbc393382a240f2386c6cb039d5a12a45f1f89258b81a77b26f60bf`; the identifier form produces `SELECT `COUNT` FROM `t / a5f4bad2563646f69f804186392000d3ce3de55155a1ad91d046500a5febc97c.
MySQL's lexer chooses the function keyword table only when the next effective character is (; otherwise COUNT is an identifier. Separate ordinary keywords from function-only names and apply parenthesis/IGNORE_SPACE rules before lookup, rather than repairing just the spaced-call case. Cover bare names, aliases, punctuation, qualified identifiers and actual calls in both modes.
P2 — DDL NULL heuristic leaks into CTAS expressions
internal/handler.go::observeToken treats any type token after CREATE/ALTER TABLE as the beginning of a column definition. In CREATE TABLE t AS SELECT BINARY 'x'=NULL, BINARY is an expression operator, but it sets inColumnDefinition at depth zero. isNullKeywordContext then retains NULL as DDL syntax.
Exact-head output is CREATE TABLE t AS SELECT BINARY ? = NULL / 8d8b4b29ac505d50d5a2e0722f93dfe666c17eb8f16c566a8909257182387707; replacing that expression literal NULL with 1 gives CREATE TABLE t AS SELECT BINARY ? = ? / ca429dee989da272d5ce071810651889275b0cd9279d3668fbd00b7e378334ba. Both literals should normalize to the latter token form. The same BINARY expression without CREATE TABLE already reduces NULL correctly.
Track actual column-definition/attribute context, terminating it at CTAS query boundaries, or use parser-informed reductions. Preserve NULL as a keyword only in the relevant grammar productions. Add CTAS/type-like expression controls alongside existing column-nullability tests.
P2 — Charset introducer normalization is special-cased to utf8mb4
handleIdentifier recognizes only _utf8mb4. Exact-head SELECT _latin1'x' produces SELECT _latin1 ? / d0ef3b28671afb3efb69ce5ca453f6c46800ff7bec47cf7c8f5d253c25167d49, whereas _utf8mb4'x' produces SELECT (_charset) ? / 04144c90cfef7b8973c07fe5b12181df7996a6db61471dfe8b365d188cea8e19. MySQL classifies recognized charset introducers through charset lookup and emits UNDERSCORE_CHARSET, including latin1. Use the supported charset registry, retaining unknown underscore names as identifiers; cover latin1, utf8mb3/utf8mb4 and binary plus unknown-name controls.
Primary-source checks: MySQL 8.4 lexer (function-keyword lookup, charset lookup, end-comment cursor handling) and grammar (null_as_literal reduces expression NULL to TOK_GENERIC_VALUE).
Validation: ran temporary exact-head counterexamples through Compute and go test -mod=mod ./pkg/sql/parsers/dialect/mysql/mysql_digest/... -count=1; both owning pure-Go packages pass. The counterexamples above are actual digest-package output, not execution against a new MySQL instance or full MatrixOne SQL server. Temporary tests were removed; no production source changed. No new resource/liveness blocker is claimed. Full SQL parse plus tokenization remains per-input CPU work; normalized text is also built although the SQL function needs only the hash.
Submission freshness: head is unchanged, but GitHub now reports mergeable=false following main drift. This review records defects in the reviewed head and is not a statement that the PR is currently ready to merge.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep review of exact head c33e943e0c49bfdac40e1a5296bdc887b5464494.
The executable-comment byte loss, bare COUNT token classification and supported charset-introducer findings are closed by the new code and exact-head counterexamples. The remote protocol reservation now preserves main's v53 and allocates v54 to STATEMENT_DIGEST. I rechecked the updated lexer/handler changes and surrounding scalar/session/protocol contracts; no additional resource or performance blocker was identified. The remaining previous CTAS finding is only partially fixed.
[P2] CTAS without optional AS still misclassifies expression NULL
pkg/sql/parsers/dialect/mysql/mysql_digest/internal/handler.go:218-227 exits the DDL column-attribute heuristic only on a top-level AS token. MySQL also permits CREATE TABLE t SELECT ... without AS (official syntax/examples); MatrixOne's own mysql_sql.y:10458,10472 accepts this form too.
Exact-head executions of the actual pure-Go digest implementation produced:
CREATE TABLE t SELECT BINARY NULL
=> CREATE TABLE `t` SELECT BINARY NULL
CREATE TABLE t SELECT BINARY 1
=> CREATE TABLE `t` SELECT BINARY ?
CREATE TABLE t AS SELECT BINARY NULL
=> CREATE TABLE `t` AS SELECT BINARY ?
CREATE TABLE t (a INT) SELECT BINARY NULL
=> CREATE TABLE `t` ( `a` INTEGER ) SELECT BINARY NULL
In the first/fourth cases BINARY is a query expression operator, but ddlTable remains true, so observing BINARY sets inColumnDefinition, and shouldKeepNull retains NULL as a DDL attribute. NULL here is an expression literal and must normalize to ?, exactly as it now does in the AS control. The resulting hash is therefore incorrect and fails to group equivalent literal-only query shapes. This is a reachable valid statement, not an invalid-SQL lexer-only artifact.
Please make entry into the CTAS query expression terminate the DDL-attribute context independently of optional AS. Account for explicit column definitions and parenthesized query forms, while preserving genuine column NULL / NOT NULL attributes and expression NULLs inside defaults. Add no-AS controls and assert text plus digest against the intended expression-literal normalization; avoid simply adding another isolated keyword exception without checking the query/DDL boundary.
Validation: go test -mod=mod ./pkg/sql/parsers/dialect/mysql/mysql_digest/... -count=1 passed; an additional temporary exact-head test logged the four counterexamples above plus the three repaired prior cases. The temporary test was removed. No native server or live MySQL run is claimed, and no CI wait was performed. Request changes for this concrete compatibility defect, not missing test evidence.
aptend
left a comment
There was a problem hiding this comment.
Re-review of exact head ec1bebb6fb8f18f9d143a8eeee21f6e3db266dd1, after reading the complete review/inline-comment history and comparing the original reviewed series from b9a57c089661ac0a230e12c2f9a14a1df7c3f188 with both the incremental and complete current diff. I independently verified that the just-pushed no-AS CTAS cases now match MySQL, and that the earlier comment-only, parameter-marker, NCHAR, dollar-quote, explicit SQL-mode, hint-placement, executable-comment, charset-introducer, and v54 remote-protocol findings have corresponding repairs. Two additional MySQL-compatibility blockers remain inline.
Validation on this exact head: make thirdparties; make -C cgo; affected package tests; digest packages under -race -count=100; focused function/process/plan/remote/WAL tests under -race -count=20; go vet on affected packages; and direct differential checks against MySQL 8.4.11. Temporary counterexample tests were removed and the worktree is clean. The full pkg/sql/compile suite also hit five Parquet fanout assertions outside the changed paths; the changed remote-expression protocol test passes repeatedly and under the race detector.
| h.inColumnDefinition = false | ||
| } | ||
| default: | ||
| if h.ddlTable && isColumnTypeToken(tok) { |
There was a problem hiding this comment.
[P1] Do not infer column-attribute context from type tokens inside expressions
This DDL-wide heuristic still treats an expression token such as BINARY as the start of a column definition and resets columnAttrDepth to the expression depth. A following literal NULL is then retained as syntax. On this exact head:
CREATE TABLE t (a INT DEFAULT (BINARY NULL))becomes... DEFAULT ( BINARY NULL ), while MySQL 8.4.11 returns... DEFAULT ( BINARY ? ).- The same mismatch reproduces for
ALTER TABLE t ADD COLUMN a INT DEFAULT (BINARY NULL)andCREATE TABLE t (a INT CHECK (BINARY NULL)).
MatrixOne parses all three statements. Replacing NULL with 1 should preserve the digest, as it does in MySQL, but the current hashes differ (911ecd... vs 2798f9... for the first pair). This splits equivalent statement shapes and breaks the advertised MySQL digest compatibility. Please track the actual column/nullability grammar boundary, or explicitly leave column-attribute mode while inside DEFAULT/CHECK expressions, rather than latching on every type-like token.
| case sqlModeHighNotPrecedence: | ||
| flags |= SQLModeFlags(SQLModeHighNotPrecedence) | ||
| case sqlModeIgnoreSpace: | ||
| flags |= SQLModeFlags(SQLModeIgnoreSpace) |
There was a problem hiding this comment.
[P1] Include IGNORE_SPACE in the ANSI composite mode
This new flag is recognized only when IGNORE_SPACE appears explicitly, but MySQL expands sql_mode=ANSI to include IGNORE_SPACE. Consequently, with sql_mode=ANSI, this implementation tokenizes SELECT COUNT (1) as the identifier form SELECT COUNT (?) (hash 339d9811...), whereas MySQL 8.4.11 tokenizes it as the function form SELECT COUNT (?) (hash 5a390b8c...). ParseSQLModeFlags("ANSI") currently confirms that SQLModeIgnoreSpace is absent. Add the new bit to the existing ANSI expansion and cover the composite-mode path, not only the explicit token.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep re-review of exact head b849ba2cdb981c1d0fe84e3e786b3f813a8cf58e. The previous no-AS CTAS finding is closed: the actual digest implementation now normalizes the expression NULL in CREATE TABLE t SELECT BINARY NULL to ?, and the new tests cover explicit definitions and parenthesized SELECT. The ANSI/IGNORE_SPACE expansion is consistent with the intended composite mode. One new correctness issue remains in the expression-context fix.
[P2] End the DEFAULT/CHECK expression context before subsequent column attributes
In pkg/sql/parsers/dialect/mysql/mysql_digest/internal/handler.go, observeToken sets inColumnExpression=true on DEFAULT/CHECK, but resets it only when the entire column definition ends or a column separator is seen. Returning from a parenthesized default/check expression, or completing a simple DEFAULT literal, therefore leaves subsequent genuine NULL / NOT NULL column attributes in expression mode. isNullKeywordContext now excludes that state and replaces their NULL keyword with the generic literal token.
Exact-head executions of the actual package produced:
CREATE TABLE t (a INT DEFAULT 1 NOT NULL)
=> CREATE TABLE `t` ( `a` INTEGER DEFAULT ? NOT ? )
CREATE TABLE t (a INT DEFAULT (1) NULL)
=> CREATE TABLE `t` ( `a` INTEGER DEFAULT (?) ? )
CREATE TABLE t (a INT CHECK (a>0) NOT NULL)
=> CREATE TABLE `t` ( `a` INTEGER CHECK ( `a` > ? ) NOT ? )
The last NULL in each statement is a column attribute, not an expression literal, and must remain NULL. This changes the digest bytes/hash for valid CREATE TABLE statements. The control CREATE TABLE t (a INT NOT NULL DEFAULT 1) retains NOT NULL, showing that the result wrongly depends on which attribute appears first. This finding is about preserving the NULL token within each statement, not requiring differently ordered SQL statements to share one digest.
Please track the expression's actual boundary and return to column-attribute context when it ends; do not retain a column-wide expression flag after DEFAULT/CHECK. Preserve the newly fixed nested BINARY/default expression cases and add both attribute orders, simple/parenthesized defaults, CHECK followed by attributes, and CREATE/ALTER controls. MySQL's column attributes are defined separately from their default/check expressions: https://dev.mysql.com/doc/refman/8.4/en/create-table.html .
Validation: go test -mod=mod ./pkg/sql/parsers/dialect/mysql/mysql_digest/... -count=1 passed on this head; a temporary exact-head test logged the counterexamples above and the fixed CTAS control, then was removed. This is a real implementation reproduction, not a native SQL/MySQL server run. The updated state adds no new synchronization or resource ownership; no separate performance/unhappy-path blocker was found in this delta. Request changes for the concrete digest corruption, not missing test evidence.
jiangxinmeng1
left a comment
There was a problem hiding this comment.
-
High: [pkg/vm/process/process_codec.go:314]((/home/jiangxinmeng/workspace/matrixone/pkg/vm/
process/process_codec.go#L314) — ConvertToProcessSessionInfo swallows time.UnmarshalBinary
failures and returns nil error. A malformed or empty TimeZone payload is therefore treated
as success, and the decoded process silently falls back to nil/time.Local behavior on the
next forward. That breaks the new cross-CN session-state contract and can change time-
sensitive semantics without surfacing any error. This should fail closed on decode. -
Medium: [pkg/sql/plan/function/func_statement_digest.go:41]((/home/jiangxinmeng/workspace/
matrixone/pkg/sql/plan/function/func_statement_digest.go#L41) — statementDigestSQLMode
reimplements sql_mode resolution instead of reusing the sentinel-safe logic already present
in process.resolveSqlMode / isStrictSqlMode. The current guard only protects the empty-
string resolver case. If a non-frontend resolver returns its compiled default while the
forwarded snapshot contains EmptySqlModeSentinel, STATEMENT_DIGEST can be evaluated under
the resolver’s mode instead of the coordinator’s explicit non-strict mode, so the same SQL
can hash differently after a second CN hop. This should use the same precedence rules as
the rest of the sql_mode consumers.
Notes
-
I could not run the broader Go tests in this environment because the cgo-dependent packages
are missing native headers (jemalloc.h, roaring.h, xxhash.h, usearch.h). The pure-Go
pkg/sql/parsers/dialect/mysql/mysql_digest(https://github.com/matrixorigin/matrixone/tree/main/pkg/sql/parsers/dialect/mysql/mysql_digest)
package tests did pass in the PR worktree.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Deep re-review of exact head 25505345b69eef495035fd96672601e8b3a82897: REQUEST_CHANGES for two confirmed issues.
[P1] Move the remote execution gate together with the v55 capability allocation
pkg/defines/const.go:92-94 correctly assigns v54 to catalog-authenticated proxy-cache reuse and v55 to STATEMENT_DIGEST. However, pkg/sql/compile/remoterun.go:1993-1996 still admits the function at MORPCVersion54 and its error still names version 54. During a rolling upgrade, a remote v54 CN lacking STATEMENT_DIGEST satisfies this guard and receives a pipeline containing an unsupported function. The new allocation therefore does not provide its intended fail-closed compatibility boundary.
Update the actual remote gate and error to v55, and align the below/at-gate tests so v54 is rejected and v55 accepted. Audit all capability references, not just MORPCLatestVersion. This is a source-confirmed mixed-version routing failure; no mixed-binary deployment run is claimed.
[P2] Resume column-attribute parsing after an unparenthesized DEFAULT, including plain NULL
The new state transitions in pkg/sql/parsers/dialect/mysql/mysql_digest/internal/handler.go:243-269 close the previous parenthesized-expression cases and the specific DEFAULT 1 NOT NULL case. They still do not end the expression before a plain NULL attribute. Exact-head pure-Go output:
CREATE TABLE t (a INT DEFAULT 1 NULL)
=> CREATE TABLE `t` ( `a` INTEGER DEFAULT ? ? )
CREATE TABLE t (a INT DEFAULT NULL NULL)
=> CREATE TABLE `t` ( `a` INTEGER DEFAULT ? ? )
In each case the final NULL is DDL nullability syntax and must remain NULL, not become another value placeholder. The declared column attribute is still lost and the MySQL-compatible digest input is wrong. The current new test only covers the plain NULL attribute after a parenthesized default, which uses the separate closing-parenthesis transition.
Please model completion of the DEFAULT expression independently of whether the next attribute starts with NOT or NULL. Preserve literal NULL inside DEFAULT/CHECK expressions while recognizing subsequent column attributes. Add unparenthesized literal defaults followed by NULL/NOT NULL and special default forms, alongside the parenthesized controls. The same probe also shows DEFAULT CURRENT_TIMESTAMP NOT NULL ends with NOT ?; avoid fixing just one observed next-token combination.
The earlier version-comment, introducer and CTAS fixes remain intact; the new explicit-empty remote sql_mode handling and propagated max_digest_length snapshot were inspected. Evidence for the DDL finding is actual Compute execution on this exact head; the new author regression tests pass alongside the failing semantic counterexamples. Temporary probe code was removed. No native SQL/MySQL run or CI wait is claimed. These are concrete compatibility/correctness defects, not missing-test-only objections.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Reviewed exact head 2d00a02af80e4a10f33d4eb3840986d606f5d781. The v55 remote protocol gate is now consistent with the advertised capability, and the previous numeric/NULL/CURRENT_TIMESTAMP DEFAULT examples are fixed. One same-family correctness issue remains.
[P2] Complete the DEFAULT-to-column-attribute transition for boolean literals
At handler.go:233-240, completion recognizes numeric/string literals, NULL and NOW, but not TRUE_SYM/FALSE_SYM. Both are legitimate unparenthesized literal defaults. Consequently inColumnExpression remains true after a boolean default, and a subsequent column nullability keyword is normalized as an expression value.
Exact-head pure-Go Compute reproduction (no error):
CREATE TABLE t (a BOOL DEFAULT TRUE NULL)
=> CREATE TABLE `t` ( `a` BOOL DEFAULT TRUE ? )
CREATE TABLE t (a BOOL DEFAULT FALSE NOT NULL)
=> CREATE TABLE `t` ( `a` BOOL DEFAULT FALSE NOT ? )
The trailing NULL is column-definition syntax and must remain NULL, not ?. This produces incorrect normalized token streams and therefore incompatible statement digest hashes for ordinary boolean column DDL. This is not a missing-test-only objection: the incorrect output above was executed against this head. MySQL documents boolean literals and permits literal constant defaults.
Please make the simple-default completion classifier cover the complete supported literal family, including TRUE_SYM/FALSE_SYM, while retaining the parenthesis-depth protection for expressions. Add CREATE/ALTER cases for both boolean values followed by NULL and NOT NULL, plus parenthesized boolean expressions, rather than only enumerating the previous counterexamples.
Validation: the complete pure-Go digest/internal package test suite passes on this head; targeted additional probes expose the above gap and confirm the previous DEFAULT examples now work. Reused the unchanged full implementation review for lexer/token budget, vector/session handling, protocol propagation and error paths, and inspected all four changed files. No additional confirmed performance/resource blocker found in this increment. I did not rerun a native SQL cluster or MySQL server and did not wait for CI.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Re-reviewed exact head c1bb21b0f8de065517ef74ab37faeee55ef46bac. The previously confirmed blockers are closed; no additional confirmed blocker found.
The Boolean DEFAULT fix now ends the simple-default state for TRUE/FALSE at the column-attribute depth. This preserves a following NULL/NOT NULL without incorrectly preserving expression NULLs inside parenthesized defaults. I ran an additional 20-case CREATE/ALTER × TRUE/FALSE/parenthesized defaults × NULL/NOT NULL matrix, including (TRUE AND NULL) to verify that expression NULL remains normalized while the trailing column attribute survives. It passes on this exact head. The temporary review probe was removed.
The full pure-Go mysql_digest/... suite also passes, including the previous literal/temporal DEFAULT, CTAS, executable-comment, function-name and charset-introducer regressions. The remote-execution gate still requires v55, consistent with the defines capability and the v54-rejection/v55-acceptance tests.
I reviewed the complete increment and reused the unchanged implementation audit for tokenization/reduction, SQL-mode and max-digest-length propagation, NULL/error handling, bounded token storage and full-input validation. The localized state transition does not add shared state, asynchronous resources, or a new asymptotic cost. No new confirmed performance/resource or unhappy-path blocker was found.
Evidence limits: source review, exact-head pure-Go suite and additional counterexample matrix, plus git diff --check; no fresh native SQL cluster, MySQL server differential run, or race suite in this round, and no wait for CI. Approval is not a claim that every possible MySQL grammar construct has been exhaustively differential-tested.
Summary
STATEMENT_DIGESTwith token normalization and SHA-256 output.NULL, empty input, SQL mode,max_digest_length, and coordinator state across remote CN execution.Fixes #23024
Validation
WITH ROLLUP, and edge cases; all valid hash outputs matched.go test ./pkg/sql/parsers/dialect/mysql/mysql_digest— passed (85.5% package coverage).mo-cgo-test ./pkg/sql/plan/function— passed.mo-cgo-test ./pkg/vm/process— passed.mo-cgo-test ./pkg/sql/parsers/dialect/mysql— passed.mo-servicebuild,make err-check, and distributed BVT — passed (24/24, twice).