fix(libravdb): make EnsureCollection non-destructive - #4
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
Collection Dimension Mismatch API
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
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 |
|
Issue/PR cross-check before review:
Why this is worth fixing:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
libravdb/collection_management_test.go (1)
71-100: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winExercise the restart/replay path in this regression test.
This only proves the in-process recreate flow. The storage fix is in
internal/storage/singlefile/engine.go, so closing and reopening before the final assertions would guard the persisted recovery contract too.Suggested expansion
func TestEnsureCollectionRecreateOnDimensionMismatchOptIn(t *testing.T) { ctx := context.Background() - db, err := Open(WithStoragePath(testDBPath(t))) + path := testDBPath(t) + db, err := Open(WithStoragePath(path)) if err != nil { t.Fatalf("Failed to create database: %v", err) } - defer db.Close() + t.Cleanup(func() { _ = db.Close() }) @@ recreated, err := db.EnsureCollectionRecreateOnDimensionMismatch(ctx, "embeddings", 4, WithFlat()) if err != nil { t.Fatalf("EnsureCollectionRecreateOnDimensionMismatch: %v", err) } if recreated == collection { t.Fatal("Expected collection to be recreated through explicit opt-in") } - if recreated.Dimension() != 4 { - t.Fatalf("Expected recreated dimension 4, got %d", recreated.Dimension()) + if err := db.Close(); err != nil { + t.Fatalf("Close before reopen: %v", err) + } + db, err = Open(WithStoragePath(path)) + if err != nil { + t.Fatalf("Reopen after recreate: %v", err) + } + recreated, err = db.GetCollection("embeddings") + if err != nil { + t.Fatalf("GetCollection after reopen: %v", err) + } + if recreated.Dimension() != 4 { + t.Fatalf("Expected recreated dimension 4 after reopen, got %d", recreated.Dimension()) } if stats := recreated.Stats(ctx); stats.VectorCount != 0 { t.Fatalf("Expected recreated collection to start empty, got %d vectors", stats.VectorCount) } }🤖 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 `@libravdb/collection_management_test.go` around lines 71 - 100, Update TestEnsureCollectionRecreateOnDimensionMismatchOptIn to exercise the persisted restart/replay path, not just the in-process recreate flow. After calling EnsureCollectionRecreateOnDimensionMismatch and before the final dimension/vector-count assertions, close the database and reopen it with Open using the same storage path, then fetch the collection again and assert the recreated state from the reopened handle. This will validate the storage recovery behavior in internal/storage/singlefile/engine.go through the EnsureCollectionRecreateOnDimensionMismatch and Stats flow.
🤖 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 `@libravdb/collection_management_test.go`:
- Around line 71-100: Update
TestEnsureCollectionRecreateOnDimensionMismatchOptIn to exercise the persisted
restart/replay path, not just the in-process recreate flow. After calling
EnsureCollectionRecreateOnDimensionMismatch and before the final
dimension/vector-count assertions, close the database and reopen it with Open
using the same storage path, then fetch the collection again and assert the
recreated state from the reopened handle. This will validate the storage
recovery behavior in internal/storage/singlefile/engine.go through the
EnsureCollectionRecreateOnDimensionMismatch and Stats flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 667585ae-289f-445c-a5c7-6026cdd9591e
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
internal/graph/registry.gointernal/storage/singlefile/engine.golibravdb/collection_management_test.golibravdb/database.golibravdb/errors.gotests/main_test.go
xDarkicex
left a comment
xDarkicex
left a comment
There was a problem hiding this comment.
hey, really appreciate the EnsureCollection fix — making dimension mismatch non-destructive is exactly the right call and the tests look thorough.
one thing that needs a revert though: the registry.go change from uintptr to *EdgeTablePage. that pointer comes from off-heap mmap allocated memory (ShardedFreeList), see store.go line 232:
page = (*EdgeTablePage)(unsafe.Pointer(&slotBytes[64]))storing it as *EdgeTablePage in a Go map tells the GC "this is a real Go pointer, trace it". but the memory backing it is mmap off-heap, managed by the memory package. if the GC concurrent sweep touches that region after Retire has reclaimed it and the backing pages get unmapped, you get a SIGSEGV with zero useful stack trace. uintptr is invisible to the GC — that was the whole reason it was used, not as a workaround for the vet check.
the Go spec is pretty explicit about this — uintptr conversions are valid but the result is not a tracked pointer. the original code was correct. i would revert the registry.go file entirely back to the original uintptr approach and keep everything else as-is.
rest of the PR looks solid, thanks for catching the dimension mismatch footgun.
2 participants
Description
Refactors
EnsureCollectionso an existing collection with an incompatible vector dimension is no longer dropped and recreated by default. Instead, callers get a typedCollectionDimensionMismatchErrorwrappingErrDimensionMismatch, preserving the existing collection and its data.Also fixes the local
go vet ./...blocker by storing typed*EdgeTablePagevalues in the graph page registry instead of round-tripping Go pointers throughuintptr.Related Issue
Fixes #5.
Type of Change
Changes Made
Testing
Commands run:
go build -v $(go list ./... | grep -v '/examples\|/benchmark\|/tests')go vet ./...golangci-lint run --timeout=5mgo test -v -short -timeout=5m -coverprofile=coverage.out $(go list ./... | grep -v '/examples\|/benchmark\|/tests')go test -v -timeout=10m -tags=integration ./tests/..../scripts/validate-examples.shgo test ./libravdb -run 'TestEnsureCollection'go test ./internal/storage/singlefilego test ./internal/graph ./libravdb -run 'TestEnsureCollection|TestBFS|TestGraph|TestEdgeTable|TestWAL|TestKindSet|TestMetrics|TestReverseIndex|TestDropNodeEdges|TestSegment|TestManifest'Note:
./scripts/lint.shnow passes formatting, goimports, andgo vet; its final repository-wide heuristic check still exits non-zero on pre-existing broad warnings: one TODO, ignored-error patterns, and manyfmt.Sprintfcalls. The CI lint command,golangci-lint run --timeout=5m, passes locally.Performance Impact
No expected performance impact. This changes the mismatch control path for collection setup and removes unsafe pointer storage from the graph registry.
Breaking Changes
This intentionally prevents the previous silent destructive behavior in
EnsureCollection. Code that relied on automatic deletion/recreation should switch toEnsureCollectionRecreateOnDimensionMismatch.Checklist
Additional Notes
EnsureCollectionRecreateOnDimensionMismatch.Summary by CodeRabbit
New Features
Bug Fixes
Tests