fix: enforce all where conditions when index optimization is partial#1582
Open
kevin-dp wants to merge 2 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR adds an ChangesIndex Optimization Correctness
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: |
![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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/db/src/utils/index-optimization.ts (1)
281-305:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winPreserve the stricter bound when two predicates use the same value.
If
gte(field, 5)is seen beforegt(field, 5)(orltebeforelt), the later strict predicate is ignored because these branches only update onvalue > from/value < to. This path then returnsisExact: trueand marks both args as covered, so boundary rows likefield === 5can be returned even though the fullANDshould exclude them.Suggested fix
for (const { operation, value } of operations) { switch (operation) { case `gt`: - if (from === undefined || value > from) { + if ( + from === undefined || + value > from || + (value === from && fromInclusive) + ) { from = value fromInclusive = false } break case `gte`: @@ case `lt`: - if (to === undefined || value < to) { + if ( + to === undefined || + value < to || + (value === to && toInclusive) + ) { to = value toInclusive = false } break case `lte`:Also applies to: 317-321
🤖 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/utils/index-optimization.ts` around lines 281 - 305, The loop that computes numeric bounds (iterating over operations with variables operation, value, from, fromInclusive, to, toInclusive, isExact) ignores stricter predicates when the new predicate has the same numeric value (e.g., gte then gt with same value), so modify the branch conditions to update when value > current bound OR when value === current bound and the new operator is stricter (for lower bounds prefer gt over gte => set fromInclusive=false when value===from and operation==='gt'; for upper bounds prefer lt over lte => set toInclusive=false when value===to and operation==='lt'). Apply the same change in the identical block around the 317-321 region so equal-valued stricter predicates overwrite inclusivity accordingly and preserve correct isExact/covered results.
🤖 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/db/src/utils/index-optimization.ts`:
- Around line 556-558: canOptimizeOrExpression is too permissive compared to
optimizeOrExpression because it only verifies canOptimizeExpression for each
disjunct without ensuring the referenced index actually supports the operation
(e.g., gt on an eq-only index). Update the canOptimize* helpers (notably
canOptimizeExpression, the simple-comparison and in helpers) so they check index
capability/support for the specific operator (range vs eq vs contains) the same
way optimizeOrExpression/optimizer would, or alternately change
canOptimizeOrExpression to call into the optimizer logic used by
optimizeOrExpression to determine OR eligibility; ensure symbols to update
include canOptimizeOrExpression, canOptimizeExpression, simple-comparison
helper, in helper, and keep behavior consistent with optimizeOrExpression for
operators like gt(ref,val).
---
Outside diff comments:
In `@packages/db/src/utils/index-optimization.ts`:
- Around line 281-305: The loop that computes numeric bounds (iterating over
operations with variables operation, value, from, fromInclusive, to,
toInclusive, isExact) ignores stricter predicates when the new predicate has the
same numeric value (e.g., gte then gt with same value), so modify the branch
conditions to update when value > current bound OR when value === current bound
and the new operator is stricter (for lower bounds prefer gt over gte => set
fromInclusive=false when value===from and operation==='gt'; for upper bounds
prefer lt over lte => set toInclusive=false when value===to and
operation==='lt'). Apply the same change in the identical block around the
317-321 region so equal-valued stricter predicates overwrite inclusivity
accordingly and preserve correct isExact/covered results.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6458b40d-a773-4d42-b681-9b2d884e70cd
📒 Files selected for processing (4)
.changeset/index-optimization-partial-and-or.mdpackages/db/src/collection/change-events.tspackages/db/src/utils/index-optimization.tspackages/db/tests/collection-indexes.test.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.changeset/index-optimization-partial-and-or.md (1)
10-12: 💤 Low valueOptional style improvement: vary sentence beginnings.
Three successive sentences begin with "Compound range conditions." Consider rephrasing one or two for readability, e.g., "When conditions share the same boundary value..." or "One-sided compound ranges...".
🤖 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 @.changeset/index-optimization-partial-and-or.md around lines 10 - 12, The three bullet lines all start with "Compound range conditions," which is repetitive; update one or two bullets to vary sentence openings for readability—for example change the first to "When conditions share the same boundary value..." and the second to "One-sided compound ranges..." while keeping the technical meaning and examples intact; edit the three bullet items shown (the lines about shared boundary value, one-sided compound range, and strict range comparisons) to use the revised phrasing but preserve terminology like "age >= 5 AND age > 5", "age > 5 AND age >= 8", and "gt/lt on BTree-indexed fields" so the content remains precise.Source: Linters/SAST tools
🤖 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 @.changeset/index-optimization-partial-and-or.md:
- Around line 10-12: The three bullet lines all start with "Compound range
conditions," which is repetitive; update one or two bullets to vary sentence
openings for readability—for example change the first to "When conditions share
the same boundary value..." and the second to "One-sided compound ranges..."
while keeping the technical meaning and examples intact; edit the three bullet
items shown (the lines about shared boundary value, one-sided compound range,
and strict range comparisons) to use the revised phrasing but preserve
terminology like "age >= 5 AND age > 5", "age > 5 AND age >= 8", and "gt/lt on
BTree-indexed fields" so the content remains precise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fd18e5dc-5e7f-4733-b5e7-0b5b285ec3f7
📒 Files selected for processing (4)
.changeset/index-optimization-partial-and-or.mdpackages/db/src/indexes/btree-index.tspackages/db/src/utils/index-optimization.tspackages/db/tests/collection-indexes.test.ts
OR expressions now require every disjunct to be index-optimizable; otherwise the query falls back to a full scan, since rows matched only by a non-optimizable disjunct cannot be recovered from index lookups. AND expressions keep partial index optimization but the optimizer now reports whether the matching keys are exact. When they are a superset (some conjuncts could not use an index, or a compound range was combined with other conditions), currentStateAsChanges re-checks each candidate row against the full where expression. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…range edge cases Compound range conditions sharing a boundary value (gte(x,5) AND gt(x,5)) now keep the strict bound regardless of argument order. Bound values are compared with the same comparator the indexes use so dates and locale strings behave correctly. Two further issues surfaced by the regression tests: - One-sided compound ranges passed an explicit undefined bound to rangeQuery, which treats present-but-undefined as the undefined sentinel and returned an empty result. Bounds are now only passed when they exist. - BTreeIndex's exclusive lower bound check compared the normalized indexed value against the raw query value, so gt on date fields included the boundary row. It now compares against the normalized key. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
kevin-dp
force-pushed
the
fix/index-optimization-partial-and-or
branch
from
11be9da to
559a10d
Compare
kevin-dp
changed the base branch from
main
to
test/showcase-index-optimization-or-and-bugs
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.
Summary
Follow-up to #1581 — fixes the index optimizer so that the seven failing tests added there pass. All fixes are correctness fixes for
whereclauses served from indexes.Commit 1 — enforce all conditions when index optimization is partial
currentStateAsChangestreats the optimizer'smatchingKeysas the exact result set, but the optimizer silently dropped conditions it couldn't serve from an index:age > 5 AND age < 10) early-returned and discarded all sibling conjuncts, even indexed ones.This also affected the initial state of filtered subscriptions / live queries (
requestSnapshotsends snapshots without re-filtering), while subsequent change events were evaluated correctly — producing inconsistent, timing-dependent state.Fix:
OptimizationResultgains anisExactflag. OR now requires every disjunct to be optimizable (missing rows can't be recovered by post-filtering), else falls back to a full scan. AND keeps partial index optimization but marks the result inexact when any conjunct was skipped; the compound-range path reports which conjuncts it covered so the rest are processed.currentStateAsChangesre-checks candidate rows against the full expression only when the result is inexact — fully-indexed queries take the same fast path as before.Commit 2 — range boundary fixes (from CodeRabbit review + what it surfaced)
gte(x, 5) AND gt(x, 5)kept the inclusive bound when the non-strict condition appeared first. Bounds are now merged strictness-aware, comparing values withmakeComparator(the same comparison semantics the indexes use) so dates and locale strings behave correctly —===would silently fail for equal-valuedDateinstances.{from, to, ...}torangeQuery, butrangeQuerydistinguishes an absent bound from an explicitundefined(which becomes the undefined-sentinel and produces an empty interval). Bounds are now only passed when they exist.rangeQuery's exclusive-lower-bound check compared the normalized indexed value (dates stored as timestamps) against the raw queryDate— never equal, so plaingt(createdAt, date)included the boundary row. It now compares against the normalized key.Testing
@tanstack/dbsuite: 100 files, 2392 tests passing.🤖 Generated with Claude Code