Conversation
github-actions
Bot
added
A-python
`JsonIndexDetails` carried only a path and the target index's type identity, so the persisted details were not sufficient to rebuild an equivalent JSON index. A JSON value has no fixed type, and when the type is not recorded the wrapper infers it by reading the type tag of the first non-null value at the path, falling back to Utf8 when every value is null. That makes a rebuild driven purely by details non-deterministic with respect to the rows it happens to see: a rebuild over a different row set, or a build split across workers each training over disjoint fragments, can infer different types with nothing to reconcile them. Add `JsonIndexDetails.target_data_type`, a `JsonTargetDataType` enum whose variants are the image of the JSONB type tags, so it covers every JSON value type. `JSON_TARGET_DATA_TYPE_UNSPECIFIED` means the type was not recorded and must be inferred as before, so existing indices load, query and compact unchanged. The field carries a normative requirement: an implementation that rewrites these details without retraining the target index must copy the value through unchanged, including a value it does not recognize, so that a variant added by a later revision is not erased by an older reader that touches the index. `JsonIndex` carries the recorded value, parsed from the details at load, and `remap` and `update` copy it forward rather than re-deriving it: `ScalarIndex::training_data_type` is implemented only by BTree, so re-deriving would drop the type for every other target on every compaction, which is the case the issue is about. It is held as the raw enum value so a variant added by a later build survives a compaction performed by this one. Details written before the field existed are upgraded in passing where the type can be recovered: always on the `update` path, which already requires the target to report the type it was trained on, and for BTree targets on the `remap` path. `details_as_json` reports the type, and so does `list_indices`. `JsonIndex::training_data_type` is deliberately unchanged, so `update` on a non-BTree target still fails as before: that guard is load-bearing, since `BitmapIndex::update` takes its value type from the incoming stream and would merge mismatched keys rather than erroring. Closes lance-format#9256 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1250911 to
2594ec3
Compare
|
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 — 0 of 3 required approvals
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}
There was a problem hiding this comment.
❌ Gate recommendation: request changes.
The format contract addresses a real rebuild-determinism gap, but this proposal needs to follow the repository format-vote boundary: keep this PR to the protobuf specification and documentation plus only the generated-API adjustments needed to compile, then put runtime propagation and behavior tests in a follow-up PR. This leaves PMC voters a focused durable contract and lets the implementation receive ordinary code review independently.
| /// or `update` performed by this build instead of being erased. | ||
| /// | ||
| /// `Unspecified` for an index built before the field existed. | ||
| recorded_target_data_type: i32, |
There was a problem hiding this comment.
The repository format-change process requires a format proposal to contain only protobuf/specification changes plus the minimum library adjustments needed to keep the build green; reader, writer, and behavior tests belong in follow-up PRs. Starting the runtime state here expands the proposal to 668 changed lines and couples the 72-hour format vote to an ordinary implementation review, so this revision does not satisfy that acceptance boundary. Please move recorded_target_data_type, its load/remap/update/training behavior, and the Rust/Python behavior tests to a follow-up; keep only the new enum/details field, format docs, and minimal JsonIndexDetails constructor defaults needed to compile in this PR.
lance-gatekeeper
Bot
added
the
K-changes
|
Superseded: splitting this into a stack so the format contract and the implementation are reviewed separately, per the gatekeeper's note and |
|
Replaced by the stack:
Both are on |
1 participant
A JSON value has no fixed type, so a JSON index picks one type for the path it indexes and trains its target index on that.
JsonIndexDetailsdid not record which type it picked, so the details were not enough to rebuild an equivalent index. Without it, a rebuild re-infers the type from the first non-null value it sees, falling back to string when everything is null — so a rebuild over different rows, or a build split across workers each training over different fragments, can end up with different types and nothing to reconcile them.This adds
JsonIndexDetails.target_data_type, an enum covering every JSON value type.JSON_TARGET_DATA_TYPE_UNSPECIFIEDmeans the index was built before the field existed, and those indices keep loading, querying and compacting exactly as before.The recorded type is read back when an index is loaded and copied forward when it is compacted or updated, rather than asked of the target index each time — only BTree targets can answer that, so re-deriving would keep losing the type for bitmap and every other target on every compaction.
list_indicesnow reports the type.Not included
No API for building an index from details — lance has no such entry point for scalar indices, and adding one is a larger design question. #4628 was considered and left out; that reasoning, and the encodings rejected for this field, are in the decision record.
protos/AGENTS.mdasks for a spec-only PR with the implementation deferred. This one deliberately carries both; voters who want only the contract should readprotos/index.protoanddocs/src/format/index/scalar/json.md.uv run make lintwas not run:uv syncbuilds the pylance extension, which needs a localpython/Cargo.tomledit in this worktree.ruffandruff-formatwere run over the diff via pre-commit instead.Fixes #9256
🤖 Generated with Claude Code