perf(orm): reuse QueryNameMapper across derived executors and clients - #2777
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe ORM detects mapped schema names with memoized metadata, reuses ChangesQuery name mapper reuse
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/orm/src/client/client-impl.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. packages/orm/src/client/executor/zenstack-query-executor.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. packages/orm/src/client/query-utils.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. 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 |
![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
🤖 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/orm/src/client/query-utils.ts`:
- Around line 83-84: The cached mapped-name predicate in schemaHasMappedNames
must include schema.enums alongside models and typeDefs, matching
QueryNameMapper’s enum rewrite inputs. Update
packages/orm/src/client/query-utils.ts lines 83-84 accordingly; add a
mapped-enum-only schema using a non-PostgreSQL provider and assertions that
mapped queries succeed in tests/regression/test/issue-2773.test.ts lines 66-89.
🪄 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.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8f8e0a92-08b2-40bb-a1bd-f8d635d0f360
📒 Files selected for processing (4)
packages/orm/src/client/client-impl.tspackages/orm/src/client/executor/zenstack-query-executor.tspackages/orm/src/client/query-utils.tstests/regression/test/issue-2773.test.ts
ymc9
left a comment
ymc9
left a comment
There was a problem hiding this comment.
Hi @matijaboban, many thanks for making this PR and providing an updated analysis. Although the win is not as much as we initially expected, still a good improvement. Overall looking great to me. I don't see any risk sharing the instance. The mapper implementation is fully sync and should stay so.
One small fact is today the mapper's behavior is not actually affected by $options, but I think it's good to guard it so it's more future proof.
I left a comment about enums there.
| return Object.values(decl.fields).some((field) => field.attributes?.some((attr) => attr.name === '@map')); | ||
| }; | ||
| cache.hasMappedNames = | ||
| Object.values(schema.models).some(hasMapAttr) || Object.values(schema.typeDefs ?? []).some(hasMapAttr); |
There was a problem hiding this comment.
I believe we need to check enum (and its fields) too as they can also carry name mapping.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant source files before reading focused ranges.
fd -t f 'query-name|name-mapper|schema.*def|schema-def' packages | sort
ast-grep outline packages/orm/src/client --items all --match 'QueryNameMapper|schemaHasMappedNames' || true
# Find enum mapping checks and QueryNameMapper enum handling with surrounding context.
rg -n -i -C 4 'enum|@@map|`@map`|QueryNameMapper' \
packages/orm/src \
packages/schema \
-g '*.ts' \
-g '!**/dist/**'Length of output: 107756
@ymc9, confirmed. This is a valid issue.
EnumDef supports @@map, and enum fields support @map. QueryNameMapper maps both enum type names and enum values. However, schemaHasMappedNames does not inspect schema.enums.
For a non-PostgreSQL schema that has only enum mappings, schemaHasMappedNames returns false. The executor then skips QueryNameMapper, so enum mappings do not apply.
Update the predicate to also inspect schema.enums, including enum declaration attributes and enum field attributes. Add a regression test for a schema with an enum-field @map and no model or type-definition mapping.
🧬 Code Graph Analysis Results
packages/orm/src/client/crud/operations/base.ts:228-242
Provides CRUD operation wrappers around query-utils.ts schema helpers:
requireModel(model)→requireModel(this.schema, model)getModel(model)→getModel(this.schema, model)requireField(model, field)→requireField(this.schema, model, field)getField(model, field)→getField(this.schema, model, field)
packages/orm/src/client/errors.ts:147-151
createInternalError(message, model?, options?) constructs an ORMError with reason INTERNAL_ERROR, assigns the optional model name, and returns it.
packages/common-helpers/src/tiny-invariant.ts:4-14
invariant(condition, message?) returns when the condition is truthy; otherwise throws an Error, using a generic production message or the supplied message in non-production mode.
packages/orm/src/client/constants.ts:29-30
Defines supported aggregate operators:
'_count', '_sum', '_avg', '_min', and '_max', plus the corresponding AggregateOperators union type.
packages/orm/src/utils/object-utils.ts:6-8
extractFields(obj, fields) returns an object containing only entries whose keys are included in the supplied field-name array.
✏️ Learnings added
Learnt from: ymc9
Repo: zenstackhq/zenstack PR: 2777
File: packages/orm/src/client/query-utils.ts:84-84
Timestamp: 2026-08-01T07:59:49.026Z
Learning: In `packages/orm/src/client/executor/name-mapper.ts`, `QueryNameMapper` applies `@map` attributes on `EnumDef` and `map` attributes on `EnumDef.fields`. Any schema-level predicate that determines whether to instantiate `QueryNameMapper`, such as `schemaHasMappedNames` in `packages/orm/src/client/query-utils.ts`, must inspect enum declarations and enum fields.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
There was a problem hiding this comment.
@ymc9 Done in 66ede0c — schemaHasMappedNames now also walks schema.enums, covering both @@map on the enum and @map on its members.
You were right and I was wrong to push back on this earlier. CodeRabbit raised the same point and I declined it, on the grounds that enumTypeMap is write-only today — two references in the whole monorepo, the declaration and the write, no reads. That fact was correct but it was the wrong basis for the decision: it describes where the code is now, not where you intend it to go, and the guard costs almost nothing.
Two things I found implementing it:
EnumDef.fieldsis optional, whereModelDef.fieldsandTypeDefDef.fieldsare required, so the sharedhasMapAttrneeded a?? {}or it would throw onObject.values(undefined).- The adjacent
schema.typeDefs ?? []used an array fallback for aRecord. It worked only becauseObject.values([])is[]. Changed to?? {}while making the three branches symmetric — say the word if you would rather I left it.
Two regression tests added, both on sqlite deliberately: postgres builds the mapper unconditionally and would mask the bug. One covers @@map on the enum, one @map on a member. I verified they actually fail without the change — reverting the predicate gives 2 failed / 5 passed, restoring it gives 7 passed.
Also rebased onto current dev.
On your other two points: agreed on $options, and thanks for confirming the mapper is intended to stay fully sync — that was the assumption the whole reuse argument rests on, so it is good to have it from you rather than inferred.
QueryNameMapper is derived purely from the client's $schema and $options, but it was
rebuilt for every derived executor and every derived client. Building it is
O(models x fields), plus an O(models x relations) pass on postgres.
A $transaction paid that twice per scope, via two independent paths:
1. ZenStackQueryExecutor.withConnectionProvider
2. new ClientImpl(...) with a baseClient but no executor, which interactiveTransaction
and sequentialTransaction both take
The executor now accepts an optional mapper and threads its own through all five derive
sites. ClientImpl passes the base client's mapper when the derived client has the same
schema and options by identity - so $use / $unuse / $setOptions, which derive with a new
options object and therefore a different dialect, still build their own.
Sharing one instance is safe because the transformer holds no cross-transform state:
its only mutable field, `scopes`, is pushed and popped in `finally` within a single
synchronous traversal, and the class contains no async, await or Promise, so two
transforms cannot interleave on a single-threaded runtime.
Measured on a 1067-model / 11938-field postgres schema, against dev:
paginated read + follow-up, no transaction 2.61 ms -> 2.18 ms
same pair inside $transaction 12.06 ms -> 3.86 ms
one connection scope in isolation 3.16 ms -> 0.07 ms
Refs zenstackhq#2773
Deciding whether a schema needs a QueryNameMapper walks every model, type def and field. It is asked on every query-executor construction, and it is not short-circuited on non-postgres providers - so a sqlite or mysql schema with no @@map/@Map anywhere paid the full walk for each derived executor, including twice per transaction scope. The answer is immutable for a schema, so it moves next to the other structural lookups in query-utils and is memoized through the existing per-schema WeakMap cache. On a synthetic 1067-model / 11737-field schema with no mapped names, the walk costs 812 us; after the first call it is 0.02 us. Refs zenstackhq#2773
schemaHasMappedNames checked models and type defs only. Enums carry name mapping too - @@Map on the enum and @Map on its members - so a schema whose only mapped name is on an enum got no QueryNameMapper on any provider other than postgres, where the mapper is built unconditionally and masked it. EnumDef.fields is optional where ModelDef.fields and TypeDefDef.fields are required, hence the ?? {} guard. Per review feedback from @ymc9 on zenstackhq#2777.
d366a5d to
66ede0c
Compare
|
@ymc9 Ready for another look — enum check added in 66ede0c, rebased onto current On verification, so you know what stands behind it rather than just "tests pass":
Two things I could not verify locally, in the interest of not overstating it:
|
2 participants
Addresses #2773. Opening this per @ymc9's invitation there ("PR is also welcome if you have time"), so the CONTRIBUTING "discuss first" step is already covered by that issue.
First, a correction to my own issue: the ~1s figure in #2773 does not apply to
dev. That changes what this PR is worth, so it belongs up front. Details under Benchmarks.The defect
QueryNameMapperis derived purely from the client's$schemaand$options, but it is rebuilt for every derived executor and every derived client. Building it walks every model and field, plus anO(models x relations)pass on postgres.A
$transactionpays that twice per scope, through two independent paths. Counted by instrumenting the constructor ondev, not inferred from timing:$transaction, 1 query$transaction, 3 queriesPer scope, not per query. Stack capture identifies the two paths:
Worth flagging because it caught me out: fixing only the executor derive sites addresses path 1 and leaves path 2, which halves the cost rather than removing it.
The change
1. Reuse the mapper across derived executors and clients.
ZenStackQueryExecutortakes an optional mapper and threads its own through all five derive sites (withPlugin,withPlugins,withPluginAtFront,withoutPlugins,withConnectionProvider). All five passthis.client, so the schema is identical by construction.ClientImplpasses the base client's mapper only when the derived client has the same$schemaand the same$optionsby identity. That is deliberate:$use,$unuse,$unuseAlland$setOptionsderive with a new options object, and the mapper's dialect comes fromgetCrudDialect(schema, options), so those still build their own. Verified by counting constructions:$transactionand$setAuthbuild 0,$usebuilds 1.2. Memoize the "does this schema need a mapper" check.
Deciding it walks every model and field, and it is asked on every executor construction. It is not short-circuited on non-postgres providers, so a SQLite or MySQL schema with no
@@map/@mappaid the full walk per derived executor. The answer is immutable per schema, so it moves next to the other structural lookups inquery-utils.tsand reuses the per-schemaWeakMapcache added in 805a6b8 (#2715). That also removes the duplicatedhasMapAttrhelper from the executor.Why sharing one instance is safe
This is the part worth your scrutiny, since it is the obvious objection to reusing rather than rebuilding.
QueryNameMapperextendsOperationNodeTransformerand holds one piece of mutable state:scopes. If two transforms could interleave, sharing an instance across executors would corrupt it. They cannot:scopesis pushed and popped infinallyinsidewithScope/withScopes, so it is empty between transforms.async,awaitorPromise:rg -n 'async|await|Promise' packages/orm/src/client/executor/name-mapper.tsreturns nothing ondev.this.nameMapper?.transformNode(query)(zenstack-query-executor.ts:647).A synchronous traversal with no yield point cannot interleave with another on a single-threaded runtime.
The mapper also reads
clientfor exactly two things:$schemaand$options. Neither is connection-scoped, so a shared instance holding a reference to the parent client retains nothing request-specific.The caveat, stated plainly: this holds for the code as it stands. If a plugin or subclass could introduce an
awaitinside the traversal, the safe shape would be hoisting the four maps out of the transformer rather than sharing it. I cannot see the plugin contract well enough to rule that out, so I have left it to your judgement rather than assuming.Benchmarks
Two builds of the same commit (
dev@ 19f0820), packed withpnpm packand installed viafile:overrides, so the delta is this change and nothing else:dev, unmodifieddev+ this PRPostgres, 1067 models / 11938 fields, a paginated read plus a follow-up query, medians:
$transaction(repeatable read)$qb.connection().execute(noop)— one scopeThe "no transaction" row is the honest frame: outside a transaction nothing changes, because the mapper was already built once per client. The whole gain is that derived clients and executors stop rebuilding whole-schema state.
Which released version carries the earlier fix, for anyone arriving from #2773. The correction at the top says the ~1s figure does not apply to
dev; it is worth pinning where that landed for users on a published version. Measured downstream on the 1067-model schema, upgrading a real monorepo's catalog rather than a packed build:$transaction { one query }$transaction { empty }So
805a6b81(#2715) already removed the bulk in a published release, not only ondev— 3.9.0 is enough to escape the ~1s floor, and this PR is a further ~3x on top of that rather than the thing that makes transactions usable. The empty-transaction row is the diagnostic: on 3.7.2 it cost the same as one doing real work, so the cost was per-scope setup rather than query execution.The gain scales with schema size, and only matters at the large end
Building the mapper is
O(models x fields)plus theO(models x relations)m2m pass on postgres, so what this saves tracks schema size directly. Deriving one client, which is one executor construction, on a postgres schema with mapped names:A transaction scope derives twice, so the per-transaction saving is roughly double the last column.
At 25 models that is about 0.15 ms per transaction, which nobody will notice. At 1067 models it is about 4 ms, which is what the 3.1x above reflects. So this is worth having for users with large schemas and is a rounding error for everyone else. I would rather state that plainly than let the headline multiplier imply a broader win than the change delivers.
Note that B still grows with schema size. Deriving a client is not free after this change either; the PR removes the mapper rebuild specifically, not every per-derive cost.
For change 2, a MySQL client on a 1067-model schema with no mapped names anywhere, timing one client derive (one executor construction, no queries):
About 0.104 ms per executor construction, so roughly 0.21 ms per transaction scope. Zero on postgres, which short-circuits that check.
On the ~1s in #2773
That number was measured on 3.7.2 and is stale for
dev.packages/orm/src/client/executor/is unchanged betweenv3.7.2anddev. The collapse came entirely from 805a6b8 ("perf(orm): memoize implicit m2m join-table and model lookups", #2715). It memoized thegetManyToManyRelationcall the mapper constructor makes per model x relation field, and shipped in v3.8.1. So anyone on 3.8.1+ already has the cheap version.The same harness on 3.7.2 reproduces the original figures within ~5%: 1021 ms in-transaction, 506 ms per scope. That is why I trust the A/B above to be measuring the same thing.
So the cost was largely fixed in June; the structural defect was not. This PR is the remaining 3.1x, not the 80x the issue implies.
What this does not fix
Two independent clients built over the same schema still build a mapper each. That is outside the scope of #2773 and would need a schema-keyed cache, which raises lifetime questions this change avoids entirely.
Testing
tests/regression/test/issue-2773.test.ts, 5 tests. They assert mapper identity across the transaction-derived client and all five executor derive sites. They also check that a different-options derive still builds its own mapper, that@@map/@mapresolve identically inside and outside a transaction, and that a schema needing no mapper still works. They assert the invariant rather than elapsed time, which would flake on CI. Verified failing on unmodifieddevwith an identity mismatch, not a missing method.pnpm testgreen, 42/42 turbo tasks.pnpm run lintclean.TEST_DB_PROVIDER=sqlite|postgresql|mysql. Two suites failed there, and I checked both against unmodifieddev.bun-e2eunder mysql fails 2 runs in 3 without this change.edge-runtime-e2epassed 2/2 on re-run after a one-off 100s timeout. Flagging in case they are known to you.Summary by CodeRabbit