feat: expose staged index segment transactions#6806
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![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: b0d5f35c19
ℹ️ 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".
| async fn build_existing_index_segments_transaction( | ||
| &self, | ||
| index_name: &str, | ||
| column: &str, | ||
| segments: Vec<impl IntoIndexSegment + Send>, | ||
| ) -> Result<Transaction>; |
There was a problem hiding this comment.
Adding build_existing_index_segments_transaction as a required method on the public DatasetIndexExt trait is a breaking API change for downstream crates that implement this trait: they will fail to compile immediately after upgrading, even if they never use this new functionality. Because DatasetIndexExt is publicly exported, this should be introduced with a default implementation (or via a new extension trait) to preserve semver compatibility.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Now inherent on Dataset instead of a trait method, so no required trait method and no default body are needed.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
| "Index name '{index_name}' already exists with different fields, please specify a different name" | ||
| ))); | ||
| } | ||
| let removed_indices = existing_named_indices |
There was a problem hiding this comment.
Zero-fragment placeholder index segments are still treated as disjoint, so committing real segments can leave the placeholder published and break index loading.
There was a problem hiding this comment.
The relocated removal logic keeps the existing_fragments.is_empty() guard (#7141), so the placeholder is still removed; asserted by test_build_existing_index_segments_transaction_removes_empty_segment.
| /// without advancing the dataset version. Callers that need a strict | ||
| /// stage-then-commit workflow can pass the returned transaction to | ||
| /// [`crate::dataset::CommitBuilder`]. | ||
| async fn build_existing_index_segments_transaction( |
There was a problem hiding this comment.
Adding this required method to a public unsealed trait breaks downstream implementors, so existing crates that implement the trait will fail to compile after upgrade.
There was a problem hiding this comment.
Moved off the trait - build_existing_index_segments_transaction is now an inherent Dataset method, so DatasetIndexExt is unchanged and downstream implementors are not affected.
Add `Dataset::build_existing_index_segments_transaction`, which builds the `Operation::CreateIndex` transaction for existing physical index segments without committing it. Callers commit the returned transaction via `CommitBuilder` for a strict stage-then-commit workflow, mirroring `InsertBuilder::execute_uncommitted`. `commit_existing_index_segments` now delegates to it. The method is inherent on `Dataset` rather than a new required method on the public `DatasetIndexExt` trait, so it is non-breaking for downstream trait implementors. Fixes lance-format#6666
53aa593 to
5cfc088
Compare
|
@Xuanwo gently bumping this :) |
| /// so commit it promptly. A concurrent index creation with the same name is | ||
| /// rejected at commit time with a retryable conflict, but other concurrent | ||
| /// changes to the same index between staging and commit — a compaction/rewrite | ||
| /// that remaps it, or dropping/renaming the indexed column — are not |
There was a problem hiding this comment.
The staged index transaction can commit after the indexed column or the same index is dropped, because those concurrent operations are treated as compatible. This can publish stale index metadata or re-create an index the user already removed.
There was a problem hiding this comment.
Both cases are now rejected at commit time (18effc9). drop_index commits CreateIndex with only removed_indices, so the same-name check now also matches the other transaction's removals — a staged commit over a concurrent drop fails with a retryable conflict instead of re-creating the index. A concurrent Project is checked against the indexed field ids — a staged commit after the column is dropped fails as incompatible instead of publishing metadata for a nonexistent field. Asserted by test_build_existing_index_segments_transaction_conflicts_with_index_drop / ..._conflicts_with_column_drop; both fail without the fix. The inline commit_existing_index_segments path shares the conflict resolver, so the same race there is closed as well.
…x/column drops Co-Authored-By: Claude Fable 5 <[email protected]>
📝 WalkthroughWalkthroughAdds a public API for building uncommitted transactions from existing index segments, refactors the existing commit path to use it, and strengthens rebasing conflicts for concurrent index removal and schema projection changes. ChangesIndex transaction staging and conflict handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Dataset
participant Transaction
participant Commit
participant TransactionRebase
Dataset->>Dataset: Build index segment transaction
Dataset->>Transaction: CreateIndex with new_indices and removed_indices
Dataset-->>Commit: Return uncommitted transaction
Commit->>TransactionRebase: Rebase against concurrent operations
TransactionRebase-->>Commit: Accept or return conflict
Commit->>Transaction: Apply transaction
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/lance/src/io/commit/conflict_resolver.rs (1)
506-537: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winInclude singleton names in concurrent-removal detection.
FRAG_REUSE_INDEX_NAMEandMEM_WAL_INDEX_NAMEare excluded fromhas_regular_name_conflict, while their dedicated checks only inspectcreated_indices. A concurrent removal-only transaction can therefore still be rebased and resurrect either singleton. Checkother_removed_indicesfor these names too.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance/src/io/commit/conflict_resolver.rs` around lines 506 - 537, Update the conflict checks around has_regular_name_conflict so FRAG_REUSE_INDEX_NAME and MEM_WAL_INDEX_NAME are also compared against other_removed_indices. Preserve the existing dedicated concurrent-creation checks, and reject rebasing when either singleton appears in the concurrent removals to prevent resurrection.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/lance/src/index.rs`:
- Around line 7330-7333: Update the conflict assertions in
rust/lance/src/index.rs:7330-7333 and rust/lance/src/index.rs:7389-7392 to
validate both the existing Error::RetryableCommitConflict variant and diagnostic
message content. In the first test, assert that the message identifies the
concurrent same-name index removal; in the second, assert that it identifies the
missing indexed field after projection.
- Around line 972-979: Update the removed_indices filtering in the CreateIndex
flow so every existing index with the same name is removed, regardless of its
type_url; do not exclude incompatible metadata from replacement. Preserve the
Operation::CreateIndex contract that same-name indices are fully replaced, or
explicitly reject the type mismatch before committing rather than retaining both
indexes.
- Around line 929-932: Update the error construction in the index-creation flow,
including the missing-column branch around self.schema().field(column) and the
incompatible existing-index-name branch, to use Error::invalid_input instead of
Error::index for caller-supplied argument failures. Preserve the existing error
messages and control flow.
- Around line 891-897: Update the transaction staging/commit flow documented
near the public index API to detect concurrent compaction or rewrite operations
that defer index remapping and reject the rebase with a retryable conflict
instead of allowing duplicate index metadata. Ensure the staged transaction is
not exposed as successfully committed unless it is correctly remapped, and
remove the documentation that permits this deferred-remap case.
---
Outside diff comments:
In `@rust/lance/src/io/commit/conflict_resolver.rs`:
- Around line 506-537: Update the conflict checks around
has_regular_name_conflict so FRAG_REUSE_INDEX_NAME and MEM_WAL_INDEX_NAME are
also compared against other_removed_indices. Preserve the existing dedicated
concurrent-creation checks, and reject rebasing when either singleton appears in
the concurrent removals to prevent resurrection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 49bcdbff-d26d-4152-b6bf-47a8a0a358d4
📒 Files selected for processing (2)
rust/lance/src/index.rsrust/lance/src/io/commit/conflict_resolver.rs
| let Some(field) = self.schema().field(column) else { | ||
| return Err(Error::index(format!( | ||
| "CreateIndex: column '{column}' does not exist" | ||
| ))); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use Error::invalid_input for invalid API arguments.
Both an unknown column and an incompatible existing index name are caller-input failures, not index subsystem failures.
Proposed fix
- return Err(Error::index(format!(
+ return Err(Error::invalid_input(format!(
"CreateIndex: column '{column}' does not exist"
)));
...
- return Err(Error::index(format!(
+ return Err(Error::invalid_input(format!(
"Index name '{index_name}' already exists with different fields, \
please specify a different name"
)));As per coding guidelines, “use Error::invalid_input for caller data issues.”
Also applies to: 962-970
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/lance/src/index.rs` around lines 929 - 932, Update the error
construction in the index-creation flow, including the missing-column branch
around self.schema().field(column) and the incompatible existing-index-name
branch, to use Error::invalid_input instead of Error::index for caller-supplied
argument failures. Preserve the existing error messages and control flow.
Source: Coding guidelines
There was a problem hiding this comment.
These are moved verbatim, and CreateIndexBuilder raises Error::index for the identical messages — changing only this path would give the same failure two different variants, and since commit_existing_index_segments now delegates here it would also change a shipped API error variant. Agreed with the guideline; deferring to a follow-up that sweeps all three paths together.
There was a problem hiding this comment.
@ragnorc, agreed—keeping the existing Error::index behavior is the right scoped decision here. Changing only the newly extracted path would introduce inconsistency with CreateIndexBuilder and alter the existing commit_existing_index_segments API behavior.
A coordinated follow-up across all creation paths is appropriate. Would you like me to open a GitHub issue to track that cleanup?
✏️ Learnings added
Learnt from: ragnorc
Repo: lance-format/lance PR: 6806
File: rust/lance/src/index.rs:930-933
Timestamp: 2026-07-19T00:29:32.460Z
Learning: In `rust/lance/src/index.rs`, `Dataset::build_existing_index_segments_transaction` preserves validation behavior extracted from `DatasetIndexExt::commit_existing_index_segments`. The identical missing-column and conflicting-index-name errors in `CreateIndexBuilder` currently use `Error::index`; changing only the staged-segment path to `Error::invalid_input` would cause inconsistent error variants and change a shipped API behavior. Normalize these errors only in a coordinated follow-up across all index-creation paths.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
- reject staged singleton (frag-reuse/MemWAL) creation over a concurrent removal-only transaction, with a resolver unit test - assert conflict message content in the new staged-transaction tests - reword the staging doc: a deferred-remap rewrite leaves stale fragment coverage until the remap catches up, not duplicate entries Co-Authored-By: Claude Fable 5 <[email protected]>
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
…ame conflicts - validate CreateIndex at manifest build time: indexed fields must still exist in the schema (closes the alter_columns type-cast gap that the pairwise Project check could not see, since a cast commits Merge with a new field id) and non-system removed segments must still be present (closes the mirror race where a drop racing a same-name replacement silently no-ops) - collapse the frag-reuse/MemWAL/regular-name conflict booleans into one symmetric name-intersection rule covering both directions - regression tests: staged commit after an alter_columns cast fails as incompatible; drop_index racing a replacement fails retryably and succeeds on retry; resolver unit test covers all conflict directions Co-Authored-By: Claude Fable 5 <[email protected]>
|
Follow-up to the review discussion: the conflict checks are now layered (c24fe74). Resolver rejects any two index transactions touching a common name in either direction - a drop racing a same-name replacement previously no-op'd silently, the mirror of the resurrection case. And |
2 participants
Summary
Adds
Dataset::build_existing_index_segments_transaction, which builds theOperation::CreateIndextransaction for existing physical index segmentswithout committing it. Callers commit the returned
TransactionviaCommitBuilderfor a strict stage-then-commit workflow, mirroringInsertBuilder::execute_uncommitted/DeleteBuilder::execute_uncommitted.commit_existing_index_segmentsnow delegates to it.Fixes #6666.
Design notes
Dataset, not a new required method on the publicDatasetIndexExttrait, so it is non-breaking for downstream traitimplementors (addresses the earlier review feedback). The trait has a single
in-tree implementor and is not object-safe, and the staging convention
(
execute_uncommitted) is otherwise inherent on builders/Dataset— so aninherent method is the most consistent, lowest-risk placement.
Transaction(no wrapper struct):CreateIndexcarries noaffected_rows/side-channel, so there is nothing extra to reconcile at commit.index transactions that touch a common index name, in either direction, with
a retryable conflict — creating over a concurrent removal must not resurrect
a dropped index, and removing over a concurrent replacement must not silently
no-op the drop.
build_manifestthen enforces the apply-time invariants thatpairwise checks cannot enumerate: indexed fields must still exist in the final
schema (a
drop_columnsprojection or analter_columnstype cast removes orre-ids the field) and non-system removed segments must still be present. Two
deliberate behavior changes: a concurrent double-drop and a commit whose staged
removals were concurrently removed now fail retryably instead of silently
succeeding. A compaction/rewrite that defers index remapping remains
optimistic; committed segments may cover already-compacted fragments until the
deferred remap catches up, so callers should still commit promptly.
on-disk files (idempotent; any files left behind on an abandoned build are
reclaimed by
cleanup_old_versions) — documented under# Side effects.Tests
test_build_existing_index_segments_transaction_does_not_commit— stagingdoes not publish the index; the returned transaction commits via
CommitBuilder.test_build_existing_index_segments_transaction_removes_empty_segment— thezero-fragment placeholder guard fires on the staged path.
test_build_existing_index_segments_transaction_commits_after_version_advances— a transaction staged at version N commits cleanly after an unrelated append
moves HEAD past N.
test_build_existing_index_segments_transaction_conflicts_with_index_drop—a staged commit over a concurrent
drop_indexfails with a retryableconflict instead of re-creating the dropped index. Fails without the fix.
test_build_existing_index_segments_transaction_conflicts_with_column_drop— a staged commit after the indexed column is dropped fails as incompatible
instead of publishing metadata for a nonexistent field. Fails without the
fix.
test_build_existing_index_segments_transaction_conflicts_with_column_cast— a staged commit after an
alter_columnstype cast (Mergewith a newfield id) fails as incompatible. Fails without the fix.
test_drop_index_conflicts_with_concurrent_replacement— a drop racing asame-name replacement fails retryably instead of silently no-op'ing, and
succeeds on retry. Fails without the fix.
test_index_transactions_conflict_on_shared_names— resolver-level matrix:transactions on a shared name conflict in every direction (regular names and
system singletons); disjoint names never conflict.
Verification
cargo fmt --allcargo clippy -p lance --tests -- -D warningscargo test -p lance --lib -- build_existing_index_segments_transaction index_transactions_conflict drop_index_conflicts(8 passed)cargo test -p lance --lib -- existing_index_segments conflict drop_index create_index stale frag_reuse mem_wal(638 passed)cargo test -p lance --doc build_existing_index_segments_transactionSummary by CodeRabbit
New Features
Bug Fixes