Repository navigation
test(db-ivm): pin what min and max promise after a delete - #2093
Open
KyleAMathews wants to merge 6 commits into
Open
KyleAMathews wants to merge 6 commits into
KyleAMathews wants to merge 6 commits into
Conversation
groupBy consolidates by hash, which merges -0 with 0 and equal Dates. After a delete, min and max return a hash-equal value, not necessarily the remaining instance. The query compiler keeps its own exact inputs for that reason; this test records the contract it relies on. Co-authored-by: Isaac <no-reply@databricks.com>
|
Contributor
Incremental update benchmarkComparing Overall median write time vs base: 1.01× · cold hydrate time: 1.03× (geometric mean of per-case ratios; lower is faster). Writes: 2 regression(s), 2 improvement(s) (threshold: ±20% and >0.05ms). Cold hydrate: 3 regression(s), 1 improvement(s) (threshold: ±50% and >5ms). Per-case flags are noisy on shared runners. Read the geometric means first. Writes
Cold hydrate
Each row aggregates the 14.4 scale/index/write-mode configurations of that query; per-configuration tables below. 100 rows/collection | source indexes: none | synced writes — geomean 0.97×, cold 0.93×, 1 change(s)
100 rows/collection | source indexes: none | optimistic writes — geomean 0.94×, cold 1.02×
100 rows/collection | source indexes: manual | synced writes — geomean 0.95×, cold 1.09×
100 rows/collection | source indexes: manual | optimistic writes — geomean 1.22×, cold 1.42×
1,000 rows/collection | source indexes: none | synced writes — geomean 0.99×, cold 0.87×, 2 change(s)
1,000 rows/collection | source indexes: none | optimistic writes — geomean 0.90×, cold 0.81×
1,000 rows/collection | source indexes: manual | synced writes — geomean 1.06×, cold 1.03×
1,000 rows/collection | source indexes: manual | optimistic writes — geomean 1.08×, cold 1.20×, 1 change(s)
10,000 rows/collection | source indexes: none | synced writes — geomean 0.91×, cold 1.06×, 1 change(s)
10,000 rows/collection | source indexes: none | optimistic writes — geomean 0.99×, cold 0.87×, 2 change(s)
10,000 rows/collection | source indexes: manual | synced writes — geomean 1.16×, cold 1.18×, 1 change(s)
10,000 rows/collection | source indexes: manual | optimistic writes — geomean 1.00×, cold 0.97×
Runner: node v24.8.0, linux 6.17.0-1022-azure, INTEL(R) XEON(R) PLATINUM 8573C. Timings on shared CI runners are noisy; treat small deltas as indicative only. |
Contributor
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to This PR adds tests and coverage notes for groupBy extrema after deletion; the current deleted-instance behavior is documented separately from the hash-equality contract. No actionable merge risk remains. Pre-merge checks |
 |
| Check name | Status | Explanation |
|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. (1 skipped: 1 … |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Description check | |
The description is complete and follows the repository template. It explains the changes, records the test command, and identifies the change as docs/CI/dev-only with no release impact. |
| Title check | |
The title clearly and concisely identifies the main change: documenting the promised behavior of db-ivm min and max after deletion. |
✨ Finishing Touches
-
-
-
-
📝 Generate docstrings
-
-
-
-
-
-
-
-
-
-
- Commit to this branch
- Create a new PR
-
-
-
-
-
-
-
-
-
🧪 Generate unit tests (beta)
-
-
-
-
- Commit to this branch
- Create a new PR
-
-
-
-
-
-
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 help to get the list of available commands.
Contributor
|
Size Change: 0 B Total Size: 214 kB ℹ️ View Unchanged
|
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: |
The contract test now clears its result before the delete and requires the delete to publish the group again, checks that a distinct remaining Date is returned as is, types its values, and no longer sits under the replay-witness header. The coverage map records the contract and why the query compiler keeps its own exact inputs. Co-authored-by: Isaac <no-reply@databricks.com>
The review showed the contract test could not fail on what it documented: it checked only a hash-equal bound with serializeValue. It now checks that bound with the hash the Index merges by, in both delete orders, and separately pins the current behavior that the deleted instance comes back, so a db-ivm change alerts the compiler owner. The group's row is read from the consolidated output, so republication is not part of the contract. The prose says the merge is over the whole aggregate input tuple. The compiler's exact-input comment points to the test, and the coverage map names the value-identity policy that would generalize it. Co-authored-by: Isaac <no-reply@databricks.com>
Split the contract from the pinned current behavior, read the group's row by integrating output multiplicities, check the max's hash for Dates, and show that a per-row aggregate input returns the remaining instance. Co-authored-by: Isaac <no-reply@databricks.com>
The deleted instance comes back whichever row was added first: a shared hash entry keeps the instance it was last given, and the delete's retraction is last. The pinned test now covers both orders, the prose names that mechanism and states which contract cases a wrong value can fail, and the hash import uses the hashing entry point. Co-authored-by: Isaac <no-reply@databricks.com>
…directions Mirror cases now delete the maximum, and the per-row cases check -0/0 in both directions for min and max. The contract comment states the positive multiplicity condition and what the compiler's exact-value keys separate, and the compiler comment names the contract instead of a test path. Co-authored-by: Isaac <no-reply@databricks.com>
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.
🎯 Changes
This PR is test-only. It records what db-ivm
minandmaxpromise after a delete.groupByconsolidates values by their hash. The hash treats-0as0, and it treats two Dates with the same time as one value. So after a delete,minandmaxreturn a value that is hash-equal to a remaining member, but not always the remaining instance:minandmaxreturn-0,0-0-0(the deleted value)new Date(5),new Date(5)1,212The query compiler needs the exact remaining value for group representatives (#2081). So it keeps its own exact inputs, and does not use these results. This was the recommended decision: keep that workaround in the compiler, and record the db-ivm contract in a test.
groupBy.test.tshas two new tests:-0/0and equal Dates, the contract promises only hash equality. The distinct and per-row cases are the ones that a wrong value fails.Both tests read the group's row by adding up the output multiplicities. A comment in
compiler/group-by.tspoints to these tests.✅ Checklist
pnpm test.🚀 Release Impact
This pull request and its description were written by Isaac.
Summary by CodeRabbit
minandmaxresults after a deletion are hash-equal to a remaining value, but may not be the same instance.-0and0or equal dates, can retain the deleted instance as the result.