Repository navigation
perf(client-engine): split render and data-mapper prerequisites - #29658
tensordreams wants to merge 12 commits into
Conversation
WalkthroughAdds direct SQL result-set mapping into Prisma objects with recursive nested and JSON value handling, cached column mappings, missing-column errors, and validation tests. Refactors query execution to merge chunks, preserve insert IDs, instrument queries, translate errors, and directly map non-raw query data maps. Reworks template SQL rendering and parameter chunking with cached flat templates, explicit placeholders, tuple/list expansion, and validation. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
size-limit report 📦
|
Merging this PR will improve performance by 60.05%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | interpreter: findUnique |
610.9 µs | 381.7 µs | +60.05% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing prisma-client-perf-render-datamapper-prereqs (5696906) with main (f2b3abd)
Footnotes
-
30 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
packages/client-engine-runtime/src/interpreter/query-interpreter.ts (1)
187-212: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUnify
querycase with the new#executeQueryhelper (duplicated execution/merge logic).
#executeQuery(409-436) re-implements the same render → comment → execute → merge-rows logic already inline in thequerycase (187-212), except it also skips comment application when there's no sqlcommenter and skips the tracing wrapper when instrumentation isn't in use. Keeping both means thequerycase never benefits from those optimizations, and the two copies can silently drift.♻️ Proposed refactor
case 'query': { - const queries = renderQuery(node.args, context.scope, context.generators, this.#maxChunkSize()) - - let results: SqlResultSet | undefined - for (const query of queries) { - const commentedQuery = applyComments(query, context.sqlCommenter) - const result = await this.#withQuerySpanAndEvent(commentedQuery, context.queryable, () => - context.queryable - .queryRaw(cloneObject(commentedQuery)) - .catch((err) => - node.args.type === 'rawSql' ? rethrowAsUserFacingRawError(err) : rethrowAsUserFacing(err), - ), - ) - if (results === undefined) { - results = result - } else { - results.rows.push(...result.rows) - results.lastInsertId = result.lastInsertId - } - } + const results = await this.#executeQuery(node.args, context) return { value: node.args.type === 'rawSql' ? this.#rawSerializer(results!) : this.#serializer(results!), lastInsertId: results?.lastInsertId, } }Also applies to: 408-436
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/client-engine-runtime/src/interpreter/query-interpreter.ts` around lines 187 - 212, Replace the inline render, comment, execution, and result-merging logic in the query case with the existing `#executeQuery` helper. Pass the query node arguments, scope, generators, queryable, and sql commenter through that helper, then serialize its merged result and preserve lastInsertId handling for rawSql and regular queries.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/client-engine-runtime/src/interpreter/data-mapper.test.ts`:
- Around line 8-86: Add tests around buildResultSetFieldMappings and
applyDataMapToResultSet covering nested rowObject mappings, valueObject/JSON
column mappings, and the DataMapperError path when a required column is missing.
Assert the recursively mapped output and the expected error, while preserving
the existing flat-field and cache-order coverage.
---
Outside diff comments:
In `@packages/client-engine-runtime/src/interpreter/query-interpreter.ts`:
- Around line 187-212: Replace the inline render, comment, execution, and
result-merging logic in the query case with the existing `#executeQuery` helper.
Pass the query node arguments, scope, generators, queryable, and sql commenter
through that helper, then serialize its merged result and preserve lastInsertId
handling for rawSql and regular queries.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 02f37528-fab4-463b-84a1-318c38474adb
📒 Files selected for processing (5)
AGENTS.mdpackages/client-engine-runtime/src/interpreter/data-mapper.test.tspackages/client-engine-runtime/src/interpreter/data-mapper.tspackages/client-engine-runtime/src/interpreter/query-interpreter.tspackages/client-engine-runtime/src/interpreter/render-query.ts
33c51d6 to
5696906
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/client-engine-runtime/src/interpreter/render-query.ts (1)
438-506: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle tied oversized fragments together when chunking.
The current tie-break only splits the first
maxParamsPerFragment; a second fragment with the same parameter count is appended unsplit to every chunk. Add a regression for two same-sizeparameterTuplefragments that together exceedmaxChunkSize, or split/group both oversized fragments so no final chunk exceeds the limit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/client-engine-runtime/src/interpreter/render-query.ts` around lines 438 - 506, Update the chunking logic in the parameterTuple and parameterTupleList branches so all fragments tied at maxParamsPerFragment are split or grouped consistently, rather than only the first tied fragment. Ensure every generated chunk remains at or below maxChunkSize when multiple same-sized oversized fragments together exceed the limit, and add a regression covering two equal-size parameterTuple fragments.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/client-engine-runtime/src/interpreter/render-query.ts`:
- Around line 508-534: Extract the shared parameter bounds and tuple-shape
validation from renderTemplateSql and the accessors getParam, getTupleParam, and
getTupleListParam into common helpers. Update both call sites to reuse those
helpers while preserving the existing error messages and validation behavior,
including empty tuple-list and non-tuple element checks.
- Around line 102-163: Add a bounds validation in getFlatTemplateSqlRendering
after counting parameter fragments and before constructing the rendering: reject
any case where paramCount exceeds dbQuery.argTypes.length with the file’s
established malformed-query error behavior, rather than calling copyArgTypes
with an oversized count. Preserve the existing exact-length and shorter-count
handling for valid plans.
---
Outside diff comments:
In `@packages/client-engine-runtime/src/interpreter/render-query.ts`:
- Around line 438-506: Update the chunking logic in the parameterTuple and
parameterTupleList branches so all fragments tied at maxParamsPerFragment are
split or grouped consistently, rather than only the first tied fragment. Ensure
every generated chunk remains at or below maxChunkSize when multiple same-sized
oversized fragments together exceed the limit, and add a regression covering two
equal-size parameterTuple fragments.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 765b9150-f698-4f84-9ab5-3f8fdf79e0ed
📒 Files selected for processing (5)
AGENTS.mdpackages/client-engine-runtime/src/interpreter/data-mapper.test.tspackages/client-engine-runtime/src/interpreter/data-mapper.tspackages/client-engine-runtime/src/interpreter/query-interpreter.tspackages/client-engine-runtime/src/interpreter/render-query.ts
| function getFlatTemplateSqlRendering(dbQuery: DeepReadonly<QueryPlanDbQuery>): FlatTemplateSqlRendering | undefined { | ||
| const cached = flatTemplateSqlCache.get(dbQuery) | ||
| if (cached !== undefined) { | ||
| return cached ?? undefined | ||
| } | ||
|
|
||
| if (dbQuery.type !== 'templateSql') { | ||
| flatTemplateSqlCache.set(dbQuery, null) | ||
| return undefined | ||
| } | ||
|
|
||
| let sql = '' | ||
| let placeholderNumber = 1 | ||
| let paramCount = 0 | ||
|
|
||
| for (const fragment of dbQuery.fragments) { | ||
| switch (fragment.type) { | ||
| case 'stringChunk': | ||
| sql += fragment.chunk | ||
| break | ||
|
|
||
| case 'parameter': | ||
| sql += formatPlaceholder(dbQuery.placeholderFormat, placeholderNumber++) | ||
| paramCount++ | ||
| break | ||
|
|
||
| case 'parameterTuple': | ||
| case 'parameterTupleList': | ||
| flatTemplateSqlCache.set(dbQuery, null) | ||
| return undefined | ||
|
|
||
| default: | ||
| assertNever(fragment, 'Invalid fragment type') | ||
| } | ||
| } | ||
|
|
||
| const rendering: FlatTemplateSqlRendering = { | ||
| sql, | ||
| paramCount, | ||
| argTypes: | ||
| paramCount === dbQuery.argTypes.length | ||
| ? (dbQuery.argTypes as ArgType[]) | ||
| : copyArgTypes(dbQuery.argTypes, paramCount), | ||
| } | ||
| flatTemplateSqlCache.set(dbQuery, rendering) | ||
| return rendering | ||
| } | ||
|
|
||
| function evaluateArgs( | ||
| args: readonly unknown[], | ||
| scope: ScopeBindings, | ||
| generators: GeneratorRegistrySnapshot, | ||
| ): unknown[] { | ||
| const result = new Array<unknown>(args.length) | ||
|
|
||
| for (let i = 0; i < args.length; i++) { | ||
| result[i] = evaluateArg(args[i], scope, generators) | ||
| } | ||
|
|
||
| return result | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add a bounds guard for paramCount vs argTypes.length.
In getFlatTemplateSqlRendering, argTypes: paramCount === dbQuery.argTypes.length ? (dbQuery.argTypes as ArgType[]) : copyArgTypes(dbQuery.argTypes, paramCount) assumes paramCount <= dbQuery.argTypes.length. If a malformed query plan ever produces more parameter fragments than argTypes entries, copyArgTypes (Line 201-207) will silently read past the array bounds and fill the result with undefined, instead of raising a clear "malformed query template" error like the sibling checks elsewhere in this file (e.g. Line 72-74, Line 229-231).
🛡️ Proposed fix
function copyArgTypes(argTypes: DeepReadonly<DynamicArgType[]>, length: number): ArgType[] {
+ if (length > argTypes.length) {
+ throw new Error(`Malformed query template. Fragments attempt to read over ${argTypes.length} parameter types.`)
+ }
const result = new Array<ArgType>(length)
for (let i = 0; i < length; i++) {
result[i] = argTypes[i] as ArgType
}
return result
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function getFlatTemplateSqlRendering(dbQuery: DeepReadonly<QueryPlanDbQuery>): FlatTemplateSqlRendering | undefined { | |
| const cached = flatTemplateSqlCache.get(dbQuery) | |
| if (cached !== undefined) { | |
| return cached ?? undefined | |
| } | |
| if (dbQuery.type !== 'templateSql') { | |
| flatTemplateSqlCache.set(dbQuery, null) | |
| return undefined | |
| } | |
| let sql = '' | |
| let placeholderNumber = 1 | |
| let paramCount = 0 | |
| for (const fragment of dbQuery.fragments) { | |
| switch (fragment.type) { | |
| case 'stringChunk': | |
| sql += fragment.chunk | |
| break | |
| case 'parameter': | |
| sql += formatPlaceholder(dbQuery.placeholderFormat, placeholderNumber++) | |
| paramCount++ | |
| break | |
| case 'parameterTuple': | |
| case 'parameterTupleList': | |
| flatTemplateSqlCache.set(dbQuery, null) | |
| return undefined | |
| default: | |
| assertNever(fragment, 'Invalid fragment type') | |
| } | |
| } | |
| const rendering: FlatTemplateSqlRendering = { | |
| sql, | |
| paramCount, | |
| argTypes: | |
| paramCount === dbQuery.argTypes.length | |
| ? (dbQuery.argTypes as ArgType[]) | |
| : copyArgTypes(dbQuery.argTypes, paramCount), | |
| } | |
| flatTemplateSqlCache.set(dbQuery, rendering) | |
| return rendering | |
| } | |
| function evaluateArgs( | |
| args: readonly unknown[], | |
| scope: ScopeBindings, | |
| generators: GeneratorRegistrySnapshot, | |
| ): unknown[] { | |
| const result = new Array<unknown>(args.length) | |
| for (let i = 0; i < args.length; i++) { | |
| result[i] = evaluateArg(args[i], scope, generators) | |
| } | |
| return result | |
| } | |
| function copyArgTypes(argTypes: DeepReadonly<DynamicArgType[]>, length: number): ArgType[] { | |
| if (length > argTypes.length) { | |
| throw new Error(`Malformed query template. Fragments attempt to read over ${argTypes.length} parameter types.`) | |
| } | |
| const result = new Array<ArgType>(length) | |
| for (let i = 0; i < length; i++) { | |
| result[i] = argTypes[i] as ArgType | |
| } | |
| return result | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/client-engine-runtime/src/interpreter/render-query.ts` around lines
102 - 163, Add a bounds validation in getFlatTemplateSqlRendering after counting
parameter fragments and before constructing the rendering: reject any case where
paramCount exceeds dbQuery.argTypes.length with the file’s established
malformed-query error behavior, rather than calling copyArgTypes with an
oversized count. Preserve the existing exact-length and shorter-count handling
for valid plans.
| function getParam(params: unknown[], index: number): unknown { | ||
| if (index >= params.length) { | ||
| throw new Error(`Malformed query template. Fragments attempt to read over ${params.length} parameters.`) | ||
| } | ||
| return params[index] | ||
| } | ||
|
|
||
| function getTupleParam(params: unknown[], index: number): unknown[] { | ||
| const value = getParam(params, index) | ||
| return Array.isArray(value) ? value : [value] | ||
| } | ||
|
|
||
| function getTupleListParam(params: unknown[], index: number): unknown[][] { | ||
| const value = getParam(params, index) | ||
| if (!Array.isArray(value)) { | ||
| throw new Error(`Malformed query template. Tuple list expected.`) | ||
| } | ||
| if (value.length === 0) { | ||
| throw new Error(`Malformed query template. Tuple list cannot be empty.`) | ||
| } | ||
| for (let i = 0; i < value.length; i++) { | ||
| if (!Array.isArray(value[i])) { | ||
| throw new Error(`Malformed query template. Tuple expected.`) | ||
| } | ||
| } | ||
| return value | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Duplicate validation logic between chunk accessors and renderTemplateSql.
getParam/getTupleParam/getTupleListParam re-implement the exact same bounds/shape checks ("Fragments attempt to read over N parameters", "Tuple list expected", "Tuple list cannot be empty", "Tuple expected") already inlined in renderTemplateSql (Line 229-231, Line 241-243, Line 256-266). Both blocks were introduced in this same refactor, so extracting a shared validation helper now avoids the two copies silently diverging later (e.g. if an error message changes in one place but not the other).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/client-engine-runtime/src/interpreter/render-query.ts` around lines
508 - 534, Extract the shared parameter bounds and tuple-shape validation from
renderTemplateSql and the accessors getParam, getTupleParam, and
getTupleListParam into common helpers. Update both call sites to reuse those
helpers while preserving the existing error messages and validation behavior,
including empty tuple-list and non-tuple element checks.
This branch has not been deployed
1 participant
Optimizes Prisma Client’s query-plan hot path ahead of the compact query-plan consumer by reducing allocations in SQL rendering and mapping database rows directly into result structures.
Changes
prisma-client-perf-compact-plan-format; the matching engines producer isprisma-client-perf-compact-plan-format-engines.Why
The compact query-plan work needs the runtime hot path to avoid avoidable iterator, array, object, and serialization overhead independently of the internal plan-format migration. Keeping these prerequisites separate makes the performance behavior reviewable without requiring a custom engines branch.
Validation
pnpm --filter @prisma/client-engine-runtime exec vitest run src/interpreter/data-mapper.test.ts src/interpreter/query-interpreter.test.ts src/interpreter/render-query.test.tspnpm --filter @prisma/client-engine-runtime test— 222 tests passedpnpm --filter @prisma/client-engine-runtime buildpnpm lint— 0 errors (repository warnings unchanged)