Repository navigation
perf(python-sdk): chunk vector batch inserts in one transaction - #30076
Conversation
XuPeng-SH
left a comment
There was a problem hiding this comment.
REQUEST_CHANGES on head 0e1e24b96bc463462821a153d495f699bf3890ba: one confirmed P2 data-compatibility regression in the new Client-owned transaction path.
Reviewed all five changed files against declared base/merge-base 3e50e1b32d8da61bbbf70c89989fb08b671809de, issue #30065, real Client/AsyncClient/Session consumers, SQL construction, tests and the logservice repair. There were no prior review threads, reviews or issue comments at intake. No subagents or source edits.
The batching/ownership design is otherwise reasonable: shared validation/chunking, reuse of the existing SQL builder and transaction owners, caller-owned Sessions left uncommitted, empty input without admission, and no retry/state/storage layer. However, ownership reuse also changes the execution contract: Client's raw-driver string path preserves literal colons, while Session's text() path interprets them as binds. Legal :tag / prefix :tag text that the base revision inserts now raises QueryError. Sync and async both reproduce. This happens before SQL reaches the database, so the SQLite isolation backend used for the reproduction does not make it a SQLite-specific issue. See inline finding.
Current source validation: full offline suite 988 passed + 86 subtests, Python 3.10.18 / SQLAlchemy 2.0.44, exit 0; focused vector modules 23 passed, Python 3.12.8 / SQLAlchemy 2.1.4. The regression independently reproduces with actual SDK Session/SQLAlchemy execution on sync 1.4.54/2.0.44/2.1.4 and async 2.0.44. Base/head controls preserve exact values. This illustrates why the green mock/numeric-vector coverage does not close the changed execution boundary.
Additional real-owner probes passed: empty/one/exact/remainder chunk boundaries and key-order variation; late duplicate, rendering failure, caller rollback, and next-operation reuse. Deterministic async cancellation after the first actual INSERT rolls back, returns pool ownership and permits reuse. These use task-owned SQLite stores; they do not claim MatrixOne-native cancellation or throughput qualification.
The +60/-13 implementation/wiring, +243 test lines and embedded docstrings were assessed separately. No redundant transaction state machine or retired single-row code was found; the single-row helper remains consumed. Chunking reduces INSERT calls to ceil(N/B), but prevalidation/session setup and holding all N rows in one transaction have costs; chunk size bounds the statement, not total transaction state. Server TP production code is unchanged. The PR's native timing numbers remain author-reported, not a universal performance guarantee.
Exact-head kernel CI 38343188568 is successful and the Go fixture matches previously validated #30083 contents. That historical run did not select the SDK execution boundary. Latest main 35c8dd585 has merged #30075; its delta does not alter the relevant vector/client/base/session production code. No full CI rerun was started.
Please fix the shared transaction-aware execution/parameter boundary and add real sync/async literal-text regression coverage while preserving atomic rollback and caller Session ownership.
0e1e24b to
7cdfac1
Compare
XuPeng-SH
left a comment
There was a problem hiding this comment.
Re-reviewed complete base 35c8dd58 → head 7cdfac13, including the Session fixes I authored. One new verified P2 remains; changes are required. This is a read-only re-review, not independent approval.
The original literal-colon thread is resolved: exact-source offline and native tests verify colon/JSON/percent fidelity, late rollback, caller ownership and cancellation/connection reuse. The batching design uses the existing renderer and transaction owner; its whole-input transaction/lock lifetime is correctly qualified. The new shared string execution path nevertheless changes an existing Session ordering contract, as detailed inline.
Reproduction uses real public sync/async Sessions on SQLAlchemy 2.1.4: an ORM object added before a string SELECT is invisible; a following string UPDATE affects zero rows and commits the original value. The unchanged explicit text() path flushes and updates correctly. Both sync Session classes and async reproduce; 1.4.54/2.0.44 and autoflush=False are matching controls. Existing tests flush only through unchanged ORM statements and miss this boundary.
Exact-source validation remains valid: six whole offline suites, five native tests, successful head CI and zero new static diagnostics. Passing those selections does not cover this new counterexample. INSERT/commit counts improve structurally, while the 2.0 SQLite execution benchmark is not a native TP guarantee. No source edit, subagent, merge or full kernel CI was performed in this review.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Re-reviewed the complete base 35c8dd5854750ef96f52f9dd298a70acd031d5e4 → head fbc446d4f342af52cad835e013a8d7ba4008cde7: REQUEST_CHANGES, two independently reproduced P2 regressions remain. This review includes fixes I authored and is not independent approval. No source edits or subagents.
Both previous review threads are correctly resolved: literal colon/JSON/percent fidelity and SQLAlchemy 2.1 Core autoflush are covered by current-source public sync/async tests, with unchanged-parent and older-version controls. The shared transaction design is sound: existing parent Session owns flush/routing/lifecycle, Client owns its transaction, supplied Sessions retain caller ownership, and there is no new retry/cache/state machine. Engine execution hooks retain the recorded raw-driver tradeoff.
The new counterexamples challenge two separate owner boundaries: batch rendering must preserve SQL scalar types when switching builders; the literal adapter must not narrow arguments still accepted by the supported parent Session. Inline comments contain exact triggers and controls. Regarding stricter input behavior: rejecting a fractional value for INT could be an intentional policy, but the confirmed finding uses 1.0, which loses no precision. The implementation converts its SQL type into a string rather than implementing a coherent validation policy.
Reviewed implementation/configuration +118/-21, tests +820, documentation 0 separately. The broad tests generally have distinct lifecycle/compatibility oracles and light fixtures, but current native rows miss scalar-type coercion and routing tests cover only bind_arguments, so these green selections do not cover the confirmed regressions. The unchanged single-row renderer still has consumers; deleting it solely for line reduction would be incorrect.
Existing six full offline suites, five native cases and zero-new incremental static diagnostics remain source-valid (13-file fingerprint verified). Fresh native MatrixOne 4.2.5 baseline/head experiments exercise actual Client/AsyncClient vector managers; all task-owned databases were removed. A separate SQLAlchemy 1.4.54 probe exercises both sync Session classes and the async public Session, comparing string/explicit-text and keyword/dictionary routing.
CI run 38381878777 is successful for this exact head: all six SDK offline combinations and CI Required passed. Online CI is skipped; local native evidence is separate. Kernel jobs are correctly skipped for SDK-only scope.
Measured local SDK execution overhead is lower (paired medians 68.3ms→48.4ms), and batching structurally reduces INSERTs to ceil(N/B), with one owned commit. This is not a universal native TP guarantee; the transaction still retains all input-row state until completion. Cancellation/rollback/pool-return/reuse evidence remains valid. No further confirmed material finding in the reviewed closure.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Re-reviewed the complete base 35c8dd5854750ef96f52f9dd298a70acd031d5e4 → head 709b03340b95486833c404aa9e77cf6ee8a58d13: REQUEST_CHANGES, one newly reproduced P2 remains, tracked in #30124. This includes fixes I authored and is not independent approval. No source edits or subagents.
All four earlier review findings are correctly resolved: literal colon handling, SQLAlchemy 2.1 autoflush, scalar SQL typing (including NumPy booleans), and supported Session keyword routing. Their tests and current-source validation cover the original triggers and controls. Transaction ownership and shared rendering remain well placed; no duplicate transaction state, retry, cache, storage or routing owner was introduced. The consumed single-row builders should remain.
The remaining flaw is a false assumption at the custom compiler boundary: a legitimate Session hook can wrap the literal statement with .columns(), making its compiled form executable. Actual MatrixOne 4.2.5 probes reproduce both silent %s → {} corruption and a 100% error across SQLAlchemy 1.4.54/2.0.44/2.1.4, both sync Session classes and async Session. Ordinary text, no-hook, explicit text() and restored-base-conversion controls pass. Existing hook tests inspect the statement and update options/routing; they do not transform it into a compiled statement. See the inline finding and issue for reproduction and acceptance criteria.
Reviewed implementation/configuration +132/-22, tests +1031/-2 and standalone documentation 0 separately. Test fixtures are lightweight and generally prove distinct public behavior; the new gap is a missing semantic dimension, not insufficient line count. All 14 validation fingerprints match. Reused six full offline suites, 13 native tests and zero-new incremental static diagnostics. Fresh targeted probes add 36 passing checks for chunk boundaries/key order, savepoint isolation, pre-commit failure, pool return and same-client reuse across the three SQLAlchemy families. Those passing checks do not cover the new compiler counterexample.
CI 38387652298 is successful for this exact head, including six SDK offline jobs and CI Required. Online CI is skipped; the native evidence is local. Measured common SDK overhead has no regression; this does not establish universal native TP throughput, and a chunk limit bounds statements rather than total transaction state. The accepted Engine-hook bypass is distinct from the newly broken Session-hook transformation. No further confirmed material blocker in the remaining reviewed closures; no merge or full kernel CI was performed.
0152dcc to
fd73beb
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? |
XuPeng-SH
left a comment
There was a problem hiding this comment.
APPROVE on b026926. Complete 12-file review: all five previous findings are closed, with real public Session/driver-formatting/typed-result controls. Transaction and execution ownership remain in the existing Client/parent Session; no duplicate state machine, cache, retry or storage owner. Fresh full offline suite passes (Python 3.12/SQLAlchemy 2.0.44), plus 12 sync/async adjacent controls for chunk boundaries, key order, savepoints, pre-commit failure, pool return and reuse. All six exact-head SDK CI matrix cells pass. Native evidence is reused by unchanged production fingerprints; no native rerun today. Implementation/config +132/-22 and tests +1385/-2 were reviewed separately. Whole-input transactions retain state until completion; SDK measurements do not establish universal native TP throughput. Inherited parent compiler/autocommit boundaries remain explicitly qualified. This includes earlier fixes authored through this account and is not an independent second-reviewer claim. No remaining actionable blocker; no source edits, subagents or merge.
Merge Queue Status
This pull request spent 40 minutes 3 seconds in the queue, including 55 seconds running CI. Required conditions to merge
|
|
Post-merge functional audit on main The dependency tuple permitted by requirements using PyMySQL 1.2.3 instead produces 15 passed / 10 failed; all ten async failures are the existing connection-reuse/pre-ping TypeError tracked in #30242, before the intended SQL. Changing only PyMySQL back to 1.1.1 restores all 25 tests. This is newly executed evidence, not a claim that this PR introduced that existing dependency defect. Details and control: #30242 (comment) . Logs and exact diagnostic overlay retained under Follow-up acceptance update (fresh executions after the initial report): SQLAlchemy 2.0.54 offline 154 passed / native 25 passed and 2.1.4 offline 162 passed / native 25 passed, using aiomysql0.3.2 + PyMySQL1.1.1, same main SDK/server SHA, all four commands exit0. Along with retained same-round 1.4.54 results, the three supported families now have real sync/async/native and literal hook coverage. Native 101-row/batch16 test observes exactly seven INSERTs and exact stored vectors; late-chunk and caller-owned rollback pass. Transferred #30065 and #30124 to Ariznawlll, with assignment readback:
The independent PyMySQL1.2.3 pre-ping defect remains with development under #30242. Production latency/throughput benchmarking and the 4.2 SDK were not qualified. New logs/overlays/exit receipts are retained in the same continue18 evidence package. |
What type of PR is this?
Which issue(s) this PR fixes:
Closes #30065
Closes #30124
What this PR does / why we need it:
Vector batch inserts previously executed and independently committed one INSERT per row. Sync and async managers now prevalidate matching column sets and execute bounded multi-row chunks (
batch_size=1000by default) in one Client-owned transaction. Late failures roll back earlier chunks. A supplied Session retains transaction ownership; empty input acquires no connection. No retry is introduced for uncertain commit outcomes.Session string execution previously parsed rendered literal data such as
':tag'as SQLAlchemy binds. The shared literal statement adapter preserves those literals while passing through the authoritative parent Session execution path. Parent Session owns autoflush, ORM hooks, routing, defaults, result handling and lifecycle; the adapter dispatches the existing raw driver on the parent-supplied connection. In particular, SQLAlchemy 2.1 pending ORM changes now flush before Core string SELECT/UPDATE as they do with ordinary SQLAlchemy text statements. Disabled/scoped autoflush and older-version behavior remain consistent with SQLAlchemy.Hooks that convert a string statement with
.columns()now use dialect text escaping during compilation. Compiler flags are changed only for a standalone literal, preserving type conversion and ordinary, expanding and literal-execute bindings in surrounding queries.The adapter has no SQL cache, duplicated flush logic or additional transaction owner. Driver-specific percent escaping respects effective
no_parameters. Execution kwargs are forwarded to the parent, including deprecated keyword routing supported by SQLAlchemy 1.4; the selected parent owns acceptance and errors. Explicit SQLAlchemy statements retain their existing path. ORM and cursor hooks are preserved; raw-driver dispatch continues to bypass Enginebefore_execute/after_executehooks. SQLAlchemy 1.4 legacy result semantics and raw-dispatch temporary autocommit connection cleanup are covered. Transformed statements retain parent compiled-execution lifecycle semantics. Existing SQL-builder escaping limitations are outside this execution-boundary correction.The shared batch renderer preserves SQL numeric and boolean types, including Decimal precision and NumPy scalars. Strings retain escaped textual storage, so callers requesting textual values supply strings. This scalar typing applies to generic Client/Session batch APIs as well as vector batches. Common strings and built-in numbers avoid ABC type classification; no cache or new renderer is added.
The complete delivery diff is SDK-only: implementation/configuration +132/-22, tests +1385/-2, standalone documentation 0. The logservice fixture repair is already upstream and is absent from this diff.
Validation
.columns()and typed subqueries through both sync Session owners and async, comparing literal percent/colon values, observable TypeDecorator conversion, ordinary/expanding/literal-execute binds and unchangedtext()controls.git diff --checkpass.aiosqliteremains test/dev-only.Performance and limits
INSERT count is
ceil(N / batch_size)and Client mode commits once; the actual budget test verifies 7 rows produce 3 INSERTs and one commit. The transaction spans all rows, so server transaction state remains live until completion.A final bounded SQLite SDK benchmark alternated the verified main Session method and current implementation in the same process/engine/session: five rounds of 2,000 SELECTs after warmup, Python 3.10/SQLAlchemy 2.0.44. Median 61.0ms before versus 46.5ms after. Paired shared-renderer measurements against the previous PR implementation use five alternating rounds of 100 calls × 100 rows: numeric/vector median 15.60ms → 15.02ms; text 8.42ms → 7.14ms, with every render round faster. This verifies local SDK overhead, not native TP throughput. Kernel TP code is unchanged.
Local checks above qualify the final source. CI on 6d888b0 failed because the new test encoded the parent limitation with an exact 2.0.44 version check, while CI installed 2.0.54. The test now compares actual parent outcomes and accepts only the specific KeyError('tag') boundary, with fixed independent success oracles. CI for the previous commit
fd73beb20epassed all six SDK offline matrix jobs andCI Requiredin run 38401155393. Full kernel jobs and SDK online CI were skipped by the workflow; the native evidence above is local. This PR remains a draft; no merge is performed.Transformed statements inherit two verified parent boundaries: Verified SQLAlchemy 2.0.44 and 2.0.54 positional compilers can mistake quoted
%(tag)sfor a positional bind; and MySQL bind-free compiled execution withno_parameters=Trueexposes dialect-doubled percent characters. Unchangedtext()controls exercise both behaviors. Ordinary raw string execution remains exact in both option modes.SQLAlchemy 1.4's deprecated autocommit mode also leaks a temporary connection if a compiled statement's
before_cursor_executehook raises before result ownership is established. An unmodified parent Session has the same failure. The comparison records one checked-out connection after the error and Session close, then explicitly closes the test-captured cursor and connection and verifies one-slot reuse. This is a documented inherited limitation, not product cleanup; ordinary transaction, transformed success/SQL errors and raw rejection paths retain strict cleanup checks.The compiler-only correction adds no work to ordinary raw execution: five alternating rounds of 2,000 public SELECTs across all three SQLAlchemy families recorded zero compiler calls. Local timing medians varied with noise; no new throughput improvement or universal TP guarantee is claimed.
Rebased onto main
f14a1fa9ee. The SDK and related CI configuration were unchanged between the old and new baselines; all 14 SDK fingerprints remained identical through the conflict-free rebase. Only the two hook-test assertion files changed afterward.The author’s concurrent named-percent test correction (
0152dcc0, replayed as4d5b633f) is preserved. Offline qualification uses unmodified SQLAlchemy ParentSession and ParentAsyncSession, checks StatementError.orig is KeyError with args ('tag',), and applies the same no_parameters option to parent and SDK queries. No duplicate offline outcome mechanism remains.Review regression qualification — 2026-10-15
The five inline findings have implementation fixes and resolved threads. The formal Request Changes decision remains in place pending reviewer reassessment.
Commit
b0269265a6adds 24 offline combinations that connect the actual public sync/async Session hook path to real PyMySQL/aiomysqlmogrifyformatting. They cover format/pyformat parameter styles, both no_parameters modes, typed.columns()results, typed subqueries, expanding/literal-execute outer binds, and exact ordinary/100%/%s/%% values against explicit text controls. SQLite provides only the final execution transport; this follow-up does not claim a new native-server run. Formatting connections are unopened, and the transport listener/dialect settings are restored on exit.Negative control: replacing only the compiler with the exact originally reviewed implementation from
f582feb01dmakes all 24 new combinations fail. The current production fix passes all 24 on SQLAlchemy 1.4.54, 2.0.54 and 2.1.4.Fresh Python 3.12 complete offline results on the final test source: 1171 / 1167 / 1175 passed, respectively, plus 86 subtests per family, zero failures/errors/skips. RuntimeWarning is treated as an error; only the existing SQLAlchemy/SQLite deprecations remain. Black, isort, fatal Flake8, syntax and diff checks pass. This follow-up adds tests only; production source is unchanged from
fd73beb20e.Published-head CI run 38664668236 completed successfully: all six SDK offline matrix cells and CI Required passed on exact commit
b0269265a6d93cb95533fcd8c01eeb6efe23c47c. SDK online and kernel jobs were skipped by the SDK-only scope. All five inline threads remain resolved; the formal Request Changes still requires reviewer reassessment.