TML-3167: Split SQL query and execute driver SPI - #29907
Conversation
2993440 to
78c43b1
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe SQL API now separates row streaming from statement execution. ChangesUnified SQL API migration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
prisma-next
@prisma/orm-extension-arktype-json
@prisma/orm-extension-middleware-cache
@prisma/orm-extension-paradedb
@prisma/orm-extension-pgvector
@prisma/orm-extension-postgis
@prisma/orm-extension-supabase
@prisma/orm-family-mongo
@prisma/orm-family-sql
@prisma/orm-framework
@prisma/orm-mongo
@prisma/orm-postgres
@prisma/orm-sqlite
@prisma/orm-target-mongo
@prisma/orm-target-postgres
@prisma/orm-target-sqlite
@prisma/orm-toolchain
commit: |
size-limit report 📦
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/3-targets/7-drivers/sqlite/src/sqlite-driver.ts (1)
110-131: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftKeep
SqliteConnectionImplpackage-private.Expose
SqlConnectionthrough acreateSqliteConnection()factory. Change public consumers, includingacquireConnection(), to depend onSqlConnectioninstead ofSqliteConnectionImpl. Agree the scope with maintainers before changing this package boundary. Based on coding guidelines, “Expose stateful services through an interface plus a createX() factory; keep the implementing class package-private and make consumers depend on the interface.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/3-targets/7-drivers/sqlite/src/sqlite-driver.ts` around lines 110 - 131, Keep SqliteConnectionImpl package-private and expose connections only through a createSqliteConnection() factory returning SqlConnection. Update acquireConnection() and other public consumers to type against SqlConnection rather than the implementation class, preserving existing behavior and coordinating the package-boundary change with maintainers.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/2-sql/5-runtime/src/sql-runtime.ts`:
- Around line 622-636: Scope `#preparedStatementHandles` entries by both the
physical client and PreparedStatementImpl, so different PostgresQueryable
instances cannot reuse each other’s prepared-statement handles. Update the
get/set closures in the prepared-statement request construction to use the
current physical client as part of the key while preserving existing handle
reuse and retry behavior.
---
Nitpick comments:
In `@packages/3-targets/7-drivers/sqlite/src/sqlite-driver.ts`:
- Around line 110-131: Keep SqliteConnectionImpl package-private and expose
connections only through a createSqliteConnection() factory returning
SqlConnection. Update acquireConnection() and other public consumers to type
against SqlConnection rather than the implementation class, preserving existing
behavior and coordinating the package-boundary change with maintainers.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 293c52bc-c239-4a9d-9129-0801376e718b
⛔ Files ignored due to path filters (4)
projects/affected-row-counts/plan.mdis excluded by!projects/**projects/affected-row-counts/slices/query-execute-split/plan.mdis excluded by!projects/**projects/affected-row-counts/slices/query-execute-split/spec.mdis excluded by!projects/**projects/affected-row-counts/spec.mdis excluded by!projects/**
📒 Files selected for processing (39)
packages/2-sql/4-lanes/relational-core/src/ast/driver-types.tspackages/2-sql/4-lanes/relational-core/test/ast/driver-types.test.tspackages/2-sql/5-runtime/src/prepared/prepared-statement.tspackages/2-sql/5-runtime/src/prepared/types.tspackages/2-sql/5-runtime/src/sql-runtime.tspackages/2-sql/5-runtime/test/async-iterable-result.test.tspackages/2-sql/5-runtime/test/intercept-decoding.test.tspackages/2-sql/5-runtime/test/marker-verification.test.tspackages/2-sql/5-runtime/test/marker-vs-intercept-ordering.test.tspackages/2-sql/5-runtime/test/plan-execution-id.test.tspackages/2-sql/5-runtime/test/prepared.test.tspackages/2-sql/5-runtime/test/raw-connection-seam.test.tspackages/2-sql/5-runtime/test/runtime-ctx-passthrough.test.tspackages/2-sql/5-runtime/test/scope-plumbing.test.tspackages/2-sql/5-runtime/test/sql-family-adapter.test.tspackages/2-sql/5-runtime/test/sql-runtime-abort.test.tspackages/2-sql/5-runtime/test/sql-runtime.test.tspackages/3-extensions/supabase/src/runtime/supabase-runtime.tspackages/3-extensions/supabase/test/supabase-runtime.test.tspackages/3-targets/6-adapters/postgres/src/core/adapter.tspackages/3-targets/6-adapters/postgres/test/adapter.test.tspackages/3-targets/6-adapters/sqlite/src/core/adapter.tspackages/3-targets/7-drivers/postgres/src/driver-error.tspackages/3-targets/7-drivers/postgres/src/exports/runtime.tspackages/3-targets/7-drivers/postgres/src/postgres-driver.tspackages/3-targets/7-drivers/postgres/test/driver.basic.test.tspackages/3-targets/7-drivers/postgres/test/driver.errors.test.tspackages/3-targets/7-drivers/postgres/test/driver.pinned-client-serialization.integration.test.tspackages/3-targets/7-drivers/postgres/test/driver.pinned-client-serialization.test.tspackages/3-targets/7-drivers/postgres/test/driver.prepared.integration.test.tspackages/3-targets/7-drivers/postgres/test/driver.prepared.test.tspackages/3-targets/7-drivers/postgres/test/driver.stream-portal-protection.integration.test.tspackages/3-targets/7-drivers/postgres/test/driver.unbound.test.tspackages/3-targets/7-drivers/postgres/test/sql-queryable-test-utils.tspackages/3-targets/7-drivers/sqlite/src/sqlite-driver.tspackages/3-targets/7-drivers/sqlite/test/runtime-driver.test.tspackages/3-targets/7-drivers/sqlite/test/sql-queryable-test-utils.tspackages/3-targets/7-drivers/sqlite/test/sqlite-driver.test.tstest/integration/test/sql-orm-client/runtime-helpers.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/3-targets/7-drivers/postgres/src/postgres-driver.ts`:
- Around line 206-225: Restrict the stale-handle retry logic in
withStaleHandleRetry: retry SQLSTATE 26000 directly, but retry 0A000 only when
the PostgreSQL error message indicates cached-plan invalidation (for example,
“cached plan must not change result type”); otherwise rethrow the original
error. Add a regression test covering a non-stale 0A000 failure and verify it is
not converted to DRIVER.PREPARE_FAILED.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 266b5d51-b556-4e3a-a5d9-da899266ffe4
📒 Files selected for processing (2)
packages/3-targets/7-drivers/postgres/src/postgres-driver.tspackages/3-targets/7-drivers/postgres/test/driver.prepared.test.ts
ac84dbe to
297e799
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Shaping artifacts for the **affected-row-counts** project — spec and three-slice plan, per the project lifecycle in `projects/README.md`. ## What this project does `updateAndCount` and `deleteAndCount` run two statements: a `SELECT` of every matching primary key, then the write — returning the *read's* row count and discarding whatever the write reported. That is not atomic (outside a transaction a concurrent insert is updated but not counted), it evaluates the filter twice and materialises every matching key in JS purely to call `.length` on it, and it builds the two `WHERE` clauses through different code paths that already drifted once for MTI variants (#940). The count already exists and is thrown away — Postgres reports it in the `CommandComplete` tag, SQLite in `sqlite3_changes64()` via `StatementSync.run()`. The gap is structural: `RuntimeScope.execute()` returns a row stream with nowhere for statement metadata to live. After this project the driver SPI splits along the question being asked, named the way every prior art names it (JDBC, ADO.NET, Go): query<Row>(req): AsyncIterable<Row> // rows execute(req): Promise<SqlStatementStats> // { affectedRows: number } `affectedRows` is not optional — absence is not a state either engine has for the statements `execute()` exists to serve. Statistics never travel through a row stream, so the seven `for await` re-wrap sites between driver and caller stop being a hazard. Prepared-ness rides on the request rather than doubling the method surface, so four driver methods become two. ## Scope boundaries Streaming write terminals, `createAndCount`, and new targets are out. Count semantics are deliberately *not* unified across targets — Postgres's command tag, SQLite's `sqlite3_changes64()`, and Mongo's `modifiedCount` each mean something different, and the project documents the difference rather than reconciling it. ## Decision provenance Three questions were settled with the operator at spec time and moved into the spec body: the execution shape (an earlier single-`execute()` frame-yielding design was considered and reversed), the naming falling out of that shape, and per-driver count semantics. Spec § Open Questions records the reversal. The project amends ADR 210 — Prepared Statements rather than adding a new ADR: every principle it states survives, only the shape they were expressed through changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Steven McClankerton <tatarintsev@prisma.io>
Six-dispatch decomposition for TML-3167, in the hard-cut-migration shape: conformance fix, then interface + postgres reference implementation, sqlite, the runtime merge, supabase, and the test-fake fan-out that closes the grep gate. Two findings from grounding that the project spec did not anticipate: the cursor-side count extraction disappears entirely (statistics no longer ride the row stream, so postgres execute() is just the buffered path), and supabase openRoleSession issues three buffered query() calls on a raw runtime connection — a judgment site the "control plane is a separate interface" boundary does not cover. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Steven McClankerton <tatarintsev@prisma.io>
…andle retry fails ADR 210 § Stale-handle retry requires a failed re-prepare to surface the ADAPTER.PREPARE_FAILED envelope with the originating driver error as `cause`; the retry path rethrew a bare normalised pg error instead, so consumers had no stable code to match on. Signed-off-by: Steven McClankerton <tatarintsev@prisma.io>
Signed-off-by: Steven McClankerton <tatarintsev@prisma.io>
Signed-off-by: Steven McClankerton <tatarintsev@prisma.io>
Signed-off-by: Steven McClankerton <tatarintsev@prisma.io>
Signed-off-by: Steven McClankerton <tatarintsev@prisma.io>
Signed-off-by: Steven McClankerton <tatarintsev@prisma.io>
Signed-off-by: Steven McClankerton <tatarintsev@prisma.io>
Signed-off-by: Steven McClankerton <tatarintsev@prisma.io>
Signed-off-by: Steven McClankerton <tatarintsev@prisma.io>
Signed-off-by: Steven McClankerton <tatarintsev@prisma.io>
Signed-off-by: Steven McClankerton <tatarintsev@prisma.io>
Signed-off-by: Steven McClankerton <tatarintsev@prisma.io>
Signed-off-by: Steven McClankerton <tatarintsev@prisma.io>
Signed-off-by: Steven McClankerton <tatarintsev@prisma.io>
Signed-off-by: Steven McClankerton <tatarintsev@prisma.io>
637a084 to
51328f4
Compare
Signed-off-by: Steven McClankerton <tatarintsev@prisma.io>
Signed-off-by: Steven McClankerton <tatarintsev@prisma.io>
Linked issue
Refs TML-3167
At a glance
Drivers now distinguish streaming rows from DML execution statistics instead of exposing separate prepared variants or a buffered query API.
Decision
This PR ships the Slice 1 driver-SPI split for affected-row counts:
query()streams rows,execute()returns{ affectedRows }, and prepared execution is represented by an optional handle on the request. PostgreSQL and SQLite implement the final surface; the SQL runtime, Supabase runtime, adapters, and test doubles use it end to end.It also makes a failed PostgreSQL stale prepared-statement retry emit the structural
DRIVER.PREPARE_FAILEDenvelope required by ADR 239, preserving the normalized driver error as its cause.Reviewer notes
rowCount; SQLite countsstmt.run().changes. Their distinct engine semantics are intentionally preserved.RETURNINGstatement routed toexecute()before execution, preventing silent row loss.pnpm test:packagesstill has unrelated telemetry/CLI harness failures:telemetry-backendcannot locateprisma-next, and seven CLI process tests exceed their timeout. The changed packages and workspace typecheck are green.How it fits together
packages/2-sql/4-lanes/relational-core/src/ast/driver-types.ts.{ affectedRows }, while SQLite mapsStatementSync.run().changesand retains its defensiveRETURNINGguard.packages/2-sql/5-runtime/src/sql-runtime.tsuse one streaming execution pipeline for ad-hoc and prepared plans.Behavior changes & evidence
execute(request).packages/3-targets/7-drivers/postgres/src/postgres-driver.ts,packages/3-targets/7-drivers/sqlite/src/sqlite-driver.tspackages/3-targets/7-drivers/postgres/test/driver.basic.test.ts,packages/3-targets/7-drivers/sqlite/test/sqlite-driver.test.tspackages/3-targets/7-drivers/postgres/src/postgres-driver.ts,packages/3-targets/7-drivers/postgres/src/driver-error.tspackages/3-targets/7-drivers/postgres/test/driver.prepared.test.tspackages/2-sql/5-runtime/src/sql-runtime.tspackages/2-sql/5-runtime/test/prepared.test.ts,packages/2-sql/5-runtime/test/plan-execution-id.test.tsCompatibility / migration / risk
This is a breaking internal driver SPI change. All in-repository implementations and fakes are migrated in this PR. ORM-visible count-terminal behavior remains unchanged; the count-terminal behavior change belongs to TML-3168.
Testing performed
pnpm typecheck— 165/165 tasks passedpnpm lint:depspassedexecutePreparedandSqlQueryResultsearches underpackages/andtest/returned zero resultspnpm test:packagesran but has the unrelated telemetry/CLI harness failures noted aboveSkill update
n/a — internal SPI refactor; no end-user skill surface changed.
Alternatives considered
executePrepared()method was rejected because preparedness is a request property, not a distinct execution operation.Checklist
git commit -s) per the DCO.TML-NNNN: <sentence-case title>form.Summary by CodeRabbit
New Features
Breaking Changes