-
Notifications
You must be signed in to change notification settings - Fork 852
feat(format): separate index keys from covering fields #9159
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
Ali2Arslan
wants to merge
1
commit into
lance-format:main
Choose a base branch
from
Ali2Arslan:feat/independent-covering-fields
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FLAG_INDEPENDENT_COVERING_FIELDSstill reinterprets everyIndexMetadatain the manifest once bit 11 is set. If a manifest already contains legacyfields=[vector, payload], covering_fields=[payload], enabling this flag makes the reader treat the physical single-key index as keyed on both fields; query planning can then use the wrong key semantics and return incorrect results. Activation therefore needs an atomic manifest-wide normalization tofields=[vector](or an enforced no-legacy-entry precondition), using the current index section on retry, and derived manifests must retain bit 11 until any reverse conversion is likewise atomic. This is the current successor to the earlier discussion.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in 8e6c8f6. The spec now states the activation rules you asked for, in a new "Activating independent declarations" section in
docs/src/format/index/index.mdand, in condensed form, on the bit-11 entry inprotos/table.proto:fieldsthe ids that appear only incovering_fieldsand leaving the key fields in key order. A writer that cannot normalize an entry must not set the bit, and may instead set it only on a manifest where no entry declares covering fields at all. No committed manifest may set the bit while holding a legacy-form entry.The enforcement code stays out of this PR deliberately, per
protos/AGENTS.md("only the library edits needed to compile... put the implementation in a follow-up PR"). It is also unreachable today: bit 11 sits aboveFLAG_UNKNOWN(1 << 9) andsupported_flags_whenstarts fromFLAG_UNKNOWN - 1, socan_read_datasetandcan_write_datasetrefuse any manifest that sets it, whichtest_independent_covering_fields_flag_is_reservedasserts.FLAG_FRAGMENT_REUSE_INDEX(bit 10) is in the same reserved state onmaintoday.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 8e6c8f6: activation now requires an atomic manifest-wide normalization (or no covered entries), retries normalize the current index section, and derived manifests retain bit 11 until any reverse conversion is atomic.