refactor(core): migrate DatagenStore onto StorageBase - #217
Merged
Conversation
Second step of lance-format#214, after lance-format#215. `ContextStore` now delegates its storage layer to `StorageBase` instead of carrying its own forked implementation. Closes the gaps that made this store diverge from rollout: - **WAL merge, which this store never had.** Flushed generations accumulated under `_mem_wal/` forever and every read unioned all of them, so read cost grew without bound in the number of writes. It now gets the same merge as rollout, exposed as `maybe_merge_wal` (count trigger), `cleanup_wal` (time trigger) and `pending_wal_generations`. - **Compaction now defers index remap.** The base table carries a fieldless MemWAL index and Lance's inline remap panics on it. - **Compaction and index creation no longer drop `storage_options`.** Both reloaded via a bare `Dataset::open`, which silently discarded credentials and broke on any authenticated object store. Both now reload through `StorageBase::load_with_options`. - **Resident writer.** `add` used to open, `put` and `close` a `ShardWriter` on every call; it now reuses one resident writer with fence-retry, so an append no longer pays a cold DNS resolution + TCP/TLS handshake + epoch claim. - **`add` takes `&self`.** The writer lives behind the base's mutex, so the server takes a read lock and concurrent appends stop serializing on the store's `RwLock`. Sharding changes to match rollout: writes go to the shard owned by this writer instance (`ContextStoreOptions::shard_id`) rather than one derived per `(bot_id, session_id)`. One instance owns exactly one shard, so the epoch-fencing invariant holds by construction, and merge has a single well-defined shard to drain. `derive_region_id` is removed. `add` visibility is preserved, not silently weakened. `StorageBase` gains a `seal_on_put` switch: rollout keeps its deferred seal (durable on return, visible via the flush sweeper or `?flush=true`), while `ContextStore` defaults to `seal_on_add: true` — required for correctness, since its id/external-id uniqueness validation, upsert and tombstones all read the table back. Callers appending trusted batches can opt into rollout's throughput profile with `seal_on_add: false`. `ContextStore` is no longer `Clone`: it owns a `StorageBase`, which owns the resident writer and a `Drop` that seals it, so it must have exactly one owner. The two clone sites now open their own handle instead — the background compactor (which only rewrites base-table fragments and would otherwise contend with the write path) and Python's `Context.fork` (which branches the in-memory context over the same dataset). Three new tests pin the behavior: `add` is visible on return by default, deferred seal is durable-but-invisible until `flush`, and WAL generations merge into the base table and stay readable. Full workspace suite passes (180 core lib tests, up from 177), clippy clean. Co-Authored-By: Claude <noreply@anthropic.com>
Third and last store of lance-format#214 Part 1, after lance-format#215 and lance-format#216. All three fixed-schema stores now share one storage implementation. Closes this store's gaps: - **Compaction, which it had none of.** Every WAL merge appends a fragment to the base table and datagen seals once per checkpoint batch, so fragments grew without bound and scans degraded with them. Now exposed as `compact` / `should_compact` / `compaction_stats`, with `defer_index_remap` set (the fieldless MemWAL index panics Lance's inline remap). - **Scalar index, which it also had none of.** `create_event_id_index` builds a ZoneMap on `event_id`; point lookups by event id previously always scanned. - **`load_with_options` dropped storage options.** It fell back to a bare `Dataset::open` whenever no options were passed, and never attached a session, so every generation read allocated Lance's 6 GiB default cache. Deletes the near-line-for-line copy of rollout's `merge_own_shard` (plus `merge_own_shard_if_ready`, the writer lifecycle, `ensure_mem_wal`, shard snapshot discovery, the `Drop` impl and a second `is_fenced_error`), and picks up the streaming-safe version in the base instead of buffering every flushed generation in memory. `seal_on_put: true`: each append is one complete checkpoint batch that must be durable *and* readable before the writer moves on, so a crash cannot expose a partially checkpointed step. This preserves the store's existing put -> seal -> drain behavior exactly. `key_column` is `event_id`, not `id` — event ids are derived deterministically (UUIDv5 over item/checkpoint/ordinal), which is what makes a retried append idempotent under LSM dedup. The base already took the key column as a parameter, so no change was needed there. New test `compaction_folds_merged_fragments_and_preserves_reads` pins the behavior that did not exist before: fragments accumulate across merges, compaction reduces them, and every event stays readable. Full suite passes — 181 core lib tests, 182 Python tests, clippy clean. Co-Authored-By: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Third and last store of #214 Part 1, after #215 and #216. All three fixed-schema stores now share one storage implementation.
Stacked on #216 — review that first; this diff is
datagen_store.rsonly.Bugs this closes
Compaction, which this store had none of. Every WAL merge appends a fragment to the base table, and datagen seals once per checkpoint batch, so fragments grew without bound and scans degraded with them. There was no
compact_filescall anywhere in the file. Nowcompact/should_compact/compaction_stats, withdefer_index_remapset — the fieldless MemWAL index panics Lance's inline remap.Scalar index, which it also had none of.
DatasetIndexExtwas imported solely forload_indicesinmem_wal_index_present.create_event_id_indexnow builds a ZoneMap onevent_id; point lookups by event id previously always scanned.load_with_optionsdropped storage options. It fell back to a bareDataset::openwhenever no options were passed, and never attached a session — so every flushed-generation read allocated Lance's 6 GiB default cache, keyed per generation URI.Deleted duplication
merge_own_shardwas a near-line-for-line copy of rollout's, and buffered every flushed generation fully in memory. Gone, along withmerge_own_shard_if_ready, the writer lifecycle (write_with_resident_writer/put_seal_drain/ensure_write_writer),ensure_mem_wal,mem_wal_index_present, shard-snapshot discovery,flushed_generation_uri,open_flushed_dataset,load_with_options,create_with_options, theDropimpl, and a second copy ofis_fenced_error.Net -269 lines in this file.
Behavior preserved exactly
seal_on_put: true. Each append is one complete checkpoint batch that must be durable and readable before the writer moves on, so a crash cannot expose a partially checkpointed step. This is the store's existingput→force_seal_active→wait_for_flush_drainbehavior, unchanged.key_columnisevent_id, notid. Event ids are derived deterministically (UUIDv5 over item/checkpoint/ordinal), which is what makes a retried append idempotent under LSM dedup. The base already took the key column as a parameter, so it needed no change to accommodate this.Verification
New test
compaction_folds_merged_fragments_and_preserves_readspins behavior that could not exist before: fragments accumulate across merges, compaction reduces the count, and every event stays readable afterward.cargo clippy --workspace --all-targetscleanWhat's left in #214
Part 1 is done after this — all three stores share
StorageBase. Part 2 (arbitrary user-defined schemas with a requiredid) builds on it: the base already validateskey_columnagainst the schema at open, which is the enforcement point that work needs.🤖 Generated with Claude Code