fix(db): support Temporal values in query comparison operators#1519
fix(db): support Temporal values in query comparison operators#1519obeattie wants to merge 9 commits into
Conversation
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: |
kevin-dp
left a comment
kevin-dp
left a comment
There was a problem hiding this comment.
Thank you for your PR. I had a look through it and have some concerns.
Main concern — inconsistency with orderBy
The codebase now has two different orderings for Temporal values:
- Filter operators (
gt/lt/…) → newcompareValues→ Temporal.compare()(by instant/value). ORDER BY→ascComparatorincomparison.ts:59-65→ lexicographica.toString()comparison.
These diverge in observable ways:
| Case | WHERE (new) |
ORDER BY (existing) |
|---|---|---|
Duration PT1H vs PT60M |
equal | "PT1H" < "PT60M" (wrong) |
ZonedDateTime same instant, diff zone |
equal | ordered by zone-name string |
PlainMonthDay |
throws | lex-compares fine |
| mixed Temporal types | throws | lex-compares fine |
A user could write .where(d => gt(d.dur, oneHour)) and .orderBy(d => d.dur) over the same column and get inconsistent semantics — and a PlainMonthDay orderBy "works" while a PlainMonthDay filter throws. Reusing compareValues inside ascComparator would unify them (and incidentally fix the lexicographic Duration bug).
Other concerns
-
gte/ltebehavior change for incomparable primitives (comparison.ts, thea < b ? -1 : a > b ? 1 : 0fallback). When neither<nor>holds but values aren't truly equal, this returns0, sogte/ltenow returntruewhere the old nativea >= breturnedfalse. The concrete case isNaN: previouslygte(NaN, NaN) === false, nowtrue(sinceisUnknownfilters only null/undefined, not NaN).gt/ltare unaffected. Not covered by the tests — worth a guard or at least a test pinning the behavior. -
a.constructor.comparedispatch works in practice (tests pass ontemporal-polyfill, and native Temporal works too), but it's mildly fragile across realms / ifconstructoris shadowed. Dispatching off the already-computedSymbol.toStringTagwould be more robust and consistent with howisTemporalandareValuesEqualalready brand-check. -
Temporal-vs-non-Temporal operand (e.g.
gt(plainDate, "2024-01-15")) falls into the native-operator branch and throws an opaquevalueOfTypeError. Same as before this PR, so no regression — but if you're already producing descriptive errors here, it'd be a natural one to catch too. -
This PR is missing a changeset
The query compiler's gt/gte/lt/lte evaluators applied JavaScript's native relational operators directly. That throws TypeError on Temporal values because Temporal types intentionally make valueOf() throw — a spec-level guard against silent miscomparison. Adds a compareValues(a, b) helper that, when both operands share a Temporal type, dispatches through that type's static .compare() (Instant, PlainDate, PlainDateTime, PlainTime, PlainYearMonth, ZonedDateTime, Duration). Mixed Temporal types and types without a defined ordering (PlainMonthDay) throw a descriptive TypeError rather than fall back to a string-lex pseudo-compare, keeping us in line with Temporal's design. Non-Temporal values continue to use native operators (Dates via valueOf, numbers, strings, etc.). The gt/gte/lt/lte cases in evaluators.ts each switch from `a > b` to `compareValues(a, b) > 0` (and equivalents). Equality (eq) is unchanged: it still goes through normalizeValue's tagged toString, which means ZonedDateTime eq treats the zone as part of identity while ordering compares by instant — matching .equals() vs .compare() semantics in the spec. Tests cover all eight Temporal types, the same-instant-different-zone asymmetry for ZonedDateTime, and Duration's equivalent-forms case (PT60M vs PT1H — equal under .compare() but not .equals()).
4aa5a51 to
c049f08
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds Temporal-aware comparison helpers, applies them to sorting and ChangesTemporal comparison support
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/db/tests/comparison.test.ts (1)
47-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd one direct
ascComparatorTemporal regression here too.This PR also changes the
orderBypath inpackages/db/src/utils/comparison.ts:83-85, but this file only pinscompareValuesNaN behavior. A single direct comparator case for a bug this PR fixes—e.g. same-instant/different-zoneZonedDateTimeorPT60MvsPT1H—would keep theascComparatorbranch covered independently of the evaluator tests. As per coding guidelines,**/*.test.{ts,tsx,js}: Always add unit tests that reproduce a bug before fixing it to ensure the bug is fixed and prevent regression.Example coverage to add
import { describe, expect, it } from 'vitest' +import { Temporal } from 'temporal-polyfill' import { ascComparator, compareValues, defaultComparator, } from '../src/utils/comparison' @@ describe(`compareValues - NaN behavior`, () => { // NaN satisfies neither < nor >, so the fallback returns 0. In practice // gt/gte/lt/lte catch NaN via isUnorderable before reaching compareValues. it(`treats NaN as equal to NaN`, () => { expect(compareValues(NaN, NaN)).toBe(0) }) @@ it(`returns 0 for NaN vs a finite number — neither < nor > holds for NaN`, () => { expect(compareValues(NaN, 5)).toBe(0) expect(compareValues(5, NaN)).toBe(0) }) }) + +describe(`ascComparator - Temporal behavior`, () => { + const opts = DEFAULT_COMPARE_OPTIONS + + it(`treats same-instant ZonedDateTimes in different zones as equal for ordering`, () => { + const nyNoon = Temporal.ZonedDateTime.from(`2024-01-15T12:00:00[America/New_York]`) + const inTokyo = nyNoon.withTimeZone(`Asia/Tokyo`) + + expect(ascComparator(nyNoon, inTokyo, opts)).toBe(0) + }) +})🤖 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/comparison.test.ts` around lines 47 - 58, Add a direct regression test for the ascComparator branch in comparison.test.ts, since this suite currently only covers compareValues NaN behavior. Use a Temporal case that exercises the orderBy path in comparison.ts, such as two ZonedDateTime values representing the same instant in different zones or PT60M versus PT1H, and assert the comparator ordering directly so this bug is covered without relying only on evaluator tests.Source: Coding guidelines
packages/db/src/utils/comparison.ts (1)
256-293: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
unknownfor the new exported helper parameters.These helpers are a public comparison choke point;
unknownkeeps the API safer without changing runtime behavior, and the existing checks can narrow before comparing.🤖 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/comparison.ts` around lines 256 - 293, The exported comparison helpers in compareTemporalValues and compareValues currently use any for their parameters, which weakens the public API. Change the parameter types to unknown and keep the existing narrowing logic in compareValues and compareTemporalValues so runtime behavior stays the same while the type surface is safer.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/utils/comparison.ts`:
- Around line 256-293: The exported comparison helpers in compareTemporalValues
and compareValues currently use any for their parameters, which weakens the
public API. Change the parameter types to unknown and keep the existing
narrowing logic in compareValues and compareTemporalValues so runtime behavior
stays the same while the type surface is safer.
In `@packages/db/tests/comparison.test.ts`:
- Around line 47-58: Add a direct regression test for the ascComparator branch
in comparison.test.ts, since this suite currently only covers compareValues NaN
behavior. Use a Temporal case that exercises the orderBy path in comparison.ts,
such as two ZonedDateTime values representing the same instant in different
zones or PT60M versus PT1H, and assert the comparator ordering directly so this
bug is covered without relying only on evaluator tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8340d19e-902c-48a4-935d-ff795f6c145b
📒 Files selected for processing (5)
.changeset/temporal-comparators.mdpackages/db/src/query/compiler/evaluators.tspackages/db/src/utils/comparison.tspackages/db/tests/comparison.test.tspackages/db/tests/query/compiler/evaluators.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/db/tests/comparison.test.ts (1)
48-79: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing negative-path tests for the TypeError behavior.
Per the PR objectives, mixed Temporal types and unorderable types (e.g.
PlainMonthDay) should throw a descriptiveTypeErrorfrom the comparison helpers instead of falling back to string comparison — this is the actual bug being fixed. This file only tests successful ordering (PlainDate,ZonedDateTime) andNaNfallback behavior; it doesn't verify the throw path directly oncompareValues/ascComparator.Since this is the direct unit-test file for these helpers, it's the natural place to assert the throwing behavior (e.g.
expect(() => compareValues(plainDate, zonedDateTime)).toThrow(TypeError)and similarly for twoPlainMonthDayinstances).As per coding guidelines,
**/*.test.{ts,tsx,js}: "Always add unit tests that reproduce a bug before fixing it to ensure the bug is fixed and prevent regression."✅ Suggested additional tests
describe(`compareValues - NaN behavior`, () => { // NaN satisfies neither < nor >, so the fallback returns 0. In practice // gt/gte/lt/lte catch NaN via isUnorderable before reaching compareValues. it(`treats NaN as equal to NaN`, () => { expect(compareValues(NaN, NaN)).toBe(0) }) it(`returns 0 for NaN vs a finite number — neither < nor > holds for NaN`, () => { expect(compareValues(NaN, 5)).toBe(0) expect(compareValues(5, NaN)).toBe(0) }) }) + +describe(`compareValues - unorderable Temporal values`, () => { + it(`throws when comparing mixed Temporal types`, () => { + const date = Temporal.PlainDate.from(`2024-01-01`) + const dateTime = Temporal.PlainDateTime.from(`2024-01-01T00:00:00`) + expect(() => compareValues(date, dateTime)).toThrow(TypeError) + }) + + it(`throws when comparing PlainMonthDay values, which lack a total ordering`, () => { + const a = Temporal.PlainMonthDay.from(`01-01`) + const b = Temporal.PlainMonthDay.from(`02-02`) + expect(() => compareValues(a, b)).toThrow(TypeError) + }) +})🤖 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/comparison.test.ts` around lines 48 - 79, Add negative-path unit tests in comparison.test.ts for the comparison helpers’ TypeError behavior. Use compareValues and ascComparator to assert that mixed Temporal types and unorderable Temporal values (such as PlainMonthDay) throw a descriptive TypeError instead of falling back to string comparison. Keep the existing PlainDate, ZonedDateTime, and NaN success cases, and add explicit expect(() => ...).toThrow(TypeError) coverage for the buggy paths.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/comparison.test.ts`:
- Around line 48-79: Add negative-path unit tests in comparison.test.ts for the
comparison helpers’ TypeError behavior. Use compareValues and ascComparator to
assert that mixed Temporal types and unorderable Temporal values (such as
PlainMonthDay) throw a descriptive TypeError instead of falling back to string
comparison. Keep the existing PlainDate, ZonedDateTime, and NaN success cases,
and add explicit expect(() => ...).toThrow(TypeError) coverage for the buggy
paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a8000d08-afb3-408d-a810-3238518aa7ee
📒 Files selected for processing (2)
packages/db/src/utils/comparison.tspackages/db/tests/comparison.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/db/src/utils/comparison.ts
|
@kevin-dp Thanks! I've made those changes so would appreciate another look. |
2 participants
🎯 Changes
Fixes a
TypeErrorwhen query filters use comparison operators (gt/gte/lt/lte) on Temporal values. The evaluators applied JS's native>/<directly, which throws on Temporal types because theirvalueOf()is designed to throw — a guard against silent miscomparison.Adds a
compareValues(a, b)helper that dispatches to the Temporal types' static.compare()when both operands share a type. Mixed Temporal types and types with no defined ordering (PlainMonthDay) throw a descriptiveTypeErrorrather than fall back to string-lex comparison. Non-Temporal values continue to use native operators.eqis unchanged — it still goes throughnormalizeValue, soZonedDateTimeequality treats the zone as part of identity while ordering compares by instant only, mirroring.equals()vs.compare()in the spec.Tests cover all eight Temporal types, the same-instant/different-zone asymmetry for
ZonedDateTime, andDuration's equivalent-forms case (PT60MvsPT1H: equal under.compare()but not.equals()).✅ Checklist
pnpm test.🚀 Release Impact
Summary by CodeRabbit
gt,gte,lt,lte, andorderByto use Temporal-native ordering semantics for consistent filtering and sorting (includingDurationandZonedDateTimeequivalence by value/instant).TypeErrorfor unorderable Temporal types (e.g.,PlainMonthDay) and for mixed Temporal types.NaN.