feat(sql-builder): runtime query builder with execution bridge#257
Merged
feat(sql-builder): runtime query builder with execution bridge#257
Conversation
|
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:
📝 WalkthroughWalkthroughThis PR introduces a major refactoring of the SQL query builder and AST infrastructure. It replaces the where-expression type hierarchy with a unified expression system, adds a query operation registry for extensibility, completely reimplements the SQL builder from types-only to a runtime-capable query builder, and updates operation template placeholder syntax from Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
SevInf
force-pushed
the
worktree/query-builder-impl
branch
from
848a82e to
72e9233
Compare
@prisma-next/runtime-executor
@prisma-next/sql-runtime
@prisma-next/extension-paradedb
@prisma-next/extension-pgvector
@prisma-next/postgres
@prisma-next/sql-orm-client
@prisma-next/contract-authoring
@prisma-next/contract-ts
@prisma-next/ids
@prisma-next/psl-parser
@prisma-next/psl-printer
@prisma-next/cli
@prisma-next/emitter
@prisma-next/eslint-plugin
@prisma-next/migration-tools
@prisma-next/vite-plugin-contract-emit
@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/family-sql
@prisma-next/sql-kysely-lane
@prisma-next/sql-lane-query-builder
@prisma-next/sql-relational-core
@prisma-next/sql-builder-new
@prisma-next/sql-lane
@prisma-next/target-postgres
@prisma-next/adapter-postgres
@prisma-next/driver-postgres
@prisma-next/core-control-plane
@prisma-next/core-execution-plane
@prisma-next/config
@prisma-next/contract
@prisma-next/operations
@prisma-next/plan
@prisma-next/utils
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/3-extensions/pgvector/src/core/descriptor-meta.ts (1)
6-10:⚠️ Potential issue | 🔴 CriticalFix template format in
cosineLowering—Mustache syntax won't work with adapter placeholder replacement.The
cosineLoweringconstant (lines 6-10) uses{{self}}/{{arg0}}syntax, but the postgres adapter only processes${self}/${arg0}templates. The adapter's placeholder replacement uses regex for\${self}and\${argN}, so the Mustache format will fail at runtime. ThepgvectorQueryOperationsalready uses the correct${self}format; updatecosineLoweringto match:const cosineLowering = { targetFamily: 'sql', strategy: 'function', template: '1 - (${self} <=> ${arg0})', } as const;Update the test expectation in
packages/3-extensions/pgvector/test/operations.test.tsline 38 accordingly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-extensions/pgvector/src/core/descriptor-meta.ts` around lines 6 - 10, The cosineLowering constant uses Mustache-style placeholders ({{self}}/{{arg0}}) which the Postgres adapter doesn't replace; change the template in cosineLowering to use adapter placeholders '1 - (${self} <=> ${arg0})' so it matches pgvectorQueryOperations, and update the corresponding expectation in the test file packages/3-extensions/pgvector/test/operations.test.ts (the assertion around line 38) to expect the updated '1 - (${self} <=> ${arg0})' SQL string.packages/3-extensions/sql-orm-client/src/model-accessor.ts (1)
82-96:⚠️ Potential issue | 🟠 MajorNull values in
eq/neqwill produce invalid SQL.The
eqandneqmethods createBinaryExprwithLiteralExpr.of(value)for all values includingnull. This producescol = NULLinstead ofcol IS NULL, which always evaluates toUNKNOWNin SQL.Per project guidance, equality/inequality with
nullshould emitIS NULL/IS NOT NULLviaNullCheckExpr.🐛 Proposed fix to handle null in eq/neq
for (const op of COMPARISON_OPS) { if (op === 'in' || op === 'notIn') { methods[op] = ((values: readonly unknown[]): BinaryExpr => new BinaryExpr( op, left, TupleExpression.fromValues(values), )) as ComparisonMethods<unknown>[typeof op]; continue; } - methods[op] = ((value: unknown): BinaryExpr => - new BinaryExpr(op, left, LiteralExpr.of(value))) as ComparisonMethods<unknown>[typeof op]; + methods[op] = ((value: unknown): Expression => { + if (value === null) { + if (op === 'eq') return NullCheckExpr.isNull(left); + if (op === 'neq') return NullCheckExpr.isNotNull(left); + } + return new BinaryExpr(op, left, LiteralExpr.of(value)); + }) as ComparisonMethods<unknown>[typeof op]; }Based on learnings: "If
value === nulland the operator iseq, emitIS NULL(viaNullCheckExpr.isNull(left)); if the operator isneq, emitIS NOT NULL."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-extensions/sql-orm-client/src/model-accessor.ts` around lines 82 - 96, The eq/neq branches in the loop over COMPARISON_OPS currently always build a BinaryExpr with LiteralExpr.of(value), which yields invalid SQL for nulls; update the methods creation (the methods object in model-accessor.ts) so that for op === 'eq' if value === null you return NullCheckExpr.isNull(left) and for op === 'neq' if value === null you return NullCheckExpr.isNotNull(left), otherwise keep returning new BinaryExpr(op, left, LiteralExpr.of(value)); keep using the same factories and types (ComparisonMethods, BinaryExpr, LiteralExpr, NullCheckExpr) so only the null case logic changes.packages/3-extensions/sql-orm-client/src/query-plan-aggregate.ts (1)
72-104:⚠️ Potential issue | 🟠 MajorReject
ParamRefin grouped HAVING before lowering.
validateGroupedComparable()still bans params, but this visitor now returnsparamnodes unchanged and skips recursion foroperation.compileGroupedAggregate()only passeswhereparams/descriptors tobuildOrmQueryPlan(), so placeholders introduced in HAVING are unbound at execution time. Please throw here, or recursively validate any operation args/self before returning the expression.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-extensions/sql-orm-client/src/query-plan-aggregate.ts` around lines 72 - 104, In validateGroupedHavingExpr, ParamRef (param) nodes are currently returned unchanged which allows unbound placeholders in HAVING; update validateGroupedHavingExpr to reject ParamRef by throwing an error (or alternatively recursively validate operation arguments/self) so that grouped HAVING expressions cannot contain params before lowering; ensure consistency with validateGroupedComparable and that compileGroupedAggregate/buildOrmQueryPlan still only receive WHERE params/descriptors by referencing validateGroupedHavingExpr, operation visitor handling, and ParamRef/param nodes in your changes.packages/3-extensions/sql-orm-client/src/grouped-collection.ts (1)
157-178:⚠️ Potential issue | 🟠 MajorUse null checks for nullable aggregate comparisons.
sum/avg/min/maxreturnHavingComparisonMethods<number | null>, but this helper lowerseq(null)andneq(null)to binary= NULL/!= NULL. Those predicates never behave correctly in SQL, so nullable HAVING comparisons break.💡 Suggested fix
function createHavingComparisonMethods<T extends number | null>( metric: AggregateExpr, ): HavingComparisonMethods<T> { - const buildBinaryExpr = (op: BinaryOp, value: unknown): Expression => - new BinaryExpr(op, metric, LiteralExpr.of(value)); + const buildExpr = (op: BinaryOp, value: unknown): Expression => { + if (value === null) { + if (op === 'eq') { + return NullCheckExpr.isNull(metric); + } + if (op === 'neq') { + return NullCheckExpr.isNotNull(metric); + } + } + return new BinaryExpr(op, metric, LiteralExpr.of(value)); + }; return { eq(value) { - return buildBinaryExpr('eq', value); + return buildExpr('eq', value); }, neq(value) { - return buildBinaryExpr('neq', value); + return buildExpr('neq', value); }, gt(value) { - return buildBinaryExpr('gt', value); + return buildExpr('gt', value); }, lt(value) { - return buildBinaryExpr('lt', value); + return buildExpr('lt', value); }, gte(value) { - return buildBinaryExpr('gte', value); + return buildExpr('gte', value); }, lte(value) { - return buildBinaryExpr('lte', value); + return buildExpr('lte', value); }, }; }Also add
NullCheckExprto the AST import.Based on learnings, "When generating SQL for comparison operators in the SQL ORM client, do not emit
col = NULLorcol <> NULLfor equality/inequality. Ifvalue === nulland the operator iseq, emitIS NULL; if the operator isneq, emitIS NOT NULL."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-extensions/sql-orm-client/src/grouped-collection.ts` around lines 157 - 178, The helper buildBinaryExpr (used to implement eq/neq/gt/lt/gte/lte for HavingComparisonMethods) currently lowers eq(null)/neq(null) to BinaryExpr with LiteralExpr.of(null), which generates invalid SQL like "col = NULL"; change buildBinaryExpr so that when value === null and op is 'eq' or 'neq' it returns a NullCheckExpr (imported into the file) instead of a BinaryExpr — use NullCheckExpr(metric, false) for 'eq' (IS NULL) and NullCheckExpr(metric, true) for 'neq' (IS NOT NULL); keep all other operators returning new BinaryExpr(op, metric, LiteralExpr.of(value)). Ensure you add NullCheckExpr to the AST imports.
🧹 Nitpick comments (9)
packages/2-sql/4-lanes/sql-builder-new/test/runtime/param-collector.test.ts (1)
5-16: Consider expanding test coverage forgetMetas()and the optionalmetaparameter.The current test validates core functionality but doesn't exercise
getMetas()or the optionalmetaargument inadd(). This is a minor gap.♻️ Suggested additional test case
+ it('tracks metadata when provided', () => { + const pc = new ParamCollector(); + pc.add(42, { codecId: 'int4' }); + pc.add('hello'); + + expect(pc.getMetas()).toEqual([{ codecId: 'int4' }, {}]); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/sql-builder-new/test/runtime/param-collector.test.ts` around lines 5 - 16, Add tests that exercise ParamCollector.add's optional meta argument and ParamCollector.getMetas(): create a ParamCollector, call add(value, meta) with different meta objects (including undefined), assert returned ParamRef instances and their indices (ParamRef.index), assert getValues() remains correct, and assert getMetas() returns the corresponding meta entries in the same order (including undefined where omitted) and that size reflects total entries; use ParamCollector, add, ParamRef, getValues, getMetas, and size to locate the code under test.packages/2-sql/4-lanes/sql-builder-new/test/integration/pagination.test.ts (1)
19-21: Use one matcher for the related pagination-output checks.These assertions are checking a single expected slice shape; consolidating them keeps the test intent tighter.
♻️ Suggested refactor
it('LIMIT + OFFSET paginates correctly', async () => { const rows = await collect(db().users.select('id').orderBy('id').limit(2).offset(1).all()); - expect(rows).toHaveLength(2); - expect(rows[0]!.id).toBe(2); - expect(rows[1]!.id).toBe(3); + expect(rows).toMatchObject([{ id: 2 }, { id: 3 }]); });As per coding guidelines
**/*.test.ts: Prefer object matchers (toMatchObject) over multiple individual expect().toBe() calls when checking 2 or more related values in tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/sql-builder-new/test/integration/pagination.test.ts` around lines 19 - 21, Replace the three related assertions on rows with a single object matcher: remove expect(rows).toHaveLength(2) and the two expect(rows[0]!.id).toBe(2)/expect(rows[1]!.id).toBe(3) and instead assert expect(rows).toMatchObject([{ id: 2 }, { id: 3 }]); target the test's rows variable in pagination.test.ts so the slice shape is checked in one matcher and drop the unnecessary non-null assertions.packages/2-sql/4-lanes/sql-builder-new/test/integration/order-by.test.ts (1)
44-54: Test assumes Alice has at least 2 posts.The assertions on line 53 use non-null assertions (
alicePosts[0]!,alicePosts[1]!) which will throw if the test data changes and Alice has fewer than 2 posts. Consider adding a length guard or using explicit index access with an assertion.Add explicit length check
const alicePosts = rows.filter((r) => r.user_id === 1); + expect(alicePosts.length).toBeGreaterThanOrEqual(2); expect(alicePosts[0]!.views).toBeGreaterThan(alicePosts[1]!.views);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/sql-builder-new/test/integration/order-by.test.ts` around lines 44 - 54, The test "multiple orderBy calls accumulate" assumes Alice has at least two posts and directly indexes alicePosts[0] and alicePosts[1]; add a guard before using those indices (e.g., assert alicePosts.length >= 2 or expect(alicePosts[1]).toBeDefined) so the test fails with a clear message instead of throwing on a null/undefined access; update the assertion block around the alicePosts filter/expect to perform that length check before comparing views.packages/2-sql/4-lanes/sql-builder-new/test/integration/subquery.test.ts (1)
23-33: Consider verifying row content, not just count.The test only asserts
rows.lengthis 4. Verifying at least one row's structure would confirm the join actually produced the expected{ name, title }shape.Suggested enhancement
expect(rows.length).toBe(4); + expect(rows[0]).toMatchObject({ name: expect.any(String), title: expect.any(String) });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/sql-builder-new/test/integration/subquery.test.ts` around lines 23 - 33, The test "subquery as join source" currently only asserts rows.length; update the test around the variables d, sub and the query built with d.users.innerJoin(sub, ...) so it also verifies the shape/content of returned rows (e.g. at least one element has properties "name" and "title" and expected types/values) after calling .select('name','title').all(), rather than only checking the count.packages/2-sql/4-lanes/sql-builder-new/test/integration/mutation.test.ts (1)
7-18: Consider consolidating assertions withtoMatchObject.Per coding guidelines, prefer object matchers when checking multiple related values.
♻️ Suggested refactor
it('INSERT returns inserted row via returning', async () => { const row = await db() .users.insert({ id: 100, name: 'NewUser', email: 'new@test.com' }) .returning('id', 'name') .first(); expect(row).not.toBeNull(); - expect(row!.id).toBe(100); - expect(row!.name).toBe('NewUser'); + expect(row).toMatchObject({ id: 100, name: 'NewUser' }); const rows = await collect(db().users.select('id', 'name').all()); expect(rows.find((r) => r.id === 100)).toEqual({ id: 100, name: 'NewUser' }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/sql-builder-new/test/integration/mutation.test.ts` around lines 7 - 18, The test performs multiple separate assertions for the inserted row; consolidate them using object matchers: replace the separate expects on row!.id and row!.name with a single expect(row).toMatchObject({ id: 100, name: 'NewUser' }) (after the .first() call), and replace the find/assert on the collected rows with a collection matcher such as expect(rows).toContainEqual({ id: 100, name: 'NewUser' }) (using the result of collect(db().users.select('id','name').all())). This uses the existing db(), .users.insert(...).returning(...).first() and collect(...) calls but simplifies assertions to object matchers.packages/2-sql/4-lanes/sql-builder-new/test/runtime/field-proxy.test.ts (1)
30-39: Consider usingtoMatchObjectfor related assertions.Per coding guidelines, prefer object matchers over multiple individual
expect().toBe()calls when checking related values.♻️ Suggested refactor
it('handles joined scope with namespaced access', () => { const proxy = createFieldProxy(joinedScope); const usersCol = proxy.users.id.buildAst() as ColumnRef; const postsCol = proxy.posts.id.buildAst() as ColumnRef; - expect(usersCol.table).toBe('users'); - expect(usersCol.column).toBe('id'); - expect(postsCol.table).toBe('posts'); - expect(postsCol.column).toBe('id'); + expect(usersCol).toMatchObject({ table: 'users', column: 'id' }); + expect(postsCol).toMatchObject({ table: 'posts', column: 'id' }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/sql-builder-new/test/runtime/field-proxy.test.ts` around lines 30 - 39, Replace the two separate assertions per column with object matchers: after creating proxy with createFieldProxy(joinedScope) and building ASTs via proxy.users.id.buildAst() and proxy.posts.id.buildAst() (typed as ColumnRef), assert the resulting objects (usersCol and postsCol) using expect(usersCol).toMatchObject({ table: 'users', column: 'id' }) and similarly for postsCol; this consolidates related checks into a single toMatchObject call for each ColumnRef.packages/2-sql/4-lanes/sql-builder-new/src/runtime/create-db.ts (1)
20-28: DeriveBuilderContextfrom one contract source.
createDb()reads tables/capabilities fromcontract, buttargetandstorageHashfromcontext.contract. If those ever diverge, the same builder will plan against one contract and execute with another contract's metadata. Please assert they match or source the whole context from one object.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/sql-builder-new/src/runtime/create-db.ts` around lines 20 - 28, In createDb(), BuilderContext is being constructed from mixed sources (contract for capabilities/tables but context.contract for target/storageHash) which can cause mismatches; update createDb() to derive all BuilderContext fields from a single source or assert equality: compare contract.target and context.contract.target and contract.storageHash and context.contract.storageHash (or simply take target/storageHash from the same contract variable used for capabilities), and if they differ throw/log a clear error before building the context (refer to createDb, BuilderContext, contract, context.contract, target, storageHash).packages/2-sql/4-lanes/sql-builder-new/test/integration/join.test.ts (1)
7-19: Assert the joined values, not just the row shape.Both cases only verify count plus field presence. A bad
ONclause or broken subquery alias can still pass if the fixture still returns four{ name, title }rows. Please assert the actual joined tuples so the test fails on incorrect join semantics, not just missing columns.As per coding guidelines, "Avoid tautological tests that only restate fixture input - tests must verify behavior, not mirror object shape passed by the test itself."
Also applies to: 50-62
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/sql-builder-new/test/integration/join.test.ts` around lines 7 - 19, The test currently only checks row count and presence of 'name' and 'title' which won’t catch incorrect ON logic; update the INNER JOIN test (and the similar case around lines 50-62) to assert actual joined tuples from db().users.innerJoin(db().posts, (f, fns) => fns.eq(f.users.id, f.posts.user_id)).select('name','title').all() by comparing the returned rows to an expected array of { name, title } pairs (or by sorting and deep-equaling sets) instead of merely checking property existence and length; use the same approach for the other test to ensure join semantics are validated.packages/2-sql/4-lanes/sql-builder-new/src/runtime/functions.ts (1)
94-95: Consider whetherand/orshould parameterize literal values like comparison operators do.Lines 94–95 use
resolveToAstwhich inlines non-expression values asLiteralExpr, while comparison operators useresolvewhich parameterizes them viaParamCollector. This means literal booleans passed toand/orwon't be parameterized (e.g.,fns.and(condition, true)would inline thetruerather than parameterize it).This appears intentional since
and/orare composed from expression results (e.g.,fns.or(fns.eq(f.id, 1), fns.eq(f.id, 4))), not raw literals. However, the type signature permits literals, so this could be clarified with a comment or a type constraint if literals are not supported.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/sql-builder-new/src/runtime/functions.ts` around lines 94 - 95, and/or currently call resolveToAst (creating LiteralExpr for raw values) while comparison operators use resolve (which parameterizes via ParamCollector); update the implementation to either (A) parameterize literals by replacing resolveToAst with resolve in the two functions (and: (...exprs: ExprOrVal<BooleanCodecType>[]) => boolExpr(AndExpr.of(exprs.map(resolve))) and similarly for or) or (B) tighten the type to only accept Expr<BooleanCodecType> and add a clarifying comment on boolExpr/AndExpr/OrExpr that raw literals are not supported; pick one approach and apply it to the and/or implementations (referencing resolveToAst, resolve, and: and, or, boolExpr, AndExpr, OrExpr, ExprOrVal<BooleanCodecType>).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/2-sql/4-lanes/sql-builder-new/src/runtime/builder-base.ts`:
- Around line 67-74: BuilderContext currently exposes a shared mutable
paramCollector which causes cloned builders to leak bind values; change the
design so that paramCollector is not part of the shared BuilderContext (remove
readonly paramCollector from the BuilderContext interface) and instead make
parameter collection builder-local: update builder implementations to create and
use a fresh ParamCollector instance during buildPlan() (or reconstruct
expression callbacks against a new collector) so each builder serializes only
its own params; specifically modify buildPlan(), the builder clone paths, and
expression evaluation code in select(), orderBy(), groupBy(), distinctOn(), and
the where()/having() logic in query-impl.ts to accept or create a local
ParamCollector rather than referencing BuilderContext.paramCollector.
In `@packages/2-sql/4-lanes/sql-builder-new/src/runtime/mutation-impl.ts`:
- Around line 173-205: The UpdateQueryImpl (and DeleteQueryImpl) currently store
a single optional predicate in `#whereCallback` so subsequent where(...) calls
replace the first and callers can omit where(...), yielding full-table
mutations; change the shape so predicates are accumulated and at least one
predicate is required: replace the single WhereCallback/nullable `#whereCallback`
with an array (e.g., `#whereCallbacks`: WhereCallback[]), update the constructor
signature and callers to accept an array (or push the initial predicate into the
array), modify the where(expr: ExpressionBuilder...) method to append the new
predicate to that array and return a new UpdateQueryImpl instance carrying the
accumulated array, and enforce at execution time (or in the builder) that
`#whereCallbacks.length` > 0 (throw or surface a compile/runtime error) to prevent
running UPDATE/DELETE without any predicate; apply the same changes to
DeleteQueryImpl and all places referencing `#whereCallback`, WhereCallback, and
the where(...) implementation so predicates are composed rather than replaced.
---
Outside diff comments:
In `@packages/3-extensions/pgvector/src/core/descriptor-meta.ts`:
- Around line 6-10: The cosineLowering constant uses Mustache-style placeholders
({{self}}/{{arg0}}) which the Postgres adapter doesn't replace; change the
template in cosineLowering to use adapter placeholders '1 - (${self} <=>
${arg0})' so it matches pgvectorQueryOperations, and update the corresponding
expectation in the test file
packages/3-extensions/pgvector/test/operations.test.ts (the assertion around
line 38) to expect the updated '1 - (${self} <=> ${arg0})' SQL string.
In `@packages/3-extensions/sql-orm-client/src/grouped-collection.ts`:
- Around line 157-178: The helper buildBinaryExpr (used to implement
eq/neq/gt/lt/gte/lte for HavingComparisonMethods) currently lowers
eq(null)/neq(null) to BinaryExpr with LiteralExpr.of(null), which generates
invalid SQL like "col = NULL"; change buildBinaryExpr so that when value ===
null and op is 'eq' or 'neq' it returns a NullCheckExpr (imported into the file)
instead of a BinaryExpr — use NullCheckExpr(metric, false) for 'eq' (IS NULL)
and NullCheckExpr(metric, true) for 'neq' (IS NOT NULL); keep all other
operators returning new BinaryExpr(op, metric, LiteralExpr.of(value)). Ensure
you add NullCheckExpr to the AST imports.
In `@packages/3-extensions/sql-orm-client/src/model-accessor.ts`:
- Around line 82-96: The eq/neq branches in the loop over COMPARISON_OPS
currently always build a BinaryExpr with LiteralExpr.of(value), which yields
invalid SQL for nulls; update the methods creation (the methods object in
model-accessor.ts) so that for op === 'eq' if value === null you return
NullCheckExpr.isNull(left) and for op === 'neq' if value === null you return
NullCheckExpr.isNotNull(left), otherwise keep returning new BinaryExpr(op, left,
LiteralExpr.of(value)); keep using the same factories and types
(ComparisonMethods, BinaryExpr, LiteralExpr, NullCheckExpr) so only the null
case logic changes.
In `@packages/3-extensions/sql-orm-client/src/query-plan-aggregate.ts`:
- Around line 72-104: In validateGroupedHavingExpr, ParamRef (param) nodes are
currently returned unchanged which allows unbound placeholders in HAVING; update
validateGroupedHavingExpr to reject ParamRef by throwing an error (or
alternatively recursively validate operation arguments/self) so that grouped
HAVING expressions cannot contain params before lowering; ensure consistency
with validateGroupedComparable and that
compileGroupedAggregate/buildOrmQueryPlan still only receive WHERE
params/descriptors by referencing validateGroupedHavingExpr, operation visitor
handling, and ParamRef/param nodes in your changes.
---
Nitpick comments:
In `@packages/2-sql/4-lanes/sql-builder-new/src/runtime/create-db.ts`:
- Around line 20-28: In createDb(), BuilderContext is being constructed from
mixed sources (contract for capabilities/tables but context.contract for
target/storageHash) which can cause mismatches; update createDb() to derive all
BuilderContext fields from a single source or assert equality: compare
contract.target and context.contract.target and contract.storageHash and
context.contract.storageHash (or simply take target/storageHash from the same
contract variable used for capabilities), and if they differ throw/log a clear
error before building the context (refer to createDb, BuilderContext, contract,
context.contract, target, storageHash).
In `@packages/2-sql/4-lanes/sql-builder-new/src/runtime/functions.ts`:
- Around line 94-95: and/or currently call resolveToAst (creating LiteralExpr
for raw values) while comparison operators use resolve (which parameterizes via
ParamCollector); update the implementation to either (A) parameterize literals
by replacing resolveToAst with resolve in the two functions (and: (...exprs:
ExprOrVal<BooleanCodecType>[]) => boolExpr(AndExpr.of(exprs.map(resolve))) and
similarly for or) or (B) tighten the type to only accept Expr<BooleanCodecType>
and add a clarifying comment on boolExpr/AndExpr/OrExpr that raw literals are
not supported; pick one approach and apply it to the and/or implementations
(referencing resolveToAst, resolve, and: and, or, boolExpr, AndExpr, OrExpr,
ExprOrVal<BooleanCodecType>).
In `@packages/2-sql/4-lanes/sql-builder-new/test/integration/join.test.ts`:
- Around line 7-19: The test currently only checks row count and presence of
'name' and 'title' which won’t catch incorrect ON logic; update the INNER JOIN
test (and the similar case around lines 50-62) to assert actual joined tuples
from db().users.innerJoin(db().posts, (f, fns) => fns.eq(f.users.id,
f.posts.user_id)).select('name','title').all() by comparing the returned rows to
an expected array of { name, title } pairs (or by sorting and deep-equaling
sets) instead of merely checking property existence and length; use the same
approach for the other test to ensure join semantics are validated.
In `@packages/2-sql/4-lanes/sql-builder-new/test/integration/mutation.test.ts`:
- Around line 7-18: The test performs multiple separate assertions for the
inserted row; consolidate them using object matchers: replace the separate
expects on row!.id and row!.name with a single expect(row).toMatchObject({ id:
100, name: 'NewUser' }) (after the .first() call), and replace the find/assert
on the collected rows with a collection matcher such as
expect(rows).toContainEqual({ id: 100, name: 'NewUser' }) (using the result of
collect(db().users.select('id','name').all())). This uses the existing db(),
.users.insert(...).returning(...).first() and collect(...) calls but simplifies
assertions to object matchers.
In `@packages/2-sql/4-lanes/sql-builder-new/test/integration/order-by.test.ts`:
- Around line 44-54: The test "multiple orderBy calls accumulate" assumes Alice
has at least two posts and directly indexes alicePosts[0] and alicePosts[1]; add
a guard before using those indices (e.g., assert alicePosts.length >= 2 or
expect(alicePosts[1]).toBeDefined) so the test fails with a clear message
instead of throwing on a null/undefined access; update the assertion block
around the alicePosts filter/expect to perform that length check before
comparing views.
In `@packages/2-sql/4-lanes/sql-builder-new/test/integration/pagination.test.ts`:
- Around line 19-21: Replace the three related assertions on rows with a single
object matcher: remove expect(rows).toHaveLength(2) and the two
expect(rows[0]!.id).toBe(2)/expect(rows[1]!.id).toBe(3) and instead assert
expect(rows).toMatchObject([{ id: 2 }, { id: 3 }]); target the test's rows
variable in pagination.test.ts so the slice shape is checked in one matcher and
drop the unnecessary non-null assertions.
In `@packages/2-sql/4-lanes/sql-builder-new/test/integration/subquery.test.ts`:
- Around line 23-33: The test "subquery as join source" currently only asserts
rows.length; update the test around the variables d, sub and the query built
with d.users.innerJoin(sub, ...) so it also verifies the shape/content of
returned rows (e.g. at least one element has properties "name" and "title" and
expected types/values) after calling .select('name','title').all(), rather than
only checking the count.
In `@packages/2-sql/4-lanes/sql-builder-new/test/runtime/field-proxy.test.ts`:
- Around line 30-39: Replace the two separate assertions per column with object
matchers: after creating proxy with createFieldProxy(joinedScope) and building
ASTs via proxy.users.id.buildAst() and proxy.posts.id.buildAst() (typed as
ColumnRef), assert the resulting objects (usersCol and postsCol) using
expect(usersCol).toMatchObject({ table: 'users', column: 'id' }) and similarly
for postsCol; this consolidates related checks into a single toMatchObject call
for each ColumnRef.
In `@packages/2-sql/4-lanes/sql-builder-new/test/runtime/param-collector.test.ts`:
- Around line 5-16: Add tests that exercise ParamCollector.add's optional meta
argument and ParamCollector.getMetas(): create a ParamCollector, call add(value,
meta) with different meta objects (including undefined), assert returned
ParamRef instances and their indices (ParamRef.index), assert getValues()
remains correct, and assert getMetas() returns the corresponding meta entries in
the same order (including undefined where omitted) and that size reflects total
entries; use ParamCollector, add, ParamRef, getValues, getMetas, and size to
locate the code under test.
🪄 Autofix (Beta)
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
Run ID: 9eb45ca2-c2b5-425b-a54a-eb34947525eb
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (92)
.claude/scripts/worktree-create.mjs.claude/settings.jsonpackages/2-sql/4-lanes/kysely-lane/src/transform/transform-expr.tspackages/2-sql/4-lanes/kysely-lane/src/where-expr.tspackages/2-sql/4-lanes/kysely-lane/test/build-plan.collect-params.test.tspackages/2-sql/4-lanes/relational-core/README.mdpackages/2-sql/4-lanes/relational-core/package.jsonpackages/2-sql/4-lanes/relational-core/src/ast/types.tspackages/2-sql/4-lanes/relational-core/src/exports/query-operations.tspackages/2-sql/4-lanes/relational-core/src/query-lane-context.tspackages/2-sql/4-lanes/relational-core/src/query-operation-registry.tspackages/2-sql/4-lanes/relational-core/test/ast/predicate.test.tspackages/2-sql/4-lanes/relational-core/test/ast/rich-ast.test.tspackages/2-sql/4-lanes/relational-core/test/ast/visitors.test.tspackages/2-sql/4-lanes/relational-core/test/utils.tspackages/2-sql/4-lanes/relational-core/tsdown.config.tspackages/2-sql/4-lanes/sql-builder-new/README.mdpackages/2-sql/4-lanes/sql-builder-new/STATUS.mdpackages/2-sql/4-lanes/sql-builder-new/package.jsonpackages/2-sql/4-lanes/sql-builder-new/src/builders.tspackages/2-sql/4-lanes/sql-builder-new/src/exports/types.tspackages/2-sql/4-lanes/sql-builder-new/src/expression.tspackages/2-sql/4-lanes/sql-builder-new/src/index.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/builder-base.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/create-db.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/expression-impl.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/field-proxy.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/functions.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/index.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/joined-tables-impl.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/mutation-impl.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/param-collector.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/query-impl.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/table-proxy-impl.tspackages/2-sql/4-lanes/sql-builder-new/src/scope.tspackages/2-sql/4-lanes/sql-builder-new/src/types/db.tspackages/2-sql/4-lanes/sql-builder-new/src/types/grouped-query.tspackages/2-sql/4-lanes/sql-builder-new/src/types/joined-tables.tspackages/2-sql/4-lanes/sql-builder-new/src/types/mutation-query.tspackages/2-sql/4-lanes/sql-builder-new/src/types/select-query.tspackages/2-sql/4-lanes/sql-builder-new/src/types/shared.tspackages/2-sql/4-lanes/sql-builder-new/src/types/table-proxy.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/distinct.test.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/execution.test.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/extension-functions.test.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/group-by.test.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/join.test.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/mutation.test.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/order-by.test.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/pagination.test.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/select.test.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/setup.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/subquery.test.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/where.test.tspackages/2-sql/4-lanes/sql-builder-new/test/playground/mutations.test-d.tspackages/2-sql/4-lanes/sql-builder-new/test/playground/pagination.test-d.tspackages/2-sql/4-lanes/sql-builder-new/test/runtime/builders.test.tspackages/2-sql/4-lanes/sql-builder-new/test/runtime/expression-impl.test.tspackages/2-sql/4-lanes/sql-builder-new/test/runtime/field-proxy.test.tspackages/2-sql/4-lanes/sql-builder-new/test/runtime/functions.test.tspackages/2-sql/4-lanes/sql-builder-new/test/runtime/param-collector.test.tspackages/2-sql/4-lanes/sql-builder-new/test/runtime/test-helpers.tspackages/2-sql/4-lanes/sql-builder-new/tsdown.config.tspackages/2-sql/4-lanes/sql-lane/src/sql/include-builder.tspackages/2-sql/4-lanes/sql-lane/src/sql/predicate-builder.tspackages/2-sql/4-lanes/sql-lane/test/utils/guards.test.tspackages/2-sql/5-runtime/src/sql-context.tspackages/3-extensions/pgvector/src/core/descriptor-meta.tspackages/3-extensions/pgvector/src/exports/runtime.tspackages/3-extensions/sql-orm-client/src/collection-internal-types.tspackages/3-extensions/sql-orm-client/src/collection.tspackages/3-extensions/sql-orm-client/src/filters.tspackages/3-extensions/sql-orm-client/src/grouped-collection.tspackages/3-extensions/sql-orm-client/src/model-accessor.tspackages/3-extensions/sql-orm-client/src/mutation-executor.tspackages/3-extensions/sql-orm-client/src/query-plan-aggregate.tspackages/3-extensions/sql-orm-client/src/query-plan-select.tspackages/3-extensions/sql-orm-client/src/types.tspackages/3-extensions/sql-orm-client/src/where-binding.tspackages/3-extensions/sql-orm-client/src/where-interop.tspackages/3-extensions/sql-orm-client/src/where-utils.tspackages/3-extensions/sql-orm-client/test/filters.test.tspackages/3-extensions/sql-orm-client/test/model-accessor.test.tspackages/3-extensions/sql-orm-client/test/mutation-executor.test.tspackages/3-extensions/sql-orm-client/test/query-plan-aggregate.test.tspackages/3-extensions/sql-orm-client/test/query-plan-select.test.tspackages/3-extensions/sql-orm-client/test/rich-filters-and-where.test.tspackages/3-extensions/sql-orm-client/test/where-binding.test.tspackages/3-extensions/sql-orm-client/test/where-interop.select-includes.test.tspackages/3-extensions/sql-orm-client/test/where-interop.test.tspackages/3-targets/6-adapters/postgres/src/core/adapter.tspackages/3-targets/6-adapters/postgres/test/adapter.test.ts
💤 Files with no reviewable changes (1)
- packages/2-sql/4-lanes/sql-builder-new/src/builders.ts
packages/2-sql/4-lanes/sql-builder-new/src/runtime/mutation-impl.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/3-extensions/sql-orm-client/src/query-plan-select.ts (1)
105-115:⚠️ Potential issue | 🟠 MajorReject
nullcursor values before building cursor predicates.Current logic only rejects
undefined. If a cursor value isnull, this path can still emit null comparisons (= NULLin prefix branches and< / > NULLin boundary), which breaks SQL truth semantics and can return incorrect/empty pages.Based on learnings, when generating SQL comparisons in this SQL ORM client, `col = NULL` / `col <> NULL` must not be emitted; null must be handled explicitly.🛠️ Proposed fix
for (const order of orderBy) { const value = cursor[order.column]; if (value === undefined) { throw new Error(`Missing cursor value for orderBy column "${order.column}"`); } + if (value === null) { + throw new Error(`Null cursor value for orderBy column "${order.column}" is not supported`); + } entries.push({ ...order, value, }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-extensions/sql-orm-client/src/query-plan-select.ts` around lines 105 - 115, The loop in query-plan-select.ts that builds CursorOrderEntry currently only rejects undefined cursor values; update the check inside the for (const order of orderBy) loop (where value = cursor[order.column]) to reject both undefined and null (e.g., if (value === undefined || value === null) throw new Error(...)) so null cursor values are not used to emit SQL comparisons; keep the same error message text but ensure it triggers for null too to prevent generating = NULL / < NULL predicates.packages/3-extensions/sql-orm-client/src/grouped-collection.ts (1)
157-166:⚠️ Potential issue | 🔴 CriticalAdd
NullCheckExprimport and handle null comparisons in HAVING predicates.The
createHavingComparisonMethodsfunction usesBinaryExprfor all values, includingnull. This produces incorrect SQL:col = NULLandcol <> NULLboth evaluate toNULL, notTRUE/FALSE, breaking HAVING filters.For
eq(null)andneq(null), useNullCheckExpr.isNull(metric)andNullCheckExpr.isNotNull(metric)respectively. This pattern is already established elsewhere in the codebase (seefilters.tslines 58–61 andmodel-accessor.tslines 98–99).Proposed fix
import { AggregateExpr, BinaryExpr, type BinaryOp, type BoundWhereExpr, ColumnRef, type Expression, LiteralExpr, + NullCheckExpr, } from '@prisma-next/sql-relational-core/ast'; function createHavingComparisonMethods<T extends number | null>( metric: AggregateExpr, ): HavingComparisonMethods<T> { - const buildBinaryExpr = (op: BinaryOp, value: unknown): Expression => - new BinaryExpr(op, metric, LiteralExpr.of(value)); + const buildBinaryExpr = (op: BinaryOp, value: unknown): Expression => { + if (value === null) { + if (op === 'eq') return NullCheckExpr.isNull(metric); + if (op === 'neq') return NullCheckExpr.isNotNull(metric); + } + return new BinaryExpr(op, metric, LiteralExpr.of(value)); + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-extensions/sql-orm-client/src/grouped-collection.ts` around lines 157 - 166, The HAVING comparison builder currently always constructs a BinaryExpr even for null, producing invalid SQL; update createHavingComparisonMethods (the buildBinaryExpr/eq/neq logic) to import and use NullCheckExpr and return NullCheckExpr.isNull(metric) for eq(null) and NullCheckExpr.isNotNull(metric) for neq(null), while keeping BinaryExpr for non-null values (i.e., call buildBinaryExpr for other values); ensure NullCheckExpr is imported at the top of grouped-collection.ts.
🧹 Nitpick comments (3)
packages/3-extensions/sql-orm-client/test/mutation-executor.test.ts (1)
56-658: Consider splitting this test file in a follow-up.The file is 658 lines, exceeding the 500-line guideline. The tests could be split by functionality (e.g.,
mutation-executor.create.test.ts,mutation-executor.update.test.ts,mutation-executor.helpers.test.ts). This is a pre-existing condition and not introduced by this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-extensions/sql-orm-client/test/mutation-executor.test.ts` around lines 56 - 658, Test file exceeds 500 lines; split tests by responsibility: move all tests exercising executeNestedCreateMutation (any describe/it that calls executeNestedCreateMutation) into a new mutation-executor.create.test.ts, move tests for executeNestedUpdateMutation into mutation-executor.update.test.ts, and move helper-specific tests (hasNestedMutationCallbacks, buildPrimaryKeyFilterFromRow and other pure helpers) into mutation-executor.helpers.test.ts; ensure each new file imports getTestContract, createMockRuntime, withTransaction, withConnection, userIdFilter/postIdFilter, and the tested functions (executeNestedCreateMutation, executeNestedUpdateMutation, hasNestedMutationCallbacks, buildPrimaryKeyFilterFromRow) so test names and expectations remain unchanged and the test suite runs identically.packages/2-sql/4-lanes/relational-core/test/ast/rich-ast.test.ts (1)
194-194: Consider renaming the fold marker fromlist:totuple:for consistency.At Line 194, the fold handler now receives a
TupleExpression, but the emitted marker string still sayslist:. Renaming it would reduce cognitive mismatch in this test.♻️ Proposed tweak
- tuple: (expr) => [`list:${expr.values.length}`], + tuple: (expr) => [`tuple:${expr.values.length}`],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/relational-core/test/ast/rich-ast.test.ts` at line 194, The fold handler for TupleExpression emits a marker string with "list:" which is inconsistent; update the handler that currently reads tuple: (expr) => [`list:${expr.values.length}`] to emit `tuple:` instead (e.g., tuple: (expr) => [`tuple:${expr.values.length}`]) so the marker matches the TupleExpression and removes the cognitive mismatch in the test.packages/3-extensions/sql-orm-client/src/query-plan-select.ts (1)
88-95: Replace casts with non-null assertions for validated array accesses.At lines 88 and 95, non-null assertions clarify that the arrays are guaranteed non-empty at those points. After the
length === 1checks, use!instead ofas Expressioncasts.♻️ Proposed cleanup
- return branchExprs[0] as Expression; + return branchExprs[0]!; ... - return branches[0] as Expression; + return branches[0]!;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-extensions/sql-orm-client/src/query-plan-select.ts` around lines 88 - 95, Replace the unsafe type assertions with non-null assertions where you’ve already validated the arrays are non-empty: after the branchExprs length check return branchExprs[0]! instead of returning it as Expression, and after the branches.length === 1 check return branches[0]! instead of using an as Expression cast; update the return sites around the AndExpr.of(branchExprs) call and the single-branch return to use the ! operator (referencing branchExprs, branches, Expression, AndExpr.of) so the compiler understands these accesses are validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/2-sql/4-lanes/sql-builder-new/README.md`:
- Around line 8-11: The README snippet references contractJson but never defines
or imports it; update the example so contractJson is explicitly provided (e.g.,
import or require the compiled contract JSON or show an inline placeholder)
before calling postgres<Contract>({ contractJson, url:
process.env['DATABASE_URL']! }); — add a line that imports/defines contractJson
(for example: import contractJson from './contract.json' or const contractJson =
{ /* contract JSON */ }) and keep the rest (Contract, postgres, db) unchanged so
the snippet is copy-pasteable.
In `@packages/2-sql/4-lanes/sql-builder-new/src/runtime/builder-base.ts`:
- Around line 269-295: resolveOrderBy currently ignores OrderByOptions.nulls;
update both branches (the string branch that creates IdentifierRef and the
function branch that uses result.buildAst()) to include options?.nulls into the
produced OrderByItem instead of only applying direction. Specifically, when
returning OrderByItem.asc(...) or OrderByItem.desc(...), pass the nulls setting
(e.g., options?.nulls) into OrderByItem (or call the appropriate API to set
nulls on the returned OrderByItem) so that OrderByOptions.nulls is reflected in
the AST; make the same change for the IdentifierRef path and the ExprCallback
path so nulls is wired through for both code paths in resolveOrderBy.
- Around line 168-175: Replace uses of the `in` operator that check for scope
membership with `Object.hasOwn()` to avoid walking the prototype chain;
specifically update mergeScopes() (replace `if (!(k in b.topLevel))` and `if
(!(k in a.topLevel))`), resolveOrderBy(), resolveGroupBy(), and
resolveDistinctOn() so they call `Object.hasOwn(b.topLevel, k)` /
`Object.hasOwn(a.topLevel, k)` (or equivalent checks against the correct scope
objects) when testing key presence, preserving the existing control flow and
behavior otherwise.
In `@packages/2-sql/4-lanes/sql-builder-new/src/runtime/functions.ts`:
- Around line 141-156: The blind cast of resolvedArgs[0] to AstExpression must
be replaced with a safe check/conversion: remove "const self = resolvedArgs[0]
as AstExpression" and instead ensure the receiver is an AstExpression by using a
type guard (e.g. isAstExpression(resolvedArgs[0])) and, if it isn't, convert the
original arg (the corresponding element from args) into an AstExpression (e.g.
call pc.add(...) or arg.buildAst() as appropriate) before using it; update the
code around resolvedArgs, self, and the OperationExpr construction (symbols:
resolvedArgs, args.map(...), self, ExpressionImpl, OperationExpr, entry) to
perform this runtime guard/conversion and throw a clear error if conversion is
impossible.
In `@packages/2-sql/4-lanes/sql-builder-new/src/runtime/joined-tables-impl.ts`:
- Around line 207-210: The return currently blind-casts SelectQueryImpl to the
wrong scope; replace the "as unknown as SelectQuery<QC, AvailableScope,
EmptyRow>" cast with the correct merged scope type produced by
MergeScopes<ParentScope, Other[typeof JoinOuterScope]> (i.e., SelectQuery<QC,
MergeScopes<AvailableScope, Other[typeof JoinOuterScope]>, EmptyRow>), so that
the SelectQueryImpl constructed from emptyState(other.buildAst(), parentMerged)
matches the LateralBuilder.from() signature; adjust the generic on the returned
SelectQuery accordingly and remove the unknown double-cast.
---
Outside diff comments:
In `@packages/3-extensions/sql-orm-client/src/grouped-collection.ts`:
- Around line 157-166: The HAVING comparison builder currently always constructs
a BinaryExpr even for null, producing invalid SQL; update
createHavingComparisonMethods (the buildBinaryExpr/eq/neq logic) to import and
use NullCheckExpr and return NullCheckExpr.isNull(metric) for eq(null) and
NullCheckExpr.isNotNull(metric) for neq(null), while keeping BinaryExpr for
non-null values (i.e., call buildBinaryExpr for other values); ensure
NullCheckExpr is imported at the top of grouped-collection.ts.
In `@packages/3-extensions/sql-orm-client/src/query-plan-select.ts`:
- Around line 105-115: The loop in query-plan-select.ts that builds
CursorOrderEntry currently only rejects undefined cursor values; update the
check inside the for (const order of orderBy) loop (where value =
cursor[order.column]) to reject both undefined and null (e.g., if (value ===
undefined || value === null) throw new Error(...)) so null cursor values are not
used to emit SQL comparisons; keep the same error message text but ensure it
triggers for null too to prevent generating = NULL / < NULL predicates.
---
Nitpick comments:
In `@packages/2-sql/4-lanes/relational-core/test/ast/rich-ast.test.ts`:
- Line 194: The fold handler for TupleExpression emits a marker string with
"list:" which is inconsistent; update the handler that currently reads tuple:
(expr) => [`list:${expr.values.length}`] to emit `tuple:` instead (e.g., tuple:
(expr) => [`tuple:${expr.values.length}`]) so the marker matches the
TupleExpression and removes the cognitive mismatch in the test.
In `@packages/3-extensions/sql-orm-client/src/query-plan-select.ts`:
- Around line 88-95: Replace the unsafe type assertions with non-null assertions
where you’ve already validated the arrays are non-empty: after the branchExprs
length check return branchExprs[0]! instead of returning it as Expression, and
after the branches.length === 1 check return branches[0]! instead of using an as
Expression cast; update the return sites around the AndExpr.of(branchExprs) call
and the single-branch return to use the ! operator (referencing branchExprs,
branches, Expression, AndExpr.of) so the compiler understands these accesses are
validated.
In `@packages/3-extensions/sql-orm-client/test/mutation-executor.test.ts`:
- Around line 56-658: Test file exceeds 500 lines; split tests by
responsibility: move all tests exercising executeNestedCreateMutation (any
describe/it that calls executeNestedCreateMutation) into a new
mutation-executor.create.test.ts, move tests for executeNestedUpdateMutation
into mutation-executor.update.test.ts, and move helper-specific tests
(hasNestedMutationCallbacks, buildPrimaryKeyFilterFromRow and other pure
helpers) into mutation-executor.helpers.test.ts; ensure each new file imports
getTestContract, createMockRuntime, withTransaction, withConnection,
userIdFilter/postIdFilter, and the tested functions
(executeNestedCreateMutation, executeNestedUpdateMutation,
hasNestedMutationCallbacks, buildPrimaryKeyFilterFromRow) so test names and
expectations remain unchanged and the test suite runs identically.
🪄 Autofix (Beta)
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
Run ID: 9de7cb49-b142-4d28-9732-acf0fb33a1f4
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (92)
.claude/scripts/worktree-create.mjs.claude/settings.jsonpackages/2-sql/4-lanes/kysely-lane/src/transform/transform-expr.tspackages/2-sql/4-lanes/kysely-lane/src/where-expr.tspackages/2-sql/4-lanes/kysely-lane/test/build-plan.collect-params.test.tspackages/2-sql/4-lanes/relational-core/README.mdpackages/2-sql/4-lanes/relational-core/package.jsonpackages/2-sql/4-lanes/relational-core/src/ast/types.tspackages/2-sql/4-lanes/relational-core/src/exports/query-operations.tspackages/2-sql/4-lanes/relational-core/src/query-lane-context.tspackages/2-sql/4-lanes/relational-core/src/query-operation-registry.tspackages/2-sql/4-lanes/relational-core/test/ast/predicate.test.tspackages/2-sql/4-lanes/relational-core/test/ast/rich-ast.test.tspackages/2-sql/4-lanes/relational-core/test/ast/visitors.test.tspackages/2-sql/4-lanes/relational-core/test/utils.tspackages/2-sql/4-lanes/relational-core/tsdown.config.tspackages/2-sql/4-lanes/sql-builder-new/README.mdpackages/2-sql/4-lanes/sql-builder-new/STATUS.mdpackages/2-sql/4-lanes/sql-builder-new/package.jsonpackages/2-sql/4-lanes/sql-builder-new/src/builders.tspackages/2-sql/4-lanes/sql-builder-new/src/exports/types.tspackages/2-sql/4-lanes/sql-builder-new/src/expression.tspackages/2-sql/4-lanes/sql-builder-new/src/index.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/builder-base.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/create-db.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/expression-impl.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/field-proxy.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/functions.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/index.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/joined-tables-impl.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/mutation-impl.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/param-collector.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/query-impl.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/table-proxy-impl.tspackages/2-sql/4-lanes/sql-builder-new/src/scope.tspackages/2-sql/4-lanes/sql-builder-new/src/types/db.tspackages/2-sql/4-lanes/sql-builder-new/src/types/grouped-query.tspackages/2-sql/4-lanes/sql-builder-new/src/types/joined-tables.tspackages/2-sql/4-lanes/sql-builder-new/src/types/mutation-query.tspackages/2-sql/4-lanes/sql-builder-new/src/types/select-query.tspackages/2-sql/4-lanes/sql-builder-new/src/types/shared.tspackages/2-sql/4-lanes/sql-builder-new/src/types/table-proxy.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/distinct.test.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/execution.test.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/extension-functions.test.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/group-by.test.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/join.test.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/mutation.test.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/order-by.test.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/pagination.test.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/select.test.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/setup.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/subquery.test.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/where.test.tspackages/2-sql/4-lanes/sql-builder-new/test/playground/mutations.test-d.tspackages/2-sql/4-lanes/sql-builder-new/test/playground/pagination.test-d.tspackages/2-sql/4-lanes/sql-builder-new/test/runtime/builders.test.tspackages/2-sql/4-lanes/sql-builder-new/test/runtime/expression-impl.test.tspackages/2-sql/4-lanes/sql-builder-new/test/runtime/field-proxy.test.tspackages/2-sql/4-lanes/sql-builder-new/test/runtime/functions.test.tspackages/2-sql/4-lanes/sql-builder-new/test/runtime/param-collector.test.tspackages/2-sql/4-lanes/sql-builder-new/test/runtime/test-helpers.tspackages/2-sql/4-lanes/sql-builder-new/tsdown.config.tspackages/2-sql/4-lanes/sql-lane/src/sql/include-builder.tspackages/2-sql/4-lanes/sql-lane/src/sql/predicate-builder.tspackages/2-sql/4-lanes/sql-lane/test/utils/guards.test.tspackages/2-sql/5-runtime/src/sql-context.tspackages/3-extensions/pgvector/src/core/descriptor-meta.tspackages/3-extensions/pgvector/src/exports/runtime.tspackages/3-extensions/sql-orm-client/src/collection-internal-types.tspackages/3-extensions/sql-orm-client/src/collection.tspackages/3-extensions/sql-orm-client/src/filters.tspackages/3-extensions/sql-orm-client/src/grouped-collection.tspackages/3-extensions/sql-orm-client/src/model-accessor.tspackages/3-extensions/sql-orm-client/src/mutation-executor.tspackages/3-extensions/sql-orm-client/src/query-plan-aggregate.tspackages/3-extensions/sql-orm-client/src/query-plan-select.tspackages/3-extensions/sql-orm-client/src/types.tspackages/3-extensions/sql-orm-client/src/where-binding.tspackages/3-extensions/sql-orm-client/src/where-interop.tspackages/3-extensions/sql-orm-client/src/where-utils.tspackages/3-extensions/sql-orm-client/test/filters.test.tspackages/3-extensions/sql-orm-client/test/model-accessor.test.tspackages/3-extensions/sql-orm-client/test/mutation-executor.test.tspackages/3-extensions/sql-orm-client/test/query-plan-aggregate.test.tspackages/3-extensions/sql-orm-client/test/query-plan-select.test.tspackages/3-extensions/sql-orm-client/test/rich-filters-and-where.test.tspackages/3-extensions/sql-orm-client/test/where-binding.test.tspackages/3-extensions/sql-orm-client/test/where-interop.select-includes.test.tspackages/3-extensions/sql-orm-client/test/where-interop.test.tspackages/3-targets/6-adapters/postgres/src/core/adapter.tspackages/3-targets/6-adapters/postgres/test/adapter.test.ts
💤 Files with no reviewable changes (1)
- packages/2-sql/4-lanes/sql-builder-new/src/builders.ts
✅ Files skipped from review due to trivial changes (32)
- .claude/scripts/worktree-create.mjs
- .claude/settings.json
- packages/2-sql/4-lanes/sql-builder-new/tsdown.config.ts
- packages/2-sql/4-lanes/relational-core/src/exports/query-operations.ts
- packages/2-sql/4-lanes/sql-builder-new/test/integration/group-by.test.ts
- packages/2-sql/4-lanes/sql-lane/src/sql/include-builder.ts
- packages/2-sql/4-lanes/sql-builder-new/test/integration/distinct.test.ts
- packages/2-sql/4-lanes/relational-core/tsdown.config.ts
- packages/2-sql/4-lanes/sql-builder-new/test/integration/pagination.test.ts
- packages/2-sql/4-lanes/sql-builder-new/src/types/joined-tables.ts
- packages/2-sql/4-lanes/sql-builder-new/test/runtime/expression-impl.test.ts
- packages/3-extensions/sql-orm-client/src/where-interop.ts
- packages/2-sql/4-lanes/sql-builder-new/test/runtime/param-collector.test.ts
- packages/2-sql/4-lanes/sql-builder-new/src/runtime/index.ts
- packages/2-sql/4-lanes/sql-builder-new/src/runtime/expression-impl.ts
- packages/3-extensions/sql-orm-client/test/where-interop.select-includes.test.ts
- packages/2-sql/4-lanes/sql-builder-new/test/playground/mutations.test-d.ts
- packages/2-sql/4-lanes/sql-builder-new/test/integration/subquery.test.ts
- packages/2-sql/4-lanes/sql-builder-new/test/integration/where.test.ts
- packages/2-sql/4-lanes/sql-builder-new/test/runtime/field-proxy.test.ts
- packages/2-sql/4-lanes/sql-builder-new/test/integration/execution.test.ts
- packages/2-sql/4-lanes/sql-builder-new/test/integration/select.test.ts
- packages/2-sql/4-lanes/sql-builder-new/test/integration/extension-functions.test.ts
- packages/2-sql/4-lanes/sql-builder-new/test/integration/order-by.test.ts
- packages/2-sql/4-lanes/sql-builder-new/test/runtime/test-helpers.ts
- packages/2-sql/4-lanes/sql-builder-new/STATUS.md
- packages/3-extensions/pgvector/src/core/descriptor-meta.ts
- packages/2-sql/4-lanes/relational-core/src/query-operation-registry.ts
- packages/2-sql/4-lanes/sql-builder-new/src/types/db.ts
- packages/2-sql/4-lanes/sql-builder-new/src/types/table-proxy.ts
- packages/2-sql/4-lanes/sql-builder-new/test/runtime/builders.test.ts
- packages/2-sql/4-lanes/sql-builder-new/src/types/mutation-query.ts
🚧 Files skipped from review as they are similar to previous changes (41)
- packages/2-sql/4-lanes/kysely-lane/test/build-plan.collect-params.test.ts
- packages/2-sql/4-lanes/kysely-lane/src/where-expr.ts
- packages/2-sql/4-lanes/relational-core/package.json
- packages/2-sql/4-lanes/relational-core/README.md
- packages/2-sql/4-lanes/relational-core/test/ast/visitors.test.ts
- packages/2-sql/4-lanes/relational-core/test/utils.ts
- packages/2-sql/4-lanes/relational-core/test/ast/predicate.test.ts
- packages/2-sql/4-lanes/sql-lane/test/utils/guards.test.ts
- packages/3-extensions/sql-orm-client/test/model-accessor.test.ts
- packages/3-extensions/pgvector/src/exports/runtime.ts
- packages/3-extensions/sql-orm-client/test/query-plan-aggregate.test.ts
- packages/2-sql/4-lanes/sql-builder-new/package.json
- packages/3-extensions/sql-orm-client/src/mutation-executor.ts
- packages/2-sql/4-lanes/kysely-lane/src/transform/transform-expr.ts
- packages/2-sql/5-runtime/src/sql-context.ts
- packages/2-sql/4-lanes/sql-builder-new/src/runtime/create-db.ts
- packages/3-extensions/sql-orm-client/test/rich-filters-and-where.test.ts
- packages/3-extensions/sql-orm-client/test/query-plan-select.test.ts
- packages/3-extensions/sql-orm-client/test/where-interop.test.ts
- packages/2-sql/4-lanes/sql-lane/src/sql/predicate-builder.ts
- packages/3-extensions/sql-orm-client/src/model-accessor.ts
- packages/3-extensions/sql-orm-client/test/filters.test.ts
- packages/3-extensions/sql-orm-client/src/collection-internal-types.ts
- packages/2-sql/4-lanes/sql-builder-new/test/integration/mutation.test.ts
- packages/2-sql/4-lanes/sql-builder-new/test/integration/join.test.ts
- packages/2-sql/4-lanes/sql-builder-new/src/runtime/param-collector.ts
- packages/2-sql/4-lanes/sql-builder-new/test/runtime/functions.test.ts
- packages/2-sql/4-lanes/sql-builder-new/src/exports/types.ts
- packages/3-targets/6-adapters/postgres/src/core/adapter.ts
- packages/3-extensions/sql-orm-client/src/filters.ts
- packages/3-extensions/sql-orm-client/src/collection.ts
- packages/3-targets/6-adapters/postgres/test/adapter.test.ts
- packages/2-sql/4-lanes/sql-builder-new/src/scope.ts
- packages/2-sql/4-lanes/sql-builder-new/src/runtime/field-proxy.ts
- packages/3-extensions/sql-orm-client/src/types.ts
- packages/2-sql/4-lanes/sql-builder-new/src/types/grouped-query.ts
- packages/3-extensions/sql-orm-client/src/where-utils.ts
- packages/3-extensions/sql-orm-client/src/query-plan-aggregate.ts
- packages/2-sql/4-lanes/sql-builder-new/src/runtime/mutation-impl.ts
- packages/2-sql/4-lanes/sql-builder-new/src/runtime/query-impl.ts
- packages/2-sql/4-lanes/relational-core/src/ast/types.ts
packages/2-sql/4-lanes/sql-builder-new/src/runtime/joined-tables-impl.ts
Show resolved
Hide resolved
SevInf
force-pushed
the
worktree/query-builder-impl
branch
from
0813b2f to
b844781
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
packages/2-sql/4-lanes/sql-builder-new/README.md (1)
8-11:⚠️ Potential issue | 🟡 MinorComplete the setup snippet.
contractJsonis still never imported or defined here, so the first example is not copy-pasteable as written.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/sql-builder-new/README.md` around lines 8 - 11, The README example uses contractJson but never defines or imports it; add an import or definition for contractJson so the snippet is copy-pasteable (e.g., import contractJson from './contract.json' or require it) and ensure it is passed into postgres<Contract>({ contractJson, url: process.env['DATABASE_URL']! }) alongside the existing Contract type and postgres call so db is created correctly.
🧹 Nitpick comments (2)
packages/2-sql/4-lanes/sql-builder-new/src/expression.ts (1)
12-15: Move the AST type import to a normal top-levelimport type.
buildAst()now uses an inlineimport(...)type. This file lives undersrc/, so it should pull that type in via a regular top-level import instead.♻️ Suggested change
+import type { Expression as AstExpression } from '@prisma-next/sql-relational-core/ast'; import type { QueryOperationTypesBase } from '@prisma-next/sql-contract/types'; import type { Expand, ExpressionType, @@ export type Expression<T extends ScopeField> = { [ExpressionType]: T; - buildAst(): import('@prisma-next/sql-relational-core/ast').Expression; + buildAst(): AstExpression; };As per coding guidelines, "Prohibit inline type imports in source files; use separate import lines."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/sql-builder-new/src/expression.ts` around lines 12 - 15, The inline type import used in the Expression<T extends ScopeField> type for buildAst() should be moved to a top-level type import: add a top-level `import type` from '@prisma-next/sql-relational-core/ast' and replace the inline `import(...)` reference in the buildAst() signature with that imported type. Update the Expression declaration (symbols: Expression, buildAst, ExpressionType) to reference the new top-level imported AST Expression type so no inline type imports remain in this source file.packages/2-sql/4-lanes/sql-builder-new/src/runtime/functions.ts (1)
72-80: Consider type guard forExpressionImplaccess.Line 77 uses
(expr as ExpressionImpl).field.codecIdto extract the codec ID. While this works assuming allExpressioninstances areExpressionImpl, a type guard would be safer and align with the no-blind-casts guideline.♻️ Proposed refactor
function numericAgg( fn: 'sum' | 'avg' | 'min' | 'max', expr: Expression<ScopeField>, ): ExpressionImpl<{ codecId: string; nullable: true }> { + if (!(expr instanceof ExpressionImpl)) { + throw new Error('Expected ExpressionImpl for aggregate function'); + } return new ExpressionImpl(AggregateExpr[fn](expr.buildAst()), { - codecId: (expr as ExpressionImpl).field.codecId, + codecId: expr.field.codecId, nullable: true as const, }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/sql-builder-new/src/runtime/functions.ts` around lines 72 - 80, numericAgg currently casts expr to ExpressionImpl to read field.codecId which risks unsafe blind-casting; add a type guard (e.g., isExpressionImpl function or an instanceof check) to assert expr is an ExpressionImpl before accessing .field.codecId and handle the non-ExpressionImpl case (throw a clear error or derive codecId safely) so numericAgg only reads codecId when the guard passes; reference the numericAgg function and ExpressionImpl/type-guard name when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/2-sql/4-lanes/sql-builder-new/src/runtime/create-db.ts`:
- Around line 13-26: The createDb factory mixes metadata from the standalone
contract parameter and the ExecutionContext (options.context), risking
mismatched plans; fix it by using the ExecutionContext as the single source of
truth: populate BuilderContext fields (capabilities, storage/tables,
queryOperationTypes, target, storageHash) from options.context (i.e.,
context.contract and context.queryOperations) instead of reading
storage/capabilities from the contract argument, and stop silently substituting
'unknown' — validate required fields (throw or assert) when
context.contract.target or storageHash are missing; update createDb and the
BuilderContext construction accordingly (references: createDb, BuilderContext,
contract, options, context, context.queryOperations, context.contract) so
schema()/sql()/orm() are built against a single ExecutionContext.
- Around line 28-35: The Proxy get handler currently reads
contract.storage.tables[prop] and treats any truthy result as a real table,
which can pick up prototype properties; change it to first check
Object.hasOwn(contract.storage.tables, prop) (or Object.hasOwn) before accessing
the value, and only then create the TableProxyImpl (referencing the Proxy get
handler, contract.storage.tables, and TableProxyImpl) so prototype members like
toString/constructor are ignored.
In `@packages/2-sql/4-lanes/sql-builder-new/src/runtime/field-proxy.ts`:
- Around line 8-19: The proxy get trap is accessing inherited properties
directly (e.g., scope.topLevel[prop] and scope.namespaces[prop]) which lets keys
like toString or constructor be treated as schema members; update the get
handler(s) to first check Object.hasOwn(scope.topLevel, prop) before
constructing new ExpressionImpl(IdentifierRef.of(prop), topField) and
Object.hasOwn(scope.namespaces, prop) before returning
createNamespaceProxy(prop, nsFields), and apply the same own-property guard to
the secondary get trap that performs the same lookups.
In `@packages/2-sql/4-lanes/sql-builder-new/src/scope.ts`:
- Around line 24-31: The getJoinOuterScope and getRowFields return types are too
broad and erase generics; update JoinSource (and the Scope interface where
getRowFields is declared) so getJoinOuterScope() returns the exact type stored
on the JoinSource[typeof JoinOuterScope].topLevel (i.e., the generic Row) and
getRowFields() returns the precise Record<Alias, ScopeField> or the exact mapped
type tied to the generic LateralRow, preserving Alias/Row generics; adjust
signatures for getJoinOuterScope and getRowFields to use those generic type
parameters so callers (e.g., in joined-tables-impl.ts) no longer need casts to
Other[typeof JoinOuterScope] or LateralRow.
---
Duplicate comments:
In `@packages/2-sql/4-lanes/sql-builder-new/README.md`:
- Around line 8-11: The README example uses contractJson but never defines or
imports it; add an import or definition for contractJson so the snippet is
copy-pasteable (e.g., import contractJson from './contract.json' or require it)
and ensure it is passed into postgres<Contract>({ contractJson, url:
process.env['DATABASE_URL']! }) alongside the existing Contract type and
postgres call so db is created correctly.
---
Nitpick comments:
In `@packages/2-sql/4-lanes/sql-builder-new/src/expression.ts`:
- Around line 12-15: The inline type import used in the Expression<T extends
ScopeField> type for buildAst() should be moved to a top-level type import: add
a top-level `import type` from '@prisma-next/sql-relational-core/ast' and
replace the inline `import(...)` reference in the buildAst() signature with that
imported type. Update the Expression declaration (symbols: Expression, buildAst,
ExpressionType) to reference the new top-level imported AST Expression type so
no inline type imports remain in this source file.
In `@packages/2-sql/4-lanes/sql-builder-new/src/runtime/functions.ts`:
- Around line 72-80: numericAgg currently casts expr to ExpressionImpl to read
field.codecId which risks unsafe blind-casting; add a type guard (e.g.,
isExpressionImpl function or an instanceof check) to assert expr is an
ExpressionImpl before accessing .field.codecId and handle the non-ExpressionImpl
case (throw a clear error or derive codecId safely) so numericAgg only reads
codecId when the guard passes; reference the numericAgg function and
ExpressionImpl/type-guard name when making the change.
🪄 Autofix (Beta)
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
Run ID: c3c2e36c-c354-4bef-af06-7e64f181448f
📒 Files selected for processing (44)
.claude/settings.jsonpackages/2-sql/4-lanes/kysely-lane/src/transform/transform-expr.tspackages/2-sql/4-lanes/kysely-lane/src/where-expr.tspackages/2-sql/4-lanes/kysely-lane/test/build-plan.collect-params.test.tspackages/2-sql/4-lanes/relational-core/README.mdpackages/2-sql/4-lanes/relational-core/package.jsonpackages/2-sql/4-lanes/relational-core/src/ast/types.tspackages/2-sql/4-lanes/relational-core/src/exports/query-operations.tspackages/2-sql/4-lanes/relational-core/src/query-lane-context.tspackages/2-sql/4-lanes/relational-core/src/query-operation-registry.tspackages/2-sql/4-lanes/relational-core/test/ast/kind-discriminants.test.tspackages/2-sql/4-lanes/relational-core/test/ast/predicate.test.tspackages/2-sql/4-lanes/relational-core/test/ast/rich-ast.test.tspackages/2-sql/4-lanes/relational-core/test/ast/visitors.test.tspackages/2-sql/4-lanes/relational-core/test/utils.tspackages/2-sql/4-lanes/relational-core/tsdown.config.tspackages/2-sql/4-lanes/sql-builder-new/README.mdpackages/2-sql/4-lanes/sql-builder-new/STATUS.mdpackages/2-sql/4-lanes/sql-builder-new/package.jsonpackages/2-sql/4-lanes/sql-builder-new/src/builders.tspackages/2-sql/4-lanes/sql-builder-new/src/exports/types.tspackages/2-sql/4-lanes/sql-builder-new/src/expression.tspackages/2-sql/4-lanes/sql-builder-new/src/index.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/builder-base.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/create-db.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/expression-impl.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/field-proxy.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/functions.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/index.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/joined-tables-impl.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/mutation-impl.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/param-collector.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/query-impl.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/table-proxy-impl.tspackages/2-sql/4-lanes/sql-builder-new/src/scope.tspackages/2-sql/4-lanes/sql-builder-new/src/types/db.tspackages/2-sql/4-lanes/sql-builder-new/src/types/grouped-query.tspackages/2-sql/4-lanes/sql-builder-new/src/types/joined-tables.tspackages/2-sql/4-lanes/sql-builder-new/src/types/mutation-query.tspackages/2-sql/4-lanes/sql-builder-new/src/types/select-query.tspackages/2-sql/4-lanes/sql-builder-new/src/types/shared.tspackages/2-sql/4-lanes/sql-builder-new/src/types/table-proxy.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/distinct.test.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/execution.test.ts
💤 Files with no reviewable changes (1)
- packages/2-sql/4-lanes/sql-builder-new/src/builders.ts
✅ Files skipped from review due to trivial changes (14)
- .claude/settings.json
- packages/2-sql/4-lanes/relational-core/README.md
- packages/2-sql/4-lanes/relational-core/tsdown.config.ts
- packages/2-sql/4-lanes/relational-core/src/exports/query-operations.ts
- packages/2-sql/4-lanes/relational-core/src/query-lane-context.ts
- packages/2-sql/4-lanes/relational-core/package.json
- packages/2-sql/4-lanes/sql-builder-new/src/runtime/index.ts
- packages/2-sql/4-lanes/sql-builder-new/src/runtime/expression-impl.ts
- packages/2-sql/4-lanes/sql-builder-new/test/integration/execution.test.ts
- packages/2-sql/4-lanes/sql-builder-new/src/types/joined-tables.ts
- packages/2-sql/4-lanes/sql-builder-new/src/runtime/param-collector.ts
- packages/2-sql/4-lanes/kysely-lane/test/build-plan.collect-params.test.ts
- packages/2-sql/4-lanes/sql-builder-new/src/types/mutation-query.ts
- packages/2-sql/4-lanes/sql-builder-new/src/runtime/query-impl.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/2-sql/4-lanes/kysely-lane/src/where-expr.ts
- packages/2-sql/4-lanes/relational-core/test/utils.ts
- packages/2-sql/4-lanes/sql-builder-new/test/integration/distinct.test.ts
- packages/2-sql/4-lanes/kysely-lane/src/transform/transform-expr.ts
- packages/2-sql/4-lanes/relational-core/test/ast/visitors.test.ts
- packages/2-sql/4-lanes/sql-builder-new/src/exports/types.ts
- packages/2-sql/4-lanes/sql-builder-new/src/types/db.ts
- packages/2-sql/4-lanes/relational-core/src/query-operation-registry.ts
- packages/2-sql/4-lanes/relational-core/test/ast/predicate.test.ts
- packages/2-sql/4-lanes/sql-builder-new/src/index.ts
- packages/2-sql/4-lanes/sql-builder-new/STATUS.md
- packages/2-sql/4-lanes/sql-builder-new/src/runtime/mutation-impl.ts
- packages/2-sql/4-lanes/relational-core/src/ast/types.ts
packages/2-sql/4-lanes/sql-builder-new/src/runtime/create-db.ts
Outdated
Show resolved
Hide resolved
packages/2-sql/4-lanes/sql-builder-new/src/runtime/joined-tables-impl.ts
Show resolved
Hide resolved
SevInf
force-pushed
the
worktree/query-builder-impl
branch
3 times, most recently
from
23bfeba to
4b5a6ae
Compare
wmadden
approved these changes
Mar 27, 2026
Contributor
wmadden
left a comment
wmadden
left a comment
There was a problem hiding this comment.
Approving pre-emptively. No blocking issues found. There are non-blocking concerns below — please address each one or note briefly why you won't. No re-review from me required; merge at your discretion once you're satisfied.
@SevInf — this isn't my area of expertise so I'd appreciate it if you also give it a careful self-review with the below in mind. Consider asking another team member with query-builder context to take a look as well.
Code review — PR #257 (TML-2110)
Review scope
- Range:
origin/main...HEADonworktree/query-builder-impl - Tip:
4b5a6ae74 - PR: #257
Summary
The implementation delivers a coherent runtime query builder and unified relational AST with strong test coverage on the happy paths. A follow-up author revision aligns ORM binding and grouped HAVING validation with the merged expression model and simplifies Postgres operation-arg lowering — good incremental hardening.
Remaining cleanup: documentation drift on LIMIT/OFFSET in STATUS vs runtime, README discovery for the new runtime entry factory, and createDb naming (see NB-F12). Capability errors vs structured codes are expected to fold into TML-1911.
Follow-up (not merge-blocking): consolidate dual operation registration — see NB-F11.
What looks solid
- Clear separation: root factory → table proxies →
buildPlan/#buildPlan→runtime.executekeeps concerns narrow. - Immutability pattern:
cloneState, frozen plans/meta, and per-methodParamCollectorcloning match the "fluent immutable builder" expectation. GatedMethod+_gate: Avoids the prior intersection/cast pattern while keeping runtime enforcement.- Postgres lowering: INSERT/UPDATE/DELETE branches integrate cleanly with existing
renderWhere/renderExprmachinery. - Execution context: Aggregating
queryOperationsfrom all stack contributors is straightforward and testable. - Test depth: Integration tests plus AST/unit tests give good evidence for the main SELECT and mutation flows.
Blocking issues
None.
Non-blocking concerns
NB-F02 — STATUS.md overstates LIMIT/OFFSET vs runtime
- Location:
STATUS.md(L30–31) - Issue: Document says "number or expression" while runtime throws for non-number overloads.
- Suggestion: Align STATUS (and README if needed) with
query-impl.ts(L74–86).
NB-F03 — UPDATE/DELETE without WHERE (reviewer correction)
- Resolution: Acceptable for the SQL query builder. Whole-table mutations are an intended capability; the query lints plugin is the supported safeguard. No API change required for merge.
NB-F04 — mergeScopes omits overlapping topLevel keys
- Location:
builder-base.ts(L168–180) - Issue: If both scopes expose the same top-level column name, neither is kept in
topLevel, which can makeorderBy/groupBystring resolution fail mysteriously. - Suggestion: Confirm whether overlapping top-level names are impossible by construction; if not, merge with explicit disambiguation rules or assert/diagnostic when collisions occur.
NB-F05 — Builder throws generic Error in multiple user-facing paths
- Locations:
builder-base.ts(assertCapability, L204–216) — capability failuresquery-impl.ts(firstOrThrow, L140–143) — "Expected at least one row"
- Issue: Plain
Erroris harder to classify at the edge than structured codes. BothassertCapabilityandfirstOrThroware user-facing and would benefit from typed error codes. - Suggestion: Track under repository error-handling taxonomy — TML-1911. Likely already in scope there; no separate parallel design needed in this PR unless TML-1911 defers builder surfaces.
NB-F06 — Duplicated plan-meta construction
- Location:
builder-base.ts(buildPlan, L123–158) vsmutation-impl.ts(buildMutationPlan, L27–65) - Issue: Near-identical logic for param descriptors, codec annotations, and
PlanMeta— drift risk on future meta fields. - Suggestion: Extract a shared helper used by SELECT and DML builders.
NB-F07 — Package README vs runtime factory workflow
- Location:
sql-builder-new/README.md - Issue: Examples center on
postgres().sql.from(tables…); the new runtime bridge uses the root factory increate-db.ts. Readers may not discover the intended entry point. - Suggestion: Add a short "Runtime" section with a minimal example matching integration tests (and rename the factory per NB-F12).
NB-F09 — Proxy get traps silently return undefined for unknown keys (pervasive)
- Locations:
create-db.ts(get trap, L28–36) — unknown table namesfunctions.ts(createFunctions, L173–184) — unknown function namesfunctions.ts(createAggregateFunctions, L194–201) — samefield-proxy.ts(createFieldProxy, L7–21) — unknown field/namespace namesfield-proxy.ts(createNamespaceProxy, L28–36) — unknown column names within a namespace
- Issue: Every runtime Proxy in the builder returns
undefinedfor unrecognized keys. TypeScript catches typos at compile time, but at runtime (e.g. viaas anycasts, dynamic keys, or JS consumers) the error surfaces downstream — "cannot read property 'select' of undefined" (tables), "undefined is not a function" (functions), or a silent wrong result (fields). The consistent pattern makes the builder feel like an "optional" API when all lookups are actually required. - Suggestion: Add a shared "strict proxy" helper that throws a clear error for unknown keys (e.g.
"Unknown table 'xyz'. Available tables: ...") and use it across all proxy sites. Optionally gate behind aprocess.env.NODE_ENV !== 'production'check if performance matters.
NB-F10 — Permissive validateGroupedComparable default (post-rereview)
- Location:
query-plan-aggregate.ts(validateGroupedComparable, L35–48) - Issue: After handling
param-ref,literal, andtuple, thedefaultbranch returnsvaluefor any other expression used as the right-hand side of a grouped HAVING binary compare. Previously, unknown kinds threw. This likely unblocks new leaf kinds (e.g.identifier-ref) but also weakens the "only known comparable shapes" invariant unless the ORM API cannot construct odd RHS trees. - Suggestion: If HAVING comparables must stay a closed set, restore exhaustiveness (explicit cases +
neverdefault) or add tests for each allowed RHS kind; if the set is intentionally open, document that invariant next to the function.
NB-F11 — Parallel queryOperations registry vs existing operation registry (follow-up task, non-blocking)
- Merge gate: No — acceptable to ship PR #257 as-is; track cleanup after merge.
- Linear: Please create a Linear ticket to track this consolidation work and link it from the PR — it is important enough that it must not rely on review comments alone.
- Location:
query-operation-registry.ts,sql-context.ts(createExecutionContext) (queryOperationsalongsideoperationsfrom@prisma-next/operations), extensionqueryOperations()contributors,sql-builder-new/functions.tsconsumers. - Issue: Extension SQL operations are registered twice: the original
OperationRegistry+SqlOperationSignaturepath and a separateQueryOperationDescriptor/createQueryOperationRegistrypath fed with overlapping intent (method, lowering, args). That duplicates data, invites drift (including template placeholder conventions), and doubles pack author burden. - Direction (preferred): Do not keep two parallel interfaces populated with the same operations. If the existing registry and
SqlOperationSignature(or equivalent) do not expose what the fluent query builder needs, extend that model and API — e.g. additional indexes, derived views, or richer fields — so packs register once and the builder reads from the single source of truth (or a projection implemented beside it, not a second hand-maintained descriptor list).
NB-F12 — Root factory should be sql(), not createDb()
- Location:
create-db.ts - Issue:
createDbsounds like it opens a database connection or manages a pool, but it only builds a stateless query-building namespace over the contract's tables. This is misleading to both users (who will look for lifecycle management) and contributors (who may assume driver state on the return value). - Suggestion: Rename to
sql(context)— short, matches the lane name, and parallels the existingorm()/schema()factory conventions in the repo. The file can becomesql.tsor staycreate-db.tswith a deprecation alias if needed for the transition.
Acceptance-criteria traceability
| Acceptance criterion | Primary implementation | Evidence (tests) |
|---|---|---|
| AC1 — SELECT executes on PGlite | create-db.ts, query-impl.ts, builder-base.ts |
test/integration/select.test.ts, execution.test.ts |
| AC2 — Joins, group, order, limit, distinct | joined-tables-impl.ts, query-impl.ts |
join.test.ts, group-by.test.ts, order-by.test.ts, distinct.test.ts, builders.test.ts |
| AC3 — Mutations + returning | mutation-impl.ts, table-proxy-impl.ts |
mutation.test.ts, mutations.test-d.ts |
AC4 — Unified Expression AST |
relational-core/src/ast/types.ts |
predicate.test.ts, rich-ast.test.ts, visitors.test.ts |
| AC5 — Extension functions | query-operation-registry.ts, sql-context.ts, pgvector descriptor |
extension-functions.test.ts |
AC6 — queryOperations on context |
sql-context.ts |
functions.test.ts |
| AC7 — Postgres lowers DML + expressions | adapter.ts |
adapter.test.ts |
Contributor
Author
|
NB-F02 - Addressed by removing expression support from type signature for now and creating a follow up task. NB-F10 - addressed |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
packages/2-sql/4-lanes/sql-builder-new/src/runtime/sql.ts (1)
23-30:⚠️ Potential issue | 🟠 MajorGuard Proxy table lookup with own-property checks and proper key typing.
Line [24] narrows
proptostringand Line [25] uses a truthy lookup oncontext.contract.storage.tables[prop]. This can resolve prototype members as tables and mis-handle non-string Proxy keys.🛡️ Proposed fix
return new Proxy({} as Db<C>, { - get(_target, prop: string) { - const table = context.contract.storage.tables[prop]; - if (table) { - return new TableProxyImpl(prop, table, prop, ctx); - } - return undefined; - }, + get(_target, prop: string | symbol) { + if (typeof prop !== 'string') { + return undefined; + } + if (!Object.hasOwn(context.contract.storage.tables, prop)) { + return undefined; + } + const table = context.contract.storage.tables[prop]!; + return new TableProxyImpl(prop, table, prop, ctx); + }, });#!/bin/bash set -euo pipefail file="packages/2-sql/4-lanes/sql-builder-new/src/runtime/sql.ts" echo "Inspect Proxy get trap around table resolution:" nl -ba "$file" | sed -n '20,40p' echo echo "Verify key handling and own-property checks:" rg -n "get\\(_target, prop|storage\\.tables\\[prop\\]|Object\\.hasOwn" "$file" -n -C2As per coding guidelines,
{packages,examples,test}/**/*.{ts,tsx,js,jsx}must useObject.hasOwn()for own-property checks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/sql-builder-new/src/runtime/sql.ts` around lines 23 - 30, The Proxy get trap should guard against non-string keys and prototype properties: in the get(_target, prop) handler first check typeof prop === "string" and return undefined for other types, then use Object.hasOwn(context.contract.storage.tables, prop) (not a truthy lookup) to verify the table exists before constructing a new TableProxyImpl; update the logic around context.contract.storage.tables[prop] and the return paths to use these checks so prototype keys aren't treated as tables and non-string keys are handled safely.
🧹 Nitpick comments (3)
packages/2-sql/4-lanes/relational-core/src/ast/types.ts (1)
1072-1072: Redundant conditional branch.Both branches of the ternary call
.rewrite(rewriter)onthis.on, making the condition unnecessary.Simplify by removing the conditional
- this.on.kind === 'eq-col-join-on' ? this.on.rewrite(rewriter) : this.on.rewrite(rewriter), + this.on.rewrite(rewriter),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/relational-core/src/ast/types.ts` at line 1072, The ternary expression that checks this.on.kind === 'eq-col-join-on' is redundant because both branches call this.on.rewrite(rewriter); replace the entire ternary with a single call to this.on.rewrite(rewriter) (i.e., remove the conditional and use the direct expression) in the method where this.on is being rewritten (reference the this.on object and the 'eq-col-join-on' kind check in types.ts to locate the code).packages/2-sql/4-lanes/sql-builder-new/src/runtime/mutation-impl.ts (2)
136-140: Consider top-level import for ParamRef type.The inline dynamic import type at line 136 (repeated at line 240) works but could be cleaner as a top-level import for better IDE support and consistency with other imports.
🔧 Suggested change
Add to imports at top of file:
import type { ParamRef } from '@prisma-next/sql-relational-core/ast';Then use directly:
- const paramValues: Record<string, import('@prisma-next/sql-relational-core/ast').ParamRef> = {}; + const paramValues: Record<string, ParamRef> = {};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/sql-builder-new/src/runtime/mutation-impl.ts` around lines 136 - 140, Replace the inline dynamic import type usage for ParamRef with a top-level type import: add "import type { ParamRef } from '@prisma-next/sql-relational-core/ast';" to the file imports, then update the type annotation for paramValues to use ParamRef (e.g., const paramValues: Record<string, ParamRef> = {}) and likewise swap any other inline occurrences (the ones used with paramCollector.add and this.#values / this.#scope.topLevel) to the new ParamRef type so IDEs and tooling can resolve the type properly.
123-131: Avoidas unknown ascasts in production code.The cast at line 130 (
as unknown as InsertQuery<QC, AvailableScope, never>) bypasses type safety. The same pattern appears inUpdateQueryImpl.returning(line 234) andDeleteQueryImpl.returning(line 341).Consider using a type predicate or restructuring the class hierarchy so that the constructor return type is directly assignable without blind casting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/sql-builder-new/src/runtime/mutation-impl.ts` around lines 123 - 131, The code is using unsafe double-casts like "as unknown as InsertQuery<QC, AvailableScope, never>" in InsertQueryImpl, and similar casts in UpdateQueryImpl.returning and DeleteQueryImpl.returning; replace these blind casts by making the implementation generics align with the public interface or by adding a typed factory/constructor overload so the constructed object already satisfies InsertQuery<QC, AvailableScope, never> (or the corresponding Update/Delete return types). Concretely, adjust InsertQueryImpl's class generic parameters or its constructor signature to produce the correct return type instead of casting, or introduce a narrow type-guard/factory function (e.g., createInsertQueryImpl(...): InsertQuery<...>) that constructs the instance and returns the correctly typed interface; apply the same pattern to UpdateQueryImpl.returning and DeleteQueryImpl.returning to remove all "as unknown as" casts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/2-sql/4-lanes/sql-builder-new/src/runtime/functions.ts`:
- Around line 47-54: The comparison helper unconditionally emits a BinaryExpr so
calls like eq(col, null) / ne(col, null) produce "col = NULL" instead of SQL "IS
NULL"/"IS NOT NULL"; update comparison in functions.ts to detect when resolve(b,
pc) (and resolve(a, pc) if operands can be swapped) yields a literal null and if
op is 'eq' emit a NullCheck expression (IS NULL) and if op is 'ne' emit a
NullCheckNot expression (IS NOT NULL) instead of constructing new BinaryExpr(op,
...); make the same change for the similar helper around lines marked 84-85 so
both code paths use null-check lowering rather than BinaryExpr for null
comparisons, referencing BinaryExpr, comparison, resolve and ParamCollector
while preserving boolExpr wrapping for boolean result.
- Around line 72-79: The numericAgg helper currently reuses the input
expression's codecId for all aggregates which is wrong for 'sum' and 'avg';
change numericAgg (and the analogous helper at lines 116-119) to choose the
result codec based on the aggregate function instead of always using (expr as
ExpressionImpl).field.codecId — implement a small mapping (e.g.
aggregateResultCodec: Record<'sum'|'avg'|'min'|'max', string>) and set codecId =
aggregateResultCodec[fn] (keep using expr.field.codecId only for min/max if
appropriate), and update both places so sum/avg have the correct result codec
ids.
- Line 31: The code is embedding Postgres-specific codecIds into the SQL-family
runtime (e.g., BOOL_FIELD and the count() result) which must stay
target-neutral; update BOOL_FIELD (BooleanCodecType) to use a neutral codecId
(e.g., remove the "pg/" prefix and use "bool@1" or another registry-agnostic id)
and change the codecId used by count() (and the related constants around lines
109-114) from "pg/int8@1" to a neutral id like "int8@1" or an equivalent
target-agnostic name, ensuring all places that create
BooleanCodecType/IntegerCodecType in this file (references to BOOL_FIELD and the
count() implementation) use the neutral codec identifiers instead of "pg/..." so
dialect-specific metadata is not baked into the packages/2-sql runtime.
In `@packages/3-extensions/sql-orm-client/src/query-plan-aggregate.ts`:
- Around line 107-109: The not(expr) handler in query-plan-aggregate.ts
currently throws for NOT expressions causing groupBy().having(not(...)) to fail;
update the not(expr) branch to treat NotExpr as valid by recursively validating
its child expression (e.g., call the same validator used for other having nodes
on expr.expr or expr.child) and return/wrap a NotExpr node containing the
validated child instead of throwing; ensure you reference the NotExpr AST shape
used elsewhere so the grouped HAVING code receives a normalized NotExpr rather
than an error.
---
Duplicate comments:
In `@packages/2-sql/4-lanes/sql-builder-new/src/runtime/sql.ts`:
- Around line 23-30: The Proxy get trap should guard against non-string keys and
prototype properties: in the get(_target, prop) handler first check typeof prop
=== "string" and return undefined for other types, then use
Object.hasOwn(context.contract.storage.tables, prop) (not a truthy lookup) to
verify the table exists before constructing a new TableProxyImpl; update the
logic around context.contract.storage.tables[prop] and the return paths to use
these checks so prototype keys aren't treated as tables and non-string keys are
handled safely.
---
Nitpick comments:
In `@packages/2-sql/4-lanes/relational-core/src/ast/types.ts`:
- Line 1072: The ternary expression that checks this.on.kind ===
'eq-col-join-on' is redundant because both branches call
this.on.rewrite(rewriter); replace the entire ternary with a single call to
this.on.rewrite(rewriter) (i.e., remove the conditional and use the direct
expression) in the method where this.on is being rewritten (reference the
this.on object and the 'eq-col-join-on' kind check in types.ts to locate the
code).
In `@packages/2-sql/4-lanes/sql-builder-new/src/runtime/mutation-impl.ts`:
- Around line 136-140: Replace the inline dynamic import type usage for ParamRef
with a top-level type import: add "import type { ParamRef } from
'@prisma-next/sql-relational-core/ast';" to the file imports, then update the
type annotation for paramValues to use ParamRef (e.g., const paramValues:
Record<string, ParamRef> = {}) and likewise swap any other inline occurrences
(the ones used with paramCollector.add and this.#values / this.#scope.topLevel)
to the new ParamRef type so IDEs and tooling can resolve the type properly.
- Around line 123-131: The code is using unsafe double-casts like "as unknown as
InsertQuery<QC, AvailableScope, never>" in InsertQueryImpl, and similar casts in
UpdateQueryImpl.returning and DeleteQueryImpl.returning; replace these blind
casts by making the implementation generics align with the public interface or
by adding a typed factory/constructor overload so the constructed object already
satisfies InsertQuery<QC, AvailableScope, never> (or the corresponding
Update/Delete return types). Concretely, adjust InsertQueryImpl's class generic
parameters or its constructor signature to produce the correct return type
instead of casting, or introduce a narrow type-guard/factory function (e.g.,
createInsertQueryImpl(...): InsertQuery<...>) that constructs the instance and
returns the correctly typed interface; apply the same pattern to
UpdateQueryImpl.returning and DeleteQueryImpl.returning to remove all "as
unknown as" casts.
🪄 Autofix (Beta)
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
Run ID: 1546cfe1-fb9e-4c0c-8f13-be8ee6653731
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (105)
.claude/settings.jsonpackages/2-sql/1-core/operations/README.mdpackages/2-sql/1-core/operations/test/operations-registry.test.tspackages/2-sql/4-lanes/kysely-lane/src/transform/transform-expr.tspackages/2-sql/4-lanes/kysely-lane/src/where-expr.ast.test.tspackages/2-sql/4-lanes/kysely-lane/src/where-expr.tspackages/2-sql/4-lanes/kysely-lane/test/build-plan.collect-params.test.tspackages/2-sql/4-lanes/relational-core/README.mdpackages/2-sql/4-lanes/relational-core/package.jsonpackages/2-sql/4-lanes/relational-core/src/ast/types.tspackages/2-sql/4-lanes/relational-core/src/exports/query-operations.tspackages/2-sql/4-lanes/relational-core/src/query-lane-context.tspackages/2-sql/4-lanes/relational-core/src/query-operation-registry.tspackages/2-sql/4-lanes/relational-core/test/ast/common.test.tspackages/2-sql/4-lanes/relational-core/test/ast/kind-discriminants.test.tspackages/2-sql/4-lanes/relational-core/test/ast/predicate.test.tspackages/2-sql/4-lanes/relational-core/test/ast/rich-ast.test.tspackages/2-sql/4-lanes/relational-core/test/ast/test-helpers.tspackages/2-sql/4-lanes/relational-core/test/ast/visitors.test.tspackages/2-sql/4-lanes/relational-core/test/column-builder-operations.test.tspackages/2-sql/4-lanes/relational-core/test/guards.test.tspackages/2-sql/4-lanes/relational-core/test/operation-capabilities.test.tspackages/2-sql/4-lanes/relational-core/test/operations-registry.test.tspackages/2-sql/4-lanes/relational-core/test/utils.tspackages/2-sql/4-lanes/relational-core/tsdown.config.tspackages/2-sql/4-lanes/sql-builder-new/README.mdpackages/2-sql/4-lanes/sql-builder-new/STATUS.mdpackages/2-sql/4-lanes/sql-builder-new/package.jsonpackages/2-sql/4-lanes/sql-builder-new/src/builders.tspackages/2-sql/4-lanes/sql-builder-new/src/exports/types.tspackages/2-sql/4-lanes/sql-builder-new/src/expression.tspackages/2-sql/4-lanes/sql-builder-new/src/index.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/builder-base.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/expression-impl.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/field-proxy.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/functions.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/index.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/joined-tables-impl.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/mutation-impl.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/param-collector.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/query-impl.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/sql.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/table-proxy-impl.tspackages/2-sql/4-lanes/sql-builder-new/src/scope.tspackages/2-sql/4-lanes/sql-builder-new/src/types/db.tspackages/2-sql/4-lanes/sql-builder-new/src/types/grouped-query.tspackages/2-sql/4-lanes/sql-builder-new/src/types/joined-tables.tspackages/2-sql/4-lanes/sql-builder-new/src/types/mutation-query.tspackages/2-sql/4-lanes/sql-builder-new/src/types/select-query.tspackages/2-sql/4-lanes/sql-builder-new/src/types/shared.tspackages/2-sql/4-lanes/sql-builder-new/src/types/table-proxy.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/distinct.test.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/execution.test.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/extension-functions.test.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/group-by.test.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/join.test.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/mutation.test.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/order-by.test.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/pagination.test.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/select.test.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/setup.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/subquery.test.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/where.test.tspackages/2-sql/4-lanes/sql-builder-new/test/playground/mutations.test-d.tspackages/2-sql/4-lanes/sql-builder-new/test/playground/pagination.test-d.tspackages/2-sql/4-lanes/sql-builder-new/test/runtime/builders.test.tspackages/2-sql/4-lanes/sql-builder-new/test/runtime/expression-impl.test.tspackages/2-sql/4-lanes/sql-builder-new/test/runtime/field-proxy.test.tspackages/2-sql/4-lanes/sql-builder-new/test/runtime/functions.test.tspackages/2-sql/4-lanes/sql-builder-new/test/runtime/param-collector.test.tspackages/2-sql/4-lanes/sql-builder-new/test/runtime/test-helpers.tspackages/2-sql/4-lanes/sql-builder-new/tsdown.config.tspackages/2-sql/4-lanes/sql-lane/src/sql/include-builder.tspackages/2-sql/4-lanes/sql-lane/src/sql/predicate-builder.tspackages/2-sql/4-lanes/sql-lane/test/plan.test.tspackages/2-sql/4-lanes/sql-lane/test/predicate-builder.test.tspackages/2-sql/4-lanes/sql-lane/test/sql-dml-vector-ops.test.tspackages/2-sql/4-lanes/sql-lane/test/utils/guards.test.tspackages/2-sql/5-runtime/src/sql-context.tspackages/3-extensions/pgvector/src/core/descriptor-meta.tspackages/3-extensions/pgvector/src/exports/runtime.tspackages/3-extensions/sql-orm-client/src/collection-internal-types.tspackages/3-extensions/sql-orm-client/src/filters.tspackages/3-extensions/sql-orm-client/src/grouped-collection.tspackages/3-extensions/sql-orm-client/src/model-accessor.tspackages/3-extensions/sql-orm-client/src/mutation-executor.tspackages/3-extensions/sql-orm-client/src/query-plan-aggregate.tspackages/3-extensions/sql-orm-client/src/query-plan-select.tspackages/3-extensions/sql-orm-client/src/types.tspackages/3-extensions/sql-orm-client/src/where-binding.tspackages/3-extensions/sql-orm-client/src/where-interop.tspackages/3-extensions/sql-orm-client/src/where-utils.tspackages/3-extensions/sql-orm-client/test/filters.test.tspackages/3-extensions/sql-orm-client/test/model-accessor.test.tspackages/3-extensions/sql-orm-client/test/mutation-executor.test.tspackages/3-extensions/sql-orm-client/test/query-plan-aggregate.test.tspackages/3-extensions/sql-orm-client/test/query-plan-select.test.tspackages/3-extensions/sql-orm-client/test/rich-filters-and-where.test.tspackages/3-extensions/sql-orm-client/test/where-binding.test.tspackages/3-extensions/sql-orm-client/test/where-interop.select-includes.test.tspackages/3-extensions/sql-orm-client/test/where-interop.test.tspackages/3-targets/6-adapters/postgres/src/core/adapter.tspackages/3-targets/6-adapters/postgres/test/adapter.test.tspackages/3-targets/6-adapters/postgres/test/operation-lowering.test.tspackages/3-targets/6-adapters/postgres/test/rich-adapter.test.ts
💤 Files with no reviewable changes (1)
- packages/2-sql/4-lanes/sql-builder-new/src/builders.ts
✅ Files skipped from review due to trivial changes (34)
- packages/2-sql/1-core/operations/README.md
- packages/2-sql/4-lanes/sql-lane/test/plan.test.ts
- packages/2-sql/4-lanes/sql-lane/src/sql/include-builder.ts
- packages/2-sql/4-lanes/relational-core/test/ast/test-helpers.ts
- packages/2-sql/4-lanes/relational-core/test/guards.test.ts
- .claude/settings.json
- packages/2-sql/4-lanes/relational-core/tsdown.config.ts
- packages/2-sql/4-lanes/relational-core/src/exports/query-operations.ts
- packages/2-sql/4-lanes/relational-core/package.json
- packages/2-sql/4-lanes/sql-builder-new/test/integration/select.test.ts
- packages/2-sql/4-lanes/sql-builder-new/test/runtime/expression-impl.test.ts
- packages/2-sql/4-lanes/kysely-lane/src/where-expr.ast.test.ts
- packages/2-sql/4-lanes/sql-builder-new/src/types/joined-tables.ts
- packages/3-extensions/sql-orm-client/test/query-plan-select.test.ts
- packages/3-targets/6-adapters/postgres/test/rich-adapter.test.ts
- packages/2-sql/4-lanes/sql-builder-new/test/runtime/param-collector.test.ts
- packages/2-sql/4-lanes/relational-core/test/ast/common.test.ts
- packages/2-sql/4-lanes/sql-builder-new/test/integration/pagination.test.ts
- packages/2-sql/4-lanes/relational-core/README.md
- packages/2-sql/4-lanes/sql-builder-new/test/integration/distinct.test.ts
- packages/3-extensions/sql-orm-client/src/where-interop.ts
- packages/2-sql/4-lanes/sql-builder-new/test/integration/subquery.test.ts
- packages/2-sql/4-lanes/sql-builder-new/test/integration/order-by.test.ts
- packages/2-sql/4-lanes/sql-builder-new/test/integration/mutation.test.ts
- packages/2-sql/4-lanes/sql-builder-new/src/types/db.ts
- packages/2-sql/4-lanes/sql-builder-new/test/runtime/field-proxy.test.ts
- packages/2-sql/4-lanes/sql-builder-new/src/runtime/expression-impl.ts
- packages/2-sql/4-lanes/sql-builder-new/test/integration/extension-functions.test.ts
- packages/2-sql/4-lanes/sql-builder-new/README.md
- packages/2-sql/4-lanes/sql-builder-new/src/runtime/param-collector.ts
- packages/2-sql/4-lanes/relational-core/src/query-operation-registry.ts
- packages/3-extensions/sql-orm-client/test/where-interop.select-includes.test.ts
- packages/2-sql/4-lanes/sql-builder-new/src/runtime/joined-tables-impl.ts
- packages/2-sql/4-lanes/sql-builder-new/src/types/mutation-query.ts
🚧 Files skipped from review as they are similar to previous changes (35)
- packages/2-sql/4-lanes/sql-builder-new/tsdown.config.ts
- packages/2-sql/4-lanes/sql-builder-new/STATUS.md
- packages/2-sql/4-lanes/kysely-lane/test/build-plan.collect-params.test.ts
- packages/3-extensions/pgvector/src/exports/runtime.ts
- packages/3-extensions/sql-orm-client/test/mutation-executor.test.ts
- packages/2-sql/4-lanes/sql-builder-new/src/runtime/index.ts
- packages/2-sql/4-lanes/sql-builder-new/test/integration/execution.test.ts
- packages/3-extensions/sql-orm-client/test/where-binding.test.ts
- packages/2-sql/4-lanes/sql-builder-new/test/playground/pagination.test-d.ts
- packages/2-sql/4-lanes/relational-core/test/ast/kind-discriminants.test.ts
- packages/3-targets/6-adapters/postgres/test/adapter.test.ts
- packages/2-sql/4-lanes/sql-builder-new/src/runtime/field-proxy.ts
- packages/3-extensions/sql-orm-client/test/filters.test.ts
- packages/2-sql/4-lanes/sql-builder-new/test/integration/where.test.ts
- packages/3-extensions/sql-orm-client/src/grouped-collection.ts
- packages/2-sql/4-lanes/sql-builder-new/package.json
- packages/2-sql/4-lanes/sql-builder-new/test/runtime/test-helpers.ts
- packages/3-extensions/sql-orm-client/src/model-accessor.ts
- packages/3-extensions/sql-orm-client/src/query-plan-select.ts
- packages/3-extensions/sql-orm-client/test/query-plan-aggregate.test.ts
- packages/3-extensions/pgvector/src/core/descriptor-meta.ts
- packages/2-sql/4-lanes/sql-builder-new/test/integration/group-by.test.ts
- packages/2-sql/4-lanes/sql-builder-new/test/integration/join.test.ts
- packages/3-extensions/sql-orm-client/src/filters.ts
- packages/2-sql/4-lanes/sql-builder-new/src/scope.ts
- packages/2-sql/4-lanes/sql-builder-new/test/runtime/functions.test.ts
- packages/2-sql/4-lanes/sql-lane/src/sql/predicate-builder.ts
- packages/2-sql/4-lanes/sql-builder-new/src/index.ts
- packages/2-sql/4-lanes/kysely-lane/src/where-expr.ts
- packages/2-sql/4-lanes/sql-builder-new/src/expression.ts
- packages/3-extensions/sql-orm-client/src/where-utils.ts
- packages/3-extensions/sql-orm-client/test/where-interop.test.ts
- packages/3-extensions/sql-orm-client/test/model-accessor.test.ts
- packages/3-extensions/sql-orm-client/src/types.ts
- packages/2-sql/4-lanes/sql-builder-new/src/runtime/query-impl.ts
packages/2-sql/4-lanes/sql-builder-new/src/runtime/functions.ts
Outdated
Show resolved
Hide resolved
SevInf
force-pushed
the
worktree/query-builder-impl
branch
2 times, most recently
from
ebbc8f7 to
b102069
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
packages/3-targets/6-adapters/postgres/src/core/adapter.ts (1)
341-349:⚠️ Potential issue | 🔴 CriticalFix
renderListLiteralto handle all expression types, not just literals.The function assumes any non-
param-refvalue is aLiteralExpr, butListExpression.valuesis typed asReadonlyArray<AnyExpression>which includesColumnRef,BinaryExpr,SubqueryExpr, and 12+ other expression types. Whenresolve()returns a complex expression throughbuildAst(), that expression gets passed torenderLiteral(v), which expectsv.valueproperty that doesn't exist on those types, causing a runtime error.🐛 Proposed fix
function renderListLiteral(expr: ListExpression, pim?: ParamIndexMap): string { if (expr.values.length === 0) { return '(NULL)'; } const values = expr.values - .map((v) => (v.kind === 'param-ref' ? renderParamRef(v, pim) : renderLiteral(v))) + .map((v) => { + switch (v.kind) { + case 'param-ref': + return renderParamRef(v, pim); + case 'literal': + return renderLiteral(v); + default: + return renderExpr(v, undefined, pim); + } + }) .join(', '); return `(${values})`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-targets/6-adapters/postgres/src/core/adapter.ts` around lines 341 - 349, renderListLiteral currently treats every non-`param-ref` value as a `LiteralExpr` and calls `renderLiteral`, causing runtime errors when `ListExpression.values` contains other `AnyExpression` kinds returned by `resolve()`/`buildAst()` (e.g., `ColumnRef`, `BinaryExpr`, `SubqueryExpr`). Update `renderListLiteral` so it still returns '(NULL)' for empty lists, but for each value: if `v.kind === 'param-ref'` call `renderParamRef(v, pim)`, otherwise pass `v` to the general expression renderer (the existing dispatcher that handles all `AnyExpression` kinds — or add one if missing, e.g., `renderExpression`/`renderAnyExpression`) instead of `renderLiteral`; this ensures every expression kind is rendered correctly and avoids accessing `v.value` on non-literal nodes.packages/3-extensions/sql-orm-client/src/grouped-collection.ts (2)
153-177:⚠️ Potential issue | 🟠 MajorHandle
eq(null)/neq(null)withNullCheckExpr.
sum()/avg()/min()/max()can benull, but this helper still lowerseq(null)/neq(null)to= NULL/<> NULL, which never match in SQL.💡 Proposed fix
import { AggregateExpr, type AnyExpression, BinaryExpr, type BinaryOp, ColumnRef, LiteralExpr, + NullCheckExpr, } from '@prisma-next/sql-relational-core/ast'; @@ - const buildBinaryExpr = (op: BinaryOp, value: unknown): AnyExpression => - new BinaryExpr(op, metric, LiteralExpr.of(value)); + const buildBinaryExpr = (op: BinaryOp, value: unknown): AnyExpression => { + if (value === null) { + if (op === 'eq') return NullCheckExpr.isNull(metric); + if (op === 'neq') return NullCheckExpr.isNotNull(metric); + } + return new BinaryExpr(op, metric, LiteralExpr.of(value)); + };Based on learnings: When generating SQL for comparison operators in the SQL ORM client, do not emit
col = NULLorcol <> NULL; useNullCheckExpr.isNull(left)/NullCheckExpr.isNotNull(left)instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-extensions/sql-orm-client/src/grouped-collection.ts` around lines 153 - 177, In createHavingComparisonMethods (used to build HavingComparisonMethods for the metric AggregateExpr), handle null specially: when eq(null) is requested return a NullCheckExpr.isNull(metric) and when neq(null) is requested return NullCheckExpr.isNotNull(metric) instead of building a BinaryExpr with LiteralExpr.of(null); preserve the existing BinaryExpr building logic for non-null values so gt/lt/gte/lte and non-null eq/neq continue to use new BinaryExpr(op, metric, LiteralExpr.of(value)).
2-9:⚠️ Potential issue | 🔴 CriticalAdd missing
AnyWhereExprtype import.The import statement omits
AnyWhereExpr, which is required bybaseFilterstype annotations at lines 27 and 46. This causes a TypeScript compilation error.Proposed fix
import { AggregateExpr, type AnyExpression, + type AnyWhereExpr, BinaryExpr, type BinaryOp, ColumnRef, LiteralExpr, } from '@prisma-next/sql-relational-core/ast';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-extensions/sql-orm-client/src/grouped-collection.ts` around lines 2 - 9, The import list from '@prisma-next/sql-relational-core/ast' is missing the AnyWhereExpr type used by the baseFilters annotations; update the import to include AnyWhereExpr so the types referenced in baseFilters (used in the grouped-collection.ts functions/variables named baseFilters) compile correctly, then ensure the baseFilters type annotations continue to reference AnyWhereExpr where currently used.packages/3-extensions/sql-orm-client/src/query-plan-aggregate.ts (1)
2-14:⚠️ Potential issue | 🔴 CriticalAdd missing
AnyWhereExprimport.The import statement omits
AnyWhereExpr, but the type is used in function signatures at lines 123 and 147, causing a TypeScript compilation error.Proposed fix
import { AggregateExpr, AndExpr, type AnyExpression, + type AnyWhereExpr, BinaryExpr, ColumnRef, NotExpr, NullCheckExpr, OrExpr,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/3-extensions/sql-orm-client/src/query-plan-aggregate.ts` around lines 2 - 14, The import list in query-plan-aggregate is missing the AnyWhereExpr type used in function signatures (e.g., the functions referencing AnyWhereExpr at lines ~123 and ~147); update the existing import from '@prisma-next/sql-relational-core/ast' to include AnyWhereExpr alongside AggregateExpr, AndExpr, BinaryExpr, ColumnRef, NotExpr, NullCheckExpr, OrExpr, ProjectionItem, SelectAst, and TableSource so the TypeScript types resolve and the file compiles.
🧹 Nitpick comments (4)
packages/2-sql/4-lanes/sql-builder-new/src/runtime/field-proxy.ts (1)
8-20: Proxy get trapproptype should bestring | symbol.The Proxy
gettrap receivesstring | symbolat runtime, butpropis typed asstring. While the runtime behavior is safe (symbols won't match any string keys and will returnundefined), the type annotation is misleading.🔧 Suggested fix
export function createFieldProxy<S extends Scope>(scope: S): FieldProxy<S> { return new Proxy({} as FieldProxy<S>, { - get(_target, prop: string) { + get(_target, prop: string | symbol) { + if (typeof prop !== 'string') { + return undefined; + } if (Object.hasOwn(scope.topLevel, prop)) {Apply the same change to
createNamespaceProxyat line 29.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/sql-builder-new/src/runtime/field-proxy.ts` around lines 8 - 20, Update the Proxy get trap signatures to accept string | symbol (change prop: string to prop: string | symbol) for both the top-level get in field-proxy.ts and the get inside createNamespaceProxy; inside each trap, narrow to string (e.g., if (typeof prop !== "string") return undefined) before calling Object.hasOwn, scope lookups, or IdentifierRef.of, or explicitly cast when passing to IdentifierRef.of (prop as string) to keep types correct.packages/2-sql/4-lanes/sql-builder-new/test/integration/subquery.test.ts (1)
50-60: Assert the joined payload, not only cardinality.This case is exercising derived-table aliasing and projection, but
rows.length === 4will still pass if the join source or projectedtitleis wrong as long as the row count stays the same. Add a deterministic ordering and assert the expectedname/titlepairs from the fixture.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/sql-builder-new/test/integration/subquery.test.ts` around lines 50 - 60, The test "subquery as join source" currently only asserts rows.length; update it to enforce deterministic ordering and validate the joined payload by adding an ORDER BY (e.g., .orderBy or .order) on known columns (like f.users.name) before .all(), then assert the exact name/title pairs returned by collect() instead of only checking length; locate the test using db(), the derived table created by d.posts.select('user_id','title').as('sub'), and the innerJoin call to ensure you assert the projection from d.users.select('name','title') matches the expected fixture rows.packages/2-sql/4-lanes/sql-builder-new/src/runtime/mutation-impl.ts (1)
76-95: Blind casts inreturning()methods warrant brief justification.Each
returning()implementation ends withas unknown as InsertQuery<...>(similarly for Update/Delete). While the coding guidelines discourage blind casts, these are used to satisfy the complex generic narrowing in the return type whereRowTypetransitions toneverand then re-infers fromnewRowFields. The actual runtime object is correctly constructed.This pattern is acceptable here given the type-level complexity, but consider adding a brief inline comment (e.g.,
// Cast required for generic row-type narrowing) for future maintainers.Also applies to: 178-198, 285-304
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/sql-builder-new/src/runtime/mutation-impl.ts` around lines 76 - 95, The blind double-casts (as unknown as InsertQuery<...>) at the end of the returning() implementation (and the analogous sites around InsertQueryImpl, Update/Delete returning implementations) need a short inline justification comment; add a one-line comment like "// Cast required for generic row-type narrowing" immediately above the cast in returning (and the corresponding cast sites at the Update/Delete returning implementations) so future maintainers understand this is intentional and required by TypeScript's complex generic narrowing rules.packages/2-sql/4-lanes/sql-builder-new/src/runtime/query-impl.ts (1)
178-188:fns as nevercast inwhere()hides potential type mismatch.Line 181 uses
fns as neverwhich completely bypasses type checking. This could mask issues ifcreateFunctions<QC>returns a type incompatible with whatexprexpects.Consider whether the cast can be narrowed. If
ExpressionBuilder<Scope, QueryContext>is the expected widened type, the functions type should also be explicitly widened rather than cast tonever.🔧 Potential refinement
where(expr: ExpressionBuilder<AvailableScope, QC>): SelectQuery<QC, AvailableScope, RowType> { const fieldProxy = createFieldProxy(this.state.scope); const fns = createFunctions<QC>(this.ctx.queryOperationTypes); - const result = (expr as ExpressionBuilder<Scope, QueryContext>)(fieldProxy, fns as never); + const result = (expr as ExpressionBuilder<Scope, QueryContext>)( + fieldProxy, + fns as Functions<QueryContext>, + ); return new SelectQueryImpl(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/2-sql/4-lanes/sql-builder-new/src/runtime/query-impl.ts` around lines 178 - 188, The code is hiding a type mismatch by using "fns as never" in where(); replace the never cast with a proper, widened type so the ExpressionBuilder and createFunctions types align. Specifically, update the where(expr: ExpressionBuilder<AvailableScope, QC>) usage so that the functions value created by createFunctions<QC>(...) is typed to the same functions shape the ExpressionBuilder expects (e.g., introduce or use a shared functions type alias like FunctionsFor<QC> or widen the ExpressionBuilder parameter to accept the createFunctions return type) and then pass fns with that concrete type instead of casting to never; keep the rest of the method (fieldProxy, result.buildAst(), SelectQueryImpl) intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/2-sql/4-lanes/sql-builder-new/src/runtime/query-impl.ts`:
- Around line 236-237: The code casts the `expr` parameter to the broad
`ExprCallback` in the `having()` implementation which hides type mismatches;
remove the `as ExprCallback` cast and instead ensure `having()` accepts the
specific callback signature (fields: FieldProxy<OrderByScope<AvailableScope,
RowType>>, fns: AggregateFunctions<QC>) => Expression<BooleanCodecType> or
introduce a named specific type and use it; update the method signature used
where `expr` is declared so the call to `(expr)(createFieldProxy(combined),
fns)` is correctly typed and then return the `new
GroupedQueryImpl(cloneState(this.state, { having: result.buildAst() }),
this.ctx)` without the cast.
---
Outside diff comments:
In `@packages/3-extensions/sql-orm-client/src/grouped-collection.ts`:
- Around line 153-177: In createHavingComparisonMethods (used to build
HavingComparisonMethods for the metric AggregateExpr), handle null specially:
when eq(null) is requested return a NullCheckExpr.isNull(metric) and when
neq(null) is requested return NullCheckExpr.isNotNull(metric) instead of
building a BinaryExpr with LiteralExpr.of(null); preserve the existing
BinaryExpr building logic for non-null values so gt/lt/gte/lte and non-null
eq/neq continue to use new BinaryExpr(op, metric, LiteralExpr.of(value)).
- Around line 2-9: The import list from '@prisma-next/sql-relational-core/ast'
is missing the AnyWhereExpr type used by the baseFilters annotations; update the
import to include AnyWhereExpr so the types referenced in baseFilters (used in
the grouped-collection.ts functions/variables named baseFilters) compile
correctly, then ensure the baseFilters type annotations continue to reference
AnyWhereExpr where currently used.
In `@packages/3-extensions/sql-orm-client/src/query-plan-aggregate.ts`:
- Around line 2-14: The import list in query-plan-aggregate is missing the
AnyWhereExpr type used in function signatures (e.g., the functions referencing
AnyWhereExpr at lines ~123 and ~147); update the existing import from
'@prisma-next/sql-relational-core/ast' to include AnyWhereExpr alongside
AggregateExpr, AndExpr, BinaryExpr, ColumnRef, NotExpr, NullCheckExpr, OrExpr,
ProjectionItem, SelectAst, and TableSource so the TypeScript types resolve and
the file compiles.
In `@packages/3-targets/6-adapters/postgres/src/core/adapter.ts`:
- Around line 341-349: renderListLiteral currently treats every non-`param-ref`
value as a `LiteralExpr` and calls `renderLiteral`, causing runtime errors when
`ListExpression.values` contains other `AnyExpression` kinds returned by
`resolve()`/`buildAst()` (e.g., `ColumnRef`, `BinaryExpr`, `SubqueryExpr`).
Update `renderListLiteral` so it still returns '(NULL)' for empty lists, but for
each value: if `v.kind === 'param-ref'` call `renderParamRef(v, pim)`, otherwise
pass `v` to the general expression renderer (the existing dispatcher that
handles all `AnyExpression` kinds — or add one if missing, e.g.,
`renderExpression`/`renderAnyExpression`) instead of `renderLiteral`; this
ensures every expression kind is rendered correctly and avoids accessing
`v.value` on non-literal nodes.
---
Nitpick comments:
In `@packages/2-sql/4-lanes/sql-builder-new/src/runtime/field-proxy.ts`:
- Around line 8-20: Update the Proxy get trap signatures to accept string |
symbol (change prop: string to prop: string | symbol) for both the top-level get
in field-proxy.ts and the get inside createNamespaceProxy; inside each trap,
narrow to string (e.g., if (typeof prop !== "string") return undefined) before
calling Object.hasOwn, scope lookups, or IdentifierRef.of, or explicitly cast
when passing to IdentifierRef.of (prop as string) to keep types correct.
In `@packages/2-sql/4-lanes/sql-builder-new/src/runtime/mutation-impl.ts`:
- Around line 76-95: The blind double-casts (as unknown as InsertQuery<...>) at
the end of the returning() implementation (and the analogous sites around
InsertQueryImpl, Update/Delete returning implementations) need a short inline
justification comment; add a one-line comment like "// Cast required for generic
row-type narrowing" immediately above the cast in returning (and the
corresponding cast sites at the Update/Delete returning implementations) so
future maintainers understand this is intentional and required by TypeScript's
complex generic narrowing rules.
In `@packages/2-sql/4-lanes/sql-builder-new/src/runtime/query-impl.ts`:
- Around line 178-188: The code is hiding a type mismatch by using "fns as
never" in where(); replace the never cast with a proper, widened type so the
ExpressionBuilder and createFunctions types align. Specifically, update the
where(expr: ExpressionBuilder<AvailableScope, QC>) usage so that the functions
value created by createFunctions<QC>(...) is typed to the same functions shape
the ExpressionBuilder expects (e.g., introduce or use a shared functions type
alias like FunctionsFor<QC> or widen the ExpressionBuilder parameter to accept
the createFunctions return type) and then pass fns with that concrete type
instead of casting to never; keep the rest of the method (fieldProxy,
result.buildAst(), SelectQueryImpl) intact.
In `@packages/2-sql/4-lanes/sql-builder-new/test/integration/subquery.test.ts`:
- Around line 50-60: The test "subquery as join source" currently only asserts
rows.length; update it to enforce deterministic ordering and validate the joined
payload by adding an ORDER BY (e.g., .orderBy or .order) on known columns (like
f.users.name) before .all(), then assert the exact name/title pairs returned by
collect() instead of only checking length; locate the test using db(), the
derived table created by d.posts.select('user_id','title').as('sub'), and the
innerJoin call to ensure you assert the projection from
d.users.select('name','title') matches the expected fixture rows.
🪄 Autofix (Beta)
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
Run ID: 6148e33e-9072-441f-8401-52e5fe24bbd0
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (101)
.claude/settings.jsonpackages/2-sql/1-core/operations/README.mdpackages/2-sql/1-core/operations/test/operations-registry.test.tspackages/2-sql/4-lanes/kysely-lane/src/transform/transform-expr.tspackages/2-sql/4-lanes/kysely-lane/src/where-expr.ast.test.tspackages/2-sql/4-lanes/kysely-lane/test/build-plan.collect-params.test.tspackages/2-sql/4-lanes/relational-core/README.mdpackages/2-sql/4-lanes/relational-core/package.jsonpackages/2-sql/4-lanes/relational-core/src/ast/types.tspackages/2-sql/4-lanes/relational-core/src/exports/query-operations.tspackages/2-sql/4-lanes/relational-core/src/query-lane-context.tspackages/2-sql/4-lanes/relational-core/src/query-operation-registry.tspackages/2-sql/4-lanes/relational-core/test/ast/common.test.tspackages/2-sql/4-lanes/relational-core/test/ast/kind-discriminants.test.tspackages/2-sql/4-lanes/relational-core/test/ast/predicate.test.tspackages/2-sql/4-lanes/relational-core/test/ast/rich-ast.test.tspackages/2-sql/4-lanes/relational-core/test/ast/test-helpers.tspackages/2-sql/4-lanes/relational-core/test/ast/visitors.test.tspackages/2-sql/4-lanes/relational-core/test/column-builder-operations.test.tspackages/2-sql/4-lanes/relational-core/test/guards.test.tspackages/2-sql/4-lanes/relational-core/test/operation-capabilities.test.tspackages/2-sql/4-lanes/relational-core/test/operations-registry.test.tspackages/2-sql/4-lanes/relational-core/test/utils.tspackages/2-sql/4-lanes/relational-core/tsdown.config.tspackages/2-sql/4-lanes/sql-builder-new/README.mdpackages/2-sql/4-lanes/sql-builder-new/STATUS.mdpackages/2-sql/4-lanes/sql-builder-new/package.jsonpackages/2-sql/4-lanes/sql-builder-new/src/builders.tspackages/2-sql/4-lanes/sql-builder-new/src/exports/types.tspackages/2-sql/4-lanes/sql-builder-new/src/expression.tspackages/2-sql/4-lanes/sql-builder-new/src/index.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/builder-base.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/expression-impl.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/field-proxy.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/functions.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/index.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/joined-tables-impl.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/mutation-impl.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/query-impl.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/sql.tspackages/2-sql/4-lanes/sql-builder-new/src/runtime/table-proxy-impl.tspackages/2-sql/4-lanes/sql-builder-new/src/scope.tspackages/2-sql/4-lanes/sql-builder-new/src/types/db.tspackages/2-sql/4-lanes/sql-builder-new/src/types/grouped-query.tspackages/2-sql/4-lanes/sql-builder-new/src/types/joined-tables.tspackages/2-sql/4-lanes/sql-builder-new/src/types/mutation-query.tspackages/2-sql/4-lanes/sql-builder-new/src/types/select-query.tspackages/2-sql/4-lanes/sql-builder-new/src/types/shared.tspackages/2-sql/4-lanes/sql-builder-new/src/types/table-proxy.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/distinct.test.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/execution.test.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/extension-functions.test.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/group-by.test.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/join.test.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/mutation.test.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/order-by.test.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/pagination.test.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/select.test.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/setup.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/subquery.test.tspackages/2-sql/4-lanes/sql-builder-new/test/integration/where.test.tspackages/2-sql/4-lanes/sql-builder-new/test/playground/mutations.test-d.tspackages/2-sql/4-lanes/sql-builder-new/test/playground/pagination.test-d.tspackages/2-sql/4-lanes/sql-builder-new/test/runtime/builders.test.tspackages/2-sql/4-lanes/sql-builder-new/test/runtime/expression-impl.test.tspackages/2-sql/4-lanes/sql-builder-new/test/runtime/field-proxy.test.tspackages/2-sql/4-lanes/sql-builder-new/test/runtime/functions.test.tspackages/2-sql/4-lanes/sql-builder-new/test/runtime/test-helpers.tspackages/2-sql/4-lanes/sql-builder-new/tsdown.config.tspackages/2-sql/4-lanes/sql-lane/src/sql/include-builder.tspackages/2-sql/4-lanes/sql-lane/src/sql/predicate-builder.tspackages/2-sql/4-lanes/sql-lane/test/plan.test.tspackages/2-sql/4-lanes/sql-lane/test/predicate-builder.test.tspackages/2-sql/4-lanes/sql-lane/test/sql-dml-vector-ops.test.tspackages/2-sql/4-lanes/sql-lane/test/utils/guards.test.tspackages/2-sql/5-runtime/src/sql-context.tspackages/3-extensions/pgvector/src/core/descriptor-meta.tspackages/3-extensions/pgvector/src/exports/runtime.tspackages/3-extensions/sql-orm-client/src/collection-internal-types.tspackages/3-extensions/sql-orm-client/src/filters.tspackages/3-extensions/sql-orm-client/src/grouped-collection.tspackages/3-extensions/sql-orm-client/src/model-accessor.tspackages/3-extensions/sql-orm-client/src/mutation-executor.tspackages/3-extensions/sql-orm-client/src/query-plan-aggregate.tspackages/3-extensions/sql-orm-client/src/query-plan-select.tspackages/3-extensions/sql-orm-client/src/types.tspackages/3-extensions/sql-orm-client/src/where-binding.tspackages/3-extensions/sql-orm-client/src/where-interop.tspackages/3-extensions/sql-orm-client/src/where-utils.tspackages/3-extensions/sql-orm-client/test/filters.test.tspackages/3-extensions/sql-orm-client/test/model-accessor.test.tspackages/3-extensions/sql-orm-client/test/mutation-executor.test.tspackages/3-extensions/sql-orm-client/test/query-plan-aggregate.test.tspackages/3-extensions/sql-orm-client/test/query-plan-select.test.tspackages/3-extensions/sql-orm-client/test/rich-filters-and-where.test.tspackages/3-extensions/sql-orm-client/test/where-binding.test.tspackages/3-extensions/sql-orm-client/test/where-interop.test.tspackages/3-targets/6-adapters/postgres/src/core/adapter.tspackages/3-targets/6-adapters/postgres/test/adapter.test.tspackages/3-targets/6-adapters/postgres/test/operation-lowering.test.tspackages/3-targets/6-adapters/postgres/test/rich-adapter.test.ts
💤 Files with no reviewable changes (1)
- packages/2-sql/4-lanes/sql-builder-new/src/builders.ts
✅ Files skipped from review due to trivial changes (36)
- packages/2-sql/4-lanes/relational-core/README.md
- packages/2-sql/1-core/operations/README.md
- packages/2-sql/4-lanes/kysely-lane/test/build-plan.collect-params.test.ts
- packages/2-sql/4-lanes/relational-core/test/ast/common.test.ts
- packages/2-sql/4-lanes/relational-core/src/exports/query-operations.ts
- packages/2-sql/4-lanes/sql-lane/test/sql-dml-vector-ops.test.ts
- packages/2-sql/4-lanes/sql-builder-new/test/runtime/expression-impl.test.ts
- packages/2-sql/4-lanes/relational-core/package.json
- packages/2-sql/4-lanes/sql-lane/test/plan.test.ts
- packages/2-sql/4-lanes/relational-core/test/guards.test.ts
- packages/2-sql/4-lanes/relational-core/tsdown.config.ts
- packages/2-sql/4-lanes/sql-builder-new/test/integration/distinct.test.ts
- packages/2-sql/4-lanes/sql-lane/src/sql/include-builder.ts
- packages/3-extensions/sql-orm-client/src/where-utils.ts
- .claude/settings.json
- packages/2-sql/4-lanes/sql-builder-new/test/integration/order-by.test.ts
- packages/2-sql/4-lanes/kysely-lane/src/where-expr.ast.test.ts
- packages/2-sql/4-lanes/sql-builder-new/test/integration/pagination.test.ts
- packages/2-sql/4-lanes/sql-builder-new/test/integration/group-by.test.ts
- packages/3-extensions/sql-orm-client/test/where-binding.test.ts
- packages/2-sql/4-lanes/sql-builder-new/src/runtime/index.ts
- packages/2-sql/4-lanes/relational-core/test/ast/test-helpers.ts
- packages/2-sql/4-lanes/sql-builder-new/test/integration/extension-functions.test.ts
- packages/2-sql/4-lanes/relational-core/test/operations-registry.test.ts
- packages/2-sql/4-lanes/sql-builder-new/test/integration/execution.test.ts
- packages/2-sql/4-lanes/sql-builder-new/src/runtime/expression-impl.ts
- packages/2-sql/4-lanes/sql-builder-new/README.md
- packages/2-sql/1-core/operations/test/operations-registry.test.ts
- packages/2-sql/4-lanes/sql-builder-new/test/integration/select.test.ts
- packages/2-sql/4-lanes/sql-builder-new/test/runtime/field-proxy.test.ts
- packages/2-sql/4-lanes/sql-builder-new/test/runtime/test-helpers.ts
- packages/2-sql/4-lanes/sql-builder-new/src/scope.ts
- packages/2-sql/4-lanes/sql-builder-new/src/types/db.ts
- packages/2-sql/4-lanes/sql-builder-new/test/playground/mutations.test-d.ts
- packages/2-sql/4-lanes/sql-builder-new/src/types/mutation-query.ts
- packages/2-sql/4-lanes/relational-core/src/ast/types.ts
🚧 Files skipped from review as they are similar to previous changes (31)
- packages/3-targets/6-adapters/postgres/test/rich-adapter.test.ts
- packages/2-sql/4-lanes/relational-core/test/operation-capabilities.test.ts
- packages/2-sql/4-lanes/sql-builder-new/tsdown.config.ts
- packages/2-sql/4-lanes/relational-core/src/query-lane-context.ts
- packages/3-extensions/sql-orm-client/test/query-plan-aggregate.test.ts
- packages/2-sql/4-lanes/relational-core/test/ast/kind-discriminants.test.ts
- packages/2-sql/4-lanes/sql-builder-new/src/types/joined-tables.ts
- packages/3-extensions/sql-orm-client/test/query-plan-select.test.ts
- packages/2-sql/4-lanes/relational-core/test/utils.ts
- packages/3-extensions/sql-orm-client/test/where-interop.test.ts
- packages/3-extensions/pgvector/src/exports/runtime.ts
- packages/2-sql/4-lanes/relational-core/test/ast/visitors.test.ts
- packages/2-sql/4-lanes/sql-builder-new/package.json
- packages/3-extensions/pgvector/src/core/descriptor-meta.ts
- packages/2-sql/4-lanes/sql-builder-new/src/runtime/sql.ts
- packages/3-extensions/sql-orm-client/src/where-interop.ts
- packages/2-sql/4-lanes/sql-builder-new/STATUS.md
- packages/3-extensions/sql-orm-client/src/model-accessor.ts
- packages/2-sql/4-lanes/relational-core/test/ast/rich-ast.test.ts
- packages/2-sql/4-lanes/sql-builder-new/src/exports/types.ts
- packages/3-extensions/sql-orm-client/test/filters.test.ts
- packages/2-sql/4-lanes/sql-lane/test/utils/guards.test.ts
- packages/2-sql/4-lanes/sql-builder-new/test/integration/join.test.ts
- packages/3-extensions/sql-orm-client/test/mutation-executor.test.ts
- packages/2-sql/4-lanes/sql-builder-new/src/index.ts
- packages/3-extensions/sql-orm-client/src/query-plan-select.ts
- packages/2-sql/4-lanes/relational-core/src/query-operation-registry.ts
- packages/2-sql/4-lanes/sql-builder-new/src/types/table-proxy.ts
- packages/2-sql/4-lanes/sql-builder-new/src/types/grouped-query.ts
- packages/2-sql/4-lanes/sql-builder-new/test/runtime/builders.test.ts
- packages/2-sql/4-lanes/sql-builder-new/src/runtime/joined-tables-impl.ts
packages/2-sql/4-lanes/sql-builder-new/src/runtime/query-impl.ts
Outdated
Show resolved
Hide resolved
SevInf
force-pushed
the
worktree/query-builder-impl
branch
from
a51a04c to
c7c108d
Compare
Unify operation lowering templates to use {{self}}/{{argN}} mustache-style
placeholders instead of ${self}/${argN}, eliminating the need for
biome-ignore noTemplateCurlyInString suppressions across 18 files.
…set, update docs - Rename createDb() → sql(), CreateDbOptions → SqlOptions, create-db.ts → sql.ts - Remove dead expression-based limit/offset branches (numeric only) - Rename package to @prisma-next/sql-builder-new - Rewrite README to match actual library API - Update STATUS.md to reflect current state
Re-enumerate allowed RHS expression kinds (literal, column-ref, identifier-ref, aggregate, operation, list) and throw on unexpected kinds instead of silently passing them through.
…amRef ParamRef is now value-carrying (value + codecId), so ParamCollector is redundant. Params are derived from the AST via collectParamRefs() at plan-build time instead of being tracked separately.
Verifies that parameters work correctly in both the parent query and a subquery used with fns.in().
Prevents prototype properties like toString/constructor from being treated as table names in the Proxy get handler.
Prevents inherited prototype properties like toString/constructor from being treated as schema fields in both top-level and namespace proxies.
Deduplicate plan-meta construction (param descriptors, codec annotations, PlanMeta) between SELECT and DML builders into a single buildQueryPlan function in builder-base.ts.
eq(col, null) now produces NullCheckExpr (IS NULL) instead of BinaryExpr (col = NULL). Same for ne(col, null) → IS NOT NULL. Handles null on either side of the comparison.
Recursively validate the child of NotExpr instead of throwing, enabling groupBy().having(not(...)) patterns. Also remove unused BoundWhereExpr imports in mutation-executor.ts and types.ts.
Replace AnyWhereExpr with AnyExpression throughout the codebase since our branch unified the expression hierarchy. Remove duplicate type definitions and stale BoundWhereExpr interface left over from rebase. Fix typecheck and biome errors across relational-core, kysely-lane, adapter-postgres, and sql-orm-client.
Make orderByScopeOf generic so it returns OrderByScope<S, R> instead of plain Scope, removing the need for ExprCallback cast in having().
… invalid HAVING predicates bindWhereExpr: 6 composite expression handlers (subquery, operation, aggregate, jsonObject, jsonArrayAgg, list) were incorrectly implemented as passthroughs, skipping binding of nested SelectAsts. Now they call bindExpression to recursively bind inner expressions. validateGroupedHavingExpr: 10 passthrough handlers silently allowed expression types that are not valid HAVING predicates (columnRef, identifierRef, subquery, operation, aggregate, jsonObject, jsonArrayAgg, literal, param, list). Now they throw, consistent with the existing exists rejection. ParamRef throws with the specific ParamRef message. Added exhaustive tests for both functions covering all expression types, edge cases, error paths, and nested logical expressions.
SevInf
force-pushed
the
worktree/query-builder-impl
branch
from
32affa3 to
4e37a75
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes TML-2110
Key snippets
Runtime query builder — SELECT
Mutations — INSERT / UPDATE / DELETE (minimal, demo-sufficient)
Mutation API covers just enough to replace
sql-lanein the demo — single-row insert, filtered update/delete, andreturning()for result retrieval.Capability gating (CapabilityGated → GatedMethod)
Intent
Deliver the complete runtime implementation for the
sql-builder-newquery builder: fluent builder classes that constructrelational-coreAST nodes, produceSqlQueryPlanobjects, and execute queries through the standard runtime pipeline. Includes both SELECT queries (joins, aggregation, subqueries, pagination, distinct) and a minimal DML layer (INSERT, UPDATE, DELETE with typedreturning()) — just enough to replacesql-lanein the demo. The type-level DSL (validated via.test-d.tsfiles) already existed; this PR bridges the gap to working runtime code with full integration tests against PGlite.Change map
insert(),update(),delete()entry points)The story
Unified AST expression hierarchy — The previous AST had a split between
WhereExpr(for boolean predicates in WHERE/HAVING) andExpression(for projections/values). This didn't match SQL, where any expression can appear in any position — a comparison can be projected, an aggregate can appear in WHERE via subquery, etc. This PR unifies them:BinaryExpr,AndExpr,OrExpr,ExistsExpr,NullCheckExprnow extendExpressiondirectly.WhereExprandWhereExprVisitorare removed. A singleExprVisitor(13 cases) replaces both.LiteralExpr,ParamRef, and the newTupleExpression(néeListLiteralExpr) are all fullExpressionsubclasses withaccept()/rewrite()/fold(). This also removed theSqlComparableunion type — everything is justExpression.New expression types —
IdentifierReffor unqualified identifiers (top-level scope access renders as"name"without table qualifier, vsColumnRefwhich renders as"table"."name").NotExprwraps any expression withNOT(...), replacing the previous De Morgan transforms.TupleExpression(renamed fromListLiteralExpr) now holdsExpression[]instead of(ParamRef | LiteralExpr)[], enabling richer tuple contents.Expression layer for the builder —
ExpressionImplwraps AST nodes withScopeFieldmetadata.createFieldProxyproducesIdentifierReffor top-level access,ColumnReffor namespaced.createFunctionsbuilds comparison/logical/aggregate operators + extension functions fromQueryOperationRegistry.Builder classes —
BuilderBaseprovidesctxand_gate()for capability gating.QueryBaseholds shared state management (clone pattern, limit/offset/distinct/groupBy/as/execution).SelectQueryImpladds select/where/orderBy.GroupedQueryImpladds having/orderBy(aggregate).TableProxyImpldelegates toJoinedTablesImplfor joins.Minimal mutation builders —
InsertQueryImpl,UpdateQueryImpl,DeleteQueryImplbuild INSERT/UPDATE/DELETE AST nodes. Covers the surface area needed by the demo: single-row insert, filtered update/delete,returning()for result retrieval, and typed values from codec input types. Integration tests verify side-effects via follow-up SELECTs.Capability gating —
GatedMethod<Cap, Req, M>conditional type makes methods invisible when capability is absent._gate()wraps methods with runtime checks. Field initializer pattern works becausesuper(ctx)runs before field init.Execution bridge —
.first(),.firstOrThrow(),.all()buildSqlQueryPlanfromBuilderStateand delegate toRuntime.execute().Extension function support —
QueryOperationRegistryin relational-core holdsQueryOperationEntry(args + returns + lowering). Pgvector extension exportspgvectorQueryOperationswithcosineDistance.createExecutionContextpopulates the registry from extension pack descriptors. Builder'screateFunctionscreates dynamic proxies from the registry entries.Behavior changes & evidence
Adds runtime query execution via fluent builder API:
createDb(contract, { context, runtime })returnsDb<Contract>with table proxies. Chaining.select(),.where(),.join(), etc. builds AST..first(),.all()execute against the runtime.Adds minimal INSERT/UPDATE/DELETE mutations:
db.table.insert(values),db.table.update(values),db.table.delete()return mutation query builders..returning('col1', 'col2')narrows the result type to the selected columns..where()filters UPDATE/DELETE targets. Values are typed against codec input types from the contract. This is the minimum surface needed to replacesql-lanein the demo — not a full DML layer.Unifies WhereExpr into Expression:
BinaryExpr,AndExpr,OrExpr,ExistsExpr,NullCheckExprnow extendExpressiondirectly instead of a separateWhereExprclass. This matches SQL semantics where any expression can appear in any position.WhereExpr,WhereExprVisitor, andSqlComparableunion type are removed. A singleExprVisitor(13 cases) replaces both visitors.LiteralExprandParamRefbecome fullExpressionsubclasses.Adds IdentifierRef AST node: Unqualified identifier expression — renders as
"name"without table qualifier. Top-level field proxy access producesIdentifierRef; namespaced access (f.users.id) still producesColumnRef("users", "id"). This enables ORDER BY / GROUP BY / DISTINCT ON to reference projection aliases.Adds NotExpr: Wraps any expression with
NOT(...). Replaces previous De Morgan transforms andnegateBinaryOp.Expression.not()base method returnsnew NotExpr(this).Renames ListLiteralExpr → TupleExpression: Values type widened from
ReadonlyArray<ParamRef | LiteralExpr>toReadonlyArray<Expression>, enabling richer tuple contents. Visitor method renamedlistLiteral→tuple. Rewrite/fold methods delegate to child expressions instead of type-switching.Adds QueryOperationRegistry for extension functions: Extension packs export
QueryOperationDescriptor[]with args, returns, and lowering template. Registry is populated increateExecutionContextand consumed by the builder's function proxy.Adds GatedMethod pattern for capability-gated methods: Replaces
CapabilityGatedintersection pattern (which poisoned types withRecord<string, never>) with per-methodGatedMethod<Cap, Req, M>conditional. Impl classes canimplementsthe full interface without casts.Adds JoinSource.getJoinOuterScope() and buildAst() methods: Replaces duck-typing (
_fromSource/_scope) with explicit interface methods. EliminatesgetJoinSourceandcreateJoinSourceFromSubqueryhelpers.Preserves literal codecId types in column descriptors: Changes postgres adapter column type exports from
: ColumnTypeDescriptortosatisfies ColumnTypeDescriptor, preserving literal types like'pg/int4@1'soExpressionOrValueaccepts raw values without casts.Compatibility / migration / risk
ListLiteralExpr→TupleExpression,listLiteral→tuplein all visitor interfaces.IdentifierRefadded toExprVisitor(all implementors must handle it). All existing consumers in the repo are updated.satisfiespreserves narrower types than: ColumnTypeDescriptor. This is strictly more precise — existing code that acceptedstringwill now see literal types, which may surface previously-hidden type errors in downstream consumers.queryOperationson ExecutionContext: Added as a required field. Test utils updated. Any code constructingExecutionContextmanually needs to provide it.Follow-ups / open questions
{{self}}vs${self}format inconsistency betweenOperationSignature.loweringandQueryOperationEntry.lowering. The builder uses${self}which the adapter renders correctly, but the two systems should be unified.TypeMapsin the contract builder doesn't includeQueryOperationTypes— extension function types resolve viavalidateContract<Contract>()workaround in integration tests.limit()/offset()overloads throw "not yet supported".Non-goals / intentionally out of scope
sql-lanepackage (eventual goal, deferred to follow-up)Summary by CodeRabbit
New Features
Improvements