fix: port two index fixes from nitrite-java - #26
Conversation
- A unique index no longer rejects a document over a key that document already holds. add_nitrite_ids treated any existing id under the key as a violation, so it counted the writer's own id against it: a unique index over an array field with a repeated element (["a", "b", "a"]) collided with the entry it had just written, and so did an index rebuild or a replayed write. Another document under the key is still a violation. (nitrite/nitrite-java#1295) - An update that leaves an indexed value unchanged no longer rewrites the index. "Affected" only meant the update carried the field, and an upsert that writes the whole document back carries every indexed field with its old value, so every index was rebuilt on every update for nothing. A dirty index is still rebuilt. (nitrite/nitrite-java#1297) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe PR updates unique-index validation to allow repeated keys from the same document and adds index-entry short-circuiting when indexed values are unchanged and the index is clean. Tests and changelog entries document both fixes. ChangesIndex correctness
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This change permits same-document unique-key rewrites while preserving cross-document uniqueness, and skips clean-index rewrites only when indexed values are unchanged. No current merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 57.14% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 2 files. (1 skipped: 1 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #26 +/- ##
=======================================
Coverage 93.79% 93.80%
=======================================
Files 156 156
Lines 73939 73974 +35
=======================================
+ Hits 69351 69388 +37
+ Misses 4588 4586 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
Two fixes found while reviewing the open PRs in
nitrite/nitrite-java, both of which turned out to be present here too.A unique index rejected a document over a key that document already holds
SimpleIndexInner::add_nitrite_idstreated any existing id under the key as a violation:so it counted the writer's own id against it. That bites:
["a", "b", "a"]visitsatwice throughfor_each_element, and the second visit collided with the entry the first had just written;The check now compares against the writer's own id: another document under the key is still a violation. From nitrite/nitrite-java#1295.
test_simple_index_add_index_element_unique_violationhad encoded the bug — it wrote the sameFieldValuestwice and expected an error. It now uses two documents, which is the case it meant to cover, and a new test pins the same-document rewrite.An update that left an indexed value unchanged rewrote the index anyway
update_index_entrytreated an index as affected whenever the update document carried the indexed field, and then removed and rewrote the entry. An update that writes the whole document back — the common upsert shape — carries every indexed field with its old value, so every index was rebuilt on every update for nothing.The old and new values are now compared (
Valuecompares structurally, so arrays and embedded documents count as equal when their contents are) and the index is left alone when they match. A dirty index is not skipped: its rebuild still has to happen on the first write. From nitrite/nitrite-java#1297.Not ported, and why
IndexedStream::nextcontinues pastOk(None), and the stream carriesDocument, not(id, Document), so there is no null row for a filter to dereference.MapMeta::frombuilds a freshHashSetand copies each name in.Numbers.comparewithout BigDecimal) — no equivalent; there is no decimal boxing on the comparison path.IndexManagercleared only the map named inIndexMetaand missed the other layout maps) — cannot happen here, since an index lives underderive_index_map_namewhichever layout it uses, and that is the nameIndexMetarecords.find_by_filterreturnsVec<NitriteId>, sofind(k = v).next()still materialises every match. Left out of this PR because it changes theNitriteIndexertrait, which is public API in a 1.0 crate, and deserves its own change.Tests
cargo test --workspacegreen: 2268 innitrite, all crates pass.cargo clippy --workspace --all-targetsclean.🤖 Generated with Claude Code
Summary by CodeRabbit