find-domains query perf fixes#2155
Merged
Merged
Conversation
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
🦋 Changeset detectedLatest commit: 2b1b894 The changes in this PR will be included in the next version bump. This PR includes changesets to release 22 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThis PR consolidates domain query construction by eliminating a multi-layer filtering pattern and moving all logic into a centralized ChangesFind-Domains Query Architecture
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
# Conflicts: # apps/ensadmin/package.json # pnpm-lock.yaml
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the Omnigraph find-domains query path to improve performance by pushing filtering/ordering into a single flat domains query (with conditional joins), and adds supporting DB indexes to make common ORDER BY/LIMIT patterns index-friendly.
Changes:
- Refactor
resolveFindDomainscall sites (Query/Account/Registry/Domain) to pass a compoundwhereobject into a centralized resolver instead of composing layered CTEs. - Rework
resolveFindDomainsto querydomainsdirectly (conditionally joining registration tables only when needed for ordering), and update cursor ordering helpers to align with new DB indexes. - Add/adjust DB indexes (including composite/prefix and partial canonical indexes) to support ordered scans for NAME/DEPTH queries.
Reviewed changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Switch some app deps from preview versions to workspace:* links; lockfile snapshot adjustments. |
| packages/enssdk/src/omnigraph/generated/schema.graphql | Update AccountDomainsWhereInput.canonical semantics (remove default, clarify meaning). |
| packages/ensdb-sdk/src/ensindexer-abstract/protocol-acceleration.schema.ts | Add secondary index to speed domain-resolver relation lookups by domainId. |
| packages/ensdb-sdk/src/ensindexer-abstract/ensv2.schema.ts | Add composite/prefix and partial indexes to accelerate NAME/DEPTH ordered scans and registry/label lookups. |
| apps/ensapi/src/omnigraph-api/schema/registry.ts | Simplify Registry.domains resolver to delegate filtering to resolveFindDomains via where.registryId. |
| apps/ensapi/src/omnigraph-api/schema/query.ts | Simplify Query.domains resolver to delegate to resolveFindDomains and enforce canonical filtering centrally. |
| apps/ensapi/src/omnigraph-api/schema/domain.ts | Update Domain.subdomains to short-circuit empty connections and delegate via where.registryId = subregistryId. |
| apps/ensapi/src/omnigraph-api/schema/domain-inputs.ts | Remove default for AccountDomainsWhereInput.canonical; replace old default-order constants with a new order-value type. |
| apps/ensapi/src/omnigraph-api/schema/account.ts | Simplify Account.domains resolver to delegate filtering to resolveFindDomains via where.ownerId. |
| apps/ensapi/src/omnigraph-api/lib/get-domain-by-interpreted-name.ts | Add span around forward-walk query for tracing/observability. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/find-domains-resolver.ts | Major refactor: build flat domain query with conditional joins, compound filters, and keyset pagination aligned to new indexes. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/find-domains-resolver-helpers.ts | Update ordering + cursor filters to use index-aligned expressions (notably left(canonical_name, N)). |
| apps/ensapi/src/omnigraph-api/lib/connection-helpers.ts | Add EMPTY_CONNECTION helper for short-circuiting connection resolvers. |
| apps/ensadmin/package.json | Switch local deps to workspace:* for monorepo consistency. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/with-ordering-metadata.ts | Removed: ordering metadata now handled directly in resolveFindDomains. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/index.ts | Removed: layered find-domains composition no longer used. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-version.ts | Removed: version filtering now handled in resolveFindDomains. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-registry.ts | Removed: registry filtering now handled in resolveFindDomains. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-parent.ts | Removed: subdomain filtering now handled via registryId/subregistryId. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-owner.ts | Removed: owner filtering now handled in resolveFindDomains. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-name.ts | Removed: name filtering now handled in resolveFindDomains. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-name-starts-with.ts | Removed: prefix filtering now handled in resolveFindDomains. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-name-in.ts | Removed: exact/in-set filtering now handled in resolveFindDomains. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-canonical.ts | Removed: canonical filtering now handled in resolveFindDomains. |
| apps/ensapi/src/omnigraph-api/lib/find-domains/layers/base-domain-set.ts | Removed: base CTE composition replaced by flat domains query approach. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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
`@apps/ensapi/src/omnigraph-api/lib/find-domains/find-domains-resolver-helpers.ts`:
- Around line 22-31: The current approach truncates canonical names to
CANONICAL_NAME_SORT_PREFIX (256) in truncateNameForCursor and in the ORDER BY
expression, which breaks true lexicographic ordering for long names; change the
logic so ordering uses the full canonical_name for comparison (keep the
tie-breaker id as needed) and stop truncating the value used for ORDER BY, while
still keeping cursor payloads compact by either encoding the full name into the
cursor (e.g., base64) or using a stable hash of the full name plus id for the
cursor; update truncateNameForCursor (and the other similar spots referenced
around the file, including the other occurrences of left(..., 256) at the places
noted) to return/encode the non-truncated value used for ordering or to produce
a hash-based cursor, and ensure cursorFilter comparisons use the same full-name
ordering semantics and tie-breaker (id) as the query.
In `@apps/ensapi/src/omnigraph-api/schema/domain.ts`:
- Around line 227-234: The Domain.subdomains resolver currently only filters by
registryId: parent.subregistryId which returns all domains under that registry;
update the resolver that calls resolveFindDomains to also scope by the current
domain as parent (e.g., add parentId: parent.id or equivalent parent reference)
so only direct descendants are returned—modify the where clause passed to
resolveFindDomains in the Domain.subdomains resolver to include both registryId:
parent.subregistryId and the parent identifier (parent.id or the appropriate
parent key) while preserving existing order and connectionArgs.
🪄 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
Run ID: bec51ee5-6f97-4c5c-b9ee-812cf3fe2fbf
⛔ Files ignored due to path filters (2)
packages/enssdk/src/omnigraph/generated/schema.graphqlis excluded by!**/generated/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (23)
apps/ensadmin/package.jsonapps/ensapi/src/omnigraph-api/lib/connection-helpers.tsapps/ensapi/src/omnigraph-api/lib/find-domains/find-domains-resolver-helpers.tsapps/ensapi/src/omnigraph-api/lib/find-domains/find-domains-resolver.tsapps/ensapi/src/omnigraph-api/lib/find-domains/layers/base-domain-set.tsapps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-canonical.tsapps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-name-in.tsapps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-name-starts-with.tsapps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-name.tsapps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-owner.tsapps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-parent.tsapps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-registry.tsapps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-version.tsapps/ensapi/src/omnigraph-api/lib/find-domains/layers/index.tsapps/ensapi/src/omnigraph-api/lib/find-domains/layers/with-ordering-metadata.tsapps/ensapi/src/omnigraph-api/lib/get-domain-by-interpreted-name.tsapps/ensapi/src/omnigraph-api/schema/account.tsapps/ensapi/src/omnigraph-api/schema/domain-inputs.tsapps/ensapi/src/omnigraph-api/schema/domain.tsapps/ensapi/src/omnigraph-api/schema/query.tsapps/ensapi/src/omnigraph-api/schema/registry.tspackages/ensdb-sdk/src/ensindexer-abstract/ensv2.schema.tspackages/ensdb-sdk/src/ensindexer-abstract/protocol-acceleration.schema.ts
💤 Files with no reviewable changes (11)
- apps/ensapi/src/omnigraph-api/lib/find-domains/layers/with-ordering-metadata.ts
- apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-canonical.ts
- apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-name.ts
- apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-parent.ts
- apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-registry.ts
- apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-owner.ts
- apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-name-starts-with.ts
- apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-name-in.ts
- apps/ensapi/src/omnigraph-api/lib/find-domains/layers/index.ts
- apps/ensapi/src/omnigraph-api/lib/find-domains/layers/filter-by-version.ts
- apps/ensapi/src/omnigraph-api/lib/find-domains/layers/base-domain-set.ts
lightwalker-eth
approved these changes
May 19, 2026
Member
lightwalker-eth
left a comment
lightwalker-eth
left a comment
There was a problem hiding this comment.
@shrugs Great to see these enhancements 👍 Reviewed and shared feedback. Please feel welcome to merge when ready!
- truncateNameForCursor uses code-point iteration to match Postgres LEFT() - registrationValueById: drop unreachable optional chain, explicit invariant - CANONICAL_NAME_SORT_PREFIX JSDoc typo fixes - add @ensnode/ensdb-sdk changeset for the new perf indexes
Member
Author
|
@greptile review |
Member
Author
|
@greptile review |
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
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.
Also Included: Bridged-Resolver Canonicality Fix
scope creep against the original perf focus, but found while inspecting the perf-test index: only 4.9M of 13.5M Domains were canonical — all mainnet, with the entire ~8.6M Basenames + Lineanames subtree non-canonical. root cause:
apps/ensindexer/src/plugins/unigraph/handlers/ensv1/ENSv1Registry.tsunconditionally calledhandleSubregistryUpdatedandhandleRegistryCanonicalDomainUpdatedon every NewOwner, clobbering the two pointers thathandleBridgedResolverChangeowns (Domain.subregistryId on the L1 origin, Registry.canonicalDomainId on the L2 target). either side's first chain-local subname event broke the bidirectional agreement.fix: guard both calls with two new SDK predicates
isBridgeOriginDomain/isBridgedTargetRegistry(added topackages/ensnode-sdk/src/shared/protocol-acceleration/is-bridged-resolver.ts, reusing the cached config list). on bridge endpoints we leave the pointer alone; non-bridged subnames behave identically to before.intended outcome (post-reindex): mainnet
linea.eth/base.eth.subregistryId stays on the L2 bridged Registry, the L2 Registry's canonicalDomainId stays on the mainnet origin Domain, agreement holds, cascade canonicalizes all ~8.6M Lineanames + Basenames. mainnet-registered subnames directly underlinea.eth/base.eth(bridge.linea.eth,devconnect.linea.eth, the handful that exist) drop out of canonicality — consistent with bridged-resolver semantics where resolution goes through L2.Reviewer Focus (Read This First)
two things worth scrutiny:
canonical_nameinstead of the full name (seetruncateNameForCursor). cursors minted by the prior deploy will not decode-compare correctly under the new code — clients with in-flight pagination state will need to restart from page 1.DomainsWhere.canonicalis nowtypeof === "boolean"-gated: bothtrueandfalseproduce aneq(canonical, ...)filter; onlynull/undefinedis "no filter".AccountDomainsWhereInput.canonicallost itsdefaultValue: false— clients omitting that arg will now receive all domains regardless of canonicality (was: effectively no-op-then-everything, sincefalseused to be a no-op).Query.domainsno longer hardcodescanonical: true— the requirednamefilter already implies it viacanonical_name IS NOT NULL ⟺ canonical = true.Problem & Motivation
three slow paths against the mainnet perf-test index (13.5M domains, 4.9M canonical):
get-domain-by-interpreted-name("eth"): 2.69s. namegraph walk's recursive CTE left-joineddomain_resolver_relationsondomain_idalone, but the PK leads with(chain_id, address)so the join had no usable index.Domain.subdomains(.eth)(3.5M children): 15.26s+. registry-scoped browse had no composite index forWHERE registry_id = X ORDER BY canonical_name LIMIT N; planner fell back to filter + sort.Query.domainstypeahead (e.g.'a%'ordered by NAME): planner was thrown by the hardcodedcanonical = truepredicate. removing it lets the planner use the GIN trigram path cleanly.separately,
find-domainshad a layered "filter-by-X" abstraction (base-domain-set → filter-by-name → filter-by-canonical → ...) that materialized intermediate CTEs and made the SELECT shape opaque to the planner.What Changed (Concrete)
packages/ensdb-sdk/src/ensindexer-abstract/:domain_resolver_relations(domain_id)(secondary lookup off the PK)domains(registry_id, left(canonical_name, 256), id)(composite for registry-scoped NAME browse)find-domains-resolver.tsand helpers): layered filter-by-X pattern collapsed into a single flatresolveFindDomainswith a compoundWHERE. SELECT projects onlyid+ a single conditionalregistration_valuecolumn. NAME/DEPTH order values are read back from the dataloader-hydratedDomaininstead of carried in a separate map. layer files deleted.parent.subregistryIdinstead of reverse-walking viaregistry.canonicalDomainId. whensubregistryIdis null, short-circuits to a newEMPTY_CONNECTIONhelper (inconnection-helpers.ts).CANONICAL_NAME_SORT_PREFIX(256) chars at encode time viatruncateNameForCursor. the filter-sideleft(${cursor.value}::text, 256)re-application is gone; cursor value is compared directly against the index expression.DomainsWhere.canonicalistypeof === "boolean"-gated, so bothtrueandfalsefilter.AccountDomainsWhereInput.canonicaldropsdefaultValue: false; description rewritten to clarify the three states.Query.domainsdrops the hardcodedcanonical: true— the required name filter does the work.opandidCmp.(typeof ensIndexerSchema.domainType.enumValues)[number]instead of a hand-rolled literal union.typeof DomainsNameFilter.\$inferInputinline at point-of-use.AccountDomainsWhereInput.canonicalno longer defaulting.Design & Planning
no design doc. all work was done incrementally against a live perf-test index (
ensindexer_perf_unigraph_pa,mainnet,unigraph + protocol-acceleration) with EXPLAIN ANALYZE driving each index decision.an early version of this PR added two partial indexes
WHERE canonical = trueto supportQuery.domains's hardcodedcanonical = truepredicate. follow-up analysis showed that the NAME partial was harmful in the broad-prefix typeahead case (planner mis-selected it and scanned 4M rows; 11s actual). dropping the hardcoded predicate and the partials together is strictly better — the GIN trigram + existing hash indexes handle the remaining shapes, and the only regression ('a%'DEPTH-order: 5.5ms → 686ms) is a single-keystroke edge case.alternative considered: enforce
InterpretedLabelbyte-length cap (255-byte DNS-encodable) and materialize a length-capped canonical name column, removing the need for theLEFT(...)expression index. documented in theCANONICAL_NAME_SORT_PREFIXJSDoc as the cleaner long-term shape; deferred because it requires changes to indexing-time semantics.Self-Review
ran
/simplify(three review agents: reuse, quality, efficiency) and applied findings.canonicalName+canonicalDepthon every row regardless of orderBy; narrowed to id + conditional registration column. planner mis-selection on partial NAME index under broad-prefix ILIKE.__orderValuemap was redundant for NAME/DEPTH — those values live on the loaded Domain.getOrderValueFromResultdeleted.truncateNameForCursor,EMPTY_CONNECTION,getDefaultOrder.DOMAINS_DEFAULT_ORDERconstant extracted.base-domain-set.ts,filter-by-*.ts,with-ordering-metadata.ts,layers/index.ts) deleted;getOrderValueFromResultdeleted; verboseDomainsWhereJSDoc trimmed; narrating comments ("Compound WHERE…", "build order clauses…") removed; the two partialWHERE canonical = trueindexes removed.Cross-Codebase Alignment
resolveFindDomains,DomainsWhereInput,canonical = true,setFilterCondition,paginateBy,byCanonicalDepthfind-events-resolver.ts(similar\$dynamic()+ conditional join shape — left independent),Registry.parents(separate path),Account.domainscallsite (now relies on the new semantics),Query.domainscallsite (no longer needs the predicate)setFilterConditionfromfind-events-resolver.tsinto a shared module sonameConditioncan reuse itseq/inhalves. cross-file refactor; out of scope here.Downstream & Consumer Impact
AccountDomainsWhereInput.canonicalno longer defaults — described in §Reviewer FocusAccountDomainsWhereInput.canonicalGraphQL description rewritten; schema.graphql regenerated.EMPTY_CONNECTIONis a module-level singleton, not a factory. pothos relay readsedges/pageInfowithout mutating, so the shared instance is safe — but if any future plugin starts mutating connection returns, this would leak across requests.Testing Evidence
pnpm lint:ciclean.get-domain-by-interpreted-name("eth"): 2.69s → 0.836msDomain.subdomains(.eth)(3.5M children): 15.26s → 19-22ms warm / 140ms coldQuery.domainsname.eqordered NAME: 0.78ms (hash index)Query.domainsname.starts_with: "vitalik"ordered NAME: 16.6ms / DEPTH: 1.4ms (GIN trigram + bitmap)Query.domainsworst-casename.starts_with: "a"(815k matches) ordered NAME: 1.8s / DEPTH: 686ms — single-keystroke typeahead edge case, sub-secondpnpm test:integration:ci) not run for the post-merge state. cursor-encoding change has no dedicated unit test (existing helpers tests cover onlyisEffectiveDesc).pageInfo/edgesconsumers don't mutate the returned object (relevant toEMPTY_CONNECTIONsingleton).Scope Reductions
setFilterConditioninto a shared helper for bothfind-eventsandfind-domains— separate refactor PRInterpretedLabelbyte-length cap to avoid theLEFT(...)expression index entirely — needs indexing-time changes; documented as future work inCANONICAL_NAME_SORT_PREFIXJSDocEMPTY_CONNECTIONto a factory function — only matters if a future pothos plugin mutates connection results; trivial to switch laterRisk Analysis
Account.domainsdefault behavior changes — clients that relied oncanonical: falseas a no-op now get a filter applied. mitigation: schema description rewritten;defaultValueremoved so omitting the arg gives the most permissive answer (all domains). a client explicitly passingcanonical: falseis the only one affected, and the new behavior matches the field's name.name.starts_with: "a") DEPTH-order is now ~700ms (was ~5ms with the dropped partial). worth mentioning to UI clients that they should debounce or require ≥2 chars; all narrower prefixes are sub-20ms.Pre-Review Checklist (Blocking)