feat: expose user-defined schemas across the API, server, client and Python - #220
Merged
Conversation
…Python Completes lance-format#214 Part 2. `GenericStore` was core-only after lance-format#218; this wires it through every layer so users can actually declare and use their own schema. **`SchemaSpec` moves to the API crate.** A schema is part of the wire contract — the server accepts one at store creation and the client needs the same type to send it — so it belongs with the other request/response types. It depends only on `arrow-schema` and `serde`, not on Lance, and core re-exports it so existing paths are unchanged. Its one tie to core (`DistanceMetric::parse`) becomes a local identifier check. **API**: `CreateGenericStoreRequest`, `GenericStoreInfo`, `AddRowsRequest`, `AddRowsResponse`, `ListRowsResponse`, and the `GenericStoreApi` trait. There is deliberately no per-column DTO: the schema is declared at runtime, so a static struct could not describe it. Rows are `serde_json` maps, making the wire form and the row form the same thing. **Server**: eight routes under `/api/v1/generic`, an LRU + registry in `AppState` mirroring datagen, and a 1 GiB body limit (generic stores hold blob columns inline — that is the point of them). **Client**: `RemoteGenericStore`, which caches the schema at connect since `GenericStoreApi::spec` is synchronous and there is no compile-time type describing the columns. **Python**: a `GenericStore` pyclass plus the `lance_context.GenericStore` wrapper. Schemas are dicts, in full form or shorthand (`{"score": "float32"}`), matching the JSON the REST API takes so the two surfaces cannot drift. Also fixes two things the survey turned up: - **Nine duplicated conversion functions removed**, not the six lance-format#214 counted: `dto_to_relationship` and `relationship_to_dto` each had a *third* copy in `routes/rollouts.rs`. All were byte-for-byte identical. They now live once in core and are re-exported. - **`shutdown` only closed rollout stores.** Datagen and generic stores hold resident MemWAL writers too, so an unsealed memtable was left to the best-effort detached close in `Drop` — rows could stay durable-but-invisible until the next WAL replay. All three kinds now close deterministically. The registry deliberately stores only name and URI. A generic store's schema lives in its own dataset, and a second writable copy is exactly the divergence this whole refactor exists to remove; the cost is that listing cannot report schemas without opening each dataset, which is why `GenericStoreInfo::schema` is `None` in list responses. 21 new tests (6 server routes, 15 Python). Full suite passes: 201 core + 57 server + 18 api Rust tests, 197 Python tests, clippy clean. Co-Authored-By: Claude <noreply@anthropic.com>
Import ordering in `__init__.py`, one over-long line wrapped, and `Path` moved into a type-checking block in the new test file to match the convention the other test modules already use. No behavior change; 197 Python tests still pass, pyright reports 0 errors. Co-Authored-By: Claude <noreply@anthropic.com>
beinan
added a commit
that referenced
this pull request
Jul 28, 2026
The three follow-ups from #220, plus a regression guard for a bug I introduced while writing them. ## 1. `seal_on_add` is now persisted in the dataset It lived only in `GenericStoreOptions`, so a store created with `seal_on_add: true` **lost read-your-write the first time it was evicted from the LRU and reopened** — the server passed a hardcoded `false` at `state.rs:642`. The failure mode is the nasty kind: no error, no data loss, just "sometimes I can't read what I just wrote" — and triggered by *cache pressure*, not by anything in the caller's code. Untestable locally, only shows up under load. `/merge-wal` would accidentally paper over it (it flushes internally), making it look intermittent. The mode now sits in the schema metadata beside the spec, so a reopen restores what the store was *created* with. Stores written before this fall back to the caller's option, so nothing breaks on upgrade. ## 2. The sweepers now visit every store kind Both `spawn_flush_sweeper` and `spawn_global_sweeper` hardcoded `state.rollout_stores`. Consequences: - **datagen**: seals on every append, so flush didn't matter — but *nothing ever merged its generations*. They accumulated forever and every read unioned all of them. This predates #217. - **generic**: defaults to deferred seal, so writes stayed invisible until someone called `/flush` by hand. Rather than copy the loop a third time — the exact duplication #214 removed one layer down — the traversal is generic over a new `Sweepable` trait in `sweeper.rs`. **Why the trait is on `Arc<RwLock<Store>>` and not on the store:** rollout's merge deliberately splits into a shared-lock prepare (the expensive object-storage reads, during which appends keep flowing) and a brief exclusive-lock commit. A trait over `&mut Store` would have forced the exclusive lock across the whole merge and quietly stalled the write path — a performance regression that no test would have caught. Letting each kind own its locking preserves that. Metric names keep their `rollout_` prefix and gain a `kind` label, so existing dashboards and alerts keep working. ## 3. Config naming: documented, not renamed `ROLLOUT_FLUSH_INTERVAL_SECS` and friends now govern all three kinds, so the prefix is misleading. But renaming breaks deployment manifests for zero functional gain, so I documented the actual scope in the flag docs and the startup warning instead. Happy to add neutral aliases if you'd rather. ## 4.⚠️ A regression I caused, and the guard for it While rewriting those doc comments my edit deleted two `#[arg(..)]` attributes, turning `--rollout-flush-interval-secs` and `--rollout-cleanup-interval-secs` into **required positional arguments**: ``` error: the following required arguments were not provided: <ROLLOUT_CLEANUP_INTERVAL_SECS> <ROLLOUT_FLUSH_INTERVAL_SECS> ``` Everything still compiled. All 203 Rust tests still passed. The server just refused to start. It surfaced only because the Python remote tests spawn the real binary — and those tests swallow stderr, so it showed up as "server did not become healthy". Two tests in `config.rs` now close that hole: every field must be an optional flag with a default, and the minimal documented invocation must parse. ## Verification I checked each fix actually fails without its fix, rather than trusting a green run: - reverting the persistence → both `seal_mode_survives_*` tests fail - dropping the `#[arg]` → both config tests fail 9 new tests, including e2e ones that go through the real server path: seal mode surviving an actual LRU eviction (`generic_stores.lock().pop()`), and a sweeper pass draining a real datagen generation. Full suite: **203 core + 62 server + 18 api** Rust, **199 Python**. clippy, ruff, pyright all clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code) 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
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.
Completes #214 Part 2.
GenericStorewas core-only after #218; this wires it through every layer so users can actually declare and use their own schema.How it looks
Same over REST:
SchemaSpecmoves to the API crateA schema is part of the wire contract — the server accepts one at creation, the client needs the same type to send it — so it belongs with the other request/response types rather than in the storage crate. It depends only on
arrow-schemaandserde, not on Lance. Core re-exports it, so existing paths are unchanged.Its one tie to core (
DistanceMetric::parse) becomes a local identifier check, with a comment noting the two must stay in sync.No per-column DTO, on purpose
The schema is declared at runtime, so a static struct can't describe it. Rows are
serde_jsonmaps in both directions — the wire form is the row form, so there's no conversion layer to drift.Two bugs the survey turned up
Nine duplicated conversion functions, not the six #214 counted.
dto_to_relationshipandrelationship_to_dtoeach had a third copy inroutes/rollouts.rs. All were byte-for-byte identical — pure copy-paste that happened to still be in sync. Now defined once in core and re-exported.shutdownonly closed rollout stores. Datagen and generic stores hold resident MemWAL writers too, so an unsealed memtable was left to the best-effort detached close inDrop— rows could stay durable-but-invisible until the next WAL replay. All three kinds now close deterministically. This one pre-existed for datagen; adding a fourth store kind would have quietly widened it.Registry stores name and URI only
A generic store's schema lives in its own dataset. A second writable copy in the registry is exactly the divergence this whole refactor exists to remove, so I didn't add one. The cost is that listing can't report schemas without opening each dataset — hence
GenericStoreInfo::schemaisNonein list responses and populated for single-store lookups. Called out explicitly rather than left to be discovered.Verification
21 new tests — 6 server route tests (create/list/add/read round-trip, invalid schema rejected before anything is created, duplicate-create conflicts, undeclared column rejected, delete unregisters, deferred seal + WAL merge) and 15 Python tests.
Full suite: 201 core + 57 server + 18 api Rust tests, 197 Python tests,
cargo clippy --workspace --all-targetsclean,typosclean.Follow-ups
seal_on_addis not persisted, so a reopened store takes the server default. Worth persisting alongside the schema if it turns out to matter.spawn_global_sweeper,spawn_flush_sweeper) still only visit rollout stores — pre-existing for datagen, now also true for generic. Generic stores default to deferred seal, so they rely on an explicit flush or themerge-walroute until that's addressed.🤖 Generated with Claude Code