fix(db): keep source identity across query scopes - #1981
Merged
Merged
Conversation
Live queries returned no rows when an include reused an alias from a joined from() subquery (#1975). The optimizer rebuilt CollectionRefs with fresh SourceIds; since #1877 compiles the optimized subquery, the compiler missed those IDs and fell back to alias text, which a sibling scope had claimed. - Optimizer copies, wraps, and collapses reuse the CollectionRef. - Compilation binds inputs by SourceId only, so a lost identity raises CollectionInputNotFoundError instead of reading another source. - No scope inside an include can shadow an ancestor alias, including in unionAll() branches and from()/join subqueries. - One query cannot give two sources the same alias. Adds a generated alpha-renaming oracle across sibling scopes with an independent recomputation model, plus the review record and contract updates. Co-authored-by: Isaac <no-reply@databricks.com>
Contributor
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (7)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe query optimizer preserves collection source identity through rewrites. The compiler resolves collection inputs by source ID and rejects duplicate or shadowing aliases in query scopes. Tests cover legal sibling-scope alias reuse and invalid alias collisions. ChangesQuery Scope Identity
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The change preserves collection identity across query rewrites and explicitly rejects ambiguous aliases. No actionable merge-blocking risk remains; merge after normal checks pass. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change prevents lost source identities from silently selecting a same-named input and rejects ambiguous query aliases. No new privilege or data-access boundary is demonstrated. Compatibility with existing callers and some advanced query-loading behavior remain only partly verified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 41.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 24 functions across 8 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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/react-router-with-db
@tanstack/rxdb-db-collection
@tanstack/solid-db
@tanstack/svelte-db
@tanstack/tauri-db-sqlite-persistence
@tanstack/trailbase-db-collection
@tanstack/vue-db
commit: |
Contributor
|
Size Change: +156 B (+0.09%) Total Size: 175 kB 📦 View Changed
ℹ️ View Unchanged
|
Contributor
|
Size Change: 0 B Total Size: 8.51 kB ℹ️ View Unchanged
|
A union row holds the branches' projected fields, not their aliases, so an include or join on a unionAll() query cannot see branch aliases. The include-shadowing check exposed them anyway and rejected a query that main accepts with correct rows. Includes now see only the parent's from and join aliases. Branch aliases inside an include are still checked against its ancestors. Adds a unionParent oracle topology and pinned witnesses for both legal union namings, and corrects the union scope rule in ARCHITECTURE.md. Co-authored-by: Isaac <no-reply@databricks.com>
2 of 3 tasks
Merged
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.
A live query returned no rows when an include used the same alias as a source in a joined
from()subquery. Since 0.11.0, this query returns[]instead of[1]:Rename the include alias, and the query returns the expected rows. The alias name changed the result, and no error or warning appeared. This PR makes the result independent of alias names for the query forms that the new oracle covers. The plan now keeps the identity of each source. Compilation reads source input by that identity only.
Cause
Each
CollectionRefhas an opaqueSourceId. The live query creates one input for eachSourceId.The optimizer copied, wrapped, and collapsed sources with
new CollectionRef(collection, alias). Each copy received a newSourceIdthat no input used. The compiler did not find that ID. It then read the input by alias text.bindSourceInputsstored the input of every scope under its alias, so the last source with that name won. Here, the include'srefinput replaced the input for the subquery'spartssource.This defect existed since #1740 added
SourceId, but it had no effect. Before #1877, the compiler compiled the original subquery and discarded the optimized copy. #1877 compiles the optimized copy when that copy contains a pushed predicate. That is the intended behavior. The orphaned IDs then reached compilation. Bisection identifies40a5aea56(#1877) as the first bad commit.This also explains the issue's "two predicates" condition. The
ref.clientIdfilter is on the nullable side of a left join. The optimizer does not push it. Thepart.activefilter moves into the subquery, and only that change makes the copy differ. One pushed source predicate is sufficient.Repair
CollectionRef.bindSourceInputsmaps caller inputs that use alias keys toSourceIdkeys once. It no longer stores inputs under alias keys.allInputs[sourceId]only.If a later rewrite loses a
SourceId, compilation now raisesCollectionInputNotFoundError. It cannot read a source with the same name from another scope. A test reverted each optimizer site with this binding in place. The existing suite then failed 3, 53, 146, and 59 tests. Compiler unit tests that supply alias-keyed inputs continue to work.Alias rules that now raise errors
ARCHITECTURE.md normative law 1 requires that a legal alias rename cannot change an explicit projection. Review of that law found two namings that the builder accepted and that produced wrong rows:
unionAll()branch in an include, and afrom()subquery in an include. These scopes can read the parent row in their callbacks. The correlation read the wrong source and returned empty children. These namings now raiseDuplicateAliasInSubqueryError.QueryCompilationError.A top-level
from()subquery has no ancestor scope. It can still use the outer alias, as the issue query does. Sibling scopes can also use the same names.A
unionAll()row holds the projected fields of its branches, not their aliases. An include or a join on aunionAll()query can therefore use a branch alias. The compiler still rejects a name that two branches of one union repeat.Oracle
The new owner is
includes alpha-renaming across sibling scopesinincludes-oracle.property.test.ts. Its grammar, model, and driver are inincludes-scope-identity-oracle.ts. It generates four topologies that put a source next to a sibling source with the same name:from()subquery with an includeunionAll()branch with an includeunionAll(), which can use a branch aliasThe grammar also changes these properties:
DISTINCTEach checkpoint occurs after preload and after each source write. Each checkpoint makes three comparisons:
loadSubsetWHERE clauses that each collection receives under the two namings.One in four scenarios uses any naming. The test then requires the builder to reject each illegal naming. That check found the duplicate-join defect.
Every pinned witness and both campaigns fail on
18abceee4and pass with this change. Each optimizer site mutant fails at least one witness. The previous oracle did not catch the collapse site. Planted model faults and an alias-dependent routing mutant each fail at their intended comparison.Limits
These legal forms are in the bug class but outside the grammar:
groupByRIGHTandFULLjoinsaliasRemappingmapProbes did not find a failure in these forms. The coverage map and the review record list them with an owner. This PR does not claim that the alias bug class is closed.
Implementation map
query/optimizer.ts,query/compiler/index.ts(bindSourceInputs,validateQueryStructure),query/compiler/joins.ts. The production diff is +65 and −16 lines. The new alias rules use most of the added lines.SourceIdpreservation and the single alias namespace acrossunionAll()branches.docs/contributing/oracle-reviews/issue-1975-scope-identity.md,oracle-coverage.mdtests/query/validate-aliases.test.tsVerification
pnpm --filter @tanstack/db test: 229 files and 7924 tests pass.TANSTACK_DB_ORACLE_RUNS_MULTIPLIER=10.18abceee4replay directly throughTANSTACK_DB_ORACLE_PROPERTY=includes.scoped-alpha-renaming.tscpass. The onlytscerrors are intests/conformance/, and they also occur onmain.Closes #1975
This pull request and its description were written by Isaac.
Summary by CodeRabbit
Bug Fixes
Tests
unionAllbranches.