fix: preserve create index transaction semantics#6204
Conversation
PR Review: fix: preserve create index transaction semanticsThis PR fixes a real correctness issue: the old name-based filtering in Summary of changes
FeedbackThe design is sound and the fix is correct. Two minor notes:
No P0/P1 issues found. LGTM. |
| async fn execute(mut self) -> Result<IndexMetadata> { | ||
| let new_idx = self.execute_uncommitted().await?; | ||
| let index_uuid = new_idx.uuid; | ||
| let removed_indices = if self.replace { |
There was a problem hiding this comment.
does this mean if we need to manually commit a CreateIndex transaction, we need to check replace everywhere?
this seems a breaking change, are we replying on this somewhere (distributed indexing?)?
There was a problem hiding this comment.
Yes. With this change, Operation::CreateIndex becomes an explicit add/remove UUID set operation instead of relying on implicit name-based replacement during manifest merge.
That means callers which want replace semantics need to populate removed_indices themselves. The additive/manual commit paths (including distributed indexing’s commit_existing_index(...)) are not relying on the old behavior. They are registering already-built indices and should continue to use removed_indices: vec![].
| please specify a different name" | ||
| ))); | ||
| } | ||
| let existing_named_indices = indices |
There was a problem hiding this comment.
There was a problem hiding this comment.
Not exactly. These two checks happen at different stages and serve different purposes.
It's possible to have a helper function to make it more clear, do you want it?
There was a problem hiding this comment.
yeah, the two implementations are diff already, maybe we don't need to check fields as we don't allow indices with the same name but diff fields. Good to have a helper function to get consistent behavior
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
westonpace
left a comment
westonpace
left a comment
There was a problem hiding this comment.
One question but the gist of it seems ok
| if !existing_named_indices.is_empty() && !self.replace { | ||
| return Err(Error::index(format!( | ||
| "Index name '{index_name}' already exists, \ | ||
| please specify a different name or use replace=True" | ||
| ))); |
There was a problem hiding this comment.
Does this mean I will need replace=true to add a uuid to an existing index?
There was a problem hiding this comment.
In fact, our current API didn't support adding a UUID to an existing index, it's our next step. We'll need a new public API for that.
|
distance::dot::tests::test_dot_f32's failure is not related. |
3 participants
This fixes the transaction semantics behind
CreateIndexso we can precisely add and remove index UUIDs without relying on implicit name-based replacement.It also closes the rebase correctness hole where concurrent same-name
CreateIndexcommits could survive with staleremoved_indicesand leave overlapping entries in the manifest. This keeps the existing uniqueness andreplace=truecontract intact while unblocking follow-up multi-segment index work.This PR is part of #6180