Skip to content

fix(index): keep the requested index name after a failed uncommitted build - #9011

Open
LuciferYang wants to merge 1 commit into
lance-format:mainfrom
LuciferYang:fix/ivf-rq-uncommitted-name-preserved
Open

fix(index): keep the requested index name after a failed uncommitted build#9011
LuciferYang wants to merge 1 commit into
lance-format:mainfrom
LuciferYang:fix/ivf-rq-uncommitted-name-preserved

Conversation

@LuciferYang

Copy link
Copy Markdown
Contributor

Problem

execute_uncommitted takes the name out of the builder before it rejects a duplicate, so a build that fails there leaves self.name empty. A caller that holds the builder and retries then falls into default-name generation and commits under <column>_idx instead of the name it asked for. The method is pub on &mut self, so holding it across a retry is a supported shape.

Fixes #9010.

What this changes

Read the name without consuming it. The same take() in execute_multi_segment_fmindex is unobservable, since its only caller consumes the builder, but it is changed for consistency.

One behaviour change worth naming: a second execute_uncommitted on a builder whose first call already committed under that name now returns Index name 'X' already exists where before it quietly succeeded under <column>_idx. Nothing in the repo does that, and silently renaming the caller's index is the bug being fixed, but it is a new error on a path that used to succeed.

Test plan

test_failed_execute_uncommitted_preserves_name builds retry_idx, then runs execute_uncommitted on a held builder against the same name so it fails on the duplicate, and asserts the name survived. It then does the retry a caller would do and asserts the dataset still has exactly one index.

Restoring the take() fails both assertions independently: the field reads None, and with that assertion removed the retry commits a second index, so the last check reports ["retry_idx", "text_idx"] against ["retry_idx"].

  • cargo test -p lance --lib 3381 passed, 3 ignored
  • cargo test -p lance --lib index::create:: 43 passed
  • cargo clippy --all --tests --benches -- -D warnings clean
  • cargo fmt --all --check clean

…build

execute_uncommitted takes the name out of the builder before it rejects a duplicate, so a build that fails there leaves self.name empty. A caller that holds the builder and retries after a transient failure then falls into default-name generation and commits under <column>_idx instead of the name it asked for.

Read the name without consuming it. The same take() in execute_multi_segment_fmindex is unobservable, since its only caller consumes the builder, but it is changed for consistency.
@github-actions github-actions Bot added the bug Something isn't working label Sep 4, 2026

@lance-gatekeeper lance-gatekeeper Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gate recommendation: approve.

The change fixes the builder-state leak at its source: a failed uncommitted build retains the explicitly requested index name, so a retry cannot silently fall back to a generated name. Copying the small name value before fallible work also keeps the error paths simple and does not alter index storage.

@lance-gatekeeper lance-gatekeeper Bot added the K-approved Latest Gatekeeper recommendation permits acceptance. label Sep 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working K-approved Latest Gatekeeper recommendation permits acceptance.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: a failed execute_uncommitted consumes the requested index name

1 participant