fix(cache): revalidate cached datasets against latest version on hit - #225
Open
hellower wants to merge 2 commits into
Open
fix(cache): revalidate cached datasets against latest version on hit#225hellower wants to merge 2 commits into
hellower wants to merge 2 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Xuanwo
requested changes
Jul 16, 2026
Xuanwo
left a comment
Xuanwo
left a comment
Member
There was a problem hiding this comment.
Requesting changes for the correctness issues below. The current Build failure is intentionally excluded from this review.
* fix(cache): revalidate cached datasets against latest version on hit The per-connection dataset cache (lance-format#183) serves whatever version was checked out when the entry was created, and invalidation (lance-format#186) only covers writes made through the same connection. Commits from external writers (another process, another connection, or another Lance client) were therefore never observed: cache hits kept returning the stale version indefinitely, and DETACH/re-ATTACH did not help because the cache key is the dataset path and the cache outlives the attach. On every cache hit, revalidate the entry against the latest committed version via a new FFI primitive, lance_dataset_checkout_latest_if_stale: - Compare the handle's manifest version with the latest committed version on storage (metadata-only lookup via latest_version_id). - If unchanged, serve the cached entry as before. - If a newer version exists, check out the latest version on a clone of the dataset and swap in a fresh cache entry; the old entry stays valid for existing holders through shared ownership, and reusing the open handle avoids re-resolving namespace endpoints or storage options. Revalidation is unconditional rather than gated behind a setting: the version check is metadata-only and cheap relative to a scan, and latest-intent reads should always observe committed data. A stale hit is reported as a cache miss in both the profiling counters and the per-scan cache-hit flag so the refresh is observable via EXPLAIN. If the latest version cannot be determined (e.g. the dataset was dropped externally), the entry is invalidated and the query fails instead of silently serving stale data. Also serialize the Rust unit tests that touch the process-global debug counters, which the new dataset-opening tests would otherwise race with under the parallel test runner. * fix(cache): compare manifest identity, not version id, when revalidating A bare version-id comparison treats a dataset that was dropped and re-created at the same URI as fresh, because the new table's version history restarts at the same version id (commonly 1). The cached entry then kept serving the old in-memory dataset instead of observing the re-created table. Compare the full manifest identity instead: version + naming scheme + manifest e-tag, mirroring Lance's own already_checked_out semantics (whose doc comment calls out exactly this drop/re-create case). A missing e-tag on either side is conservatively treated as stale so the entry is refreshed rather than trusted. The lookup reuses the public latest_manifest() API; on the fresh path it resolves only the latest manifest location, and on the stale path the manifest read is keyed by (version, e-tag) so a re-created dataset is loaded from storage rather than served from the metadata cache. Covered by a new Rust unit test and a sqllogictest section that drop and re-create a dataset at the same URI (same version id) behind a cached reader. * fix(cache): re-describe namespace tables when revalidating cached entries Namespace-backed dataset entries are cached by endpoint/table id rather than by physical URI. Revalidating only the already-resolved dataset handle therefore missed an external drop/re-create that re-points the table to a new location: the manifest check ran against the old URI and either reported the old manifest as current or failed once the old data was removed, instead of opening the table now registered in the namespace. Add lance_dataset_namespace_checkout_latest_if_stale, which first re-describes the table through the namespace and reopens via the namespace path when the resolved location changed (re-applying managed versioning and namespace-provided storage options). When the location is unchanged it falls back to the shared manifest-identity check, which covers ordinary new commits and same-location re-creates via the e-tag. The re-describe costs one namespace round trip per cache hit - the same order as the namespace open path, which itself starts with a describe. The C++ cache now takes a per-path revalidation callback: plain and directory-namespace entries (whose physical location is derived deterministically and cannot be re-pointed) keep the handle-based check, while REST-namespace entries revalidate through the namespace and update the cached display URI when the table moved. Covered by Rust unit tests against a minimal in-process REST namespace mock that re-points the table location between calls; end-to-end REST namespace coverage remains gated behind LANCE_TEST_NAMESPACE like the rest of the REST namespace suite. * fix(catalog): fail closed when external schema evolution outdates a table entry Same-connection ALTERs rebuild the catalog entry via AlterEntry, but a schema change committed by an external writer leaves the entry columns captured at discovery time while the (revalidated) dataset handle serves the new schema. DuckDB binds column ids against the entry columns and the scan resolves those ids through the live dataset schema, so an external drop/rename could mislabel columns or trip internal errors (reproduced: an out-of-range column id aborts with an internal assertion in debug builds). Detect the divergence at bind time in LanceTableEntry::GetScanFunction by comparing the entry's declared columns with the schema produced from the live dataset handle (both are built by the same coercion/population pipeline, so the comparison is exact). On mismatch, fail closed with an explicit "changed externally" error and evict the stale entry using the same system-transaction drop + tombstone cleanup mechanism as DROP TABLE, so the next access lazily re-discovers the table with its current schema and succeeds. Covered by a sqllogictest that evolves the schema (drop column, rename column) through a second attached catalog acting as the external writer: the reader gets an explicit error instead of mislabeled data and self-heals on the following query. * fix(namespace): request table_uri when revalidating namespace tables The revalidation describe request left with_table_uri at its default and hard-required response.location. Namespaces that rely on table_uri for the complete physical URI, or that omit location entirely (the response model allows it), could still open through DatasetBuilder::from_namespace but then failed every subsequent cache hit with "table location not found". Mirror the other describe paths: request with_table_uri and accept either field. The cached handle is treated as current when either reported form matches its URI - from_namespace derives dataset.uri() from location, so requiring the preferred table_uri form to match would flag a perpetual (false) move on servers that report both fields in different spellings. The mock-server unit test now drives the raw describe response body and covers a table_uri-only response (fails with "table location not found" without this fix). * fix(catalog): include coerced-column state in schema freshness check External schema changes between Arrow types that DuckDB maps to the same LogicalType (for example float16 and float32 both surface as FLOAT after the reader-boundary coercion) passed the name/type freshness check, so the stale catalog entry kept serving. Its coerced-column list then no longer described the dataset: writers either kept rejecting a column that was no longer coerced or, in the opposite direction, could allow writes after a column became coerced. Compare the coerced-column list produced by the live schema's coercion pass against the entry's stored list as part of the freshness check. Both lists come from the same LanceCoerceArrowSchemaForDuckDB traversal over the (entry-build vs live) schema, so the comparison is exact. Mismatches now evict and rebuild the entry like any other external schema change. Adds test/data/float16_evolution_fixture.lance (a minimal float16 dataset; regenerate with a pylance snippet following test/scripts/gen_float16_fixture.py) and a regression test that evolves the float16 column to real float32 through a second attached catalog: the reader fails closed instead of silently serving the stale entry, and writes succeed after the entry self-heals. Like dml_alter_table_schema_evolution.test, the test evolves a checked-in fixture in place, so a dirty-tree rerun requires restoring test/data. * fix(catalog): validate nullability and write-only DML against live schema Two gaps remained in the external-schema-change freshness check: 1. Nullability. An external ALTER ... SET/DROP NOT NULL only changes the Arrow flags: the DuckDB names/types and the coerced-column list stay identical, so the stale entry kept exposing and enforcing the old nullability until an unrelated schema change occurred. Include the NOT NULL state in the comparison, and populate NOT NULL constraints from the Arrow flags in the discovery paths (directory and REST, both JSON-schema and dataset-open) exactly as the post-ALTER rebuild path already did, so both sides of the comparison come from the same pipeline. 2. Write-only DML. The freshness check only ran while binding a scan, so a plain INSERT (which does not scan its target) kept planning against a stale entry indefinitely: after an external float16 -> float32 evolution, INSERT kept rejecting the column as coerced until the user happened to run a SELECT first. Extract the comparator into LanceTableEntry::ValidateLiveSchemaOrEvict, expose VerifySchemaFreshness for statements that do not bind a scan, and call it from INSERT planning. UPDATE, DELETE and MERGE bind a scan of the target and are already covered by the scan path; TRUNCATE does not consult the entry's column state. Covered by regression tests that evolve nullability and the coerced state through a second attached catalog acting as the external writer, including a direct INSERT with no prior SELECT. Adds the test/data/float16_dml_fixture.lance fixture (evolved in place by the test, like the other schema-evolution fixtures). * fix(scan): force rebind of prepared statements pinning dataset handles The scan, exec-pushdown, and search bind data all pin a dataset handle checked out at bind time. A cached prepared statement kept using the handle produced at PREPARE time, so later EXECUTEs never passed through the cache revalidation that runs at bind and kept scanning the old version indefinitely after an external commit. Override SupportStatementCache to false on every bind data that pins a dataset handle (the same mechanism DuckDB's multi-file scans use), so the binder marks such statements always-require-rebind and each EXECUTE rebinds: revalidation and the schema freshness check then run exactly as for ad-hoc statements. A prepared statement whose table changed schema externally fails closed with the same "changed externally" error and succeeds again after re-discovery. Covered by a regression test that re-executes prepared statements over both a path scan and a catalog table across external appends and an external column drop. * fix(namespace): reopen tables when namespace-vended storage options change Revalidation compared only the resolved location, so a REST namespace that re-describes the same physical URI with different storage options (e.g. rotated object-store credentials) took the same-location branch and the cached handle kept using its stale object store. Track the storage options the namespace vended when the handle was opened (captured with an explicit describe at open time - one extra namespace round trip, and opens only happen on cache misses) and compare them against each revalidation describe. When they differ, reopen through the namespace path so the new options are applied; the refreshed handle records the options it was opened with. Namespaces that vend per-describe temporary credentials will reopen on every hit, which is the cost of always reading with the freshest credentials. Covered by a mock-server unit test that rotates the vended storage options for an unchanged location and expects a reopen for each rotation and none while the options are stable. * fix(catalog): refresh stale table entries at catalog resolution Catalog-only SQL such as DESCRIBE or information_schema.columns exposes the entry's column and NOT NULL metadata without ever binding a scan or planning DML, so none of the existing freshness checks ran and the first access after an external schema or nullability change kept returning the old metadata. Run the freshness check when the entry is resolved instead: - LanceSchemaEntry::LookupEntry probes the resolved LanceTableEntry against the live dataset schema (a non-throwing IsSchemaStale split out of the existing verifier). Stale entries are evicted with the established DROP TABLE mechanism and the lookup retried once, so the caller transparently receives the freshly discovered entry rather than an error. The probe costs one dataset-cache access per resolution - the same order as the per-hit revalidation scans already perform at bind. - LanceSchemaEntry::Scan runs the same probe before enumerations (information_schema, duckdb_columns, SHOW). Names are collected first: the probe does storage I/O and eviction mutates the catalog set, neither of which may run under the set's scan lock. - Infrastructure errors (e.g. dataset unreachable) fail open for catalog resolution so metadata access and DROP TABLE cleanup keep working; scans surface the real error with proper context. Since entry resolution now heals stale entries before statements bind, external schema changes are observed transparently: reads and writes bind against the current schema immediately instead of failing once with the "changed externally" error (which remains as the fail-closed safety net for races between resolution and bind). The affected regression tests are updated accordingly, and index_ddl.test's cache hit expectations now account for the probe warming the dataset cache during resolution.
- Parse qualified REST table ids with the configured delimiter once in the revalidation path and reuse the segments for describe and reopen (post-lance-format#220 parity); the mock namespace server now validates the multi-segment id array, with multi-level and custom-delimiter tests. - Drop the storage-options snapshot/reopen mechanism entirely: DatasetBuilder::from_namespace installs Lance's dynamic storage-options provider, so namespace-vended credential rotation is refreshed by the object store itself. The open path is back to a single describe (asserted by request count), and revalidation reopens only when the resolved location moved. - Memoize cache-hit revalidation per statement and cache key so every reference in one statement shares a single dataset generation; the profiling line now reports actual revalidations, asserted by a self-join test. - Opt LanceDuckCatalog out of catalog version identity so prepared statements rebind on every EXECUTE, closing the stale-plan hole for target-only DML whose plans pin a LanceTableEntry; covered for INSERT/UPDATE/DELETE/MERGE. - Replace stale catalog entries through the transactional catalog version chain (CatalogSet::AlterEntry with a stock-serializable refresh marker) instead of destroying them eagerly: the superseded generation stays alive until undo-buffer cleanup, so raw references held by active operators and prepared plans remain valid. The heal upgrades the attached catalog's bind-time transaction to read-write, and user ALTER/DROP surgery fails closed on uncommitted refresh heads before any dataset side effects (with a surgery snapshot that treats committed heals as current).
hellower
force-pushed
the
upstream-dataset-cache-revalidation
branch
from
July 16, 2026 11:22
86f116b to
a5c926f
Compare
Author
|
Thanks for the review — all five findings are addressed in a5c926f (details in the per-thread replies), on top of a rebase onto current Verification: @Xuanwo PTAL |
This was referenced Jul 16, 2026
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
2 participants
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.
Problem
The per-connection dataset cache (#183) serves whatever version was checked out when the entry was created, and invalidation (#186) only covers writes made through the same DuckDB connection. Commits from external writers — another process, another connection, or another Lance client using the Rust API — are never observed: cache hits keep returning the stale version indefinitely, and DETACH + re-ATTACH does not help because the cache key is the dataset path and the cache outlives the attach.
Reproduced from an embedding application: an external Lance delete commit lands on storage (verified via
_versions/manifests), yet subsequent SELECT/COUNT through the extension keep serving the pre-delete version on every connection that had already touched the dataset.Fix
On every cache hit, revalidate the entry against the latest committed state before serving it:
lance_dataset_checkout_latest_if_stale): compare the cached handle's manifest location (version + naming scheme + e-tag, mirroringDataset::already_checked_outsemantics in lance itself) with the latest committed manifest — a metadata-only lookup. When stale,checkout_lateston a clone and swap in a fresh entry; existing holders stay safe via shared ownership. The e-tag comparison also catches drop/recreate at the same URI where the version id aliases (commonly both version 1).lance_dataset_namespace_checkout_latest_if_stale): re-describe through the namespace on every hit (with_table_urirequested,table_uri/locationor-match) so re-pointed tables reopen at their new location. Qualified table ids are parsed with the configured delimiter once and the segment vector is reused for every describe and reopen (post-Fix/rest namespace multilevel #220 parity). Namespace-vended credential rotation needs no handling here:DatasetBuilder::from_namespaceinstalls Lance's dynamic storage-options provider, so fresh credentials are fetched from the namespace by the object store itself — the open path performs exactly one describe.revalidationsprofiling counter.LanceTableEntrymetadata is refreshed transparently at catalog resolution (covers DESCRIBE /information_schema/ SHOW as first access), and the freshness check — names, types, coerced-column state (e.g. float16→float32 both surface as FLOAT), and nullability — also guards write-only DML planning (INSERTdoes not bind a scan). The stale entry is replaced through DuckDB's transactional catalog version chain (CatalogSet::AlterEntrywith an internal refresh marker), so the superseded generation stays alive until undo-buffer cleanup reclaims it once no active transaction can reference it — raw references held by active scans, DML operators, and prepared plans remain valid. User-issued ALTER/DROP fail closed (before any dataset side effects) if they land on a not-yet-committed refresh. A bind-time fail-closed error remains as a race safety net; the next statement self-heals.SupportStatementCache() = false(same pattern DuckDB uses for multi-file scans), and the catalog opts out of catalog-version identity (GetCatalogVersionreturns invalid — the escape hatchCheckCatalogIdentitydocuments), so every EXECUTE rebinds and observes the same latest data as ad-hoc statements — including target-only DML (INSERT/UPDATE/DELETE/MERGE), whose cached plans would otherwise outlive theLanceTableEntrythey pin.Revalidation is unconditional rather than gated behind a setting: the identity check is metadata-only and cheap relative to a scan, and latest-intent reads should always observe committed data. A stale hit is surfaced as a cache miss in profiling counters and the
Lance Dataset Cache Hitexplain flag, keeping the transition observable.Tests
Eight sqllogictest files + Rust unit tests (in-process mock REST namespace server that validates multi-segment table ids on every request), each verified red-green against the pre-fix behavior:
dataset_cache_external_writer_revalidation.test— external DELETE/APPEND visibility, drop/recreate at same URI, EXPLAIN cache-hit transitionsdataset_cache_external_schema_evolution.test/_schema_coercion.test/_nullability.test— external DROP/RENAME COLUMN, float16→float32 coercion-state change, SET/DROP NOT NULL; catalog entries heal without mislabeled bindings (pre-fix this aborted on a DuckDB internal assertion); plus DROP-of-a-pending-refresh fails closed with the dataset intactdataset_cache_external_dml_freshness.test— write-only INSERT after external evolution (no prior SELECT)dataset_cache_catalog_only_freshness.test— DESCRIBE /duckdb_columns()as the first access after external changedataset_cache_prepared_statements.test— PREPARE → external commit → EXECUTE observes the new versiondataset_cache_prepared_dml.test— prepared target-only INSERT after external schema evolution rebinds instead of reusing the stale plan (also exercises the entry lifetime a cached plan depends on); UPDATE/DELETE/MERGE observe external commits between executionsdataset_cache_statement_memoization.test— self-join revalidates once per statement (revalidationscounter), memo resets per statementVerification
cargo fmt --check,cargo clippy --all-targets(0 issues),cargo test14/14make format-checkcleanscan_limit_through_filter.test) reproduces identically on pristinemainin this environment (pre-existing, unrelated)