-
Notifications
You must be signed in to change notification settings - Fork 826
docs: correct stable row ID migration and storage #8852
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
ad36407
6682e7d
01197a5
5fe6f68
c562b59
e2a07d0
ade6899
ba4c908
6cd4cd1
efdb715
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -372,23 +372,25 @@ message DataFragment { | |
| // That is, if a fragment has 3 rows, and the row ids are [1, 42, 3], then the | ||
| // first row is row 1, the second row is row 42, and the third row is row 3. | ||
| oneof row_id_sequence { | ||
| // If small (< 200KB), the row ids are stored inline. | ||
| // Current Lance writers store row ids inline regardless of encoded size. | ||
| bytes inline_row_ids = 5; | ||
| // Otherwise, stored as part of a file. | ||
| // Supported by current Lance readers, but not emitted by current Lance writers. | ||
| ExternalFile external_row_ids = 6; | ||
| } // row_id_sequence | ||
|
|
||
| oneof last_updated_at_version_sequence { | ||
| // If small (< 200KB), the row latest updated versions are stored inline. | ||
| // Current Lance writers store last-updated versions inline regardless of encoded size. | ||
| bytes inline_last_updated_at_versions = 7; | ||
| // Otherwise, stored as part of a file. | ||
| // Valid external alternative. Current Lance writers do not emit this field, | ||
| // and current Lance readers cannot load it. | ||
| ExternalFile external_last_updated_at_versions = 8; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fields 8 and 10 cannot be reclassified as reserved. Commit 85d44b6 introduced them as valid external alternatives, and that commit is an ancestor of the released v10.0.0 format; these fields are governed by stable feature flag bit 2, not an unstable flag. This wording makes a previously conforming external-version manifest forbidden, violating the stable persisted-format contract. The current RowDatasetVersionMeta::load_sequence TODO is an implementation gap, not permission to retreat from that contract. Keep both external alternatives valid while documenting that built-in writers choose inline storage and current Lance readers lack support; compatible reader support can follow separately.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in 6682e7d: external version references remain valid wire alternatives, while the documentation now distinguishes current inline writer behavior and unsupported reader loading from format validity.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in 6682e7ddc: fields 8 and 10 remain valid external wire alternatives, while the comments now distinguish that contract from current writer and reader limitations. |
||
| } // last_updated_at_version_sequence | ||
|
|
||
| oneof created_at_version_sequence { | ||
| // If small (< 200KB), the row created at versions are stored inline. | ||
| // Current Lance writers store created-at versions inline regardless of encoded size. | ||
| bytes inline_created_at_versions = 9; | ||
| // Otherwise, stored as part of a file. | ||
| // Valid external alternative. Current Lance writers do not emit this field, | ||
| // and current Lance readers cannot load it. | ||
| ExternalFile external_created_at_versions = 10; | ||
| } // created_at_version_sequence | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Index creation must be quiesced too. An index build started on the pre-migration snapshot stores physical row addresses. If migration commits first, CreateIndex explicitly rebases over a non-MemWAL Merge, so that stale index can attach to the stable-ID manifest; searches then treat address values as stable IDs and silently miss rows. The current wording allows this because an index build is not a data-modifying writer. Require no in-flight index builds or commits during migration and state that every index entry must be absent; the implementation follow-up should make migration activation conflict with CreateIndex in either order.
Reproducer run on ad36407
I added this regression to dataset_migrations.rs and ran it against the observed head:
Command:
Expected one row; observed zero, so the assertion failed with left 0 and right 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 6682e7d: migration documentation now requires stopping index builds and commits, removing every index entry, and keeping index creation quiesced until migration completes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 6682e7ddc: the migration procedure now stops index builds and commits, requires every secondary-index entry to be removed, and keeps index creation quiesced through activation, excluding the reproduced stale-index rebase.