build(deps): bump Lance 6.0.1 → 7.0.0 (correct-by-design substrate alignment) - #229
Merged
Conversation
….11.4) Arrow stays 58 and DataFusion stays 53 (no change). The only transitive bump is object_store 0.12.5 → 0.13.2. 141 upstream commits reviewed; no fixes lost (the 6.0.x release-branch backports are all forward-ported into 7.0.0). - object_store 0.13 moved get/put/head/rename/delete behind a new ObjectStoreExt trait (list/list_with_delimiter/put_opts stay on the core trait). Add `use object_store::ObjectStoreExt` in storage.rs and db/manifest/namespace.rs; no call-site changes. Mirrors Lance's own migration in PR #6672. - roaring pinned to 0.11.4 (cargo update -p roaring --precise 0.11.4). Lance 7.0.0's UpdatedFragmentOffsets newtype (lance#6650) derives Eq over HashMap<u64, RoaringBitmap>, which needs RoaringBitmap: Eq, added in roaring 0.11.4; the loose `roaring = "0.11"` constraint otherwise resolves 0.11.3 and lance itself fails to compile. - lance#6774: merge-insert INSERT rows now stamp _row_created_at_version with the commit version (was a fallback of 1). Flip the lance_version_columns assertion to `== v2` and correct the changes/mod.rs rationale comment. Production change-detection keys on _row_last_updated_at_version + ID membership, so its logic is unaffected. Refs lance#6650, lance#6774, lance#6672.
…t flip) lance#6755 flipped the WriteParams::auto_cleanup default from on (a full cleanup pass every 20th commit) to None. On 6.0.1 the on-by-default hook could silently GC versions that __manifest pins for snapshots/time-travel. OmniGraph owns cleanup explicitly (optimize.rs::cleanup_all_tables) and never set auto_cleanup, so it was relying on a default that is both wrong for our snapshot model and now changed upstream. Pin auto_cleanup: None explicitly at all 11 production WriteParams sites (table_store ×6, commit_graph ×2, recovery_audit ×1, manifest/graph ×2 — the __manifest + sub-table Create paths). Removes the dependency on a default-flag value and locks in the snapshot-safe behavior regardless of future upstream re-flips. Refs lance#6755.
lance#6796 (issue #6792) fixed a BTREE scalar-index range-query bound inclusiveness bug: `x <= hi AND x > lo` returned the wrong boundary row. Add lance_surface_guards.rs::btree_range_query_boundary_is_correct, which reproduces the exact #6792 shape (5 rows + an explicit BTREE drives the index path even on tiny data) and pins the corrected inclusive-<= / exclusive-> semantics. It turns red if a future Lance regression reintroduces the bug. OmniGraph today builds BTREE only on string @key columns and queries them by equality/IN, so its current patterns do not hit this; the guard protects any future BTREE-range path (BTREE-on-properties, range-on-key). Refs lance#6796.
- docs/dev/lance.md: new 2026-06-14 alignment stanza for the 6.0.1 → 7.0.0 bump (object_store ObjectStoreExt move, roaring 0.11.4, #6774/#6796/#6755 behavior, #6658 shipped → MR-A unblocked but separate, #6666 + blob compaction still open); prior 6.0.1 stanza demoted to historical. - AGENTS.md: storage substrate 6.x → 7.x (line + architecture diagram). - docs/dev/invariants.md: deletes/vector known gap updated — the staged two-phase delete API (lance#6658) now exists and MR-A is unblocked, but delete_where stays inline and D2 stays in place until the migration lands; create_vector_index still gated on lance#6666.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29d61d47a5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let params = WriteParams { | ||
| mode: WriteMode::Append, | ||
| allow_external_blob_outside_bases: true, | ||
| auto_cleanup: None, |
There was a problem hiding this comment.
When writing to a graph whose Lance tables were created before this bump, those datasets can already carry lance.auto_cleanup.interval / older_than config from the old default. Setting auto_cleanup: None here only affects create-time config and is not used by the later commit_staged CommitBuilder, whose skip_auto_cleanup default remains false, so upgraded graphs can still run Lance auto-cleanup after enough writes and delete versions that older OmniGraph manifests/time-travel snapshots still pin. The staged commit path needs to explicitly skip auto-cleanup or clear/migrate the legacy dataset config before writes continue.
Useful? React with 👍 / 👎.
Contributor
Author
There was a problem hiding this comment.
Valid P1 — verified against Lance 7.0.0 source and fixed in 90a2028.
Confirmed: 6.0.1 defaulted WriteParams::auto_cleanup = Some(AutoCleanupParams::default()) (ON), auto_cleanup is create-time-only ("no effect on existing datasets" per write.rs), and the hook fires off the dataset's stored config at commit time (io/commit.rs: if !commit_config.skip_auto_cleanup). So auto_cleanup: None alone left pre-bump graphs exposed, and the staged path (commit_staged → CommitBuilder, skip_auto_cleanup default false) would still GC __manifest-pinned versions.
Fix: skip_auto_cleanup on every commit path —
commit_staged→CommitBuilder::with_skip_auto_cleanup(true)(staged data path)__manifestpublisher →MergeInsertBuilder::skip_auto_cleanup(true)- all 11
WriteParams→skip_auto_cleanup: true(direct write/append;auto_cleanup: Nonekept so new datasets store no config)
Tests: lance_surface_guards::skip_auto_cleanup_suppresses_version_gc (negative control proves the config GCs without the skip; positive proves the skip preserves the version) and staged_writes::commit_staged_skips_auto_cleanup_so_pinned_versions_survive (omnigraph's commit_staged on a legacy-config dataset). Both green.
…-impact-audit # Conflicts: # crates/omnigraph/tests/lance_surface_guards.rs # docs/dev/lance.md
…sets Addresses PR #229 review (Codex P1). `WriteParams::auto_cleanup` is create-time config with no effect on existing datasets (Lance write.rs docs), so the previous `auto_cleanup: None` change alone did NOT protect graphs created before the v7 bump: 6.0.1 defaulted auto_cleanup ON, leaving `lance.auto_cleanup.*` config on those datasets, and Lance's per-commit hook (io/commit.rs: `if !commit_config.skip_auto_cleanup`) fires off that stored config — so omnigraph's own writes would GC versions the __manifest pins for snapshots/time-travel. Skip the hook on every commit path, covering new and legacy datasets alike: - commit_staged: CommitBuilder::with_skip_auto_cleanup(true) — the staged data path. - __manifest publisher: MergeInsertBuilder::skip_auto_cleanup(true). - all 11 WriteParams: skip_auto_cleanup: true (direct Dataset::write/append paths; auto_cleanup: None retained so new datasets store no cleanup config at all). Tests: - lance_surface_guards::skip_auto_cleanup_suppresses_version_gc — substrate: negative control (config GCs v1 without skip) + with-skip survival. - staged_writes::commit_staged_skips_auto_cleanup_so_pinned_versions_survive — omnigraph usage: commit_staged on a legacy-config dataset preserves the pinned create version. Refs lance#6755.
…_insert UPDATE Addresses PR #229 review follow-up. `lance_merge_insert_update_preserves_created_at_version` documented (in a comment) that a merge_insert UPDATE preserves created_at and bumps updated_at, but only asserted the value change — leaving the change-feed invariant unguarded. Add the two missing assertions: - bob created_at == v1 (preserved across UPDATE; what the test name promises; lance#6774 only changed INSERT-row stamping). - bob updated_at == v2 (bumped to the commit version) — the invariant OmniGraph's insert/update classification relies on (changes/mod.rs keys on _row_last_updated_at_version). A regression here would silently drop updates from the diff/change feed.
Contributor
Author
|
Addressed the follow-up ( The test now asserts both halves it previously only described in a comment:
|
aaltshuler
added a commit
that referenced
this pull request
Jun 14, 2026
… + native namespace) (#236) * fix(engine): make the v1→v2 manifest migration idempotent under Lance 7's immutable unenforced primary key Lance 7 (dataset/transaction.rs) makes the unenforced primary key immutable once set: any write touching the reserved `lance-schema:unenforced-primary-key` field metadata after the PK is set errors "cannot be changed once set" — even re-applying the same value. `migrate_v1_to_v2` previously relied on the old Lance 6 idempotency (re-applying the annotation was a no-op-ish bump), which it needs for crash-recovery: a v1 graph that crashes after the field-set but before the stamp bump re-enters the migration with the PK already present. Under Lance 7 that re-entry now errors, so a real pre-v0.4.0 graph crashing in that window could never complete its migration. Guard the field-set with `schema().unenforced_primary_key().is_empty()` so a genuine first-set still runs but a re-set is skipped — restoring crash-idempotency by construction. (Fresh graphs bake the PK into manifest_schema() at init and never run this migration.) The existing test_publish_migrates_pre_stamp_manifest_to_current_version is the regression guard: red under Lance 7 before this change, green after. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(engine): realign the native-namespace surface guard to Lance 7 (TableNotFound) `test_directory_namespace_direct_publish_cannot_replace_native_omnigraph_write_path` pokes Lance's NATIVE DirectoryNamespace (not omnigraph's production write path, which is the manifest merge_insert publisher) to document that it cannot replace omnigraph's authority. Lance 7's DirectoryNamespace routes list/describe/create_table_version through `check_table_status`, which now reports an omnigraph-manifest-tracked table as absent — so all three return TableNotFound for `node:Person` (observed). The native namespace is now fully decoupled from omnigraph's manifest: it cannot enumerate, inspect, or publish over omnigraph's tables. This strengthens the guard's thesis. Realigned the assertions to the v7 behavior and kept the authority check (omnigraph's refresh ignores the direct append; row_count stays 0). Test-only; no production impact. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(lance): document the 2 runtime behavior changes in the 7.0.0 alignment stanza The #229 stanza verified a clean engine *build* but not the test suite, and claimed "no Lance API surface omnigraph uses changed." Two runtime behaviors did, caught only by the full test suite: - the unenforced primary key is immutable once set in v7 (transaction.rs) — broke the v1→v2 manifest migration's crash-idempotency; fixed by an is-set guard; - the native DirectoryNamespace returns TableNotFound for omnigraph manifest-tracked tables (dir.rs) — test-only; the surface guard was realigned. Corrects the over-broad "no surface changed" claim, adds both findings, and notes the lesson: a clean build is not a clean alignment — run cargo test --workspace before declaring a Lance bump done. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <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.
Summary
Initial Lance 6.0.1 → 7.0.0 substrate bump and the immediate changes required to be correct-by-design and aligned with the substrate. Realizes the dev-graph ticket
iss-lance-v7-bump; scoped to the bump only (follow-on adoptions are separate tickets).Arrow stays 58, DataFusion stays 53 — no change to the compiler/DataFusion surface. The only transitive bump is object_store 0.12.5 → 0.13.2. 141 upstream commits were reviewed (6.0.1 → 7.0.0); no fixes are lost (the 6.0.x release-branch backports are all forward-ported).
Changes (4 commits)
build(deps)—lance-*→ 7.0.0,object_store→ 0.13.2, and a mandatoryroaring0.11.4 pin.get/put/head/rename/deletebehind a newObjectStoreExttrait →use object_store::ObjectStoreExtinstorage.rs+db/manifest/namespace.rs(no call-site changes; mirrors Lance's own PR #6672).UpdatedFragmentOffsets(lance#6650) derivesEqoverHashMap<u64, RoaringBitmap>, which needsRoaringBitmap: Eq(added in roaring 0.11.4). The looseroaring = "0.11"constraint otherwise keeps the broken 0.11.3 and lance itself fails to compile._row_created_at_versionto the commit version →lance_version_columnsassertion updated; production change-detection (keyed on_row_last_updated_at_version+ ID membership) is unaffected.fix(storage)— pinWriteParams::auto_cleanup = Noneat all 11 production sites. lance#6755 flipped this default off; on 6.0.1 the on-by-default cleanup could silently GC versions__manifestpins for time-travel. We own cleanup explicitly, so this removes the default dependency and locks in the snapshot-safe behavior.test(lance)— pin BTREE range-boundary correctness (lance#6796 / issue #6792) via a surface guard reproducing the exact 5-row + BTREE shape.docs(dev)— new 7.0.0 alignment stanza indocs/dev/lance.md;AGENTS.mdsubstrate 6.x → 7.x;invariants.mddeletes/vector gap updated (lance#6658 shipped → MR-A unblocked but separate).Verification
cargo test --workspace --locked: 174 passed, including the full engine suite (surface guards,lance_version_columns,point_in_time,maintenance,consistency,recovery,writes,search,traversal).local_cluster_apply_then_server_boots_from_cluster_state,local_cluster_full_lifecycle_declare_serve_evolve_delete, both spawned-server multi-restart e2es) reproduce identically on unmodified 6.0.1 — they are pre-existing/sandbox-environmental (auth/startup-shaped:401 missing bearer token; missingqueriesarray), not caused by this bump.testing.mddocuments these as sandbox-sensitive withOMNIGRAPH_SKIP_SYSTEM_E2E=1. The single-server cluster e2e passes.scripts/check-agents-md.sh: OK (57 links, 54 docs).Out of scope (separate tickets, unblocked by this bump)
delete_where+ retire D2 (iss-950) — lance#6658 shipped in 7.0.0.Version surveyed: 0.7.0) is intentionally deferred.🤖 Generated with Claude Code
Note
High Risk
This is a major storage-substrate bump affecting every Lance write/commit path and version GC behavior; incorrect auto-cleanup handling could delete time-travel/snapshot versions, though the PR adds explicit skips and regression tests for that.
Overview
Bumps the workspace from Lance 6.0.1 to 7.0.0 (Arrow 58 / DataFusion 53 unchanged) and object_store 0.12.5 → 0.13.2, with a Cargo.lock pin on roaring 0.11.4 so Lance 7 compiles.
Dependency / API touch-ups:
ObjectStoreExtis imported where object_store 0.13 moved convenience methods off the core trait (storage.rs, manifest namespace). Docs (AGENTS.md,docs/dev/lance.md,invariants.md) now describe Lance 7.x and record the alignment audit (lance#6774, #6755, #6796, #6658).Snapshot / time-travel safety (lance#6755): Production writes set
WriteParams { auto_cleanup: None, skip_auto_cleanup: true }on creates/appends/overwrites,CommitBuilder::with_skip_auto_cleanup(true)oncommit_staged, andMergeInsertBuilder::skip_auto_cleanup(true)on__manifestCAS — so Lance’s per-commit hook cannot GC versions still pinned by__manifest, including on graphs upgraded from 6.x with legacy stored auto-cleanup config.Behavior & tests: Change-feed comments and
lance_version_columnsexpectations reflect lance#6774 (merge-insert INSERT rows stamp_row_created_at_versionat commit version; UPDATE still preserves created_at and bumps_row_last_updated_at_version). New guards cover BTREE range boundaries (lance#6796) and skip_auto_cleanup (surface +staged_writesregression).delete_wherestays inline; docs note MR-A is unblocked now that lance#6658 shipped in 7.0.0.Reviewed by Cursor Bugbot for commit e2099fc. Bugbot is set up for automated code reviews on this repo. Configure here.
Greptile Summary
This PR upgrades Lance 6.0.1 → 7.0.0 (object_store 0.12.5 → 0.13.2, roaring 0.11.4 pin via lock file) and makes the codebase correct-by-design against the two critical behavior changes: lance#6755's default-off auto-cleanup and lance#6774's INSERT
_row_created_at_versionstamping.auto_cleanup: None+skip_auto_cleanup: trueare applied at all 11WriteParamssites;CommitBuilder::with_skip_auto_cleanup(true)is wired intocommit_staged;MergeInsertBuilder::skip_auto_cleanup(true)protects the__manifestpublisher.lance_version_columns.rsassertions updated for lance#6774 and the previously-flagged missingbob.2/bob.3UPDATE invariant guards are now added.staged_writes.rsadds a full-stackcommit_stagedregression against a legacy cleanup config.Confidence Score: 5/5
Safe to merge; all write paths that commit through CommitBuilder, WriteParams, or MergeInsertBuilder now suppress Lance's per-commit auto-cleanup hook, protecting __manifest-pinned versions on both new and pre-bump graphs.
Every production write path has been updated with skip_auto_cleanup. The previously missing update-invariant assertions in lance_version_columns.rs are now resolved. Guard 18 includes a self-validating negative control. The roaring 0.11.4 transitive pin is correctly locked via Cargo.lock and CI uses --locked.
No files require special attention beyond the already-known stale 6.0.1 comments in namespace.rs flagged in a prior review.
Important Files Changed
Comments Outside Diff (3)
crates/omnigraph/tests/lance_version_columns.rs, line 263-270 (link)bob.1 == 99(the data value). The load-bearing contract forchanges/mod.rs'sdiff_table_same_lineageis that_row_last_updated_at_versionis bumped on every update; a future Lance regression there would silently miss all updates in change feeds without any test turning red. Since this PR is already updating assertions in this file for the lance#6774 behavior change, completing the guard here is natural.crates/omnigraph/src/db/manifest/namespace.rs, line 235-239 (link)lance-namespace 6.0.1after the bump to 7.0.0. The same stale reference appears inStagedTableNamespace::describe_table~150 lines below. AGENTS.md rule 6 says to fix stale text in the same change rather than leaving it silently wrong.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
crates/omnigraph/tests/lance_version_columns.rs, line 263-270 (link)bob.1 == 99(the data value). The load-bearing contract forchanges/mod.rs'sdiff_table_same_lineageis that_row_last_updated_at_versionis bumped on every merge_insert UPDATE; a future Lance regression here would silently drop all updates from change feeds without any test turning red. Since this PR is already updating assertions in this file for the lance#6774 behavior change, the guard should be completed here.Reviews (3): Last reviewed commit: "test(lance): assert created_at-preserved..." | Re-trigger Greptile