-
Notifications
You must be signed in to change notification settings - Fork 833
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 79 commits
ad36407
6682e7d
01197a5
5fe6f68
c562b59
e2a07d0
ade6899
ba4c908
6cd4cd1
efdb715
ac6ccbe
7c43b53
ae0fdfe
df82305
469f4d8
fe4f1ad
709e1d0
babd27d
457e5eb
b88ba38
9ec4373
e28be4c
5e5d514
53c70fd
ecfe405
9bb5ae7
708326a
aeef6ad
edc31a4
5284aaf
fe1e986
7aa678d
82ab036
4b49bf6
789894e
42cda3e
807a2b5
40cba18
8189bdd
372e0f7
46f75c6
f02ea0c
07ea9d1
95984b2
3bde203
291ad63
4c22e18
e3de446
d4d9273
fe4cb53
7e146ce
bb3d1a6
7ad0229
528b14e
abae758
2395088
319619a
0ef2bb5
e7221d0
2541d4e
525cafb
bf0e43f
8ba74ff
58e731f
50455e3
cca243f
487e2fe
9e4f376
66f20c6
2370e3a
bc35cda
f24bfc9
0c9b64a
87d1f60
f86da2c
4592b75
42f0988
6f82716
645e1f0
b3f7e10
c5cb6ac
c5e3538
ce2b142
f87d191
a5209fb
3d89c80
ba4b51b
1d81337
1fbe766
825eba8
6389cc2
620475f
7e28185
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 |
|---|---|---|
|
|
@@ -72,8 +72,12 @@ This protocol mirrors fragment ID assignment and ensures row IDs are unique acro | |
|
|
||
| Stable row IDs are a dataset-level feature recorded in the table manifest. | ||
|
|
||
| - Stable row IDs **must be enabled when the dataset is first created**. | ||
| - Currently, they **cannot be turned on later** for an existing dataset. Attempts to write with `enable_stable_row_ids = true` against a dataset that was created without stable row IDs will not change the dataset's configuration. | ||
| - Stable row IDs may be enabled when a dataset is created or by migrating an existing dataset. | ||
| - An ordinary write with `enable_stable_row_ids = true` does not migrate an existing dataset. Use the stable row ID migration operation instead; the Rust API exposes it as `Dataset::migrate_to_stable_row_ids`. | ||
| - Before migrating a dataset whose current manifest has no writer version, use the current Lance writer to commit an ordinary no-op deletion with predicate `false`, then reopen the latest version. This metadata-upgrade commit recomputes the authoritative physical row count for every fragment. Do not invoke stable row ID migration directly on such a legacy manifest: affected releases may have recorded stale counts, which would produce incomplete row ID sequences. | ||
| - Before migration, stop all index builds and index commits, drop every secondary index so no index entry remains in the dataset metadata, and keep index creation quiesced until migration completes. An in-flight index commit from a pre-migration snapshot can otherwise attach stale physical row addresses after activation. Recreate indices after migration. | ||
| - Quiesce data-modifying writers during migration. The migration uses a single atomic merge commit and does not retry when a concurrent write causes a conflict; the caller must retry the migration. | ||
| - Migration assigns an ID to every physical row position, including deleted positions, and atomically enables the feature and advances `next_row_id`. Migrating a dataset that already uses stable row IDs is a no-op. | ||
|
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. This guarantee does not hold for legacy manifests affected by #1531: those manifests can have Reproducer run on 9e4f376Added this test to #[tokio::test]
async fn repro_migrate_to_stable_row_ids_recomputes_legacy_physical_rows() {
let mut dataset =
make_simple_dataset("memory://migrate_legacy_physical_rows", 10).await;
let mut manifest = dataset.manifest.as_ref().clone();
manifest.writer_version = None;
let mut fragments = manifest.fragments.as_ref().clone();
fragments[0].physical_rows = Some(5);
manifest.fragments = Arc::new(fragments);
dataset.manifest = Arc::new(manifest);
dataset.migrate_to_stable_row_ids().await.unwrap();
dataset.validate().await.unwrap();
}Command: Expected the migrated dataset to validate. It instead failed with
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 66f20c6: migration guidance now requires a current Lance writer to commit a no-op deletion with predicate
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 66f20c655: the documented no-op deletion and reopen now upgrades a legacy manifest before migration; the reproduced stale-count case recomputed 10 physical rows before ID assignment and validated after migration. |
||
| - When stable row IDs are disabled, the `_rowid` column (if requested) is not stable and should not be used as a persistent identifier. | ||
|
|
||
| Row-level version tracking (`_row_created_at_version`, `_row_last_updated_at_version`) and the row ID index described below are only available when stable row IDs are enabled. | ||
|
|
@@ -181,11 +185,14 @@ The implementation selects the most compact encoding based on the value range, c | |
|
|
||
| </details> | ||
|
|
||
| #### Inline vs External Storage | ||
| #### Inline and External Storage | ||
|
|
||
| Row ID sequences are stored either inline in the fragment metadata or in external files. | ||
| Sequences smaller than ~200KB are stored inline to avoid additional I/O, while larger sequences are written to external files referenced by path and offset. | ||
| This threshold balances manifest size against the overhead of separate file reads. | ||
| `DataFragment` defines inline and external metadata fields as valid wire alternatives for row ID sequences and row version sequences. | ||
| These fields do not currently imply a size-based switching threshold. | ||
| Current Lance writers store all three sequence types inline in the fragment metadata regardless of their encoded size and do not emit the external alternatives. | ||
|
|
||
| Current Lance readers can load externally stored row ID sequences. | ||
| The format also permits external created-at and last-updated-at version sequences, but current Lance readers cannot load them; this is an implementation limitation, not an invalid encoding. | ||
|
|
||
| <details> | ||
| <summary>DataFragment row_id_sequence field</summary> | ||
|
|
@@ -360,4 +367,3 @@ WHERE _row_created_at_version <= {begin_version} | |
| ``` | ||
|
|
||
| This query excludes newly inserted rows by requiring `_row_created_at_version <= {begin_version}`, ensuring only pre-existing rows that were subsequently updated are returned. | ||
|
|
||
| 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.