feat(sql-builder): add arithmetic operations and type casts#286
Draft
feat(sql-builder): add arithmetic operations and type casts#286
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe changes introduce arithmetic operators (add, sub, mul, div, mod) and type casting functionality across the SQL query builder stack. New AST nodes and operator types are added, exposed through builtin functions in the expression API, implemented in the runtime, and rendered by the Postgres adapter with comprehensive test coverage. Changes
Sequence DiagramsequenceDiagram
participant User as User Code
participant Builder as Expression Builder
participant Runtime as Runtime Functions
participant AST as AST Layer
participant Adapter as Postgres Adapter
participant SQL as SQL Output
rect rgba(100, 150, 200, 0.5)
Note over User,SQL: Arithmetic Operation Flow
User->>Builder: fns.add(expr1, expr2)
Builder->>Runtime: create ExpressionImpl
Runtime->>AST: BinaryExpr.add(left, right)
AST->>Runtime: return BinaryExpr('add', ...)
Runtime->>Builder: wrap in ExpressionImpl
Builder->>User: Expression<T>
User->>Adapter: lower query with expression
Adapter->>Adapter: renderBinary('add', left, right)
Adapter->>SQL: ("col" + 1)
SQL->>User: SQL text
end
rect rgba(200, 150, 100, 0.5)
Note over User,SQL: Cast Operation Flow
User->>Builder: fns.cast(expr, {codecId, nullable})
Builder->>Runtime: create ExpressionImpl
Runtime->>AST: CastExpr.of(buildAst(), codecId)
AST->>Runtime: return CastExpr
Runtime->>Builder: wrap in ExpressionImpl
Builder->>User: Expression<Target>
User->>Adapter: lower query with cast expression
Adapter->>Adapter: renderCast(CastExpr)
Adapter->>Adapter: resolveNativeType(codecId)
Adapter->>SQL: ("col")::text
SQL->>User: SQL text
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
@prisma-next/runtime-executor
@prisma-next/mongo-core
@prisma-next/mongo-orm
@prisma-next/mongo-runtime
@prisma-next/sql-runtime
@prisma-next/extension-paradedb
@prisma-next/extension-pgvector
@prisma-next/postgres
@prisma-next/sql-orm-client
@prisma-next/target-mongo
@prisma-next/adapter-mongo
@prisma-next/driver-mongo
@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/migration-tools
@prisma-next/vite-plugin-contract-emit
@prisma-next/mongo-emitter
@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-lane-query-builder
@prisma-next/sql-relational-core
@prisma-next/sql-builder
@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: |
SevInf
force-pushed
the
sql-builder-arithmetic-cast
branch
from
22f3799 to
48da40c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/src/expression.ts`:
- Around line 132-135: The cast function currently takes a target nullable flag
and uses it for the resulting Expression, allowing callers to incorrectly cast a
nullable source to a non-null type; update the cast signature and implementation
so the resulting Expression preserves the source expression's nullability
instead of trusting target.nullable: keep TargetCodecId generic for codecId but
derive Nullable from the input expr (e.g. read expr.field.codecId.nullable or
the expr type parameter) and construct the returned Expression<{ codecId:
TargetCodecId; nullable: SourceNullable }>, and ensure ExpressionImpl.field is
assigned using that source-nullable value rather than target.nullable.
In `@packages/2-sql/4-lanes/sql-builder/src/runtime/functions.ts`:
- Around line 80-87: The arithmetic function currently copies a single operand's
field, causing incorrect metadata for mixed codecs and nullability; update
arithmetic(a,b,op) (which constructs new ExpressionImpl(new BinaryExpr(...))) to
derive a new field instead of reusing a.field or b.field: compute codecId by
resolving both operand codecs and promoting/merging them (e.g., choose a
compatible numeric supertype or fallback to 'unknown' when incompatible), and
set nullable = (leftField?.nullable || rightField?.nullable); ensure you extract
field metadata from ExpressionImpl instances only (leftField/rightField), handle
missing metadata safely, and use that derived field when creating the new
ExpressionImpl for the BinaryExpr.
In
`@packages/2-sql/4-lanes/sql-builder/test/integration/arithmetic-and-cast.test.ts`:
- Around line 63-70: The test currently uses rows.every(...) which is vacuously
true for an empty result set; update the test in arithmetic-and-cast.test.ts to
assert the query returns results before checking the predicate (e.g., assert
rows.length > 0) or replace the loose predicate with a concrete expected result
comparison; locate the test using
runtime().execute(db().posts.select('id','views').where((f,fns) =>
fns.gt(fns.add(f.views,50),150)).build()) and add a precondition on rows (or an
explicit expectedRows assertion) before calling rows.every(...) to ensure the
filter actually matched 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: 2bd0a21a-1223-4217-8c48-2f25bef9f714
📒 Files selected for processing (8)
packages/2-sql/4-lanes/relational-core/src/ast/types.tspackages/2-sql/4-lanes/relational-core/test/ast/kind-discriminants.test.tspackages/2-sql/4-lanes/sql-builder/src/expression.tspackages/2-sql/4-lanes/sql-builder/src/runtime/functions.tspackages/2-sql/4-lanes/sql-builder/test/integration/arithmetic-and-cast.test.tspackages/2-sql/4-lanes/sql-builder/test/runtime/functions.test.tspackages/3-targets/6-adapters/postgres/src/core/adapter.tspackages/3-targets/6-adapters/postgres/test/adapter.test.ts
Comment on lines
+132
to
+135
| cast: <TargetCodecId extends string, Nullable extends boolean>( | ||
| expr: Expression<ScopeField>, | ||
| target: { codecId: TargetCodecId; nullable: Nullable }, | ||
| ) => Expression<{ codecId: TargetCodecId; nullable: Nullable }>; |
There was a problem hiding this comment.
cast() should preserve source nullability.
This signature lets fns.cast(nullableExpr, { codecId: 'pg/text@1', nullable: false }) type-check as non-null, and the runtime implementation copies that flag straight into ExpressionImpl.field. SQL casts do not turn NULL into non-NULL, so the output nullability should be derived from the source expression instead of caller input.
🐛 Proposed fix
- cast: <TargetCodecId extends string, Nullable extends boolean>(
- expr: Expression<ScopeField>,
- target: { codecId: TargetCodecId; nullable: Nullable },
- ) => Expression<{ codecId: TargetCodecId; nullable: Nullable }>;
+ cast: <Source extends ScopeField, TargetCodecId extends string>(
+ expr: Expression<Source>,
+ target: { codecId: TargetCodecId },
+ ) => Expression<{ codecId: TargetCodecId; nullable: Source['nullable'] }>;🤖 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/src/expression.ts` around lines 132 - 135,
The cast function currently takes a target nullable flag and uses it for the
resulting Expression, allowing callers to incorrectly cast a nullable source to
a non-null type; update the cast signature and implementation so the resulting
Expression preserves the source expression's nullability instead of trusting
target.nullable: keep TargetCodecId generic for codecId but derive Nullable from
the input expr (e.g. read expr.field.codecId.nullable or the expr type
parameter) and construct the returned Expression<{ codecId: TargetCodecId;
nullable: SourceNullable }>, and ensure ExpressionImpl.field is assigned using
that source-nullable value rather than target.nullable.
Comment on lines
+80
to
+87
| function arithmetic(a: ExprOrVal, b: ExprOrVal, op: BinaryOp): ExpressionImpl { | ||
| const field = | ||
| a instanceof ExpressionImpl | ||
| ? a.field | ||
| : b instanceof ExpressionImpl | ||
| ? b.field | ||
| : { codecId: 'unknown', nullable: false }; | ||
| return new ExpressionImpl(new BinaryExpr(op, resolve(a), resolve(b)), field); |
There was a problem hiding this comment.
Derive arithmetic result metadata instead of copying one operand.
Lines 82-86 reuse a single operand field verbatim. That makes mixed-codec arithmetic like int4 + float8 keep whichever codec happens to come first, and nonNull + nullable stay non-nullable even though the SQL result can still be NULL.
🐛 Proposed fix
function arithmetic(a: ExprOrVal, b: ExprOrVal, op: BinaryOp): ExpressionImpl {
- const field =
- a instanceof ExpressionImpl
- ? a.field
- : b instanceof ExpressionImpl
- ? b.field
- : { codecId: 'unknown', nullable: false };
+ const leftField = a instanceof ExpressionImpl ? a.field : undefined;
+ const rightField = b instanceof ExpressionImpl ? b.field : undefined;
+
+ if (leftField && rightField && leftField.codecId !== rightField.codecId) {
+ throw new Error(
+ `Arithmetic between ${leftField.codecId} and ${rightField.codecId} is not typed yet`,
+ );
+ }
+
+ const field = {
+ codecId: leftField?.codecId ?? rightField?.codecId ?? 'unknown',
+ nullable: Boolean(leftField?.nullable || rightField?.nullable),
+ };
+
return new ExpressionImpl(new BinaryExpr(op, resolve(a), resolve(b)), field);
}🤖 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/src/runtime/functions.ts` around lines 80
- 87, The arithmetic function currently copies a single operand's field, causing
incorrect metadata for mixed codecs and nullability; update arithmetic(a,b,op)
(which constructs new ExpressionImpl(new BinaryExpr(...))) to derive a new field
instead of reusing a.field or b.field: compute codecId by resolving both operand
codecs and promoting/merging them (e.g., choose a compatible numeric supertype
or fallback to 'unknown' when incompatible), and set nullable =
(leftField?.nullable || rightField?.nullable); ensure you extract field metadata
from ExpressionImpl instances only (leftField/rightField), handle missing
metadata safely, and use that derived field when creating the new ExpressionImpl
for the BinaryExpr.
Comment on lines
+63
to
+70
| it('arithmetic in WHERE clause filters correctly', async () => { | ||
| const rows = await runtime().execute( | ||
| db() | ||
| .posts.select('id', 'views') | ||
| .where((f, fns) => fns.gt(fns.add(f.views, 50), 150)) | ||
| .build(), | ||
| ); | ||
| expect(rows.every((r) => r.views + 50 > 150)).toBe(true); |
There was a problem hiding this comment.
This WHERE assertion is vacuously true on an empty result set.
rows.every(...) returns true for [], so this test still passes if the predicate is rendered incorrectly and filters everything out. Assert against a concrete expected row set, or at least verify the query returned rows before checking the predicate.
🤖 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/test/integration/arithmetic-and-cast.test.ts`
around lines 63 - 70, The test currently uses rows.every(...) which is vacuously
true for an empty result set; update the test in arithmetic-and-cast.test.ts to
assert the query returns results before checking the predicate (e.g., assert
rows.length > 0) or replace the loose predicate with a concrete expected result
comparison; locate the test using
runtime().execute(db().posts.select('id','views').where((f,fns) =>
fns.gt(fns.add(f.views,50),150)).build()) and add a precondition on rows (or an
explicit expectedRows assertion) before calling rows.every(...) to ensure the
filter actually matched rows.
Comment on lines
+584
to
+588
| function renderInsert(ast: InsertAst, ctx: RenderCtx): string { | ||
| const contract = ctx.contract; | ||
| if (!contract) { | ||
| throw new Error('INSERT requires a contract'); | ||
| } |
There was a problem hiding this comment.
Only require contract when INSERT rendering actually needs schema metadata.
Line 586 now throws for every INSERT, including explicit row inserts and single-row DEFAULT VALUES that can already be rendered without a contract. That is a regression for callers lowering raw AST inserts outside the contract-aware builder path.
🐛 Proposed fix
function renderInsert(ast: InsertAst, ctx: RenderCtx): string {
const contract = ctx.contract;
- if (!contract) {
- throw new Error('INSERT requires a contract');
- }
const table = quoteIdentifier(ast.table.name);
const rows = ast.rows;
@@
if (!hasExplicitValues) {
if (rows.length === 1) {
return `INSERT INTO ${table} DEFAULT VALUES`;
}
+ if (!contract) {
+ throw new Error('INSERT with multiple DEFAULT rows requires a contract');
+ }
const defaultColumns = getInsertColumnOrder(rows, contract, ast.table.name);
@@
- const columnOrder = getInsertColumnOrder(rows, contract, ast.table.name);
+ const columnOrder = [...new Set(rows.flatMap((row) => Object.keys(row)))];
SevInf
force-pushed
the
sql-builder-arithmetic-cast
branch
from
48da40c to
a9e4240
Compare
Add arithmetic operators (add, sub, mul, div, mod) and type cast expressions to the SQL builder DSL, needed for drizzle-benchmark parity. - Extend BinaryOp with arithmetic ops, reusing BinaryExpr - Add CastExpr AST node rendering as Postgres (expr)::type syntax - Resolve cast native types from CodecRegistry via RenderCtx - Refactor adapter render functions to use RenderCtx object
SevInf
force-pushed
the
sql-builder-arithmetic-cast
branch
from
a9e4240 to
ec30b9c
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
1 participant
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.
Summary
Adds missing SQL builder expression features needed for running drizzle benchmarks with the sql-builder DSL.
add,sub,mul,div,mod) by extendingBinaryOpand reusingBinaryExprCastExprAST node for type casts, rendering as Postgres(expr)::typesyntaxRenderCtxin the postgres adapter to threadCodecRegistrythrough render functions, resolving cast target types from the registry instead of global codec definitionsSummary by CodeRabbit
Release Notes
New Features
Tests