Skip to content

feat: add skip_merge for FTS index build#5570

Merged
BubbleCal merged 1 commit intomainfrom
fts-skip-merge
Dec 24, 2025
Merged

feat: add skip_merge for FTS index build#5570
BubbleCal merged 1 commit intomainfrom
fts-skip-merge

Conversation

@BubbleCal
Copy link
Copy Markdown
Contributor

No description provided.

@github-actions github-actions Bot added enhancement New feature or request python labels Dec 24, 2025
@github-actions
Copy link
Copy Markdown
Contributor

Code Review

This PR adds a skip_merge option to the FTS index build, enabling distributed/fragment-level indexing where merge is handled separately. The implementation looks solid overall. A few observations:

P1: Missing Error Handling for Partial Copy Failures

In builder.rs:318-359, if a file copy fails partway through the skip_merge path, the destination store may be left in an inconsistent state with some partition files copied but not others. Consider:

  • Adding cleanup logic on error, or
  • Documenting this as a caller responsibility

Minor Observations (not blocking):

  1. Python docstring placement: The skip_merge parameter documentation is inserted between with_position and base_tokenizer. While functional, grouping it with other distributed indexing params (fragment_ids, index_uuid) might improve discoverability.

  2. Test coverage is good: The test validates the happy path and verifies partition files are written correctly. Edge cases like empty partitions or error recovery could be considered for future work.

The implementation correctly handles both fresh builds and incremental indexing scenarios. The test is well-structured.

@codecov
Copy link
Copy Markdown

codecov Bot commented Dec 24, 2025

Codecov Report

❌ Patch coverage is 74.10714% with 29 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
rust/lance-index/src/scalar/inverted/builder.rs 73.58% 11 Missing and 17 partials ⚠️
rust/lance-index/src/scalar/inverted/tokenizer.rs 83.33% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@BubbleCal BubbleCal merged commit c3e115b into main Dec 24, 2025
30 checks passed
@BubbleCal BubbleCal deleted the fts-skip-merge branch December 24, 2025 11:15
jackye1995 pushed a commit to jackye1995/lance that referenced this pull request Jan 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants