test: failing tests for AND/OR queries mixing indexed and non-indexed conditions#1581
test: failing tests for AND/OR queries mixing indexed and non-indexed conditions#1581kevin-dp wants to merge 7 commits into
Conversation
…d non-indexed conditions These tests assert the expected results of currentStateAsChanges for AND/OR where clauses that combine conditions on indexed fields with conditions that cannot be served by an index. They currently fail. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds tests for strict date ChangesComplex Query Optimization & Date-bound Tests
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 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: |
Adds expected-behaviour tests for range conditions: - compound ranges sharing a boundary value must apply the strictest bound regardless of argument order, including for date values - one-sided compound ranges must return the matching rows - strict comparisons (gt) on date fields must exclude the boundary row Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A compound range condition where one bound is undefined (e.g. gt(score, undefined) AND lt(score, 90)) must match nothing, since a comparison against undefined is never true. The index-optimized path must agree with a full scan. This test currently fails. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ries A comparison against null/undefined is never true, but BTree indexes store and return rows with nullish indexed values (they sort as the smallest key). These tests assert that the index-optimized snapshot matches a full predicate scan for: - eq against undefined - IN with an undefined member - a range comparison over a field that has rows with undefined values - an upper-bounded compound range over such a field They currently fail. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two more cases where an index-optimized snapshot must match a full
predicate scan:
- a string range predicate (e.g. name > 'z') must return a row whose
value satisfies the JS relational comparison ('ö' > 'z'), even though
a locale-collated index orders that value differently
- eq and IN against NaN must not match a NaN-valued row, since NaN is
never equal to itself
They currently fail.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/db/tests/collection-indexes.test.ts (1)
1377-1394: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider extracting the repeated collection-setup boilerplate into a helper.
The
createCollection({ getKey, startSync, autoIndex, defaultIndexType, sync: { sync: ({begin,write,commit,markReady}) => {...} } })block is duplicated near-identically across these three tests (and appears to follow the same shape used earlier in the file). A small helper such ascreateSeededCollection(rows)taking the seed rows would cut the noise and keep each test focused on its predicate/assertion.As per coding guidelines: "Extract common logic into utility functions when identical or near-identical code blocks appear in multiple places."
🤖 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/tests/collection-indexes.test.ts` around lines 1377 - 1394, The test setup for creating seeded collections is duplicated across multiple cases, so extract the repeated createCollection/getKey/startSync/autoIndex/defaultIndexType/sync boilerplate into a helper. Add a small helper near the existing collection-index tests, such as a seeded collection factory that accepts the row seed data and performs the begin/write/commit/markReady sequence, then update the affected tests (including the stringCollection setup) to use it so each test only states its specific rows and assertion.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/tests/collection-indexes.test.ts`:
- Around line 1377-1394: The test setup for creating seeded collections is
duplicated across multiple cases, so extract the repeated
createCollection/getKey/startSync/autoIndex/defaultIndexType/sync boilerplate
into a helper. Add a small helper near the existing collection-index tests, such
as a seeded collection factory that accepts the row seed data and performs the
begin/write/commit/markReady sequence, then update the affected tests (including
the stringCollection setup) to use it so each test only states its specific rows
and assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5acff077-b247-41ef-aa93-9ec98cfb0ee9
📒 Files selected for processing (1)
packages/db/tests/collection-indexes.test.ts
… domains Three more cases where an index-optimized range query must match a full predicate scan: - an array-valued field (the evaluator compares with standard relational operators, which differ from the index's recursive array ordering) - a field indexed with a custom comparator - a numeric field that also contains a NaN value They currently fail. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
NaN and invalid Dates have no natural order. They should still get a consistent, well-defined position (alongside nulls) so that: - the comparator produces a stable total order; - ordering a collection by such a field is deterministic; - a range query on a field that contains such a value can still be served by the index rather than falling back to a full scan. These tests currently fail. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Superseded by #1582. These failing tests were split out purely to show the red → green progression. They're now included directly in #1582 (passing) alongside the fixes that make them pass, plus the PostgreSQL Closing in favor of #1582. |
Summary
Adds seven unit tests to
collection-indexes.test.tsthat assert the expected results ofcurrentStateAsChangesforwhereclauses that the index optimizer currently gets wrong. All tests are written purely in terms of expected behaviour.Mixing indexed and non-indexed conditions (e.g.
length(name) > 6):Range boundary handling:
4. Strictest lower bound —
gte(age, 25) AND gt(age, 25)must reduce toage > 25regardless of argument order. Currently the inclusive bound wins.5. Strictest upper bound —
lte(age, 30) AND lt(age, 30)must reduce toage < 30. Currently the inclusive bound wins.6. Date bounds at the same value — distinct
Dateinstances representing the same time must be treated as equal when picking the strictest bound. (Also exercises one-sided compound ranges, which currently return an empty result.)7. Strict comparisons on date fields —
gt(createdAt, date)must exclude the boundary row. Currently the BTree index includes it because it compares the normalized indexed value against the rawDate.mainby design — they pin down the expected behaviour. The follow-up PR #1582 provides the fixes that make them pass.🤖 Generated with Claude Code
Summary by CodeRabbit
and/orbehavior when mixing indexed and computed predicates, including “strictest bound wins” logic.undefinedvalues andNaNsemantics in equality andINqueries.