refactor!: rename FMIndexIndexDetails to FMIndexDetails#7397
Open
westonpace wants to merge 3 commits into
Open
refactor!: rename FMIndexIndexDetails to FMIndexDetails#7397westonpace wants to merge 3 commits into
westonpace wants to merge 3 commits into
Conversation
…case The proto message was misnamed with a double "Index", causing get_plugin_name_from_details_name to need a special case to map "fmindex" back to "fm". With the corrected name FMIndexDetails, stripping "IndexDetails" yields "fm" directly like all other plugins. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
Important This PR touches the Lance format specification. Substantive changes to the format specification — the If this is a meaningful format change:
|
github-actions
Bot
added
A-index
Contributor
|
ACTION NEEDED The PR title and description are used as the merge commit message. Please update your PR title and description to match the specification. For details on the error please inspect the "PR Title Check" action. |
westonpace
changed the title
westonpace
changed the title
Member
Author
|
CC @beinan |
westonpace
changed the title
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
After renaming FMIndexIndexDetails to FMIndexDetails in the proto, segment_has_fmindex_details was still matching the old suffix, causing merge_existing_index_segments to fail. Also add "FM" to the legacy type name mapping since type_name_from_uri now strips IndexDetails from FMIndexDetails to yield "FM" instead of "FMIndex". Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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
1 participant
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.
I'm not sure if the original naming was intentional or not. However, it required a special case in
get_plugin_name_from_details_nameto renamefmindextofmso I'm guessing it was accidental?We are trying to convert indexes to be "generic plugins" and this means we cannot have special cases lying around.
An index's short-name is the name of the type URL minus the suffix
IndexDetails. So if we wantfmthen it should beFmIndexDetails(which this PR implements). If we wantfmindexthen it should beFmIndexIndexDetails(which it had before). If we want both then we should invent some kind of formal alias mechanism where an index plugin can register potential aliases. However, I think it'd be simplest to avoid that.This change would be a breaking change to any existing FM indexes! That index type has not (AFAIK) been formally released yet so I think this is ok. However, if this misses the 8.0.0 release then we will probably need to find a different way (and possibly forever be locked into carrying around this special case).