feat(format): separate index keys from covering fields - #9159
Ali2Arslan wants to merge 1 commit into
Conversation
|
Important Format specification voteThis PR modifies the Lance format specification, so it requires 3 binding +1 votes from PMC members (excluding the proposer), at least one of them on the latest commit, and a minimum 72-hour voting period, weekends excluded, before it can merge. Vote by approving this PR (+1) or requesting changes (−1, a veto). See the voting process. Approvals carry over across pushes, so a rebase or a typo fix does not send everyone back to re-vote. Whoever approves the latest commit is vouching that nothing substantive has changed since the earlier approvals; if something has, ask for fresh votes. Status: ❌ Blocked — vetoed by @wjones127
Updated automatically by the format-spec vote gate, which re-checks every 15 minutes — just voted? Re-check now (press Run workflow; leave the input blank to re-check every open format PR). A PMC member may apply the |
wjones127
left a comment
There was a problem hiding this comment.
Need to validate my assumption, but if we can I'm fine with making a breaking change to the existing spec so we avoid having these two separate flags.
| * * 1 << 11: IndexMetadata.fields and covering_fields are independent | ||
| * declarations. This bit requires 1 << 7 in both the reader and writer | ||
| * flags. Fields contains only the columns the index is keyed on, while | ||
| * covering_fields contains the columns whose values it carries. A field may | ||
| * occur in both lists. Implementations that only support the trailing-suffix | ||
| * contract must refuse the dataset. |
There was a problem hiding this comment.
suggestion: I don't think there's any existing covering indexes implements. Right @vivek-bharathan? If that's the case, then can we just change the meaning of 1 << 7 now rather than introducing more possible states?
There was a problem hiding this comment.
It's probably easier to reuse the existing flag, at the cost of not knowing if anyone was using the public Rust/Python transaction APIs?
There was a problem hiding this comment.
Update: I rolled that revision back for now while we work through the compatibility-safe representation. The PR is restored to 0888867 with the distinct bit-11 proposal.
0888867 to
fbf5f0f
Compare
fbf5f0f to
0888867
Compare
0888867 to
f3ea036
Compare
The main issues with the current approach, from my perspective, are:
I don't necessairly think that we can't work around these in the implementation, but it definitely increases the complexity imho. |
Yes - multi-field keys would require a reworking of the contract. Maybe enforce that they are the leading prefix
This is being addressed in #8856 - particularly to support performing refine without an additional take. But that said - I agree that this is the cleaner contract. The primary reason we went with the other approach was to simplify state tracking and the commit transaction logic. Either way we pay the cost elsewhere. I'm fine with taking this approach |
|
I've merged #8856 which added (a different) concept of "this is how you specify a key column is also covered" since it already had 3 votes. Let's go ahead and rebase this PR on top of the new wording (since it seems we have alignment on this approach over the other approach). Then we can merge this PR too. @Ali2Arslan let me know if you need any help. Thanks! |
f3ea036 to
64ab4d4
Compare
Co-authored-by: Cursor <cursoragent@cursor.com>
64ab4d4 to
8e6c8f6
Compare
There was a problem hiding this comment.
✅ Gate recommendation: approve.
The activation contract now closes the remaining compatibility gap: transition is atomic, retries normalize the current index section, and bit 11 remains sticky until any reverse conversion is atomic. The distinct bit preserves released bit-7 manifests and remains unsupported until a follow-up implementation, while the rebase retains #8856ʼs carried-column storage contract.
Thanks @westonpace , I updated the wordage if you guys want to take another look. |
Summary
IndexMetadatacontract sofieldscontains key fields only andcovering_fieldsis an independent declaration that may overlap itfieldsandcovering_fieldsFLAG_INDEPENDENT_COVERING_FIELDS(bit 11 / 2048) as a paired reader/writer fence and keep it unsupported until the implementation landsCompatibility
The subset contract for covering fields (added in #8535, refined by the now-merged #8856) shipped in Lance 11.0.0, so the same protobuf bytes cannot be reinterpreted unconditionally. A manifest using the new contract must set both
FLAG_COVERED_INDEX_METADATAandFLAG_INDEPENDENT_COVERING_FIELDS. A manifest with onlyFLAG_COVERED_INDEX_METADATAretains the stable legacy interpretation, wherecovering_fieldsis a subset offieldswith no id repeated.The new bit makes older releases reject independent declarations instead of treating covering fields as keys. No protobuf field number or wire type changes. Bit 11 avoids the reserved bit 9 and the bit 10
FLAG_FRAGMENT_REUSE_INDEXfrom #9136.This PR intentionally reserves and specifies the contract without implementing readers or writers, following the format-change process. Bit 11 sits above
FLAG_UNKNOWN(1 << 9) andsupported_flags_whenstarts fromFLAG_UNKNOWN - 1, so no build can open or write a manifest that sets it;FLAG_FRAGMENT_REUSE_INDEXis in the same reserved state onmaintoday. Consequently the existingkeyed_fieldsderivation, which subtracts the carried count fromfields, can never observe an independent declaration and is left for the implementation PR. This PR layers on the carried-column storage contract from #8856 so physical storage and query behavior can follow the approved metadata contract.Testing
cargo fmt --allcargo test -p lance-table feature_flags --libcargo clippy --all --tests --benches -- -D warningsRUSTDOCFLAGS='-D warnings' cargo doc -p lance-table --no-depsuv run --project docs python ci/check_proto_comments.pycd docs && uv run mkdocs build