refactor(index): introduce lance-index-core crate#7713
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change adds ChangesIndex core extraction
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
A-python
westonpace
changed the title
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/lance-index/src/scalar/lance_format.rs (1)
222-255: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winEmpty-range fast path ignores
projection, returning the wrong schema.In
read_range,read_ranges, andread_range_stream, the empty-input short-circuit builds the returned (empty) batch/stream fromself.0.schema()— the reader's full schema — even when aprojectionwas requested. Per theIndexReader::read_rangecontract ("If projection is Some, only return the columns in the projection"), this should return a batch/stream with the projected schema, not the full schema. Callers relying on schema consistency (e.g. downstream concatenation or column-count assumptions) can break specifically when the queried range happens to be empty.🐛 Proposed fix (illustrative, applies to all three methods)
async fn read_range( &self, range: std::ops::Range<usize>, projection: Option<&[&str]>, ) -> Result<RecordBatch> { - if range.is_empty() { - return Ok(RecordBatch::new_empty(Arc::new( - self.0.schema().as_ref().into(), - ))); - } let projection = if let Some(projection) = projection { ReaderProjection::from_column_names( self.0.metadata().version(), self.0.schema(), projection, )? } else { ReaderProjection::from_whole_schema(self.0.schema(), self.0.metadata().version()) }; + if range.is_empty() { + return Ok(RecordBatch::new_empty(Arc::new( + projection.schema.as_ref().into(), + ))); + } let batches = self .0 .read_stream_projected(Also applies to: 257-266, 337-347
🤖 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-index/src/scalar/lance_format.rs` around lines 222 - 255, Update the empty-input fast paths in read_range, read_ranges, and read_range_stream to derive the returned empty RecordBatch or stream schema from the requested projection rather than always using self.0.schema(). Reuse the same ReaderProjection/from_column_names logic as the non-empty path, preserving full-schema behavior when projection is None and ensuring projected column order and schema are returned consistently.
🤖 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-index-core/src/scalar.rs`:
- Around line 132-135: Replace the unwrap in Scalar's with_params method with
expect and provide a clear message describing that parameter serialization must
succeed.
- Around line 462-473: Update `filter_row_ids` for the `Self::Fragments` variant
to decode each row ID through `RowAddress::from(id).fragment_id()` instead of
manually shifting and casting `(id >> 32) as u32`; preserve the existing
`to_keep.contains(...)` behavior and ensure the appropriate `RowAddress` symbol
is available.
- Line 87: Update the fallback conversion branch in the `IndexType` handling to
include the offending value in the `Error::index` message, using the matched
value’s debug representation and identifying it as the invalid index type
instead of returning only a generic message.
In `@rust/lance-index/src/scalar.rs`:
- Around line 51-83: Add Rustdoc usage examples to both public helpers,
index_files_to_table and table_files_to_index, demonstrating construction of the
source IndexFile vector, conversion, and an assertion on the converted fields;
include appropriate rustdoc code fences and preserve links to the relevant
types.
In `@rust/lance-index/src/scalar/lance_format.rs`:
- Around line 368-372: Prevent num_batches from panicking when batch_size is
zero: update the IndexReader::num_batches contract/implementation around
FileReader::num_rows and div_ceil to return an appropriate Result/error (or
other existing non-panicking API representation), and handle the invalid input
consistently with callers. Add coverage for batch_size == 0.
In `@rust/lance/src/index/scalar/inverted.rs`:
- Line 17: Run cargo fmt and apply its formatting to the import, ensuring the
lance_index::scalar::index_files_to_table use statement matches the repository’s
Rust formatting rules.
In `@rust/lance/src/index/vector/ivf/v2.rs`:
- Line 5188: Move the inline `use futures::TryStreamExt as _;` import out of the
function and add it with the file-level imports at the top of `v2.rs`,
preserving the existing alias and removing the in-function declaration.
- Around line 5175-5190: Guard the collected `batches` in the code reading the
projected stream before accessing `batches[0]`. Use the reader schema (or
another available schema) directly when calling `arrow::compute::concat_batches`
so empty auxiliary files are handled without indexing an empty vector.
---
Outside diff comments:
In `@rust/lance-index/src/scalar/lance_format.rs`:
- Around line 222-255: Update the empty-input fast paths in read_range,
read_ranges, and read_range_stream to derive the returned empty RecordBatch or
stream schema from the requested projection rather than always using
self.0.schema(). Reuse the same ReaderProjection/from_column_names logic as the
non-empty path, preserving full-schema behavior when projection is None and
ensuring projected column order and schema are returned consistently.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 2fb24690-f535-4f2f-99a8-b0dc148269a6
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockjava/lance-jni/Cargo.lockis excluded by!**/*.lockpython/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (38)
Cargo.tomlpython/src/utils.rsrust/lance-index-core/Cargo.tomlrust/lance-index-core/src/lib.rsrust/lance-index-core/src/metrics.rsrust/lance-index-core/src/scalar.rsrust/lance-index/Cargo.tomlrust/lance-index/src/frag_reuse.rsrust/lance-index/src/lib.rsrust/lance-index/src/mem_wal.rsrust/lance-index/src/metrics.rsrust/lance-index/src/scalar.rsrust/lance-index/src/scalar/bloomfilter.rsrust/lance-index/src/scalar/btree.rsrust/lance-index/src/scalar/expression.rsrust/lance-index/src/scalar/lance_format.rsrust/lance-index/src/scalar/registry.rsrust/lance-index/src/scalar/rtree/sort/hilbert_sort.rsrust/lance-index/src/scalar/zonemap.rsrust/lance-index/src/vector/flat/transform.rsrust/lance-index/src/vector/hnsw/builder.rsrust/lance-index/src/vector/kmeans.rsrust/lance-namespace-impls/src/dir/manifest.rsrust/lance-table/src/system_index/frag_reuse.rsrust/lance-table/src/system_index/mem_wal.rsrust/lance/src/dataset/optimize.rsrust/lance/src/index.rsrust/lance/src/index/append.rsrust/lance/src/index/create.rsrust/lance/src/index/scalar.rsrust/lance/src/index/scalar/bitmap.rsrust/lance/src/index/scalar/btree.rsrust/lance/src/index/scalar/fmindex.rsrust/lance/src/index/scalar/inverted.rsrust/lance/src/index/scalar/label_list.rsrust/lance/src/index/scalar/zonemap.rsrust/lance/src/index/vector/ivf/io.rsrust/lance/src/index/vector/ivf/v2.rs
| /// Convert a `Vec<`[`lance_index_core::scalar::IndexFile`]`>` to a | ||
| /// `Vec<`[`lance_table::format::IndexFile`]`>`. | ||
| /// | ||
| /// This is primarily for convenience to avoid a bunch of string | ||
| /// constants and provide some auto-complete. This type should not | ||
| /// be used in the manifest as plugins cannot add new entries. | ||
| #[derive(Debug, Clone, PartialEq, Eq, DeepSizeOf)] | ||
| pub enum BuiltinIndexType { | ||
| BTree, | ||
| Bitmap, | ||
| LabelList, | ||
| NGram, | ||
| ZoneMap, | ||
| BloomFilter, | ||
| RTree, | ||
| Inverted, | ||
| Fm, | ||
| } | ||
|
|
||
| impl BuiltinIndexType { | ||
| pub fn as_str(&self) -> &str { | ||
| match self { | ||
| Self::BTree => "btree", | ||
| Self::Bitmap => "bitmap", | ||
| Self::LabelList => "labellist", | ||
| Self::NGram => "ngram", | ||
| Self::ZoneMap => "zonemap", | ||
| Self::Inverted => "inverted", | ||
| Self::BloomFilter => "bloomfilter", | ||
| Self::RTree => "rtree", | ||
| Self::Fm => "fm", | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl TryFrom<IndexType> for BuiltinIndexType { | ||
| type Error = Error; | ||
|
|
||
| fn try_from(value: IndexType) -> Result<Self> { | ||
| match value { | ||
| IndexType::BTree => Ok(Self::BTree), | ||
| IndexType::Bitmap => Ok(Self::Bitmap), | ||
| IndexType::LabelList => Ok(Self::LabelList), | ||
| IndexType::NGram => Ok(Self::NGram), | ||
| IndexType::ZoneMap => Ok(Self::ZoneMap), | ||
| IndexType::Inverted => Ok(Self::Inverted), | ||
| IndexType::BloomFilter => Ok(Self::BloomFilter), | ||
| IndexType::RTree => Ok(Self::RTree), | ||
| IndexType::Fm => Ok(Self::Fm), | ||
| _ => Err(Error::index("Invalid index type".to_string())), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #[derive(Debug, Clone, PartialEq)] | ||
| pub struct ScalarIndexParams { | ||
| /// The type of index to create | ||
| /// | ||
| /// Plugins may add additional index types. Index type lookup is case-insensitive. | ||
| pub index_type: String, | ||
| /// The parameters to train the index | ||
| /// | ||
| /// This should be a JSON string. The contents of the JSON string will be specific to the | ||
| /// index type. If not set, then default parameters will be used for the index type. | ||
| pub params: Option<String>, | ||
| } | ||
|
|
||
| impl Default for ScalarIndexParams { | ||
| fn default() -> Self { | ||
| Self { | ||
| index_type: BuiltinIndexType::BTree.as_str().to_string(), | ||
| params: None, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| impl ScalarIndexParams { | ||
| /// Creates a new ScalarIndexParams from one of the builtin index types | ||
| pub fn for_builtin(index_type: BuiltinIndexType) -> Self { | ||
| Self { | ||
| index_type: index_type.as_str().to_string(), | ||
| params: None, | ||
| } | ||
| } | ||
|
|
||
| /// Create a new ScalarIndexParams with the given index type | ||
| pub fn new(index_type: String) -> Self { | ||
| Self { | ||
| index_type, | ||
| params: None, | ||
| } | ||
| } | ||
|
|
||
| /// Set the parameters for the index | ||
| pub fn with_params<ParamsType: Serialize>(mut self, params: &ParamsType) -> Self { | ||
| self.params = Some(serde_json::to_string(params).unwrap()); | ||
| self | ||
| } | ||
| } | ||
|
|
||
| impl IndexParams for ScalarIndexParams { | ||
| fn as_any(&self) -> &dyn std::any::Any { | ||
| self | ||
| } | ||
|
|
||
| fn index_name(&self) -> &str { | ||
| LANCE_SCALAR_INDEX | ||
| } | ||
| /// These two structs have identical fields; this helper bridges the crate | ||
| /// boundary without relying on orphan-rule–violating `From` impls. | ||
| pub fn index_files_to_table( | ||
| files: Vec<lance_index_core::scalar::IndexFile>, | ||
| ) -> Vec<lance_table::format::IndexFile> { | ||
| files | ||
| .into_iter() | ||
| .map(|f| lance_table::format::IndexFile { | ||
| path: f.path, | ||
| size_bytes: f.size_bytes, | ||
| }) | ||
| .collect() | ||
| } | ||
|
|
||
| /// Convert a `Vec<`[`lance_table::format::IndexFile`]`>` to a | ||
| /// `Vec<`[`lance_index_core::scalar::IndexFile`]`>`. | ||
| /// | ||
| /// These two structs have identical fields; this helper bridges the crate | ||
| /// boundary without relying on orphan-rule–violating `From` impls. | ||
| pub fn table_files_to_index( | ||
| files: Vec<lance_table::format::IndexFile>, | ||
| ) -> Vec<lance_index_core::scalar::IndexFile> { | ||
| files | ||
| .into_iter() | ||
| .map(|f| lance_index_core::scalar::IndexFile { | ||
| path: f.path, | ||
| size_bytes: f.size_bytes, | ||
| }) | ||
| .collect() | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add doc examples to the new public conversion helpers.
index_files_to_table and table_files_to_index are new public functions with doc comments describing purpose and linking to the relevant structs, but neither includes a usage example. As per coding guidelines, "All public APIs must have documentation with examples, and documentation should link to relevant structs and methods."
📝 Example doc addition
/// Convert a `Vec<`[`lance_index_core::scalar::IndexFile`]`>` to a
/// `Vec<`[`lance_table::format::IndexFile`]`>`.
///
/// These two structs have identical fields; this helper bridges the crate
/// boundary without relying on orphan-rule–violating `From` impls.
+///
+/// # Examples
+///
+/// ```
+/// # use lance_index::scalar::index_files_to_table;
+/// let core_files = vec![lance_index_core::scalar::IndexFile {
+/// path: "index.idx".to_string(),
+/// size_bytes: 1024,
+/// }];
+/// let table_files = index_files_to_table(core_files);
+/// assert_eq!(table_files[0].path, "index.idx");
+/// ```
pub fn index_files_to_table(🤖 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-index/src/scalar.rs` around lines 51 - 83, Add Rustdoc usage
examples to both public helpers, index_files_to_table and table_files_to_index,
demonstrating construction of the source IndexFile vector, conversion, and an
assertion on the converted fields; include appropriate rustdoc code fences and
preserve links to the relevant types.
Source: Coding guidelines
| lance_encoding::decoder::FilterExpression::no_filter(), | ||
| ) | ||
| .await?; | ||
| use futures::TryStreamExt as _; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Move use futures::TryStreamExt to the top of the file.
Inline use inside a function body violates the repo's import-placement convention.
As per coding guidelines, "Place use imports at the top of the file, not inline within function bodies."
♻️ Proposed fix
- use futures::TryStreamExt as _;
let batches = batches.try_collect::<Vec<_>>().await?;Add near the top of the file instead:
use futures::TryStreamExt as _;🤖 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/vector/ivf/v2.rs` at line 5188, Move the inline `use
futures::TryStreamExt as _;` import out of the function and add it with the
file-level imports at the top of `v2.rs`, preserving the existing alias and
removing the in-function declaration.
Source: Coding guidelines
| let batch = self.hnsw.to_batch()?; | ||
| let metadata = batch.schema_ref().metadata().clone(); | ||
| writer.write_record_batch(batch).await?; | ||
| writer.write(&[batch]).await?; |
There was a problem hiding this comment.
The write_record_batch method was inadvertently being provided by the IndexWriter trait but calling write is fine.
westonpace
force-pushed
the
refactor/core-index-crate
branch
from
57dccaa to
8ef30c2
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
rust/lance/src/index/vector/ivf/v2.rs (1)
1660-1839: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftConsider extracting the new batched-search scheduling into a dedicated submodule.
This block implements a producer/consumer batching scheduler (channel sizing, greedy draining, cancellation probing, per-batch
spawn_cpudispatch) inlined into an already very large file (v2.rs, 6500+ lines). The logic itself looks correct (cancellation checked synchronously so no CPU-pool thread parks on a channel, errors propagated after flushing partial batches, clean producer teardown on receiver drop), but its size and self-contained nature make it a good candidate for a dedicated module.As per coding guidelines, "Extract substantial new logic such as bin packing or scheduling into dedicated submodules instead of inlining it into large files."
🤖 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/vector/ivf/v2.rs` around lines 1660 - 1839, Extract the producer/consumer batched-search scheduling logic from the enclosing method into a dedicated submodule, including channel setup, greedy draining, cancellation probing, per-batch spawn_cpu execution, and error propagation. Expose a focused helper using the existing PreparedPartitionSearch and search-control inputs, then replace the inline block with a concise call while preserving batching, cancellation, teardown, and partial-batch error behavior.Source: Coding guidelines
rust/lance-index/src/scalar/lance_format.rs (1)
222-255: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
read_rangemust not assert a single batch
read_stream_projectedcan legally split a single range into multiple batches when a column would otherwise exceed Arrow’s size limits, soassert_eq!(batches.len(), 1)can panic on valid reads. Merge the batches here instead of assuming one output.🤖 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-index/src/scalar/lance_format.rs` around lines 222 - 255, The read_range method incorrectly assumes read_stream_projected always returns one batch. Replace the batches.len() assertion and direct indexing with logic that merges all collected batches into a single RecordBatch, preserving the existing empty-range and projection behavior.Source: Coding guidelines
rust/lance-index/src/frag_reuse.rs (1)
81-83: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReturn
NotSupportedinstead of panicking.
FragReuseIndexHandleis exposed asdyn Index, so a generic call tocalculate_included_fragscurrently terminates the process.Proposed fix
async fn calculate_included_frags(&self) -> Result<RoaringBitmap> { - unimplemented!() + Err(lance_core::Error::not_supported( + "calculating included fragments is not supported for fragment reuse indices", + )) }As per coding guidelines: “Return
LanceError::NotSupportedinstead oftodo!()orunimplemented!()for unsupported code paths.”🤖 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-index/src/frag_reuse.rs` around lines 81 - 83, Replace the unimplemented!() in FragReuseIndexHandle::calculate_included_frags with an Err(LanceError::NotSupported(...)) result, using the project’s existing error-construction conventions and preserving the method’s Result<RoaringBitmap> signature.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.
Inline comments:
In `@Cargo.toml`:
- Line 71: Refresh java/lance-jni/Cargo.lock so its lance-index-core dependency
resolves to version 9.0.0-beta.20, matching the updated workspace dependency and
the other lockfiles; regenerate the lockfile using Cargo rather than editing
unrelated dependency entries.
In `@rust/lance-index-core/src/lib.rs`:
- Around line 13-42: Complete rustdoc for the public plugin API, including the
modules, Index trait and all public methods, and IndexType variants in the
referenced sections. Add concise usage examples where appropriate, use intra-doc
links to related structs, methods, and types, replace ordinary comments with
semantic rustdoc, and describe each enum variant’s behavior rather than only
naming it.
In `@rust/lance-index-core/src/metrics.rs`:
- Around line 6-117: Document the complete public metrics API: add semantic
rustdoc for each metric constant and the public fields and methods of
LocalMetricsCollector, including dump_into. Extend MetricsCollector
documentation with runnable examples covering implementing or using a collector,
forwarding aggregated metrics via dump_into, and attaching the optional IoStats
returned by io_stats for I/O attribution.
In `@rust/lance-index-core/src/scalar.rs`:
- Around line 164-167: Replace the HashMap<String, String> parameter in
finish_with_metadata with a dedicated strongly typed metadata struct that models
the supported fields and valid combinations; update implementations and callers
to use this type, converting its values to string key-value pairs only in the
storage/serialization adapter.
- Around line 28-71: Add comprehensive rustdoc for the extracted scalar-index
API, covering public types, constants, enum variants, configuration fields,
constructors, and remapping methods across the referenced sections. Add concise
usage examples for each applicable public API and cross-link related contracts
using intra-doc links to symbols such as IndexFile, BuiltinIndexType, and
associated methods, ensuring all documented behavior and configuration semantics
are clear.
In `@rust/lance-index/src/frag_reuse.rs`:
- Around line 8-9: Move the Index and RowIdRemapper implementations from the
public wrapper in rust/lance-index/src/frag_reuse.rs to FragReuseIndex in
lance-table, which owns the type. Remove the wrapper and its
conversion/adaptation logic, then update imports and callers to use
FragReuseIndex directly while preserving the existing trait behavior.
In `@rust/lance-index/src/scalar/btree.rs`:
- Line 5329: Update the test loop around the identity-preservation scenario so
rows in the 0..1000 range remain mapped to their original row IDs, while only
the 10000..15000 range is remapped to new IDs. Strengthen the assertions to
explicitly verify identity mapping for the first range and remapping behavior
for the second range, using the surrounding test’s existing setup and assertion
symbols.
In `@rust/lance/src/index/vector/ivf/v2.rs`:
- Around line 145-158: Replace the process-panicking assert in
STREAMING_SEARCH_BATCH_SIZE initialization with graceful handling: when the
parsed batch size is zero, emit a warning and use
DEFAULT_STREAMING_SEARCH_BATCH_SIZE instead. Preserve the existing parse error
behavior and ensure the resulting value is always positive without asserting.
---
Outside diff comments:
In `@rust/lance-index/src/frag_reuse.rs`:
- Around line 81-83: Replace the unimplemented!() in
FragReuseIndexHandle::calculate_included_frags with an
Err(LanceError::NotSupported(...)) result, using the project’s existing
error-construction conventions and preserving the method’s Result<RoaringBitmap>
signature.
In `@rust/lance-index/src/scalar/lance_format.rs`:
- Around line 222-255: The read_range method incorrectly assumes
read_stream_projected always returns one batch. Replace the batches.len()
assertion and direct indexing with logic that merges all collected batches into
a single RecordBatch, preserving the existing empty-range and projection
behavior.
In `@rust/lance/src/index/vector/ivf/v2.rs`:
- Around line 1660-1839: Extract the producer/consumer batched-search scheduling
logic from the enclosing method into a dedicated submodule, including channel
setup, greedy draining, cancellation probing, per-batch spawn_cpu execution, and
error propagation. Expose a focused helper using the existing
PreparedPartitionSearch and search-control inputs, then replace the inline block
with a concise call while preserving batching, cancellation, teardown, and
partial-batch error behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 98b8b498-8947-4830-819e-ccbdc893c98c
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockjava/lance-jni/Cargo.lockis excluded by!**/*.lockpython/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (36)
Cargo.tomlpython/src/utils.rsrust/lance-index-core/Cargo.tomlrust/lance-index-core/src/lib.rsrust/lance-index-core/src/metrics.rsrust/lance-index-core/src/scalar.rsrust/lance-index/Cargo.tomlrust/lance-index/src/frag_reuse.rsrust/lance-index/src/lib.rsrust/lance-index/src/mem_wal.rsrust/lance-index/src/metrics.rsrust/lance-index/src/scalar.rsrust/lance-index/src/scalar/bloomfilter.rsrust/lance-index/src/scalar/btree.rsrust/lance-index/src/scalar/expression.rsrust/lance-index/src/scalar/lance_format.rsrust/lance-index/src/scalar/registry.rsrust/lance-index/src/scalar/rtree/sort/hilbert_sort.rsrust/lance-index/src/scalar/zonemap.rsrust/lance-index/src/vector/flat/transform.rsrust/lance-index/src/vector/hnsw/builder.rsrust/lance-index/src/vector/kmeans.rsrust/lance-namespace-impls/src/dir/manifest.rsrust/lance/src/dataset/optimize.rsrust/lance/src/index.rsrust/lance/src/index/append.rsrust/lance/src/index/create.rsrust/lance/src/index/scalar.rsrust/lance/src/index/scalar/bitmap.rsrust/lance/src/index/scalar/btree.rsrust/lance/src/index/scalar/fmindex.rsrust/lance/src/index/scalar/inverted.rsrust/lance/src/index/scalar/label_list.rsrust/lance/src/index/scalar/zonemap.rsrust/lance/src/index/vector/ivf/io.rsrust/lance/src/index/vector/ivf/v2.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
rust/lance/src/index/vector/ivf/v2.rs (1)
1660-1839: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftConsider extracting the new batched-search scheduling into a dedicated submodule.
This block implements a producer/consumer batching scheduler (channel sizing, greedy draining, cancellation probing, per-batch
spawn_cpudispatch) inlined into an already very large file (v2.rs, 6500+ lines). The logic itself looks correct (cancellation checked synchronously so no CPU-pool thread parks on a channel, errors propagated after flushing partial batches, clean producer teardown on receiver drop), but its size and self-contained nature make it a good candidate for a dedicated module.As per coding guidelines, "Extract substantial new logic such as bin packing or scheduling into dedicated submodules instead of inlining it into large files."
🤖 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/vector/ivf/v2.rs` around lines 1660 - 1839, Extract the producer/consumer batched-search scheduling logic from the enclosing method into a dedicated submodule, including channel setup, greedy draining, cancellation probing, per-batch spawn_cpu execution, and error propagation. Expose a focused helper using the existing PreparedPartitionSearch and search-control inputs, then replace the inline block with a concise call while preserving batching, cancellation, teardown, and partial-batch error behavior.Source: Coding guidelines
rust/lance-index/src/scalar/lance_format.rs (1)
222-255: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
read_rangemust not assert a single batch
read_stream_projectedcan legally split a single range into multiple batches when a column would otherwise exceed Arrow’s size limits, soassert_eq!(batches.len(), 1)can panic on valid reads. Merge the batches here instead of assuming one output.🤖 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-index/src/scalar/lance_format.rs` around lines 222 - 255, The read_range method incorrectly assumes read_stream_projected always returns one batch. Replace the batches.len() assertion and direct indexing with logic that merges all collected batches into a single RecordBatch, preserving the existing empty-range and projection behavior.Source: Coding guidelines
rust/lance-index/src/frag_reuse.rs (1)
81-83: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReturn
NotSupportedinstead of panicking.
FragReuseIndexHandleis exposed asdyn Index, so a generic call tocalculate_included_fragscurrently terminates the process.Proposed fix
async fn calculate_included_frags(&self) -> Result<RoaringBitmap> { - unimplemented!() + Err(lance_core::Error::not_supported( + "calculating included fragments is not supported for fragment reuse indices", + )) }As per coding guidelines: “Return
LanceError::NotSupportedinstead oftodo!()orunimplemented!()for unsupported code paths.”🤖 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-index/src/frag_reuse.rs` around lines 81 - 83, Replace the unimplemented!() in FragReuseIndexHandle::calculate_included_frags with an Err(LanceError::NotSupported(...)) result, using the project’s existing error-construction conventions and preserving the method’s Result<RoaringBitmap> signature.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.
Inline comments:
In `@Cargo.toml`:
- Line 71: Refresh java/lance-jni/Cargo.lock so its lance-index-core dependency
resolves to version 9.0.0-beta.20, matching the updated workspace dependency and
the other lockfiles; regenerate the lockfile using Cargo rather than editing
unrelated dependency entries.
In `@rust/lance-index-core/src/lib.rs`:
- Around line 13-42: Complete rustdoc for the public plugin API, including the
modules, Index trait and all public methods, and IndexType variants in the
referenced sections. Add concise usage examples where appropriate, use intra-doc
links to related structs, methods, and types, replace ordinary comments with
semantic rustdoc, and describe each enum variant’s behavior rather than only
naming it.
In `@rust/lance-index-core/src/metrics.rs`:
- Around line 6-117: Document the complete public metrics API: add semantic
rustdoc for each metric constant and the public fields and methods of
LocalMetricsCollector, including dump_into. Extend MetricsCollector
documentation with runnable examples covering implementing or using a collector,
forwarding aggregated metrics via dump_into, and attaching the optional IoStats
returned by io_stats for I/O attribution.
In `@rust/lance-index-core/src/scalar.rs`:
- Around line 164-167: Replace the HashMap<String, String> parameter in
finish_with_metadata with a dedicated strongly typed metadata struct that models
the supported fields and valid combinations; update implementations and callers
to use this type, converting its values to string key-value pairs only in the
storage/serialization adapter.
- Around line 28-71: Add comprehensive rustdoc for the extracted scalar-index
API, covering public types, constants, enum variants, configuration fields,
constructors, and remapping methods across the referenced sections. Add concise
usage examples for each applicable public API and cross-link related contracts
using intra-doc links to symbols such as IndexFile, BuiltinIndexType, and
associated methods, ensuring all documented behavior and configuration semantics
are clear.
In `@rust/lance-index/src/frag_reuse.rs`:
- Around line 8-9: Move the Index and RowIdRemapper implementations from the
public wrapper in rust/lance-index/src/frag_reuse.rs to FragReuseIndex in
lance-table, which owns the type. Remove the wrapper and its
conversion/adaptation logic, then update imports and callers to use
FragReuseIndex directly while preserving the existing trait behavior.
In `@rust/lance-index/src/scalar/btree.rs`:
- Line 5329: Update the test loop around the identity-preservation scenario so
rows in the 0..1000 range remain mapped to their original row IDs, while only
the 10000..15000 range is remapped to new IDs. Strengthen the assertions to
explicitly verify identity mapping for the first range and remapping behavior
for the second range, using the surrounding test’s existing setup and assertion
symbols.
In `@rust/lance/src/index/vector/ivf/v2.rs`:
- Around line 145-158: Replace the process-panicking assert in
STREAMING_SEARCH_BATCH_SIZE initialization with graceful handling: when the
parsed batch size is zero, emit a warning and use
DEFAULT_STREAMING_SEARCH_BATCH_SIZE instead. Preserve the existing parse error
behavior and ensure the resulting value is always positive without asserting.
---
Outside diff comments:
In `@rust/lance-index/src/frag_reuse.rs`:
- Around line 81-83: Replace the unimplemented!() in
FragReuseIndexHandle::calculate_included_frags with an
Err(LanceError::NotSupported(...)) result, using the project’s existing
error-construction conventions and preserving the method’s Result<RoaringBitmap>
signature.
In `@rust/lance-index/src/scalar/lance_format.rs`:
- Around line 222-255: The read_range method incorrectly assumes
read_stream_projected always returns one batch. Replace the batches.len()
assertion and direct indexing with logic that merges all collected batches into
a single RecordBatch, preserving the existing empty-range and projection
behavior.
In `@rust/lance/src/index/vector/ivf/v2.rs`:
- Around line 1660-1839: Extract the producer/consumer batched-search scheduling
logic from the enclosing method into a dedicated submodule, including channel
setup, greedy draining, cancellation probing, per-batch spawn_cpu execution, and
error propagation. Expose a focused helper using the existing
PreparedPartitionSearch and search-control inputs, then replace the inline block
with a concise call while preserving batching, cancellation, teardown, and
partial-batch error behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 98b8b498-8947-4830-819e-ccbdc893c98c
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockjava/lance-jni/Cargo.lockis excluded by!**/*.lockpython/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (36)
Cargo.tomlpython/src/utils.rsrust/lance-index-core/Cargo.tomlrust/lance-index-core/src/lib.rsrust/lance-index-core/src/metrics.rsrust/lance-index-core/src/scalar.rsrust/lance-index/Cargo.tomlrust/lance-index/src/frag_reuse.rsrust/lance-index/src/lib.rsrust/lance-index/src/mem_wal.rsrust/lance-index/src/metrics.rsrust/lance-index/src/scalar.rsrust/lance-index/src/scalar/bloomfilter.rsrust/lance-index/src/scalar/btree.rsrust/lance-index/src/scalar/expression.rsrust/lance-index/src/scalar/lance_format.rsrust/lance-index/src/scalar/registry.rsrust/lance-index/src/scalar/rtree/sort/hilbert_sort.rsrust/lance-index/src/scalar/zonemap.rsrust/lance-index/src/vector/flat/transform.rsrust/lance-index/src/vector/hnsw/builder.rsrust/lance-index/src/vector/kmeans.rsrust/lance-namespace-impls/src/dir/manifest.rsrust/lance/src/dataset/optimize.rsrust/lance/src/index.rsrust/lance/src/index/append.rsrust/lance/src/index/create.rsrust/lance/src/index/scalar.rsrust/lance/src/index/scalar/bitmap.rsrust/lance/src/index/scalar/btree.rsrust/lance/src/index/scalar/fmindex.rsrust/lance/src/index/scalar/inverted.rsrust/lance/src/index/scalar/label_list.rsrust/lance/src/index/scalar/zonemap.rsrust/lance/src/index/vector/ivf/io.rsrust/lance/src/index/vector/ivf/v2.rs
🛑 Comments failed to post (8)
Cargo.toml (1)
71-71: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail for lock in Cargo.lock python/Cargo.lock java/lance-jni/Cargo.lock; do echo "Checking ${lock}" test -f "${lock}" rg -n -A5 -B1 '^name = "lance-index-core"$' "${lock}" doneRepository: lance-format/lance
Length of output: 736
Refresh
java/lance-jni/Cargo.lock
Cargo.lockandpython/Cargo.lockalready includelance-index-coreat9.0.0-beta.20, butjava/lance-jni/Cargo.lockstill pins9.0.0-beta.19. Refresh the Java lockfile 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 `@Cargo.toml` at line 71, Refresh java/lance-jni/Cargo.lock so its lance-index-core dependency resolves to version 9.0.0-beta.20, matching the updated workspace dependency and the other lockfiles; regenerate the lockfile using Cargo rather than editing unrelated dependency entries.Source: Coding guidelines
rust/lance-index-core/src/lib.rs (1)
13-42: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Complete the documentation for the new public plugin API.
The public modules, traits, methods, and most
IndexTypevariants lack examples and relevant intra-doc links. Several variants also use ordinary comments instead of semantic rustdoc.As per coding guidelines: “All public APIs must have documentation with examples, and documentation should link to relevant structs and methods” and “Document enum variants with behavioral semantics, not just labels.”
Also applies to: 44-82, 173-281, 284-288
🤖 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-index-core/src/lib.rs` around lines 13 - 42, Complete rustdoc for the public plugin API, including the modules, Index trait and all public methods, and IndexType variants in the referenced sections. Add concise usage examples where appropriate, use intra-doc links to related structs, methods, and types, replace ordinary comments with semantic rustdoc, and describe each enum variant’s behavior rather than only naming it.Source: Coding guidelines
rust/lance-index-core/src/metrics.rs (1)
6-117: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Document the complete metrics contract with examples.
The public constants and
LocalMetricsCollectorfields lack semantic rustdoc, while the trait and public methods have no usage examples showing collection, forwarding, and optional I/O attribution.As per coding guidelines: “All public APIs must have documentation with examples” and “Add doc comments to magic constants, thresholds, and non-obvious transformation functions.”
🤖 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-index-core/src/metrics.rs` around lines 6 - 117, Document the complete public metrics API: add semantic rustdoc for each metric constant and the public fields and methods of LocalMetricsCollector, including dump_into. Extend MetricsCollector documentation with runnable examples covering implementing or using a collector, forwarding aggregated metrics via dump_into, and attaching the optional IoStats returned by io_stats for I/O attribution.Source: Coding guidelines
rust/lance-index-core/src/scalar.rs (2)
28-71: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Add examples and cross-links for the extracted scalar-index API.
Most public types and methods lack examples. Public constants, enum variants, configuration fields, constructors, and remapping methods also need fuller semantic documentation and links to related contracts.
As per coding guidelines: “All public APIs must have documentation with examples, and documentation should link to relevant structs and methods.”
Also applies to: 92-135, 148-168, 170-294, 322-426, 428-585
🤖 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-index-core/src/scalar.rs` around lines 28 - 71, Add comprehensive rustdoc for the extracted scalar-index API, covering public types, constants, enum variants, configuration fields, constructors, and remapping methods across the referenced sections. Add concise usage examples for each applicable public API and cross-link related contracts using intra-doc links to symbols such as IndexFile, BuiltinIndexType, and associated methods, ensuring all documented behavior and configuration semantics are clear.Source: Coding guidelines
164-167: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Replace the stringly typed metadata map with a dedicated type.
HashMap<String, String>makes the new plugin boundary dependent on undocumented key names and permits invalid combinations. Introduce a typed metadata structure and convert to strings only in the storage adapter.As per coding guidelines: “Use strongly-typed structs instead of
HashMap<String, String>in APIs, and convert to strings only at serialization boundaries.”🤖 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-index-core/src/scalar.rs` around lines 164 - 167, Replace the HashMap<String, String> parameter in finish_with_metadata with a dedicated strongly typed metadata struct that models the supported fields and valid combinations; update implementations and callers to use this type, converting its values to string key-value pairs only in the storage/serialization adapter.Source: Coding guidelines
rust/lance-index/src/frag_reuse.rs (1)
8-9: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Implement these traits on
FragReuseIndexinlance-table.Because
lance-tableownsFragReuseIndex, orphan rules do not prevent it from implementing the core traits. Keeping this public wrapper contradicts the stated dependency migration and forces every caller to adapt the type.The PR objective explicitly states that the
FragReuseIndextrait implementations move tolance-table.Also applies to: 27-30, 52-106
🤖 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-index/src/frag_reuse.rs` around lines 8 - 9, Move the Index and RowIdRemapper implementations from the public wrapper in rust/lance-index/src/frag_reuse.rs to FragReuseIndex in lance-table, which owns the type. Remove the wrapper and its conversion/adaptation logic, then update imports and callers to use FragReuseIndex directly while preserving the existing trait behavior.rust/lance-index/src/scalar/btree.rs (1)
5329-5329: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail FILE="rust/lance-index/src/scalar/btree.rs" wc -l "$FILE" sed -n '5308,5345p' "$FILE"Repository: lance-format/lance
Length of output: 1636
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '5345,5405p' rust/lance-index/src/scalar/btree.rsRepository: lance-format/lance
Length of output: 2597
Keep the identity-mapped rows identity-mapped
rust/lance-index/src/scalar/btree.rs:5329The loop remaps both ranges to new ids, so this test no longer covers the “rows 0-1000 keep the same row_ids” case described above. The current assertions only check presence/absence, which can hide regressions in that path.🤖 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-index/src/scalar/btree.rs` at line 5329, Update the test loop around the identity-preservation scenario so rows in the 0..1000 range remain mapped to their original row IDs, while only the 10000..15000 range is remapped to new IDs. Strengthen the assertions to explicitly verify identity mapping for the first range and remapping behavior for the second range, using the surrounding test’s existing setup and assertion symbols.rust/lance/src/index/vector/ivf/v2.rs (1)
145-158: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
assert!panics the process on a bad env var instead of falling back.Setting
LANCE_IVF_STREAMING_SEARCH_BATCH_SIZE=0crashes on first access of thisLazyLockrather than clamping to a safe default with a warning. This isn't a data-corruption invariant, so it should be handled gracefully.🛡️ Proposed fix: clamp with a warning instead of panicking
pub(crate) static STREAMING_SEARCH_BATCH_SIZE: LazyLock<usize> = LazyLock::new(|| { - let batch_size = std::env::var("LANCE_IVF_STREAMING_SEARCH_BATCH_SIZE") - .map(|value| { - value - .parse() - .expect("failed to parse LANCE_IVF_STREAMING_SEARCH_BATCH_SIZE") - }) - .unwrap_or(DEFAULT_STREAMING_SEARCH_BATCH_SIZE); - assert!( - batch_size > 0, - "LANCE_IVF_STREAMING_SEARCH_BATCH_SIZE must be greater than 0, got {batch_size}" - ); - batch_size + match std::env::var("LANCE_IVF_STREAMING_SEARCH_BATCH_SIZE") { + Ok(value) => match value.parse::<usize>() { + Ok(batch_size) if batch_size > 0 => batch_size, + _ => { + log::warn!( + "invalid LANCE_IVF_STREAMING_SEARCH_BATCH_SIZE={value:?}, \ + falling back to default {DEFAULT_STREAMING_SEARCH_BATCH_SIZE}" + ); + DEFAULT_STREAMING_SEARCH_BATCH_SIZE + } + }, + Err(_) => DEFAULT_STREAMING_SEARCH_BATCH_SIZE, + } });As per coding guidelines, "Prefer
debug_assert!overassert!for non-safety invariants; reserveassert!for conditions preventing data corruption, and always include descriptive messages."📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.pub(crate) static STREAMING_SEARCH_BATCH_SIZE: LazyLock<usize> = LazyLock::new(|| { match std::env::var("LANCE_IVF_STREAMING_SEARCH_BATCH_SIZE") { Ok(value) => match value.parse::<usize>() { Ok(batch_size) if batch_size > 0 => batch_size, _ => { log::warn!( "invalid LANCE_IVF_STREAMING_SEARCH_BATCH_SIZE={value:?}, \ falling back to default {DEFAULT_STREAMING_SEARCH_BATCH_SIZE}" ); DEFAULT_STREAMING_SEARCH_BATCH_SIZE } }, Err(_) => DEFAULT_STREAMING_SEARCH_BATCH_SIZE, } });🤖 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/vector/ivf/v2.rs` around lines 145 - 158, Replace the process-panicking assert in STREAMING_SEARCH_BATCH_SIZE initialization with graceful handling: when the parsed batch size is zero, emit a warning and use DEFAULT_STREAMING_SEARCH_BATCH_SIZE instead. Preserve the existing parse error behavior and ensure the resulting value is always positive without asserting.Source: Coding guidelines
Xuanwo
left a comment
Xuanwo
left a comment
There was a problem hiding this comment.
Nice work, the only thing left is to update https://github.com/lance-format/lance/blob/main/.bumpversion.toml
westonpace
force-pushed
the
refactor/core-index-crate
branch
from
9edfb0a to
bc4cb04
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
rust/lance-index/src/scalar.rs (1)
51-82: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd doc examples to
index_files_to_table/table_files_to_index.Still missing usage examples on these new public conversion helpers.
As per coding guidelines, "All public APIs must have documentation with examples, and documentation should link to relevant structs and methods."
🤖 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-index/src/scalar.rs` around lines 51 - 82, Add Rust doc examples to the public helpers index_files_to_table and table_files_to_index, demonstrating conversion of at least one IndexFile and asserting the resulting path and size_bytes fields. Link the documentation to the relevant source and destination IndexFile types while preserving the existing conversion behavior.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.
Inline comments:
In `@rust/lance-index-core/Cargo.toml`:
- Line 8: Resolve the invalid readme reference in the lance-index-core package
manifest by adding the missing README.md to the crate or updating the readme
field to an existing file. Ensure the manifest’s readme path points to a file
included with the crate before publishing.
In `@rust/lance-index-core/src/lib.rs`:
- Around line 1-15: Add crate-level documentation at the top of lib.rs before
the use statements, describing lance-index-core as the secondary index library
and documenting the API stability guarantees for external index-plugin authors.
Follow the established wording and structure used by the sibling lance-index
crate.
- Around line 19-42: Document the public API symbols in the Index trait,
IndexType variants, is_scalar/is_vector/is_system helpers, and IndexParams trait
with meaningful behavioral descriptions and usage examples. Replace label-only
variant comments with semantics and valid-use guidance, and link related public
types and methods where relevant so plugin authors can implement
ScalarIndex/Index correctly.
In `@rust/lance-index-core/src/metrics.rs`:
- Around line 89-94: Add a Rust doc comment to the public LocalMetricsCollector
struct explaining that it accumulates metrics locally and is forwarded to the
shared metrics destination through dump_into. Include a concise usage example
and link to the relevant dump_into method and destination metrics struct.
In `@rust/lance-index-core/src/scalar.rs`:
- Around line 192-206: Update read_ranges to limit concurrent read_range
operations using the trait’s io_parallelism() value instead of passing every
range directly to try_join_all. Preserve the existing empty-range handling,
batch concatenation, and error propagation while ensuring the number of
in-flight reads never exceeds the configured parallelism.
In `@rust/lance-index/src/scalar/btree.rs`:
- Line 6286: Move the crate::frag_reuse import containing FragReuseIndex,
FragReuseIndexDetails, and FragReuseIndexHandle from the test function into the
top-level mod tests import block, leaving the test body without inline use
statements.
In `@rust/lance-index/src/scalar/lance_format.rs`:
- Around line 227-240: Construct the ReaderProjection from the requested
projection before the range.is_empty() early return in the relevant IndexReader
read paths. Use the projection-derived schema for empty RecordBatch/stream
results, while retaining the whole-schema behavior when no projection is
provided; apply the same ordering and schema selection in the additional
empty-input branches around the other read methods.
- Around line 175-178: Update read_record_batch to validate offset and
batch-size arithmetic before calling read_batch: convert the relevant values
with i32::try_from, use checked_mul and checked_add for offset * batch_size and
start + batch_size, and return Error::invalid_input on any conversion or
overflow failure. Preserve the existing read_batch behavior only after all
validated values are available.
In `@rust/lance/src/index/vector/ivf/io.rs`:
- Around line 494-508: In the non-cancelled JoinError branch of the task-await
loop, replace Error::io with Error::internal while preserving the existing panic
message and cancellation handling. Update only the error classification in this
match around the task handles.
---
Duplicate comments:
In `@rust/lance-index/src/scalar.rs`:
- Around line 51-82: Add Rust doc examples to the public helpers
index_files_to_table and table_files_to_index, demonstrating conversion of at
least one IndexFile and asserting the resulting path and size_bytes fields. Link
the documentation to the relevant source and destination IndexFile types while
preserving the existing conversion behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 35b21e83-ab27-4897-97b3-5129054c8866
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockjava/lance-jni/Cargo.lockis excluded by!**/*.lockpython/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (37)
.bumpversion.tomlCargo.tomlpython/src/utils.rsrust/lance-index-core/Cargo.tomlrust/lance-index-core/src/lib.rsrust/lance-index-core/src/metrics.rsrust/lance-index-core/src/scalar.rsrust/lance-index/Cargo.tomlrust/lance-index/src/frag_reuse.rsrust/lance-index/src/lib.rsrust/lance-index/src/mem_wal.rsrust/lance-index/src/metrics.rsrust/lance-index/src/scalar.rsrust/lance-index/src/scalar/bloomfilter.rsrust/lance-index/src/scalar/btree.rsrust/lance-index/src/scalar/expression.rsrust/lance-index/src/scalar/lance_format.rsrust/lance-index/src/scalar/registry.rsrust/lance-index/src/scalar/rtree/sort/hilbert_sort.rsrust/lance-index/src/scalar/zonemap.rsrust/lance-index/src/vector/flat/transform.rsrust/lance-index/src/vector/hnsw/builder.rsrust/lance-index/src/vector/kmeans.rsrust/lance-namespace-impls/src/dir/manifest.rsrust/lance/src/dataset/optimize.rsrust/lance/src/index.rsrust/lance/src/index/append.rsrust/lance/src/index/create.rsrust/lance/src/index/scalar.rsrust/lance/src/index/scalar/bitmap.rsrust/lance/src/index/scalar/btree.rsrust/lance/src/index/scalar/fmindex.rsrust/lance/src/index/scalar/inverted.rsrust/lance/src/index/scalar/label_list.rsrust/lance/src/index/scalar/zonemap.rsrust/lance/src/index/vector/ivf/io.rsrust/lance/src/index/vector/ivf/v2.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 9
♻️ Duplicate comments (1)
rust/lance-index/src/scalar.rs (1)
51-82: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd doc examples to
index_files_to_table/table_files_to_index.Still missing usage examples on these new public conversion helpers.
As per coding guidelines, "All public APIs must have documentation with examples, and documentation should link to relevant structs and methods."
🤖 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-index/src/scalar.rs` around lines 51 - 82, Add Rust doc examples to the public helpers index_files_to_table and table_files_to_index, demonstrating conversion of at least one IndexFile and asserting the resulting path and size_bytes fields. Link the documentation to the relevant source and destination IndexFile types while preserving the existing conversion behavior.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.
Inline comments:
In `@rust/lance-index-core/Cargo.toml`:
- Line 8: Resolve the invalid readme reference in the lance-index-core package
manifest by adding the missing README.md to the crate or updating the readme
field to an existing file. Ensure the manifest’s readme path points to a file
included with the crate before publishing.
In `@rust/lance-index-core/src/lib.rs`:
- Around line 1-15: Add crate-level documentation at the top of lib.rs before
the use statements, describing lance-index-core as the secondary index library
and documenting the API stability guarantees for external index-plugin authors.
Follow the established wording and structure used by the sibling lance-index
crate.
- Around line 19-42: Document the public API symbols in the Index trait,
IndexType variants, is_scalar/is_vector/is_system helpers, and IndexParams trait
with meaningful behavioral descriptions and usage examples. Replace label-only
variant comments with semantics and valid-use guidance, and link related public
types and methods where relevant so plugin authors can implement
ScalarIndex/Index correctly.
In `@rust/lance-index-core/src/metrics.rs`:
- Around line 89-94: Add a Rust doc comment to the public LocalMetricsCollector
struct explaining that it accumulates metrics locally and is forwarded to the
shared metrics destination through dump_into. Include a concise usage example
and link to the relevant dump_into method and destination metrics struct.
In `@rust/lance-index-core/src/scalar.rs`:
- Around line 192-206: Update read_ranges to limit concurrent read_range
operations using the trait’s io_parallelism() value instead of passing every
range directly to try_join_all. Preserve the existing empty-range handling,
batch concatenation, and error propagation while ensuring the number of
in-flight reads never exceeds the configured parallelism.
In `@rust/lance-index/src/scalar/btree.rs`:
- Line 6286: Move the crate::frag_reuse import containing FragReuseIndex,
FragReuseIndexDetails, and FragReuseIndexHandle from the test function into the
top-level mod tests import block, leaving the test body without inline use
statements.
In `@rust/lance-index/src/scalar/lance_format.rs`:
- Around line 227-240: Construct the ReaderProjection from the requested
projection before the range.is_empty() early return in the relevant IndexReader
read paths. Use the projection-derived schema for empty RecordBatch/stream
results, while retaining the whole-schema behavior when no projection is
provided; apply the same ordering and schema selection in the additional
empty-input branches around the other read methods.
- Around line 175-178: Update read_record_batch to validate offset and
batch-size arithmetic before calling read_batch: convert the relevant values
with i32::try_from, use checked_mul and checked_add for offset * batch_size and
start + batch_size, and return Error::invalid_input on any conversion or
overflow failure. Preserve the existing read_batch behavior only after all
validated values are available.
In `@rust/lance/src/index/vector/ivf/io.rs`:
- Around line 494-508: In the non-cancelled JoinError branch of the task-await
loop, replace Error::io with Error::internal while preserving the existing panic
message and cancellation handling. Update only the error classification in this
match around the task handles.
---
Duplicate comments:
In `@rust/lance-index/src/scalar.rs`:
- Around line 51-82: Add Rust doc examples to the public helpers
index_files_to_table and table_files_to_index, demonstrating conversion of at
least one IndexFile and asserting the resulting path and size_bytes fields. Link
the documentation to the relevant source and destination IndexFile types while
preserving the existing conversion behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 35b21e83-ab27-4897-97b3-5129054c8866
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockjava/lance-jni/Cargo.lockis excluded by!**/*.lockpython/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (37)
.bumpversion.tomlCargo.tomlpython/src/utils.rsrust/lance-index-core/Cargo.tomlrust/lance-index-core/src/lib.rsrust/lance-index-core/src/metrics.rsrust/lance-index-core/src/scalar.rsrust/lance-index/Cargo.tomlrust/lance-index/src/frag_reuse.rsrust/lance-index/src/lib.rsrust/lance-index/src/mem_wal.rsrust/lance-index/src/metrics.rsrust/lance-index/src/scalar.rsrust/lance-index/src/scalar/bloomfilter.rsrust/lance-index/src/scalar/btree.rsrust/lance-index/src/scalar/expression.rsrust/lance-index/src/scalar/lance_format.rsrust/lance-index/src/scalar/registry.rsrust/lance-index/src/scalar/rtree/sort/hilbert_sort.rsrust/lance-index/src/scalar/zonemap.rsrust/lance-index/src/vector/flat/transform.rsrust/lance-index/src/vector/hnsw/builder.rsrust/lance-index/src/vector/kmeans.rsrust/lance-namespace-impls/src/dir/manifest.rsrust/lance/src/dataset/optimize.rsrust/lance/src/index.rsrust/lance/src/index/append.rsrust/lance/src/index/create.rsrust/lance/src/index/scalar.rsrust/lance/src/index/scalar/bitmap.rsrust/lance/src/index/scalar/btree.rsrust/lance/src/index/scalar/fmindex.rsrust/lance/src/index/scalar/inverted.rsrust/lance/src/index/scalar/label_list.rsrust/lance/src/index/scalar/zonemap.rsrust/lance/src/index/vector/ivf/io.rsrust/lance/src/index/vector/ivf/v2.rs
🛑 Comments failed to post (9)
rust/lance-index-core/Cargo.toml (1)
8-8: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash fd -t f README.md rust/lance-index-coreRepository: lance-format/lance
Length of output: 156
🏁 Script executed:
#!/bin/bash set -euo pipefail printf 'Tracked files:\n' git ls-files rust/lance-index-core | sed -n '1,200p' printf '\nREADME lookup:\n' fd -t f README.md rust/lance-index-core || true printf '\nManifest excerpt:\n' sed -n '1,30p' rust/lance-index-core/Cargo.tomlRepository: lance-format/lance
Length of output: 1143
Add
rust/lance-index-core/README.mdor pointreadmeat an existing file
rust/lance-index-core/Cargo.tomlreferencesREADME.md, but that file is not present in the crate. This should be fixed before publishing.🤖 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-index-core/Cargo.toml` at line 8, Resolve the invalid readme reference in the lance-index-core package manifest by adding the missing README.md to the crate or updating the readme field to an existing file. Ensure the manifest’s readme path points to a file included with the crate before publishing.rust/lance-index-core/src/lib.rs (2)
1-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a crate-level doc comment.
This new file jumps straight from the SPDX header to
usestatements with no//!crate doc. The siblinglance-indexcrate documents its purpose and stability guarantees at the top oflib.rs(//! Lance secondary index library, plus an API-stability warning) — this new plugin-facing crate should do the same, since it's the intended entry point for external index-plugin authors.🤖 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-index-core/src/lib.rs` around lines 1 - 15, Add crate-level documentation at the top of lib.rs before the use statements, describing lance-index-core as the secondary index library and documenting the API stability guarantees for external index-plugin authors. Follow the established wording and structure used by the sibling lance-index crate.Source: Coding guidelines
19-42: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Document the new public API surface (trait methods, enum variants, helper methods).
Several public items lack doc comments entirely or only restate the label:
Index::as_any,as_index,statistics,index_type(Lines 20-35) have no doc comments.IndexTypevariant comments (Lines 48-81, e.g.BTree = 1, // BTree) just repeat the name instead of describing behavior/valid usage.is_scalar,is_vector,is_system(Lines 174-206) have no doc comments.IndexParamstrait (Lines 284-288) has no doc comment.As per coding guidelines: "All public APIs must have documentation with examples, and documentation should link to relevant structs and methods" and "Document enum variants with behavioral semantics, not just labels." This crate is explicitly meant to let plugin authors implement
ScalarIndex/Indexwith fewer dependencies, so documentation quality on this exact surface has outsized value for onboarding external implementers.Also applies to: 44-82, 173-206, 284-288
🤖 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-index-core/src/lib.rs` around lines 19 - 42, Document the public API symbols in the Index trait, IndexType variants, is_scalar/is_vector/is_system helpers, and IndexParams trait with meaningful behavioral descriptions and usage examples. Replace label-only variant comments with semantics and valid-use guidance, and link related public types and methods where relevant so plugin authors can implement ScalarIndex/Index correctly.Source: Coding guidelines
rust/lance-index-core/src/metrics.rs (1)
89-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a doc comment to
LocalMetricsCollector.This struct is a cross-cutting accumulator consumed by downstream index implementations (e.g., via
dump_into), but has no doc comment explaining its purpose or intended lifecycle (accumulate locally, then forward once viadump_into).As per coding guidelines: "All public APIs must have documentation with examples, and documentation should link to relevant structs and methods."
🤖 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-index-core/src/metrics.rs` around lines 89 - 94, Add a Rust doc comment to the public LocalMetricsCollector struct explaining that it accumulates metrics locally and is forwarded to the shared metrics destination through dump_into. Include a concise usage example and link to the relevant dump_into method and destination metrics struct.Source: Coding guidelines
rust/lance-index-core/src/scalar.rs (1)
192-206: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Check whether this exact default existed pre-extraction (pure move) vs. newly introduced. rg -n 'async fn read_ranges' -B3 -A20 rust/lance-index/src/scalar/*.rs 2>/dev/null git log --oneline -- rust/lance-index-core/src/scalar.rs | head -5Repository: lance-format/lance
Length of output: 2164
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== current scalar.rs ==" cat -n rust/lance-index-core/src/scalar.rs | sed -n '180,230p' echo echo "== pre-extraction scalar implementation ==" cat -n rust/lance-index/src/scalar/lance_format.rs | sed -n '240,320p' echo echo "== read_ranges implementations and overrides ==" rg -n "async fn read_ranges|buffer_unordered|io_parallelism|try_join_all" rust/lance-index-core rust/lance-index -g '*.rs'Repository: lance-format/lance
Length of output: 11897
Bound
read_rangeswithio_parallelism(). The default still spawns one future per range viatry_join_all, but the trait already exposesio_parallelism(), so it can cap fan-out directly instead of forcing large-range readers to override.🤖 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-index-core/src/scalar.rs` around lines 192 - 206, Update read_ranges to limit concurrent read_range operations using the trait’s io_parallelism() value instead of passing every range directly to try_join_all. Preserve the existing empty-range handling, batch concatenation, and error propagation while ensuring the number of in-flight reads never exceeds the configured parallelism.rust/lance-index/src/scalar/btree.rs (1)
6286-6286: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Move the new import to the test module’s import block.
FragReuseIndexHandleis imported inside the test function. Move thecrate::frag_reuseimport to the top-levelmod testsimports.As per coding guidelines, “Place
useimports at the top of the file, not inline within function bodies.”🤖 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-index/src/scalar/btree.rs` at line 6286, Move the crate::frag_reuse import containing FragReuseIndex, FragReuseIndexDetails, and FragReuseIndexHandle from the test function into the top-level mod tests import block, leaving the test body without inline use statements.Source: Coding guidelines
rust/lance-index/src/scalar/lance_format.rs (2)
175-178: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Validate batch offsets before conversion and arithmetic.
The previous reader truncates
offsettoi32, while the current reader can overflowoffset * batch_sizeorstart + batch_size. ReturnError::invalid_inputusingi32::try_from,checked_mul, andchecked_addinstead of wrapping or panicking.As per coding guidelines, “Use
checked_addandchecked_mulinstead ofwrapping_addandwrapping_mulfor counters and IDs, and return an error on overflow.” <coding_guidelines>Also applies to: 211-215
🤖 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-index/src/scalar/lance_format.rs` around lines 175 - 178, Update read_record_batch to validate offset and batch-size arithmetic before calling read_batch: convert the relevant values with i32::try_from, use checked_mul and checked_add for offset * batch_size and start + batch_size, and return Error::invalid_input on any conversion or overflow failure. Preserve the existing read_batch behavior only after all validated values are available.Source: Coding guidelines
227-240: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve the requested projection for empty reads.
All three empty-input branches return a batch/stream with the complete file schema before processing
projection. Non-empty calls return the projected schema, so empty projected reads violate theIndexReadercontract and can fail downstream schema-sensitive concatenation. Construct the projected schema before the early return.Also applies to: 262-278, 342-356
🤖 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-index/src/scalar/lance_format.rs` around lines 227 - 240, Construct the ReaderProjection from the requested projection before the range.is_empty() early return in the relevant IndexReader read paths. Use the projection-derived schema for empty RecordBatch/stream results, while retaining the whole-schema behavior when no projection is provided; apply the same ordering and schema selection in the additional empty-input branches around the other read methods.rust/lance/src/index/vector/ivf/io.rs (1)
494-508: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use
Error::internalfor a task panic, notError::io.A
JoinErrorfrom an aborted-but-already-finished task (i.e. a panic) isn't an I/O failure;lance_core::Error::internal(already used for this kind of unexpected-failure classification elsewhere in this PR, e.g.frag_reuse.rs/mem_wal.rsstatistics serialization) better matches the root cause and keeps error-variant triage meaningful.As per coding guidelines, "Match the
Errorvariant to the root cause: useError::invalid_inputfor caller data issues,Error::corrupt_filefor format or integrity issues,Error::not_foundfor missing resources, andError::iofor I/O failures."🐛 Proposed fix
- Err(join_err) => errors.push(Error::io(format!( + Err(join_err) => errors.push(Error::internal(format!( "HNSW partition build task panicked: {join_err}" ))),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.let mut errors = Vec::with_capacity(tasks.len()); for task in tasks.iter_mut() { let Some(handle) = task.take() else { continue; }; match handle.await { Ok(Ok(_)) => {} Ok(Err(e)) => errors.push(e), Err(join_err) if join_err.is_cancelled() => {} Err(join_err) => errors.push(Error::internal(format!( "HNSW partition build task panicked: {join_err}" ))), } } errors🤖 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/vector/ivf/io.rs` around lines 494 - 508, In the non-cancelled JoinError branch of the task-await loop, replace Error::io with Error::internal while preserving the existing panic message and cancellation handling. Update only the error classification in this match around the task handles.Source: Coding guidelines
westonpace
force-pushed
the
refactor/core-index-crate
branch
from
bc4cb04 to
e130956
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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-index-core/src/lib.rs`:
- Around line 16-288: Complete the standalone public API documentation across
rust/lance-index-core/src/lib.rs:16-288,
rust/lance-index-core/src/metrics.rs:6-117, and
rust/lance-index-core/src/scalar.rs:28-585. In lib.rs, document Index,
IndexParams, all IndexType variants and their semantics, valid values and
effects, and add an implementation example; in metrics.rs, document metric
constants and public fields and add a custom collector example; in scalar.rs,
document every public state and field and add examples for implementing
IndexWriter, IndexReader, IndexStore, ScalarIndex, and RowIdRemapper, linking
related structs and methods where appropriate.
In `@rust/lance-index-core/src/scalar.rs`:
- Around line 192-205: Update IndexReader::read_ranges to avoid unbounded
try_join_all fan-out and retaining every per-range batch simultaneously. Process
ranges through bounded batches or a streaming accumulation path, preserving the
empty-range behavior and returning one concatenated RecordBatch for all
requested ranges.
In `@rust/lance/src/dataset/optimize.rs`:
- Around line 3617-3704: The test
test_defer_index_remap_mixed_records_all_groups must ensure indexed and
unindexed fragments are rewritten in distinct groups rather than allowing all
fragments to coalesce. Adjust the compaction setup or assertions to create and
verify at least two rewrite groups: one containing indexed fragments and another
containing only unindexed fragments, while preserving the FRI assertion that
records the unindexed group.
- Around line 1466-1472: Update the exclusive fragment-bound calculation in the
index fragment-map construction to use checked arithmetic for max_fragment_id +
1. Handle the u32::MAX overflow explicitly according to the surrounding
error-handling contract, ensuring the required FRI is not skipped and normal
bounds remain unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: eeb6d291-b412-40e7-aa5a-72b6429e77b6
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockjava/lance-jni/Cargo.lockis excluded by!**/*.lockpython/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (37)
.bumpversion.tomlCargo.tomlpython/src/utils.rsrust/lance-index-core/Cargo.tomlrust/lance-index-core/src/lib.rsrust/lance-index-core/src/metrics.rsrust/lance-index-core/src/scalar.rsrust/lance-index/Cargo.tomlrust/lance-index/src/frag_reuse.rsrust/lance-index/src/lib.rsrust/lance-index/src/mem_wal.rsrust/lance-index/src/metrics.rsrust/lance-index/src/scalar.rsrust/lance-index/src/scalar/bloomfilter.rsrust/lance-index/src/scalar/btree.rsrust/lance-index/src/scalar/expression.rsrust/lance-index/src/scalar/lance_format.rsrust/lance-index/src/scalar/registry.rsrust/lance-index/src/scalar/rtree/sort/hilbert_sort.rsrust/lance-index/src/scalar/zonemap.rsrust/lance-index/src/vector/flat/transform.rsrust/lance-index/src/vector/hnsw/builder.rsrust/lance-index/src/vector/kmeans.rsrust/lance-namespace-impls/src/dir/manifest.rsrust/lance/src/dataset/optimize.rsrust/lance/src/index.rsrust/lance/src/index/append.rsrust/lance/src/index/create.rsrust/lance/src/index/scalar.rsrust/lance/src/index/scalar/bitmap.rsrust/lance/src/index/scalar/btree.rsrust/lance/src/index/scalar/fmindex.rsrust/lance/src/index/scalar/inverted.rsrust/lance/src/index/scalar/label_list.rsrust/lance/src/index/scalar/zonemap.rsrust/lance/src/index/vector/ivf/io.rsrust/lance/src/index/vector/ivf/v2.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 4
🤖 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-index-core/src/lib.rs`:
- Around line 16-288: Complete the standalone public API documentation across
rust/lance-index-core/src/lib.rs:16-288,
rust/lance-index-core/src/metrics.rs:6-117, and
rust/lance-index-core/src/scalar.rs:28-585. In lib.rs, document Index,
IndexParams, all IndexType variants and their semantics, valid values and
effects, and add an implementation example; in metrics.rs, document metric
constants and public fields and add a custom collector example; in scalar.rs,
document every public state and field and add examples for implementing
IndexWriter, IndexReader, IndexStore, ScalarIndex, and RowIdRemapper, linking
related structs and methods where appropriate.
In `@rust/lance-index-core/src/scalar.rs`:
- Around line 192-205: Update IndexReader::read_ranges to avoid unbounded
try_join_all fan-out and retaining every per-range batch simultaneously. Process
ranges through bounded batches or a streaming accumulation path, preserving the
empty-range behavior and returning one concatenated RecordBatch for all
requested ranges.
In `@rust/lance/src/dataset/optimize.rs`:
- Around line 3617-3704: The test
test_defer_index_remap_mixed_records_all_groups must ensure indexed and
unindexed fragments are rewritten in distinct groups rather than allowing all
fragments to coalesce. Adjust the compaction setup or assertions to create and
verify at least two rewrite groups: one containing indexed fragments and another
containing only unindexed fragments, while preserving the FRI assertion that
records the unindexed group.
- Around line 1466-1472: Update the exclusive fragment-bound calculation in the
index fragment-map construction to use checked arithmetic for max_fragment_id +
1. Handle the u32::MAX overflow explicitly according to the surrounding
error-handling contract, ensuring the required FRI is not skipped and normal
bounds remain unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: eeb6d291-b412-40e7-aa5a-72b6429e77b6
⛔ Files ignored due to path filters (3)
Cargo.lockis excluded by!**/*.lockjava/lance-jni/Cargo.lockis excluded by!**/*.lockpython/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (37)
.bumpversion.tomlCargo.tomlpython/src/utils.rsrust/lance-index-core/Cargo.tomlrust/lance-index-core/src/lib.rsrust/lance-index-core/src/metrics.rsrust/lance-index-core/src/scalar.rsrust/lance-index/Cargo.tomlrust/lance-index/src/frag_reuse.rsrust/lance-index/src/lib.rsrust/lance-index/src/mem_wal.rsrust/lance-index/src/metrics.rsrust/lance-index/src/scalar.rsrust/lance-index/src/scalar/bloomfilter.rsrust/lance-index/src/scalar/btree.rsrust/lance-index/src/scalar/expression.rsrust/lance-index/src/scalar/lance_format.rsrust/lance-index/src/scalar/registry.rsrust/lance-index/src/scalar/rtree/sort/hilbert_sort.rsrust/lance-index/src/scalar/zonemap.rsrust/lance-index/src/vector/flat/transform.rsrust/lance-index/src/vector/hnsw/builder.rsrust/lance-index/src/vector/kmeans.rsrust/lance-namespace-impls/src/dir/manifest.rsrust/lance/src/dataset/optimize.rsrust/lance/src/index.rsrust/lance/src/index/append.rsrust/lance/src/index/create.rsrust/lance/src/index/scalar.rsrust/lance/src/index/scalar/bitmap.rsrust/lance/src/index/scalar/btree.rsrust/lance/src/index/scalar/fmindex.rsrust/lance/src/index/scalar/inverted.rsrust/lance/src/index/scalar/label_list.rsrust/lance/src/index/scalar/zonemap.rsrust/lance/src/index/vector/ivf/io.rsrust/lance/src/index/vector/ivf/v2.rs
🛑 Comments failed to post (4)
rust/lance-index-core/src/lib.rs (1)
16-288: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift
Complete the public documentation for the new plugin crate.
The extracted crate is now a standalone public API, but its traits, fields, enum variants, and extension hooks lack the required implementation examples and semantic documentation.
rust/lance-index-core/src/lib.rs#L16-L288: documentIndex,IndexParams, valid values/effects, and everyIndexTypevariant; include an implementation example.rust/lance-index-core/src/metrics.rs#L6-L117: document metric constants and public fields, and add a custom collector example.rust/lance-index-core/src/scalar.rs#L28-L585: add examples for implementingIndexWriter,IndexReader,IndexStore,ScalarIndex, andRowIdRemapper; document all public states and fields.As per coding guidelines: “Document all public APIs with examples and links to relevant structs and methods” and “Document enum variants with behavioral semantics, not just labels.”
📍 Affects 3 files
rust/lance-index-core/src/lib.rs#L16-L288(this comment)rust/lance-index-core/src/metrics.rs#L6-L117rust/lance-index-core/src/scalar.rs#L28-L585🤖 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-index-core/src/lib.rs` around lines 16 - 288, Complete the standalone public API documentation across rust/lance-index-core/src/lib.rs:16-288, rust/lance-index-core/src/metrics.rs:6-117, and rust/lance-index-core/src/scalar.rs:28-585. In lib.rs, document Index, IndexParams, all IndexType variants and their semantics, valid values and effects, and add an implementation example; in metrics.rs, document metric constants and public fields and add a custom collector example; in scalar.rs, document every public state and field and add examples for implementing IndexWriter, IndexReader, IndexStore, ScalarIndex, and RowIdRemapper, linking related structs and methods where appropriate.Source: Coding guidelines
rust/lance-index-core/src/scalar.rs (1)
192-205: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Inspect callers and overrides to determine realistic range counts and limits. rg -nP --type=rust -C4 '\bread_ranges\s*\(' rustRepository: lance-format/lance
Length of output: 11963
🏁 Script executed:
#!/bin/bash set -euo pipefail ast-grep outline rust/lance-index-core/src/scalar.rs --view expanded printf '\n--- callers ---\n' rg -n --type=rust '\.read_ranges\(' rust/lance-index-core rust/lance-indexRepository: lance-format/lance
Length of output: 5419
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '--- read_ranges implementations ---\n' rg -n --type=rust 'async fn read_ranges\(' rust printf '\n--- lazy_docset caller context ---\n' sed -n '330,380p' rust/lance-index/src/scalar/inverted/lazy_docset.rs printf '\n--- IndexReader trait context ---\n' sed -n '172,235p' rust/lance-index-core/src/scalar.rs printf '\n--- read_range_stream usage ---\n' rg -n --type=rust 'read_range_stream\(' rustRepository: lance-format/lance
Length of output: 247
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- read_ranges implementations ---' rg -n --type=rust 'async fn read_ranges\(' rust echo echo '--- lazy_docset caller context ---' sed -n '330,390p' rust/lance-index/src/scalar/inverted/lazy_docset.rs echo echo '--- IndexReader trait context ---' sed -n '172,235p' rust/lance-index-core/src/scalar.rs echo echo '--- read_range_stream usage ---' rg -n --type=rust 'read_range_stream\(' rustRepository: lance-format/lance
Length of output: 5335
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- scalar/lance_format.rs read_ranges implementation ---' sed -n '240,330p' rust/lance-index/src/scalar/lance_format.rs echo echo '--- lazy_docset resolve_row_ids context ---' sed -n '345,375p' rust/lance-index/src/scalar/inverted/lazy_docset.rs echo echo '--- IndexReader implementors ---' rg -n --type=rust 'impl .*IndexReader' rustRepository: lance-format/lance
Length of output: 5759
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- IndexReader impls and read_ranges methods in each file ---' for f in \ rust/lance-index/src/scalar/ngram.rs \ rust/lance-index/src/scalar/btree.rs \ rust/lance-index/src/scalar/bitmap.rs \ rust/lance-index/src/scalar/inverted/index.rs \ rust/lance-index/src/scalar/lance_format.rs do echo echo "### $f" rg -n --no-heading 'impl IndexReader for|async fn read_ranges\(' "$f" doneRepository: lance-format/lance
Length of output: 447
Bound
IndexReader::read_ranges.
The default path spawns one future per range and keeps every batch until concatenation.resolve_row_ids()can pass one 1-row range per doc id, so large lookups can fan out heavily and spike I/O and memory. Use bounded batching or a streaming path here.🤖 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-index-core/src/scalar.rs` around lines 192 - 205, Update IndexReader::read_ranges to avoid unbounded try_join_all fan-out and retaining every per-range batch simultaneously. Process ranges through bounded batches or a streaming accumulation path, preserving the empty-range behavior and returning one concatenated RecordBatch for all requested ranges.Source: Coding guidelines
rust/lance/src/dataset/optimize.rs (2)
1466-1472: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use checked arithmetic for the exclusive fragment bound.
When
max_fragment_id == u32::MAX,m + 1panics or wraps to zero, producing incorrect index coverage and potentially skipping the required FRI.Proposed fix
- let frags = 0..dataset_at_index - .manifest - .max_fragment_id - .map_or(0, |m| m + 1); + let end = match dataset_at_index.manifest.max_fragment_id { + Some(max_fragment_id) => max_fragment_id.checked_add(1).ok_or_else(|| { + Error::invalid_input(format!( + "max_fragment_id={max_fragment_id} cannot be converted to an exclusive u32 upper bound" + )) + })?, + None => 0, + }; + let frags = 0..end;As per coding guidelines: “Use
checked_addandchecked_mulinstead of wrapping arithmetic for counters and IDs.”📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// max_fragment_id is inclusive (the highest id); +1 for an exclusive // upper bound so the last fragment is covered (None => empty range). let end = match dataset_at_index.manifest.max_fragment_id { Some(max_fragment_id) => max_fragment_id.checked_add(1).ok_or_else(|| { Error::invalid_input(format!( "max_fragment_id={max_fragment_id} cannot be converted to an exclusive u32 upper bound" )) })?, None => 0, }; let frags = 0..end; index_fragmaps.push(RoaringBitmap::from_sorted_iter(frags).unwrap());🤖 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/dataset/optimize.rs` around lines 1466 - 1472, Update the exclusive fragment-bound calculation in the index fragment-map construction to use checked arithmetic for max_fragment_id + 1. Handle the u32::MAX overflow explicitly according to the surrounding error-handling contract, ensuring the required FRI is not skipped and normal bounds remain unchanged.Source: Coding guidelines
3617-3704: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Guarantee distinct indexed and unindexed rewrite groups.
With only 600 rows and a 100,000-row target, all fragments can coalesce into one mixed group. A per-group filtering regression would then still pass. Build or assert at least two groups—one indexed and one entirely unindexed—before committing.
As per coding guidelines: “All Rust bug fixes and features must include corresponding tests” and “Include multi-fragment scenarios.”
🤖 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/dataset/optimize.rs` around lines 3617 - 3704, The test test_defer_index_remap_mixed_records_all_groups must ensure indexed and unindexed fragments are rewritten in distinct groups rather than allowing all fragments to coalesce. Adjust the compaction setup or assertions to create and verify at least two rewrite groups: one containing indexed fragments and another containing only unindexed fragments, while preserving the FRI assertion that records the unindexed group.Source: Coding guidelines
… trait layer Extracts the abstract trait layer for scalar indices into a new `lance-index-core` crate (~17 deps vs 75+ in lance-index), enabling index plugin authors to implement ScalarIndex without depending on the full lance-index implementation crate. Moved to lance-index-core: Index, IndexType, IndexParams, MetricsCollector, ScalarIndex, AnyQuery, IndexStore/Reader/Writer, RowIdRemapper, SearchResult, CreatedIndex, UpdateCriteria, OldIndexDataFilter, ScalarIndexParams, BuiltinIndexType, TrainingCriteria/TrainingOrdering, IndexFile. lance-index and lance-table re-export all moved types for backward compatibility. FragReuseIndex/MemWalIndex trait impls move to lance-table where those types are defined. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> refactor(index): break lance-table → lance-index-core dependency Remove the unintended `lance-table → lance-index-core` dependency introduced in the previous commit. Key changes: - Restore `IndexFile` as a standalone struct in `lance-table` (duplicate of the one in `lance-index-core`), removing the re-export that forced the dep - Add `index_files_to_table()` / `table_files_to_index()` free functions in `lance-index` to convert between the two distinct `IndexFile` types - Introduce newtype wrappers `FragReuseIndexHandle` and `MemWalIndexHandle` in `lance-index` so that `Index` / `RowIdRemapper` (now in `lance-index-core`) can be implemented on the `lance-table` types without violating orphan rules - Remove the `lance-index-core` trait impls that were added directly on `FragReuseIndex` / `MemWalIndex` in `lance-table` - Update all call sites in `lance`, `lance-index`, and `lance-namespace-impls` to use the handle newtypes and conversion functions Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
westonpace
force-pushed
the
refactor/core-index-crate
branch
from
e130956 to
d5ee5be
Compare
2 participants
Summary
Creates a new
lance-index-corecrate containing the abstract trait layer for scalar indices. The goal is to allow index plugin authors to implementScalarIndexwithout depending on the fulllance-indexcrate (~17 dependencies vs 75+).What moved
The following types were extracted from
lance-indexintolance-index-coreand re-exported from their original locations for backward compatibility:Index,IndexParams,ScalarIndex,AnyQuery,IndexStore,IndexReader,IndexWriter,RowIdRemapper,MetricsCollectorIndexType,SearchResult,CreatedIndex,UpdateCriteria,OldIndexDataFilter,ScalarIndexParams,BuiltinIndexType,TrainingCriteria,TrainingOrdering,IndexFileThe concrete query type implementations (
SargableQuery,LabelListQuery,TextQuery,BloomFilterQuery,FullTextSearchQuery,TokenQuery,GeoQuery) stay inlance-index.Dependency graph
Non-trivial changes
TrainingCriteria/TrainingOrderinglifted out of the plugin registryThese two types were defined in
lance-index::scalar::registryalongside the plugin infrastructure. BecauseUpdateCriteria— a field on theScalarIndextrait — referencesTrainingCriteria, both types need to live inlance-index-core. They are re-exported from their original path for backward compatibility:Newtype wrappers for
IndexReaderinlance_format.rsIndexReaderwas previously implemented directly onlance_file::previous::reader::FileReaderandlance_file::reader::FileReader. WithIndexReadernow inlance-index-core, both the trait and the target types are foreign tolance-index, violating the orphan rule. Two private newtype wrappers resolve this:These implement
IndexReaderlocally and are boxed asArc<dyn IndexReader>before being returned fromopen_index_file. No public API changed.The existing blanket
impl IndexWriter for PreviousFileWriter<M>was also removed for the same reason (foreign trait over a foreign generic type). The one call site that used it was updated to callPreviousFileWriter::writeandfinish_with_metadatadirectly.Newtype wrappers for system-index types (
FragReuseIndex,MemWalIndex)FragReuseIndexandMemWalIndexare defined inlance-table. Previously,lance-indeximplementedIndexandRowIdRemapperdirectly on those types — valid because both trait and impl were inlance-index. With the traits now inlance-index-core, the orphan rule requires the impl to live in the trait crate or the type crate. To avoid adding alance-table → lance-index-coredependency,lance-indexinstead defines two newtype wrappers:Call sites in
lancethat previously castArc<FragReuseIndex>directly toArc<dyn Index>now wrap it in the handle first.lance-tableretains zero knowledge oflance-index-core.Dual
IndexFiletypes with explicit conversion functionsIndexFile(path: String,size_bytes: u64) exists in bothlance-index-core::scalar(for plugin use) andlance-table::format(for protobuf serialization and manifest tracking). The two are kept as independent structs — aFromimpl is not possible due to the orphan rule, and havinglance-tablere-export fromlance-index-corewould introduce the unwanted dependency. Two free functions inlance-indexbridge them at the boundary:Summary by CodeRabbit