feat(format): specify carried-column storage and allow carrying a keyed column - #8856
Conversation
|
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 |
fb3db9d to
201a9e1
Compare
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
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
c501e2e to
29c957e
Compare
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
There was a problem hiding this comment.
My concerns have been addressed, thanks!
#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.