Repository navigation
Conversation
## What type of PR is this? - [x] BUG - [x] Test and CI ## Which issue(s) this PR fixes: Closes #30094 ## What this PR does / why we need it: Repeated planning of an external-table AST reset `NullMap` after `InitNullMap` had consumed its SET expressions. Subsequent prepared CREATE executions stored an empty map, turning a configured null sentinel into a non-null string. With the existing Unicode/null CSV fixture, MAX/MIN/non-null COUNT changed from `('你好','你',2)` to `('你好','null',3)`. Keep the existing normalization result when the first assignment has already been consumed. The production change is six added lines; it retains current unsupported SET boundaries. Cover parsed CREATE EXTERNAL and LOAD ASTs, repeated real BuildPlan calls and serialized catalog properties, multiple columns/sentinels, and unsupported expressions both on the same column and on a different column. Add an end-to-end SQL/golden case with direct and repeated prepared CREATE execution; existing aggregate expectations remain unchanged. This was found while diagnosing the [single external-table BVT failure on #30073](https://github.com/matrixorigin/matrixone/actions/runs/38343181326/job/115120263337). That artifact lacks evidence that the failing DDL retried; the repeated-binding bug itself is independently reproduced, including the exact wrong aggregate and erased catalog rule. ## Validation - Unmodified main: the new parsed-AST and actual BuildPlan regression assertions fail on the second binding. - Final patch: all ten parsed-AST subcases and the real BuildPlan regression pass with `-count=20`; each test also performs twenty bindings. - Entire `pkg/sql/plan` test package passed with the final patch (24.888s). - Native baseline built from the exact #30073 CI Go source: 200 prepared CREATE/drop executions, first correct and the subsequent 199 wrong. The catalog NullMap changes from `{"col3":["null"]}` to `{}`; direct creation is correct. - Final native source build: 500 prepared CREATE/drop executions on each of two local CNs, **1,000 / 1,000 correct**; assert MAX, MIN, COUNT(col3), COUNT(*) and inspect persisted metadata on every iteration. - Every statement in the added BVT SQL case was executed against the native service; all three result rows match its golden. This local check uses PyMySQL, not the GitHub Compose/mo-tester runner. - `gofmt` and `git diff --check` pass. Remote CI is tracked separately from these local results. Native tests use synthetic databases, local Linux amd64 source builds and existing local native libraries. No released binary or production certification claim. An immediate cross-CN read additionally exposed metadata visibility lag; cross-CN validation must synchronize committed metadata rather than misclassifying table-not-found as a null-conversion failure. ## Dependency The shared gossip-fixture compatibility repair is already present in main through merged #30075, and is outside this PR's delivery diff. The same null-map correction is applied to #30073; its full required CI has passed. ### Cross-CN validation A further 200 prepared CREATE/drop executions on CN1 with aggregate reads on CN2 passed after an explicit `mo_ctl('cn','syncCommit','')` metadata synchronization barrier. The first unsynchronized cross-CN attempt failed with table-not-found and is recorded separately; the test does not retry incorrect aggregate results. This brings final-patch native validation to 1,200 correct executions. ## Remote CI [MatrixOne ALL CI run 38351140587, attempt 2](https://github.com/matrixorigin/matrixone/actions/runs/38351140587) is successful on head `6def8a120a76bc80f92187126bb959c4d3709442`. Full x86 unit tests, SCA, shared native build, UT coverage, both Compose BVT groups, coverage aggregation and `CI Required` passed. The first unit-test job failed before test execution during GitHub TLS checkout; rerunning failed jobs preserved the already successful checks and completed the full unit suite. This PR remains a draft. --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
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: three confirmed P2 regressions in the complete nine-file review. Shared substitution/serialization ownership is the right design, but its new lexical and vector boundaries are incomplete. Fresh full offline suite passes 1076 tests with no failures/errors/skips; the attached adjacent probes show why that coverage does not close these cases. Public sync/async SDK probes confirm NumPy vector failure with exact original-source and ordinary-float controls. Public Session quote probes use an isolated SQLite transport with matching lexical rules; actual MatrixOne parser execution independently confirms the corresponding NBSE/backtick/executable-comment SQL is valid. This is not a new native end-to-end server run. Reviewed implementation +192/-170 and tests +235/-8 separately; no prior review threads at intake. Existing six-cell CI/native evidence is reused within its tested scope. No source edits, subagents or merge.
| if isinstance(value, dict): | ||
| value = json.dumps(value) | ||
| elif isinstance(value, (list, tuple)): | ||
| value = json.dumps(value, separators=(',', ':')) |
There was a problem hiding this comment.
[P2] Preserve NumPy elements when rendering vector sequences
The new shared path calls json.dumps directly on lists. Both public VectorManager.insert and AsyncVectorManager.insert now reject embedding=[np.float32(1), np.float32(2)] (and np.int64 elements) with QueryError: Object of type float32/int64 is not JSON serializable. The exact previous vector builder renders and inserts these values; ordinary float and np.float64 controls still succeed. The latest NumPy correction handles top-level scalars only, so its tests miss the normal nested-vector representation, such as list(np.array(..., dtype=np.float32)). Preserve supported numeric sequence elements at the shared serialization owner and cover real sync/async vector inserts; do not restore a separate vector serializer.
| start, quote = i, char | ||
| i += 1 | ||
| while i < len(sql): | ||
| if sql[i] == '\\': |
There was a problem hiding this comment.
[P2] Match quote boundaries to identifier rules and active SQL mode
This unconditionally treats backslash as an escape inside all three quote types. MatrixOne's backtick identifiers do not use backslash escaping, so valid SQL SELECT 1 AS tail\, ? with (7,) now raises Too many SQL parameters instead of returning (1,7). Under NO_BACKSLASH_ESCAPES, SELECT '', ? has the same regression. Exact base substitution and actual public Session controls succeed; the real MatrixOne parser independently accepts the rendered queries. Distinguish identifier quotes from string quotes and honor the authoritative connection/server SQL-mode boundary. The existing mode tests use generated hex literals, which avoid rather than exercise this raw-SQL boundary.
| output.append(sql[i:end]) | ||
| i = end | ||
| continue | ||
| if sql.startswith('/*', i): |
There was a problem hiding this comment.
[P2] Do not skip executable MySQL comment bodies
Not every /.../ range is an inert comment: MatrixOne executes /!.../ bodies. For example SELECT /*!50000 ? / with (7,) previously renders SELECT /!50000 7 */ (accepted by the actual MatrixOne parser), while this head skips the parameter and raises Too many SQL parameters. Named parameters have the same gap. Preserve ordinary-comment exclusion while classifying executable comments consistently with the server scanner, and add the executable/ordinary comment controls.
|
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-xupeng · review
Commit: 7f924eeb3fd0b48d6e5ff75bdbb37c4cb54f2b7e
Assessment: request_changes
Reviewed the complete 13-file delta and surrounding execution owners. The shared renderer resolves prior literal-boundary concerns, but two reproduced regressions disconnect SQL-mode discovery from Session routing and snapshot execution.
Model: gpt-6-astra / xhigh
Session: 01a14d34-4e51-79a3-a62d-6c9c16279dc8 · slot 0 · generation 0
-
P2
clients/python/matrixone/_session_execution.py:93: Execute parameterized raw SQL containing a backslash through a Session whose do_orm_execute hook selects its bind. Both synchronous Session classes and AsyncSession are affected.
Impact: Connection lookup runs before the parent execution hook. An otherwise valid unbound Session raises QueryError before routing; a Session with a default bind probes that connection instead of the hook-selected connection, potentially parsing against the wrong SQL mode. Discover mode through the authoritative execution path.
Evidence: test_review_public_reachability.py reproduces three routing failures on HEAD; ordinary parameterized SQL and parent TextClause controls succeed. Identical tests pass against the frozen merge-base SDK. mode-owner-probes.log additionally records the default connection probed once and the routed NO_BACKSLASH_ESCAPES connection never probed, followed by 'Too many SQL parameters'. The asynchronous equivalent is line 64. -
P2
clients/python/matrixone/client.py:1478: Call snapshot.execute("SELECT '\', ?", (7,)) with NO_BACKSLASH_ESCAPES enabled. SnapshotClient.execute at lines 3188–3190 calls this newly mode-sensitive helper without sql_mode, then forwards rendered SQL without parameters.
Impact: The default empty mode treats the backslash as escaping the closing quote, hides the placeholder, and raises 'Too many SQL parameters' before execution. Direct Client.execute succeeds. Forward snapshot-rewritten SQL and parameters to Client.execute so its connection-aware owner performs substitution.
Evidence: The public snapshot case in test_review_public_reachability.py fails on HEAD and passes against the merge-base SDK. Direct-client and ordinary snapshot controls pass. mode-owner-probes.log separately confirms failure with an injected NO_BACKSLASH_ESCAPES mode and zero mode-discovery queries.
Validation:
-
Reviewed all changed production/test files, surrounding callers and supplied comments. Prior concerns involving nested NumPy literals, quoted identifiers and executable comments are addressed.
-
Preflight passed. Python 3.13.5 / SQLAlchemy 2.0.55 focused suite: 218 passed, no skips; focused.log and checks/1792311671504203039-3.json.
-
Four public regression cases failed on HEAD and all four passed against the frozen merge-base SDK: reachability.log with checks/1792311778344331957-3.json; baseline-reachability.log with checks/1792311792124327205-3.json. Four additional ownership probes reproduced the failures.
-
Commands, source fingerprints, exit receipts, logs and JUnit outcomes were reconciled in review-evidence.json; reconciliation receipt checks/1792311944642852744-3.json. All 49 extracted baseline SDK files match Git blobs; source remains unchanged. Artifacts reside under /home/xupeng/.local/state/mo-bot/jobs/1f718c2492e0cb3ad393b0b1/attempt-1.
-
Frozen evidence records six successful exact-head offline CI cells; local_validation contains no reusable worker gates.
Limitations:
-
Local reproductions exercise real SDK/SQLAlchemy entry points with SQLite; mode-ownership probes inject the server mode. No native MatrixOne server run was performed.
-
Other Python/SQLAlchemy combinations rely on frozen CI evidence. Author-reported native runs lack retained receipts here and were not independently verified.
-
The linked issue's full body was not supplied. Review covers the complete delta and available contracts; native integration validation remains outside the completed local checks.
|
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-xupeng · review
Commit: c3bbb7f7af97a0d650742af94750be39ffa2783b
Assessment: request_changes
Reviewed the complete 13-file delta and supplied review concerns. All 237 focused tests pass, but the new global execution listener introduces a confirmed performance regression for ordinary SQLAlchemy queries.
Model: gpt-6-astra / xhigh
Session: 01a14d34-4e51-79a3-a62d-6c9c16279dc8 · slot 0 · generation 0
- P2
clients/python/matrixone/_session_execution.py:73: After an SDK Session raw-SQL execution installs the Engine-wide listener, execute ordinary cached Core/ORM statements—even on an unrelated engine and without deferred SDK literals.
Impact: Every execution traverses the entire expression graph before checking whether rendering is needed. Shared CTE references cause exponential revisits, imposing substantial CPU and latency costs on unrelated queries. Restrict this work to executions containing deferred SDK literals and avoid repeated traversal of shared nodes.
Evidence: Retained probe_shared_cte.py executes real SQLite queries with result assertions. An 883-character, 11-level shared-CTE query causes 20,477 visits and takes 77.48 ms median with the listener versus 38–52 µs when only that listener is removed. probe_regular_sql.py also measures 4.3× and 7.8× overhead for cached 50- and 500-predicate statements; reinstalling the listener restores the overhead.
Validation:
-
Evidence retained under /home/xupeng/.local/state/mo-bot/jobs/bf7944239fa47c357c0fcf50/attempt-1; review-record.json maps commands, receipts, logs, artifact hashes, scope and comment reconciliation.
-
focused.log and focused.xml confirm 237 passed, zero failures/errors/skips on Python 3.13.5 and SQLAlchemy 2.0.55. Receipt: checks/1792325437605442177-4.json.
-
Performance probes completed successfully: regular-sql-cost.log with checks/1792325330066847415-4.json; shared-cte-cost.log with checks/1792325402297217116-4.json.
-
Preflight passed. All four receipts identify the assigned head and clean source fingerprint; final checkout remained unchanged.
-
Frozen evidence records six successful SDK offline CI cells. Source review and focused tests cover the earlier NumPy, SQL-mode, executable-comment, Session-routing and snapshot concerns.
Limitations:
-
Performance measurements isolate client-side SQLAlchemy overhead using SQLite; they are not MatrixOne server throughput measurements.
-
local_validation is empty. The author's reported 207 native passes lack supplied execution receipts, and online CI was skipped; native behavior was not independently revalidated.
-
The standalone linked issue body was not supplied; its contract was assessed through the PR description, supplied comments and frozen source.
|
mo-bot review · Commit: Review completed. Read the assessment. |
|
mo-bot review · Commit: "interrupted" |
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: f4c3d8a4e829825b287cd6101ceaa875a580fa36
Assessment: request_changes
Reviewed the complete 14-file delta and affected callers. Centralized literal encoding and connection-local rendering are reasonable designs, and the earlier routing, ownership and listener-performance concerns are addressed. Two missing MatrixOne lexical forms introduce confirmed compatibility regressions.
Model: gpt-6-astra / xhigh
Session: 01a13dcf-1517-7f90-b3f9-c5f9f9e7031b · slot 0 · generation 1
-
P2
clients/python/matrixone/_sql_literals.py:147: Call Client.execute("SELECT ? // ?\n", (7,)). MatrixOne recognizes // as a line comment, but the renderer counts its second question mark as another parameter.
Impact: A previously valid query now fails with 'Not enough SQL parameters' before reaching the driver. Named markers inside these comments similarly cause missing-parameter errors. The shared renderer propagates this regression to client and session APIs.
Evidence: The frozen server scanner handles // comments at pkg/sql/parsers/dialect/mysql/scanner.go:277–286. Public-entrypoint checks demonstrate eight cases passing on the frozen base and failing on this head. Recognize // comments through newline or EOF and add positional/named regression coverage. Reproductions and results are retained in test_review_lexical.py and lexical-comparison.json. -
P2
clients/python/matrixone/_sql_literals.py:130: Call Client.execute("SELECT$$O'Reilly$$ , ?", (7,)), or use a tagged dollar string with a named parameter outside it.
Impact: The apostrophe inside the dollar string incorrectly starts an ordinary quoted region, hiding the actual parameter. Previously valid queries now fail with 'Too many SQL parameters' or unused named-parameter errors.
Evidence: Dollar strings are supported by scanner.go:215–233 and 543–575, with parser cases in mysql_sql_test.go:4957–4965. Eight public-entrypoint cases pass on the frozen base and fail on this head. Recognize tagged and untagged dollar-string boundaries before interpreting their contents; add apostrophe and parameter-marker coverage. Results are retained in lexical-comparison.json.
Validation:
-
244 focused SDK tests passed on Python 3.13.5 / SQLAlchemy 2.0.55.
-
Public API regression matrix: head 20 failed/10 passed; frozen base 4 failed/26 passed. Comparison confirms 16 new failures and 10 passing controls. Four preexisting async named-parameter failures were excluded from findings.
-
Listener enabled/disabled median costs were 30.06/30.09 µs for 50 predicates, 243.20/243.68 µs for 500 predicates, and 9398.65/9427.61 µs for an 11-level shared CTE. These SQLite measurements show no meaningful ordinary-path overhead.
-
Reconciled eight command/source/exit receipts with retained logs and verified all 49 extracted base SDK files. Final audit confirmed the exact head, clean source and passing diff check.
-
Evidence and change-map records: /home/xupeng/.local/state/mo-bot/jobs/09769c148ad53130e3b85f6f/attempt-1/evidence-index.json and review-record.json. Final audit receipt: checks/1792383819570984969-5.json.
-
Frozen CI evidence reports six successful offline matrix cells; online validation was skipped.
Limitations:
-
Regression checks exercised real SDK and SQLAlchemy entrypoints with explicit SQLite syntax translation. MatrixOne syntax support was verified against frozen scanner/parser source; no native server execution was performed locally.
-
local_validation is empty. Author-reported native results lack retained receipts here and were not treated as independently verified execution.
-
The standalone linked issue #30059 body was not supplied; its contract was assessed from the frozen PR description and comments.
aptend
left a comment
aptend
left a comment
There was a problem hiding this comment.
Reviewed exact head f4c3d8a4e829825b287cd6101ceaa875a580fa36 against exact base 3fc00709eccc277c175c7a09eed2605ca1aaee29. REQUEST_CHANGES: two current-head lexical regressions remain independently reproducible on a real MatrixOne CN.
- [P2] Exclude native // line comments from parameter rendering —
clients/python/matrixone/_sql_literals.py:147.
client.execute("SELECT ? // ?\n", (7,))
client.execute("SELECT :v // :ignored\n", {"v": 7})MatrixOne accepts // comments through newline/EOF. The renderer recognizes only # and --, so it interprets inert comment markers as executable parameters. These valid calls now raise Not enough SQL parameters / Missing named SQL parameter: ignored before execution. Recognize the server's // boundary and retain comment bytes; cover positional/named markers and newline/EOF controls.
- [P2] Preserve tagged and untagged dollar-string boundaries —
clients/python/matrixone/_sql_literals.py:130.
client.execute("SELECT $$O'Reilly$$ AS marker, ? AS value", (7,))
client.execute("SELECT $txt$O'Reilly$txt$ AS marker, :v AS value", {"v": 7})Both are valid MatrixOne SQL. The apostrophe inside the dollar string incorrectly starts an ordinary quoted region, hiding the real outside marker. Calls fail with Too many SQL parameters / unused named parameters. Recognize actual server dollar-string delimiters before interpreting their contents; preserve apostrophes and placeholder-looking literal data.
Fresh causal evidence: the unchanged 20-case public API matrix runs against the same native server built from this candidate. Exact-base SDK: 20/20 passed. Candidate SDK: 16 failed / 4 raw server controls passed, exactly eight failures for each finding. Coverage includes Client, both synchronous Session exports, AsyncClient and AsyncSession; async named-parameter cases were deliberately excluded because the base does not support them. Controls directly execute rendered native // and tagged/untagged dollar SQL and verify returned values. This uses no SQLite syntax translation or injected mode. Only SDK source differs between the separate head/base worktrees; the complete PR diff is under clients/python, so native/kernel inputs match.
Full review/evidence:
- All 14 files and the latest two-file delta since
c3bbb7f7af; all 4 reviews, 5 discussion comments, 3 inline comments and 3 unresolved threads; complete issue #30059 contract. - Independent design-first architecture assessment: shared literal owner and selected-connection mode discovery are appropriate; authoritative parent Session routing/options retained. The two lexical gaps violate the stated preservation contract.
- Fresh full offline suites: SQLAlchemy 1.4.54: 1261, 2.0.55: 1257, 2.1.5: 1265, each plus 86 subtests, no failures/skips (Python 3.13.5). Fresh committed native test file: 44/44 passed.
- Historical NumPy-vector, quote/NBSE, executable-comment, hook-routing and snapshot issues are closed in the inspected code and fresh relevant tests. The new listener's per-call guard, unique DAG discovery and next-unmarked-call controls pass; no ordinary-query graph walk or persistent mode cache remains.
- Exact-head CI 39197235400 has six successful offline SDK cells; online jobs were skipped. Passing those suites does not cover the counterexamples above.
| Unhappy-path audit | Evidence |
|---|---|
| Q1 ownership | Parent Session/actual connection remains authoritative; renderer shared; no global mode owner. |
| Q2 preparation failure/cleanup | Existing result-owned connection cleanup and public error paths checked; native fixture/database cleaned afterward. |
| Q3 reuse/growth | Copied options, unique-node discovery and subsequent unmarked-query controls pass. No fresh throughput claim. |
No source fixes or pushes. Existing reviewers' findings were checked independently, not used as a reason to omit this review.
| i += 1 | ||
| output.append(sql[start:i]) | ||
| continue | ||
| if char == '#' or (sql.startswith('--', i) and (i + 2 == len(sql) or sql[i + 2].isspace())): |
There was a problem hiding this comment.
[P2] MatrixOne also recognizes // as a line comment (native scanner handles it alongside /*). This branch only skips # and --, so client.execute("SELECT ? // ?\n", (7,)) consumes the comment marker and raises Not enough SQL parameters. The named variant SELECT :v // :ignored with {'v':7} raises Missing named SQL parameter: ignored. Eight fresh native public-API cases fail here and pass with the exact-base SDK; direct rendered-SQL server controls return 7. Preserve // through newline/EOF as inert comment bytes and add positional/named controls.
| i = 0 | ||
| while i < len(sql): | ||
| char = sql[i] | ||
| if char in ("'", '"', '`'): |
There was a problem hiding this comment.
[P2] Recognize tagged/untagged dollar strings before interpreting apostrophes inside them. client.execute("SELECT
This branch was successfully deployed
4 participants
What type of PR is this?
Which issue(s) this PR fixes:
Closes #30059
What this PR does / why we need it:
Problem and behavior
Legacy substitution replaced quoted/commented markers and confused :id with :id2. Use a shared lexical renderer and typed scalar/identifier encoding across sync/async, single/batch/vector builders. Preserve quoted data, reject missing/extra parameters and non-finite numbers, encode backslash/NUL values independently of SQL mode, and serialize NumPy vector elements correctly. Native driver-binding API migration remains separate (#30069).
Session and snapshot execution
Keep the merged #30076 parent Session execution path: parent autoflush, do_orm_execute routing, call options, result typing and connection ownership remain authoritative. Parameterized raw SQL containing a backslash is deferred until the selected connection executes it. Mode discovery no longer calls Session.connection before routing, and no legacy bind facade remains. Wrapped TextualSelect/subquery expressions replace only deferred SDK leaves in a connection before_execute handler; ordinary SQLAlchemy statements/binds are returned unchanged. Sync/async Sessions use the same path. SnapshotClient forwards rewritten SQL plus original parameters into Client.execute.
Backtick/double-identifier boundaries, live NO_BACKSLASH_ESCAPES/ANSI_QUOTES and executable MySQL comments are supported. Ordinary parameter queries and encoded parameter data add no mode probe; no engine mode cache is used. The exact SQLAlchemy 1.4 aiomysql pre-ping guard from #30074 is retained.
Bound listener work (2026-10-19, f4c3d8a)
The Engine-wide listener now has a constant-time guard: only the current SDK call containing a deferred mode-sensitive literal opts in through a copied execution option. Ordinary Core/ORM executions never traverse their expression graph. Marked executions discover each distinct graph node once, render deferred leaves with one actual-connection SQL-mode probe, and retain SQLAlchemy's memoized replacement traversal, outer binds and authoritative parent Session routing. Discovery/rendering errors retain legacy result-owned connection cleanup. There is no persistent mode cache or per-connection marker.
Seven new deterministic regressions fail on c3bbb7f and pass after the change. They cover cached unrelated Core/ORM/shared-CTE queries, unique-node discovery, both sync Session exports and AsyncSession, correct actual mode, unchanged caller options and a subsequent unmarked query. A depth-11 shared CTE previously generated 2,048 mode probes per execution; it now generates one. Timing thresholds are kept out of these tests.
Validation (2026-10-19)
Updated-head CI 39197235400 passed all six SDK offline cells and CI Required. Previous-head run 39125088453 is retained as historical evidence. Kernel/online jobs follow the SDK-only workflow scope; local native results are separate. Formal reviewer reassessment is still required.