sqlitevec: use DELETE by key instead of IN for virtual table deletes - #53
Conversation
|
@dotnet-policy-service agree |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Add active vec0 integration coverage, use one transaction for per-key deletes, and bump the provider version.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (2)
What changed in this PR
Replaces IN-based vec0 deletes with reusable per-key deletes to improve deletion performance.
Changes:
- Adds a parameterized single-key delete command.
- Applies per-key deletion to delete and upsert paths.
- Adds command-builder tests and updates SourceLink.
| File | Description |
|---|---|
MEVD/test/SqliteVec.UnitTests/SqliteCommandBuilderTests.cs |
Tests generated key-based delete SQL. |
MEVD/src/SqliteVec/SqliteCommandBuilder.cs |
Builds reusable parameterized delete commands. |
MEVD/src/SqliteVec/SqliteCollection.cs |
Uses per-key vector deletion. |
Directory.Packages.props |
Updates the SourceLink package version. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
adamsitnik
left a comment
adamsitnik
left a comment
There was a problem hiding this comment.
@rossdonald big thanks for your contribution!
Benchmarks show major perf wins (with only one regression):
Please address the remaining feedback, thank you!
|
I've addressed all the changes. Thanks for the test script, Using transactions, DeleteBatch_MissingKeys is another 2x faster and UpsertBatch_ExistingRecords is ~250x faster. I can submit a PR for #54 also. |
For the sqlite vector table, replace the slow IN operator with a loop using a statement that deletes by key.
Fixes: #52