fix(query): lazy-join loads through the collection the join key resolves to#1614
fix(query): lazy-join loads through the collection the join key resolves to#1614kevin-dp wants to merge 5 commits into
Conversation
…ction Add a failing reproduction: when a subquery used in a JOIN clause selects its join key from the joined side of the subquery, the outer join key resolves to a collection that is already indexed. The lazy-join loader should load through that index, but today it emits a "Join requires an index" warning naming the already-indexed collection (and falls back to a full load). Data is still correct via the fallback. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthrough
ChangesLazy join alias resolution
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related issues
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 |
More templates
@tanstack/angular-db
@tanstack/browser-db-sqlite-persistence
@tanstack/capacitor-db-sqlite-persistence
@tanstack/cloudflare-durable-objects-db-sqlite-persistence
@tanstack/db
@tanstack/db-ivm
@tanstack/db-sqlite-persistence-core
@tanstack/electric-db-collection
@tanstack/electron-db-sqlite-persistence
@tanstack/expo-db-sqlite-persistence
@tanstack/node-db-sqlite-persistence
@tanstack/offline-transactions
@tanstack/powersync-db-collection
@tanstack/query-db-collection
@tanstack/react-db
@tanstack/react-native-db-sqlite-persistence
@tanstack/rxdb-db-collection
@tanstack/solid-db
@tanstack/svelte-db
@tanstack/tauri-db-sqlite-persistence
@tanstack/trailbase-db-collection
@tanstack/vue-db
commit: |
Import BTreeIndex from src/indexes/btree-index.js (not collection/index) and type the collections via factory-wrapper ReturnType so the test passes typecheck. The index-warning assertion still fails as intended. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
When a subquery used in a JOIN clause selects its join key from a joined source rather than its own from clause, followRef traced the index requirement to the resolved collection while the lazy loader still subscribed to the subquery's from alias. The two diverged, producing a misleading "Join requires an index" warning that named an already-indexed collection and an unnecessary full-load fallback. followRef now also returns the alias of the source the ref resolves to; getLazyLoadTargets uses it as the subscription alias (falling back to the from-clause remapping when the key resolves directly to the from source), so lazy loading drives through the correct collection's index. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/db/src/query/compiler/lazy-targets.ts (1)
58-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
??instead of||for the alias default chain.
||treats an empty-string alias as falsy and would skip it, whereas??only falls through onnull/undefined. Switch to nullish coalescing per the project convention.♻️ Proposed change
- alias: followRefResult.alias || aliasRemapping[lazyAlias] || lazyAlias, + alias: followRefResult.alias ?? aliasRemapping[lazyAlias] ?? lazyAlias,As per coding guidelines: "Use the nullish coalescing operator
??for default values instead of checking for null and undefined separately".🤖 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/db/src/query/compiler/lazy-targets.ts` around lines 58 - 64, The alias fallback in the lazy target mapping uses a falsy chain, so update the logic in the result object construction to use nullish coalescing instead of the current defaulting behavior. In the code path that builds the array return value in lazy-targets.ts, adjust the alias selection in the followRefResult/aliasRemapping/lazyAlias chain so empty-string aliases are preserved and only null or undefined falls through, matching the project convention.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@packages/db/src/query/compiler/lazy-targets.ts`:
- Around line 58-64: The alias fallback in the lazy target mapping uses a falsy
chain, so update the logic in the result object construction to use nullish
coalescing instead of the current defaulting behavior. In the code path that
builds the array return value in lazy-targets.ts, adjust the alias selection in
the followRefResult/aliasRemapping/lazyAlias chain so empty-string aliases are
preserved and only null or undefined falls through, matching the project
convention.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 585dcc2b-743d-498e-820a-5d4d9dd7de14
📒 Files selected for processing (3)
.changeset/lazy-join-resolved-collection.mdpackages/db/src/query/compiler/lazy-targets.tspackages/db/src/query/ir.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/lazy-join-resolved-collection.md
What
Fixes #1494.
When a subquery used in a
JOINclause selects its join key from a joined source rather than from its ownfromclause, the outer join key resolves to a different collection than the subquery'sfromsource. The lazy-join loader diverged here:followReftraced the index requirement to the collection the key actually resolves to (e.g.teams.id), butfromalias (e.g.members).Because those two diverged, the loader queried the wrong subscription, fell back to a full load, and emitted a
Join requires an indexwarning naming the already-indexed collection — so the advice ("create an index") was already satisfied and unactionable. Data was still correct via the fallback, so this was a diagnostic-/efficiency-quality bug, not a correctness one.Fix
followRefnow also returns the alias of the source the ref ultimately resolves to.getLazyLoadTargetsuses that alias as the subscription target (falling back to the existing from-clause remapping when the key resolves directly to thefromsource). Lazy loading then drives through the correct collection's index — no spurious warning, no unnecessary full-load.Tests
Adds a regression test in
packages/db/tests/query/join-subquery.test.ts: a subquery whose join key resolves to an already-indexed collection must load through that index without emitting aJoin requires an indexwarning that points at it. The test fails without the fix and passes with it.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests