docs(index): fix the stale default for num_indices_to_merge - #9240
Open
jackylee-ch wants to merge 1 commit into
Open
jackylee-ch wants to merge 1 commit into
jackylee-ch wants to merge 1 commit into
Conversation
The doc comment on `OptimizeOptions::num_indices_to_merge` says the default is `1`, but `impl Default for OptimizeOptions` sets it to `None`, and the paragraph right below already describes the `None` behavior. The field became an `Option<usize>` in lance-format#4837, whose description notes that "the default is `None` (`1` before this)" -- the first line of the comment was left behind. A caller reading the published docs concludes that `OptimizeOptions::default()` merges one delta index per optimize call and therefore keeps the delta count bounded, when the actual default creates a new delta index unless a partition is split. The Python binding already documents this correctly. Match the wording to `retrain`, which states its default the same way in the same struct. ## Testing `cargo fmt --all -- --check` and `cargo clippy --release -p lance-index --all-features --all-targets -- -D warnings`.
github-actions
Bot
added
A-index
![lance-gatekeeper[bot]](https://avatars.githubusercontent.com/in/4433141?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
✅ Gate recommendation: approve.
This corrects the stale Rust API documentation to match OptimizeOptions::default() and the None semantics already documented for incremental index optimization. The change is appropriately limited to the misleading default statement.
lance-gatekeeper
Bot
added
the
K-approved
This branch has not been deployed
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.
The doc comment on
OptimizeOptions::num_indices_to_mergesays the default is1, butimpl Default for OptimizeOptionssets it toNone, and the paragraph right below already describes theNonebehavior. The field became anOption<usize>in #4837, whose description notes that "the default isNone(1before this)" -- the first line of the comment was left behind.A caller reading the published docs concludes that
OptimizeOptions::default()merges one delta index per optimize call and therefore keeps the delta count bounded, when the actual default creates a new delta index unless a partition is split. The Python binding already documents this correctly.Match the wording to
retrain, which states its default the same way in the same struct.Testing
cargo fmt --all -- --checkandcargo clippy --release -p lance-index --all-features --all-targets -- -D warnings.