Skip to content

refactor(index): remove dead segment commit branch - #7971

Open
ddupg wants to merge 1 commit into
lance-format:mainfrom
ddupg:chore/ddu-322-remove-segment-commit-branch
Open

refactor(index): remove dead segment commit branch#7971
ddupg wants to merge 1 commit into
lance-format:mainfrom
ddupg:chore/ddu-322-remove-segment-commit-branch

Conversation

@ddupg

@ddupg ddupg commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Remove the dead segment-commit branch from CreateIndexBuilder::execute.
  • Remove helpers used solely by that branch and rename the stale IVF-RQ test.
  • The branch became redundant in #6889, which replaced its segment metadata conversion with the same CreateIndex transaction as the fallback branch but left the guard in place.

Testing

  • cargo fmt --all -- --check
  • cargo clippy --all --tests --benches -- -D warnings
  • Focused index::create vector, IVF-RQ, and BTree tests

@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

CreateIndexBuilder::execute now builds create-index transactions with new_idx.dataset_version through a single path. Obsolete segment-commit selection helpers are removed, and the related IVF-RQ test is renamed without changing its assertions.

Changes

Index transaction construction

Layer / File(s) Summary
Unify index transaction construction
rust/lance/src/index/create.rs
CreateIndexBuilder::execute uses one TransactionBuilder path with the dataset version and transaction properties; obsolete path-selection helpers are removed and the affected test is renamed.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

Suggested labels: A-index

Suggested reviewers: xuanwo, jackye1995, wjones127

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 83.33% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly reflects the main change: removing the dead segment-commit branch in index creation.
Description check ✅ Passed The description matches the changeset and explains the removed branch, helper cleanup, and test rename.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@ddupg
ddupg marked this pull request as ready for review July 24, 2026 08:54
@codecov

codecov Bot commented Jul 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@ddupg
ddupg force-pushed the chore/ddu-322-remove-segment-commit-branch branch from ef796d0 to a8e243f Compare July 27, 2026 01:58

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.

🟡 Other comments (1)
rust/lance/src/index/create.rs-1831-1834 (1)

1831-1834: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Assert the concrete error variant as well as the message.

These tests only check to_string(), so an incorrect error type with the same text would still pass. Assert the expected Error variant and retain the message assertion.

As per coding guidelines, tests must assert both the error variant and message content.

Also applies to: 3121-3124

🤖 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/create.rs` around lines 1831 - 1834, Update the affected
error tests around the merge-error assertion and the corresponding assertion at
the other reported location to match the concrete expected Error variant before
retaining the existing “future dataset version” message check. Ensure both
assertions validate the error type and message content, using the project’s
established variant-matching pattern.

Source: Coding guidelines

🤖 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.

Other comments:
In `@rust/lance/src/index/create.rs`:
- Around line 1831-1834: Update the affected error tests around the merge-error
assertion and the corresponding assertion at the other reported location to
match the concrete expected Error variant before retaining the existing “future
dataset version” message check. Ensure both assertions validate the error type
and message content, using the project’s established variant-matching pattern.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: QUIET

Plan: Pro Plus

Run ID: 941b7c73-892b-4749-b4a5-e52d6bbd4814

📥 Commits

Reviewing files that changed from the base of the PR and between ef796d0 and a8e243f.

📒 Files selected for processing (1)
  • rust/lance/src/index/create.rs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant