TML-2729: drop LATERAL include codegen for correlated-only read path#667
TML-2729: drop LATERAL include codegen for correlated-only read path#667tensordreams wants to merge 2 commits into
Conversation
…-only read path The read-path include dispatch carried two single-query builders — buildLateralIncludeArtifacts (LEFT JOIN LATERAL + json_agg) for Postgres and buildCorrelatedIncludeProjection (correlated subquery) for SQLite — selected on the `lateral` capability flag. Benchmarking on PG 17.5/17.10 proved the two forms compile to structurally identical plans for the top-N-per-parent shape (the per-parent LIMIT forbids de-correlation of either form), so LATERAL offers no planner advantage for includes. Route every include through the correlated path and remove the strategy axis entirely (TML-2657 already removed the multi-query fallback, so the dispatch was binary): delete buildLateralIncludeArtifacts, drop the `strategy` parameter from every include builder, simplify buildNestedIncludeArtifacts to projections-only (correlated emits no joins), rename compileSelectWithIncludeStrategy to compileSelectWithIncludes, and delete include-strategy.ts (selectIncludeStrategy / IncludeStrategy). The `lateral` capability flag, JoinAst.lateral, the renderer LATERAL emission, and the public lateralJoin() DSL are independent consumers and are untouched. A new regression guard pins that a lateral-capable contract now resolves includes in a single execution with no LATERAL join and no LATERAL keyword in the lowered SQL. Refs: TML-2729 Signed-off-by: Alexey Orlenko's AI Agent <robot@aqrln.net>
|
Warning Review limit reached
More reviews will be available in 17 minutes and 49 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (20)
✨ Finishing Touches🧪 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 |
size-limit report 📦
|
@prisma-next/extension-author-tools
@prisma-next/mongo-runtime
@prisma-next/family-mongo
@prisma-next/sql-runtime
@prisma-next/family-sql
@prisma-next/extension-arktype-json
@prisma-next/middleware-cache
@prisma-next/mongo
@prisma-next/extension-paradedb
@prisma-next/extension-pgvector
@prisma-next/extension-postgis
@prisma-next/postgres
@prisma-next/sql-orm-client
@prisma-next/sqlite
@prisma-next/target-mongo
@prisma-next/adapter-mongo
@prisma-next/driver-mongo
@prisma-next/contract
@prisma-next/utils
@prisma-next/config
@prisma-next/errors
@prisma-next/framework-components
@prisma-next/operations
@prisma-next/ts-render
@prisma-next/contract-authoring
@prisma-next/ids
@prisma-next/psl-parser
@prisma-next/psl-printer
@prisma-next/cli
@prisma-next/cli-telemetry
@prisma-next/emitter
@prisma-next/migration-tools
prisma-next
@prisma-next/vite-plugin-contract-emit
@prisma-next/mongo-codec
@prisma-next/mongo-contract
@prisma-next/mongo-value
@prisma-next/mongo-contract-psl
@prisma-next/mongo-contract-ts
@prisma-next/mongo-emitter
@prisma-next/mongo-schema-ir
@prisma-next/mongo-query-ast
@prisma-next/mongo-orm
@prisma-next/mongo-query-builder
@prisma-next/mongo-lowering
@prisma-next/mongo-wire
@prisma-next/sql-contract
@prisma-next/sql-errors
@prisma-next/sql-operations
@prisma-next/sql-schema-ir
@prisma-next/sql-contract-psl
@prisma-next/sql-contract-ts
@prisma-next/sql-contract-emitter
@prisma-next/sql-lane-query-builder
@prisma-next/sql-relational-core
@prisma-next/sql-builder
@prisma-next/target-postgres
@prisma-next/target-sqlite
@prisma-next/adapter-postgres
@prisma-next/adapter-sqlite
@prisma-next/driver-postgres
@prisma-next/driver-sqlite
commit: |
…s with correlated lowering `includeMany` is no longer a method on any surface — the ORM exposes `.include(...)` and the SQL builder has no include method. Clean up the references this leaves behind, and align the adapter docs with the correlated-only include lowering this branch introduces: - Delete the obsolete `include-many-patterns.mdc` rulecard (it documented a removed `SelectBuilderImpl.includeMany()` API) and its index entry. - Remove the dead `HasIncludeManyCapabilities` type (zero usages; it encoded the now-defunct lateral+jsonAgg include gate). - Postgres README: includes now lower to a correlated subquery, not `LEFT JOIN LATERAL`; note LATERAL emission is retained only for the public `lateralJoin()` DSL. - SQLite README: rename includeMany -> include; correlated is the strategy, not a fallback. - AGENTS.md (capability-gating example), the queries skill gotcha, and a stale e2e describe label: includeMany -> include / a real capability. - Remove the broken `### Queries with includeMany` SQL-builder example from query-patterns.md (it called a nonexistent `.includeMany()`). ADRs and the Query Lanes subsystem doc are left as-is (historical / architecture docs handled separately). Refs: TML-2729 Signed-off-by: Alexey Orlenko's AI Agent <robot@aqrln.net>
Linked issue
Refs TML-2729.
Builds on TML-2657 (already merged), which removed the multi-query include fallback and left the read-path dispatch as a binary
lateral/correlatedchoice. This PR removes the remaining axis.At a glance
The whole include lowering in
compileSelectWithIncludesis now a single unconditional correlated arm — no capability branch, no LATERAL builder:Previously this loop branched on
selectIncludeStrategy(contract): alateral-capable target (Postgres) went throughbuildLateralIncludeArtifacts(LEFT JOIN LATERAL … json_agg), everything else through the correlated subquery. Both compiled to structurally identical query plans, so the branch bought nothing.Decision
Route every SQL ORM include through the correlated-subquery path and remove the lateral-vs-correlated strategy axis entirely. Concretely, this PR:
buildLateralIncludeArtifacts()andinclude-strategy.ts(selectIncludeStrategy+ theIncludeStrategytype).strategy: 'lateral' | 'correlated'parameter from every include builder in the chain, so the correlated path is unconditional.compileSelectWithIncludeStrategy→compileSelectWithIncludes(it no longer selects anything).lateralcapability flag,JoinAst.lateral, the Postgres renderer'sLATERALemission, and the publiclateralJoin()/outerLateralJoin()SQL-builder DSL.The justification is empirical, not aesthetic. On PostgreSQL 17.5 (PGlite) and native 17.10, LATERAL and the correlated subquery produce byte-identical results and structurally identical
EXPLAIN (ANALYZE, BUFFERS)plans for the contested top-N-per-parent shape (include({ take, orderBy })) — same innerAggregate → Limit → Index Scan, same buffers, same loop counts. The per-parentLIMITforbids Postgres from de-correlating either form, so both are forced into the same parameterized per-parent plan; LATERAL offers no planner advantage for includes. Full verbatim plans and a self-contained repro are in the TML-2729 ticket comment. This is a codegen simplification with a hard "must not be slower" bar, which the measurements satisfy with margin.How it fits together
Collapse the dispatch.
collection-dispatch.tsno longer imports or callsselectIncludeStrategy;dispatchWithIncludescalls the renamedcompileSelectWithIncludesdirectly. The stale "lateral / correlated builders pick on the capability flag" comments are reworded to describe the single correlated single-query builder.Unconditional correlated lowering. In
query-plan-select.tsthestrategyparameter is removed frombuildNestedIncludeArtifacts,buildIncludeChildRowsSelect,buildDistinctNonLeafChildRowsSelect,buildIncludeChildCombineSelect,buildIncludeChildCombineBranchSelect, andbuildIncludeChildRowsAggregateSelect.buildNestedIncludeArtifactsis simplified to return projections only (correlated emits no joins), which lets the child SELECT drop its now-always-empty.withJoins(nestedJoins)wiring.Delete the dead surface.
buildLateralIncludeArtifacts,selectIncludeStrategy, theIncludeStrategytype, and their test file are removed; the re-export inquery-plan.tsis renamed. A repo-wide grep for all four symbols (excludingdist) returns zero hits.Re-express the tests against one shape. The dual-strategy unit and integration suites collapse to a single correlated path per scenario (depth-2/3 nesting, self-relations,
combine(), scalar reducers, distinct-non-leaf, pagination), and a new guard pins this PR's intent: alateral-capable contract still resolves includes in one execution with no LATERAL.Behavior changes & evidence
Postgres includes now lower to correlated subqueries instead of
LEFT JOIN LATERAL. Results, ordering,take/skip,distinct,combine(), and scalar reducers are unchanged — only the SQL primitive differs. Implementation:query-plan-select.ts. Evidence:test/sql-orm-client/include.test.tsrewrites its two former lateral-join assertions to assert the correlated-subquery shape (no lateral join, noLATERALkeyword).The
lateralcapability flag is now inert for include codegen. Advertisinglateral: trueno longer changes the emitted include SQL. Evidence: a dedicated guard intest/sql-orm-client/nested-includes-strategy.test.tsasserts alateral-capable contract resolves a depth-2 include in one execution,ast.joinscontains no.lateraljoin, and the lowered SQL contains noLATERALkeyword.Single-execution (no N+1) guarantee preserved at every depth.
compileSelectWithIncludesstill builds one AST → one query plan; nested includes embed as nested correlated subqueries. Evidence: the execution-count assertions (executions.toHaveLength(1)at depth-2, depth-3, and self-relation) innested-includes-strategy.test.ts.Reviewer notes
query-plan-select.test.tsshrinks by ~560 lines: thelateral-specific describe blocks and the duplicated lateral arms of each scenario are gone, with the unique coverage (count-over-where-filtered, drops-orderBy, distinctOn/offset) re-expressed against the correlated emission. Net across the PR is −794 lines. Spot-check that no behavior coverage was lost rather than moved.include.test.tsdeviation. Two of its tests assertedjoin.lateral === truefor an include join — those would now fail at runtime, so I rewrote them to assert the correlated shape. This is slightly beyond a comment-only touch but is required by the change (the lateral flag is now inert for includes).includeJoinsis not dead. After removing the lateral arm fromcompileSelectWithIncludes,includeJoinsis still populated by MTI/polymorphism variant joins and flows into the AST — it's just no longer fed by includes. TheJoinAstimport remains in genuine use (combine branches still useJoinAst.inner).projects/middleware-intercept-and-cache/follow-ups.mdnamed the renamed symbol; updated tocompileSelectWithIncludes.includeManyreferences surfaced during review.includeManyis no longer a method on any surface (the ORM exposes.include(...); the SQL builder has no include method). This commit deletes the obsoleteinclude-many-patterns.mdcrulecard, removes the deadHasIncludeManyCapabilitiestype, aligns the Postgres/SQLite adapter READMEs with correlated lowering (Postgres no longer documentsLEFT JOIN LATERALfor includes), fixes a capability-gating example inAGENTS.md, the queries-skill gotcha, and a stale e2edescribelabel, and removes a broken.includeMany()SQL-builder example fromdocs/reference/query-patterns.md. Reviewable independently of the codegen change.Compatibility / migration / risk
Internal only — no public API, SPI, CLI, contract, or error-code surface changes. The four LATERAL consumers the ticket flags as off-limits are verified untouched (the Postgres renderer,
JoinAst.lateral, thelateralcapability flag, and thelateralJoin()DSL are not in the diff). Risk is a SQL-plan regression on Postgres, ruled out by the benchmark in the ticket and the unchanged single-execution guards.Testing performed
Run on final HEAD:
pnpm build— 65/65 packages.pnpm typecheck— 135/135 (includes@prisma-next/integration-tests).pnpm lint:deps— clean (970 modules, no dependency violations).@prisma-next/sql-orm-clientpackage tests — 485 passed, 3 skipped.@prisma-next/integration-teststest/sql-orm-client/— 187 passed (24 files), including the new lateral-flag-inert guard.Two pre-existing, environmental failure sets reproduced and were ruled out as unrelated to this change (both pass / are absent in the touched blast radius): the postgres adapter's
migrations/runner.policy.integration.test.tsthrows an uncaughtConnection terminated(needs a live PG that drops in the sandbox) and pollutes a few CLI test files in the sametest:packagesworker — those CLI files pass in isolation; and thetest/integration/cli-journeys/*.e2e.test.tssuites fail onpnpm install --no-frozen-lockfile(no network in the sandbox).Skill update
n/a — internal only. No user-facing surface (CLI, public TypeScript API, config, error codes) changed.
Follow-ups
take, large-fanout case (aGROUP BYpre-aggregate + join) that neither LATERAL nor correlated emits today; pursuing it would be a separate optimization, not a regression introduced here.docs/reference/query-patterns.mdanddocs/architecture docs/subsystems/3. Query Lanes.mdcarry broader staleness thanincludeMany— they document adb.sql.from(...)/db.schema.tablesSQL-builder surface that no longer exists. Left for a dedicated doc audit rather than rewritten piecemeal here. The three ADRs that mentionincludeManyare left as point-in-time historical records.Alternatives considered
'correlated'but keep thestrategyparameter threaded through. Rejected: TML-2657 already removed the multi-query fallback, leaving a binary that a dead parameter would only obscure. Removing the axis entirely is the honest end-state.GROUP BYpre-aggregate plan for includes. Rejected for this PR: it's incompatible with per-parentLIMIT(no top-N-per-group form) and is emitted by neither the old nor the new path, so it's a separate optimization rather than part of this simplification.Checklist
git commit -s) per the DCO.TML-NNNN: <sentence-case title>form.