fix(index): keep the requested index name after a failed uncommitted build - #9011
Open
LuciferYang wants to merge 1 commit into
Open
fix(index): keep the requested index name after a failed uncommitted build#9011LuciferYang wants to merge 1 commit into
LuciferYang wants to merge 1 commit into
Conversation
…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.
Contributor
There was a problem hiding this comment.
✅ 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
execute_uncommittedtakes the name out of the builder before it rejects a duplicate, so a build that fails there leavesself.nameempty. A caller that holds the builder and retries then falls into default-name generation and commits under<column>_idxinstead of the name it asked for. The method ispubon&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()inexecute_multi_segment_fmindexis unobservable, since its only caller consumes the builder, but it is changed for consistency.One behaviour change worth naming: a second
execute_uncommittedon a builder whose first call already committed under that name now returnsIndex name 'X' already existswhere 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_namebuildsretry_idx, then runsexecute_uncommittedon 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 readsNone, 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 --lib3381 passed, 3 ignoredcargo test -p lance --lib index::create::43 passedcargo clippy --all --tests --benches -- -D warningscleancargo fmt --all --checkclean