Skip to content

feat(format): separate index keys from covering fields - #9159

Open
Ali2Arslan wants to merge 1 commit into
lance-format:mainfrom
Ali2Arslan:feat/independent-covering-fields
Open

feat(format): separate index keys from covering fields#9159
Ali2Arslan wants to merge 1 commit into
lance-format:mainfrom
Ali2Arslan:feat/independent-covering-fields

Conversation

@Ali2Arslan

@Ali2Arslan Ali2Arslan commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

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
  • update index lifecycle, overlay-staleness, and feature-flag specifications to use the independent declarations

Compatibility

The trailing-suffix contract added in #8535 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 suffix of fields.

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 bit 9 and bit 10 reservations currently proposed in #9119 and #9136.

This PR intentionally reserves and specifies the contract without implementing readers or writers, following the format-change process. It isolates the metadata question raised in #8856 so physical storage and query behavior can layer on the approved 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

@github-actions github-actions Bot added A-format On-disk format: protos and format spec docs format-change A change to the format spec, which requires a vote. Remove if minor (e.g. fixing typo). labels Sep 11, 2026
@Ali2Arslan
Ali2Arslan marked this pull request as ready for review September 11, 2026 19:27
@github-actions github-actions Bot added the enhancement New feature or request label Sep 11, 2026
@github-actions

github-actions Bot commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Important

Format specification vote

This 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 @wjones127

Approvals (this commit) none (0/3)
Vetoes @wjones127
Voting period ends Wed 2026-09-16 19:27 UTC (12:27 PDT)

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 format-waived label to waive the vote for a trivial edit (typo, wording, formatting).

@lance-gatekeeper lance-gatekeeper Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gate recommendation: request changes.

The independent declarations are the cleaner long-term contract, but activation needs a manifest-wide transition rule because the flag reinterprets every existing index entry. Define an atomic normalization (or a no-legacy-entries precondition) so an implementation cannot make legacy covered indexes look keyed on carried fields when it first sets the flag.

For backwards compatibility, a manifest with `FLAG_COVERED_INDEX_METADATA` but without
`FLAG_INDEPENDENT_COVERING_FIELDS` uses the legacy contract: `covering_fields` is a
trailing suffix of `fields`, and the preceding entries are the key fields. Readers must
select the contract from the manifest flags before interpreting any `IndexMetadata`.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FLAG_INDEPENDENT_COVERING_FIELDS changes the interpretation of every IndexMetadata in the manifest index section, including entries copied from an earlier suffix-form manifest. For example, an existing fields=[vector, payload], covering_fields=[payload] entry becomes metadata for an index keyed on both fields as soon as bit 11 is set, even though its physical index was built only for vector; a reader trusting the new declaration can consequently plan against the wrong key semantics and return incorrect results. Please make activation atomic: before setting the flag, validate and normalize every existing legacy entry to its keyed prefix in the same manifest commit (and base retries on the current index section), or require that no legacy covered entries remain. The contract should likewise retain the flag on derived manifests unless all entries are atomically converted back.

@lance-gatekeeper lance-gatekeeper Bot added the K-changes Latest Gatekeeper recommendation requests changes. label Sep 11, 2026
@Ali2Arslan Ali2Arslan changed the title feat(format): separate index keys from covering fields feat(format)!: separate index keys from covering fields Sep 11, 2026
@lance-gatekeeper lance-gatekeeper Bot added K-changes Latest Gatekeeper recommendation requests changes. and removed K-changes Latest Gatekeeper recommendation requests changes. labels Sep 11, 2026

@wjones127 wjones127 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread protos/table.proto
Comment on lines +137 to +142
* * 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@lance-gatekeeper lance-gatekeeper Bot added K-changes Latest Gatekeeper recommendation requests changes. and removed K-changes Latest Gatekeeper recommendation requests changes. labels Sep 11, 2026
@Ali2Arslan
Ali2Arslan force-pushed the feat/independent-covering-fields branch from 0888867 to fbf5f0f Compare September 11, 2026 22:21
@lance-gatekeeper lance-gatekeeper Bot removed the K-changes Latest Gatekeeper recommendation requests changes. label Sep 11, 2026
lance-gatekeeper[bot]

This comment was marked as outdated.

@lance-gatekeeper lance-gatekeeper Bot added K-changes Latest Gatekeeper recommendation requests changes. and removed K-changes Latest Gatekeeper recommendation requests changes. labels Sep 11, 2026
@Ali2Arslan
Ali2Arslan force-pushed the feat/independent-covering-fields branch from fbf5f0f to 0888867 Compare September 11, 2026 22:55
@Ali2Arslan Ali2Arslan changed the title feat(format)!: separate index keys from covering fields feat(format): separate index keys from covering fields Sep 11, 2026
@lance-gatekeeper lance-gatekeeper Bot removed the K-changes Latest Gatekeeper recommendation requests changes. label Sep 11, 2026

@lance-gatekeeper lance-gatekeeper Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gate recommendation: request changes.

The bit-7 redefinition has been reverted, so that compatibility issue is closed. The distinct bit-11 design is the safer direction, but it still needs an atomic manifest-wide transition and retention rule before activation can preserve legacy suffix-form entries.

`covering_fields` separately contains the carried fields. The lists have no positional
relationship and may overlap. The index dependency set is their union.

For backwards compatibility, a manifest with `FLAG_COVERED_INDEX_METADATA` but without

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FLAG_INDEPENDENT_COVERING_FIELDS still reinterprets every IndexMetadata in the manifest once bit 11 is set. If a manifest already contains legacy fields=[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 to fields=[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.

@lance-gatekeeper lance-gatekeeper Bot added the K-changes Latest Gatekeeper recommendation requests changes. label Sep 11, 2026
Co-authored-by: Cursor <cursoragent@cursor.com>
@Ali2Arslan
Ali2Arslan force-pushed the feat/independent-covering-fields branch from 0888867 to f3ea036 Compare September 14, 2026 16:42
@lance-gatekeeper lance-gatekeeper Bot removed the K-changes Latest Gatekeeper recommendation requests changes. label Sep 14, 2026

@lance-gatekeeper lance-gatekeeper Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gate recommendation: request changes.

The rebase preserves the safer distinct bit-11 fence and correctly keeps it unsupported in this build. The remaining format-contract issue is unchanged: activation must atomically normalize legacy suffix-form entries (or require none), use the current index section on retry, and retain the flag on derived manifests until any reverse conversion is atomic.

@lance-gatekeeper lance-gatekeeper Bot added the K-changes Latest Gatekeeper recommendation requests changes. label Sep 14, 2026

@westonpace westonpace left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ultimately it seems like we could make either approach work correct?

  • Sometimes we will want to know just which fields are key fields.
    • For example, to test if a filter can be applied
  • Sometimes we will want to know just which fields are covering fields.
    • For example, to test which columns we don't need to take
  • Sometimes we will want to know all fields, covered or not.
    • For example, to test if a column change invalidates an index

The rationale for the current approach was that much of the commit transaction logic was the third category. By just extending fields we were able to avoid changes to that code.

However, I think it ultimately doesn't matter, as long as we can distinguish between the three cases (and I think we can in both cases).

I will admit that this approach is slightly more intuitive for those new to the spec. I'll leave it up to you @Ali2Arslan and @vivek-bharathan which approach we want. As long as both of you agree we can definitely merge this PR.

Or am I missing something and is there something we could not identify in the previous implementation?

@Ali2Arslan

Copy link
Copy Markdown
Contributor Author

Ultimately it seems like we could make either approach work correct?

  • Sometimes we will want to know just which fields are key fields.

    • For example, to test if a filter can be applied
  • Sometimes we will want to know just which fields are covering fields.

    • For example, to test which columns we don't need to take
  • Sometimes we will want to know all fields, covered or not.

    • For example, to test if a column change invalidates an index

The rationale for the current approach was that much of the commit transaction logic was the third category. By just extending fields we were able to avoid changes to that code.

However, I think it ultimately doesn't matter, as long as we can distinguish between the three cases (and I think we can in both cases).

I will admit that this approach is slightly more intuitive for those new to the spec. I'll leave it up to you @Ali2Arslan and @vivek-bharathan which approach we want. As long as both of you agree we can definitely merge this PR.

Or am I missing something and is there something we could not identify in the previous implementation?

The main issues with the current approach, from my perspective, are:

  • It allows for invalid states to be represented, and passes the responsibility to the code to enforce them. It allows you to interleave ordering b/t key and covering fields (assuming we will have multi-field keys in the future).
  • It does not specify the behavior when we want to include a field as both a key and covering field. This might be important in the future if the key encoding cannot preserve the bits of the value losslessly, for stuff like floats, collations, etc.

I don't necessairly think that we can't work around these in the implementation, but it definitely increases the complexity imho.

@lance-gatekeeper lance-gatekeeper Bot added K-changes Latest Gatekeeper recommendation requests changes. and removed K-changes Latest Gatekeeper recommendation requests changes. labels Sep 14, 2026
@vivek-bharathan

Copy link
Copy Markdown
Contributor
  • It allows for invalid states to be represented, and passes the responsibility to the code to enforce them. It allows you to interleave ordering b/t key and covering fields (assuming we will have multi-field keys in the future).

Yes - multi-field keys would require a reworking of the contract. Maybe enforce that they are the leading prefix

  • It does not specify the behavior when we want to include a field as both a key and covering field. This might be important in the future if the key encoding cannot preserve the bits of the value losslessly, for stuff like floats, collations, etc.

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 this was to simplify state tracking and the commit transaction logic. Either way we pay the cost elsewhere. I'm fine with taking this approach

@lance-gatekeeper lance-gatekeeper Bot added K-changes Latest Gatekeeper recommendation requests changes. and removed K-changes Latest Gatekeeper recommendation requests changes. labels Sep 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-format On-disk format: protos and format spec docs breaking-change enhancement New feature or request format-change A change to the format spec, which requires a vote. Remove if minor (e.g. fixing typo). K-changes Latest Gatekeeper recommendation requests changes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants