feat(format): specify carried-column storage and allow carrying a keyed column - #8856
Conversation
github-actions
Bot
added
A-format
|
Important Format specification voteThis PR modifies the Lance format specification, so it requires 3 binding +1 votes from PMC members (excluding the proposer) 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. Status: ❌ Blocked — vetoed by @westonpace
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 |
![lance-gatekeeper[bot]](https://avatars.githubusercontent.com/in/4433141?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
lance-gatekeeper
Bot
added
the
K-changes
fb3db9d to
201a9e1
Compare
lance-gatekeeper
Bot
removed
the
K-changes
lance-gatekeeper
Bot
added
the
K-approved
jackye1995
left a comment
jackye1995
left a comment
There was a problem hiding this comment.
looks good, please raise a corresponding voting thread
Happy to do that - but it looks like the process was recently changed
|
wjones127
left a comment
wjones127
left a comment
There was a problem hiding this comment.
-1 until the blocking questions is decided. Have you considered using field ids instead of string names / paths? If so, what made you reject that?
201a9e1 to
c501e2e
Compare
lance-gatekeeper
Bot
removed
the
K-approved
c501e2e to
29c957e
Compare
westonpace
left a comment
westonpace
left a comment
There was a problem hiding this comment.
This looks good but I have a few questions. Thanks for working on this!
| * presence tracking for repeated fields, and the empty list is a distinct, meaningful | ||
| * state here: | ||
| * | ||
| * * absent: no narrowing computed; materialize every covering column declared. |
There was a problem hiding this comment.
Is this true? Wouldn't the default be to materialize nothing?
There was a problem hiding this comment.
Absent means nobody computed a projection -- either an encoder older than this
field, or a planner that skipped the narrowing step. Intended to reproduce the behavior from before this field existed, when a covered index always materialized everything it declared
| * executor declares the same search output schema the planner did; without it the | ||
| * executor's node is wider than the plan it came from, and the surrounding nodes | ||
| * were built against the planner's narrower schema. |
There was a problem hiding this comment.
I understand the need for the two sides to agree on the schema before-hand but can we do this?
If the planner has to specify the covering projection, and the available fields are determined from index storage, does this mean the planner will have to load the index into memory? Isn't that something we probably want to avoid?
There was a problem hiding this comment.
That is correct. Today it reads both index files' footers and schema metadata, plus their IVF models. But it probably makes sense to build a version that excludes the IVF models. I can do that as a follow up - I do not think that changes this PR though
29c957e to
feace76
Compare
…ed column lance-format#8535 declared `IndexMetadata.covering_fields` but left the physical side unspecified; this specifies it -- carried values are extra columns in `auxiliary.idx`, discovered by exclusion against the quantizer's internal columns and bound to their dataset fields by a new `covering_field_ids` metadata key, with no `index_version` bump. It also permits an index to carry a column it is also keyed on. Such a column is listed once in `fields` and named in `covering_fields`, so `covering_fields` is a subset rather than a suffix, and readers must take the carried set from `covering_fields` rather than subtract the keyed prefix. Adds `VectorQueryProto.covering_projection` (field 15) to reserve the tag, with the one initializer the new field forces on `query_to_proto`; no writer emits carried values yet, so the implementation follows separately.
feace76 to
db9f848
Compare
|
@westonpace @wjones127 - I believe all concerns are addressed now. ptal |
westonpace
left a comment
westonpace
left a comment
There was a problem hiding this comment.
My concerns have been addressed, thanks!
## Summary - define the new `IndexMetadata` contract so `fields` contains key fields only and `covering_fields` is an independent declaration that may overlap it - define the index dependency set as the union of `fields` and `covering_fields` - reserve `FLAG_INDEPENDENT_COVERING_FIELDS` (bit 11 / 2048) as a paired reader/writer fence and keep it unsupported until the implementation lands - specify activation: setting the flag atomically normalizes every legacy-form entry of the index section in the same manifest (or requires that none declares covering fields), a retry normalizes against the index section it is retrying against, and derived manifests retain the bit - update index lifecycle, overlay-staleness, and feature-flag specifications to use the independent declarations ## Compatibility 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_METADATA` and `FLAG_INDEPENDENT_COVERING_FIELDS`. A manifest with only `FLAG_COVERED_INDEX_METADATA` retains the stable legacy interpretation, where `covering_fields` is a subset of `fields` with 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_INDEX` from #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`) and `supported_flags_when` starts from `FLAG_UNKNOWN - 1`, so no build can open or write a manifest that sets it; `FLAG_FRAGMENT_REUSE_INDEX` is in the same reserved state on `main` today. Consequently the existing `keyed_fields` derivation, which subtracts the carried count from `fields`, 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 --all` - `cargo test -p lance-table feature_flags --lib` - `cargo clippy --all --tests --benches -- -D warnings` - `RUSTDOCFLAGS='-D warnings' cargo doc -p lance-table --no-deps` - `uv run --project docs python ci/check_proto_comments.py` - `cd docs && uv run mkdocs build` Co-authored-by: Cursor <cursoragent@cursor.com>
4 participants
#8535 declared
IndexMetadata.covering_fieldsbut left the physical side unspecified; this specifies it -- carried values are extra columns inauxiliary.idx, discovered by exclusion against the quantizer's internal columns and bound to their source fields by a newcovering_field_idsmetadata key, with noindex_versionbump.It also permits an index to carry a column it is also keyed on, the only case where an id repeats in
fields, so readers must take the carried set fromcovering_fieldsrather than subtract the keyed prefix.Adds
VectorQueryProto.covering_projection(field 15) to reserve the tag; no writer emits carried values yet, so the implementation follows separately.