fix: name the index on uncommitted segment builds - #793
Conversation
Both distributed segment builders created their uncommitted segment without
setting an index name, leaving Lance to derive its own default of
`{column}_idx`. Core runs a name-collision pre-check before it does any build
work, and it consults `replace` even on the uncommitted path, so every executor
task failed with "Index name '{column}_idx' already exists" whenever an index by
that name was already on the column. `{column}_idx` is exactly the name Lance
assigns an index created without one, which makes it a name users hit easily:
`CREATE INDEX id_idx USING zonemap (id)` worked once and then failed on every
task when re-run.
Name each segment after the logical index it will join and set `replace`. On the
uncommitted path `replace` feeds nothing but that pre-check -- the removal logic
sits in core's execute(), which the uncommitted path never reaches -- so setting
it changes nothing beyond suppressing the false collision. The driver's single
commitExistingIndexSegments transaction still decides which existing segments
survive. Naming the segment also moves the genuine "already exists with
different fields" check onto the real name, so it now fails before the
distributed build rather than after it.
![lance-gatekeeper[bot]](https://avatars.githubusercontent.com/in/4433141?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The change fixes repeated creation under Lance’s derived default name at the correct boundary. Both distributed builders now stage segments under the requested logical name while the driver remains the sole atomic publication and replacement point; focused regression coverage passes for the scalar-fragment and range-BTree paths.
lance-gatekeeper
Bot
added
the
K-approved
lance-gatekeeper
Bot
added
K-approved
lance-gatekeeper
Bot
removed
the
K-approved
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The follow-up only trims comments; the verified implementation and regression coverage are unchanged. Both distributed builders still stage named, replace-flagged segments without publishing them, and the driver remains the sole atomic replacement point.
lance-gatekeeper
Bot
added
the
K-approved
geruh
left a comment
geruh
left a comment
There was a problem hiding this comment.
Thanks @ivscheianu, tested locally, and logic looks good! Just left a few nits lmk!
| // pre-sorted data. No index name or UUID is set: Lance generates the | ||
| // segment UUID, and the fragment ids declare the segment's coverage so | ||
| // the per-partition segments stay disjoint. | ||
| // pre-sorted data. No UUID is set: Lance generates the segment UUID, and |
There was a problem hiding this comment.
nit: can we clean this comment up while we are here we don't have to leave the story there. wdyt just something like:
// replace is for Lance's name check, and the driver commit still publishes.
|
|
||
| /** | ||
| * A task to create a scalar index segment on a batch of fragments. | ||
| * A task to create a scalar index segment on a batch of fragments. Named after the logical index |
There was a problem hiding this comment.
nit: we can drop the change here. the comment is enough for me to follow
2 participants
Part of #789.
What
Both uncommitted segment builders set
.replace(false)and never call.withIndexName(...):ScalarSegmentIndexTask.execute()RangeBTreeIndexBuilder.buildForFragmentGroup()With no name, Lance derives its own default
{column}_idxand then runs a name-collision pre-check before any build work, even on the uncommitted path (rust/lance/src/index/create.rs,if !existing_named_indices.is_empty() && !self.replace). So whenever an index named{column}_idxalready exists on that column, every executor task fails with:{column}_idxis the name Lance itself assigns an unnamed index, so it is a common one to hit. Re-runningCREATE INDEX id_idx USING zonemap (id)fails today.Change
Name the segment after the index it will join, and set
replace. On the uncommitted pathreplaceis consulted only by that pre-check — the removal logic lives inexecute(), which the uncommitted path bypasses — so this is inert apart from suppressing a collision that does not apply. The driver's singlecommitExistingIndexSegmentstransaction still decides which existing segments to keep.Naming the segment explicitly has a second benefit: the genuine "already exists with different fields" check now runs against the real index name, so a
CREATE INDEX name_idx USING btree (id)against an existingname_idxon a different column fails before the distributed build rather than after it.