feat: support segmented RTree indexes#7932
Conversation
📝 WalkthroughWalkthroughRTree index segments can now be merged with row and null filtering, fragment row remapping, stale-coverage pruning, and dataset-version validation. Python distributed indexing recognizes RTREE, with tests covering merge, commit, rewrites, deletions, spatial queries, and indexed execution. ChangesRTree segment merging
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Dataset
participant merge_segments
participant merge_rtree_indices
participant RTreeIndexPlugin
Dataset->>merge_segments: merge existing RTree segments
merge_segments->>merge_rtree_indices: pass opened indexes and filters
merge_rtree_indices->>RTreeIndexPlugin: train merged bbox and null streams
RTreeIndexPlugin-->>merge_segments: return CreatedIndex
merge_segments-->>Dataset: return merged IndexMetadata
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
A-python
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/lance/src/index.rs (1)
1415-1461: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPrefer an explicit error over
unreachable!()for the non-geo RTree branch.
all_rtreeis hard-coded tofalsewhengeois disabled, so this branch is unreachable today, but a panic is a much worse failure mode than aResult::Errif that invariant is ever broken by a future refactor (e.g. someone adjusts the#[cfg]gating ofall_rtree). ReturningError::NotSupported/Error::invalid_inputkeeps this a graceful, catchable failure instead of a crash.🛡️ Proposed fix
} else if all_rtree { #[cfg(feature = "geo")] { crate::index::scalar::rtree::merge_segments(self, source_segments).await? } #[cfg(not(feature = "geo"))] - unreachable!("RTree segments require the geo feature") + return Err(Error::NotSupported { + source: "RTree segment merge requires the `geo` feature".into(), + location: location!(), + }); } else {As per coding guidelines, "Do not silently guard against impossible conditions; use
debug_assert!, return an explicit error, or remove the check" and "Never use.unwrap(),.expect(),panic!(), orassert!()in library code for fallible operations."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance/src/index.rs` around lines 1415 - 1461, Replace the non-geo `unreachable!()` branch in `merge_existing_index_segments` with an explicit catchable error, such as `Error::NotSupported` or `Error::invalid_input`, while preserving the geo-enabled `rtree::merge_segments` path.Source: Coding guidelines
🧹 Nitpick comments (3)
rust/lance-index/src/scalar/rtree.rs (1)
457-467: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUndocumented magic column index in
remap_rtree_data.
remapper.remap_row_ids_record_batch(batch, 1)hard-codes1as the row-id column position (matchingBBOX_ROWID_SCHEMA's column order). A short comment would prevent this from silently breaking if the schema's column order ever changes.As per coding guidelines, "Add doc comments to magic constants, thresholds, and non-obvious transformation functions, explaining what the value represents and why it was chosen."📝 Proposed comment
let remapped = data.map(move |batch_result| { let batch = batch_result?; + // Row id is the second column (index 1) in BBOX_ROWID_SCHEMA. Ok(remapper.remap_row_ids_record_batch(batch, 1)?) });🤖 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/rtree.rs` around lines 457 - 467, Add a concise inline comment at the `remapper.remap_row_ids_record_batch(batch, 1)` call in `remap_rtree_data` documenting that index `1` is the row-ID column position defined by `BBOX_ROWID_SCHEMA`, and must stay aligned with that schema’s column order.Source: Coding guidelines
rust/lance/src/index/api.rs (2)
80-88: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
into_parts()silently dropsdataset_version.The only current call site reads
dataset_version()before callinginto_parts(), so there's no live bug, but the tuple returned here quietly omits the new field. If a future caller callsinto_parts()first (as the name implies "component parts"), the dataset version will be lost without a compiler warning.🤖 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/api.rs` around lines 80 - 88, Update Segment::into_parts to include dataset_version in its returned tuple, preserving all existing component fields and matching the struct’s complete state. Adjust the return type and tuple construction together so callers are required to handle the added value.
34-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a
with_builder instead of mutating a freshly-constructed segment.
IndexSegment::new(...)is immediately followed by direct mutation ofsegment.dataset_versionininto_index_segment. Prefer a fluentwith_dataset_version(self, version: u64) -> Selfbuilder so callers outside this module (and future refactors) can't accidentally forget to set it, and the pattern stays idiomatic for optional config on this struct.♻️ Proposed builder method
pub fn new<I>( uuid: Uuid, fragment_bitmap: I, index_details: Arc<prost_types::Any>, index_version: i32, ) -> Self where I: IntoIterator<Item = u32>, { Self { uuid, fragment_bitmap: fragment_bitmap.into_iter().collect(), index_details, index_version, dataset_version: None, } } + + /// Attach the dataset version this segment's contents were built against. + pub fn with_dataset_version(mut self, version: u64) -> Self { + self.dataset_version = Some(version); + self + }- let mut segment = IndexSegment::new( + let segment = IndexSegment::new( self.uuid, fragment_bitmap.iter(), index_details, self.index_version, - ); - segment.dataset_version = Some(self.dataset_version); + ) + .with_dataset_version(self.dataset_version); Ok(segment)As per coding guidelines, "Use
with_-prefixed builder methods for optional config (for example,MyStruct::new(required).with_option(v)), and do not create separate constructor variants."Also applies to: 103-126
🤖 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/api.rs` around lines 34 - 53, Update IndexSegment::new and the into_index_segment construction flow to avoid direct mutation of dataset_version. Add a fluent with_dataset_version(self, version: u64) -> Self builder on IndexSegment, and chain it after construction wherever the dataset version is available, preserving None for callers that do not set it.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/src/scalar/rtree.rs`:
- Around line 469-561: Update merge_rtree_indices so the bbox computed by
process_and_analyze_bbox_stream remains in stats; remove the overwrite using the
unfiltered source metadata union while retaining the null_map override. Expand
the public merge_rtree_indices documentation with doc examples covering the
merge behavior.
---
Outside diff comments:
In `@rust/lance/src/index.rs`:
- Around line 1415-1461: Replace the non-geo `unreachable!()` branch in
`merge_existing_index_segments` with an explicit catchable error, such as
`Error::NotSupported` or `Error::invalid_input`, while preserving the
geo-enabled `rtree::merge_segments` path.
---
Nitpick comments:
In `@rust/lance-index/src/scalar/rtree.rs`:
- Around line 457-467: Add a concise inline comment at the
`remapper.remap_row_ids_record_batch(batch, 1)` call in `remap_rtree_data`
documenting that index `1` is the row-ID column position defined by
`BBOX_ROWID_SCHEMA`, and must stay aligned with that schema’s column order.
In `@rust/lance/src/index/api.rs`:
- Around line 80-88: Update Segment::into_parts to include dataset_version in
its returned tuple, preserving all existing component fields and matching the
struct’s complete state. Adjust the return type and tuple construction together
so callers are required to handle the added value.
- Around line 34-53: Update IndexSegment::new and the into_index_segment
construction flow to avoid direct mutation of dataset_version. Add a fluent
with_dataset_version(self, version: u64) -> Self builder on IndexSegment, and
chain it after construction wherever the dataset version is available,
preserving None for callers that do not set it.
🪄 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: QUIET
Plan: Pro Plus
Run ID: 1aa71326-6f38-494f-b6c9-2680c32f4bef
📒 Files selected for processing (7)
python/python/lance/dataset.pypython/python/tests/test_geo.pyrust/lance-index/src/scalar/rtree.rsrust/lance/src/index.rsrust/lance/src/index/api.rsrust/lance/src/index/scalar.rsrust/lance/src/index/scalar/rtree.rs
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Xuanwo
left a comment
Xuanwo
left a comment
There was a problem hiding this comment.
The RTree-specific merge mechanism is aligned with segment-native indexing: a merge produces a new self-contained physical segment. However, the shared IndexSegment contract currently makes that lifecycle unsafe:
-
IndexSegmentdoes not carryfields.IndexMetadata::into_index_segmenttherefore discards the physical field provenance, andbuild_index_metadata_from_segmentsrecreatesfieldsfrom the caller-provided column before the later validation. That check is tautological. A segment built overacan be committed as an index on a compatible-typedb, so queries onbcan return false negatives from physical data encoded froma. -
dataset_versionis optional, and the Python and JavaIndexMetadata -> IndexSegmentconversions drop it by callingIndexSegment::new.build_index_metadata_from_segmentsinterprets the missing value as the current manifest version. If an older segment is committed after the dataset has advanced, the segment is falsely marked current and overlay re-evaluation can be skipped, which can produce false negatives. -
RTree accepts
page_size = 1without validation. Two valid one-row source segments can reachwrite_indexwithnum_items = 2; every level is chunked into two one-item pages, sonext_num_itemsnever decreases and the merge loops forever.
A physical segment descriptor needs to preserve its build provenance and conservative freshness watermark across every binding; commit-time logical naming cannot recreate either. Until that invariant and the RTree parameter boundary are enforced, I don't think this is safe to merge yet.
d36596f to
861e7b3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (2)
python/python/lance/dataset.py-4299-4300 (1)
4299-4300: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winInclude RTREE in the preceding supported-type list.
The docstring now says RTREE segments may be merged, but the public distributed-build list at Lines 4281-4283 still omits RTREE. Add it there so the API documentation consistently advertises RTREE support.
🤖 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 `@python/python/lance/dataset.py` around lines 4299 - 4300, Update the public distributed-build supported-type list in the nearby dataset documentation to include RTREE, matching the merge_existing_index_segments documentation. Keep the existing listed index types unchanged.python/python/tests/test_geo.py-175-201 (1)
175-201: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExercise the merged index across all fragments.
The query only covers IDs 10–20, all from the first 40-row fragment.
merged.fragment_idschecks metadata, not that remapped RTree data from later segments is queryable. Assert the expected fragment count and query a range spanning all three fragments (for example, 10–110).Proposed test adjustment
fragments = ds.get_fragments() + assert len(fragments) == 3 segments = [ ... - WHERE St_Intersects(point, ST_GeomFromText('LINESTRING (10 10, 20 20)')) + WHERE St_Intersects(point, ST_GeomFromText('LINESTRING (10 10, 110 110)')) ... - assert indexed["id"].to_pylist() == list(range(10, 21)) + assert indexed["id"].to_pylist() == list(range(10, 111))As per coding guidelines, “Every bugfix and feature must have corresponding tests.”
🤖 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 `@python/python/tests/test_geo.py` around lines 175 - 201, The geo index test currently queries only the first fragment, so it does not validate merged RTree data across all fragments. In the test around create_index_uncommitted and merge_existing_index_segments, assert that three fragments are present and change the spatial query and expected IDs to span all three fragments, such as IDs 10–110.Source: Coding guidelines
🧹 Nitpick comments (2)
rust/lance-index/src/scalar/rtree.rs (1)
485-494: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winIgnored doctest — replace with a compiling example.
The doc example uses
```ignoreformerge_rtree_indices. As per coding guidelines: "avoid ignored doctests by writing compiling Rust doctests." Given the setup complexity (constructingRTreeIndex/IndexStoreinstances), ano_runblock (which still compiles, just doesn't execute) would satisfy the guideline with minimal extra effort, or the example could reference a minimal in-crate helper.🤖 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/rtree.rs` around lines 485 - 494, Replace the ignored doctest in the documentation for merge_rtree_indices with a compiling no_run doctest, adding the necessary setup or a minimal in-crate helper to construct the referenced segments, destination, and filters. Preserve the example’s async call and error propagation while removing the ignore directive.Source: Coding guidelines
rust/lance/src/index/scalar/rtree.rs (1)
37-55: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider skipping
open_scalar_indexfor segments the filter already excludes.
old_data_filtersis computed before this loop, but every segment'sRTreeIndexis still opened/downcast even when its corresponding filter will causemerge_rtree_indicesto skip it entirely (fully-retired-fragment segments).bloomfilter.rs'smerge_segmentsavoids this by skippingopen_scalar_indexfor segments whose effective coverage is already known to be empty — worth mirroring here to avoid needless I/O during compaction-heavy merges.🤖 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/scalar/rtree.rs` around lines 37 - 55, Update the source-index loop in the R-tree merge flow to skip `open_scalar_index` for segments whose corresponding `old_data_filters` indicate empty effective coverage, matching the optimization used by bloomfilter `merge_segments`. Keep excluded segments out of `source_indices`, while preserving the existing open, downcast, and error handling for segments that still participate in `merge_rtree_indices`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/lance/src/index.rs`:
- Around line 548-555: Remove the geo feature gate from
segment_has_rtree_details so it can classify RTree metadata in all builds, then
update the all_rtree calculation in the merge validation flow to always inspect
the segments rather than defaulting to false without geo. Preserve the
subsequent Error::not_supported path so non-geo builds report that RTree segment
merging requires the geo feature.
---
Other comments:
In `@python/python/lance/dataset.py`:
- Around line 4299-4300: Update the public distributed-build supported-type list
in the nearby dataset documentation to include RTREE, matching the
merge_existing_index_segments documentation. Keep the existing listed index
types unchanged.
In `@python/python/tests/test_geo.py`:
- Around line 175-201: The geo index test currently queries only the first
fragment, so it does not validate merged RTree data across all fragments. In the
test around create_index_uncommitted and merge_existing_index_segments, assert
that three fragments are present and change the spatial query and expected IDs
to span all three fragments, such as IDs 10–110.
---
Nitpick comments:
In `@rust/lance-index/src/scalar/rtree.rs`:
- Around line 485-494: Replace the ignored doctest in the documentation for
merge_rtree_indices with a compiling no_run doctest, adding the necessary setup
or a minimal in-crate helper to construct the referenced segments, destination,
and filters. Preserve the example’s async call and error propagation while
removing the ignore directive.
In `@rust/lance/src/index/scalar/rtree.rs`:
- Around line 37-55: Update the source-index loop in the R-tree merge flow to
skip `open_scalar_index` for segments whose corresponding `old_data_filters`
indicate empty effective coverage, matching the optimization used by bloomfilter
`merge_segments`. Keep excluded segments out of `source_indices`, while
preserving the existing open, downcast, and error handling for segments that
still participate in `merge_rtree_indices`.
🪄 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: QUIET
Plan: Pro Plus
Run ID: bd2d5975-0389-4ae3-9424-88009d5797f2
📒 Files selected for processing (12)
java/lance-jni/src/blocking_dataset.rspython/python/lance/dataset.pypython/python/tests/test_geo.pypython/src/dataset.rspython/src/indices.rsrust/lance-index/src/scalar/rtree.rsrust/lance/src/dataset/index.rsrust/lance/src/index.rsrust/lance/src/index/api.rsrust/lance/src/index/create.rsrust/lance/src/index/scalar.rsrust/lance/src/index/scalar/rtree.rs
Xuanwo
left a comment
Xuanwo
left a comment
There was a problem hiding this comment.
Thanks for carrying fields and dataset_version through the Rust, Python, and Java paths, and for preventing non-reducing page sizes on new builds. The leaf-repack merge mechanism now matches the segment model. I still found four correctness / data-safety gaps on 861e7b3b:
-
A staged RTree can be committed after the indexed geometry changes and still claim coverage of that fragment. I reproduced
V1 build -> V2 RewriteColumns -> commit V1 segment: the unindexed query returns row 0, while the sameScalarIndexQueryreturns no rows after the segment is committed. Nested geometry makes the provenance mismatch explicit: the segment records parent field[1], while the update / overlay records leaf fields[2, 3]. The commit path does not reconcile intervening updates, andoverlay_exclusion_offsetsuses exact field-ID comparison, so the old RTree can produce false negatives even though its watermark is preserved. -
RTree still accepts a caller-supplied
index_uuid. Reusing a currently committed UUID overwrites the fixedpage_data.lance/nulls.lancefiles before any manifest transaction. After an uncommitted fragment-0 build using the same UUID, I reproduced the current manifest's query failing withfailed to fill whole buffer. A cancelled or rejected commit cannot repair the current or time-travel versions that already reference those files. -
RTreeIndex::loadnow rejects everypage_size < 2. Thev8.0.0writer can legitimately write and read a single-pagepage_size = 1, num_items = 1index, while the RTree format version is still0. Rejecting new non-reducing builds makes sense, but applying the same rule to the reader makes an existing valid artifact unreadable. -
merge_existing_index_segmentsvalidates only the merged output'smin(dataset_version). Sources{V-1, V+1}therefore produceV-1, and the later future-version check passes without ever rejecting the future source.
Regression reproducers run against this head
import lance
import numpy as np
import pyarrow as pa
from geoarrow.rust.core import point, points
def query_ids(dataset, wkt):
sql = f"""
SELECT id, point FROM dataset
WHERE St_Intersects(point, ST_GeomFromText('{wkt}'))
"""
return [
value
for batch in dataset.sql(sql).build().to_batch_records()
for value in batch.column("id").to_pylist()
]
def test_staged_rtree_after_rewrite_columns(tmp_path):
uri = str(tmp_path / "stale_rtree.lance")
point_type = point("xy")
schema = pa.schema([
pa.field("id", pa.int64()),
pa.field(point_type).with_name("point"),
])
dataset = lance.write_dataset(
pa.Table.from_arrays([
pa.array([0, 1], type=pa.int64()),
points([np.array([0.0, 1.0]), np.array([0.0, 1.0])]),
], schema=schema),
uri,
)
segment = dataset.create_index_uncommitted(
column="point", index_type="RTREE", name="point_rtree", fragment_ids=[0]
)
update_schema = pa.schema([
pa.field("_rowid", pa.uint64()),
pa.field(point_type).with_name("point"),
])
update = pa.Table.from_arrays([
pa.array([0], type=pa.uint64()),
points([np.array([10.0]), np.array([10.0])]),
], schema=update_schema)
fragment, fields = dataset.get_fragment(0).update_columns(update)
updated = lance.LanceDataset.commit(
uri,
lance.LanceOperation.Update(
updated_fragments=[fragment], fields_modified=fields
),
read_version=dataset.version,
)
assert query_ids(updated, "POINT (10 10)") == [0]
committed = updated.commit_existing_index_segments(
"point_rtree", "point", [segment]
)
assert query_ids(committed, "POINT (10 10)") == [0] # actual: []
def test_rtree_uuid_is_write_once(tmp_path):
uri = str(tmp_path / "uuid_reuse.lance")
num_points = 120
point_type = point("xy")
schema = pa.schema([
pa.field("id", pa.int64()),
pa.field(point_type).with_name("point"),
])
dataset = lance.write_dataset(
pa.Table.from_arrays([
pa.array(range(num_points), type=pa.int64()),
points([
np.arange(num_points, dtype=np.float64),
np.arange(num_points, dtype=np.float64),
]),
], schema=schema),
uri,
max_rows_per_file=40,
)
dataset.create_scalar_index("point", "RTREE")
index = dataset.list_indices()[0]
wkt = "LINESTRING (100 100, 110 110)"
assert query_ids(dataset, wkt) == list(range(100, 111))
dataset.create_index_uncommitted(
column="point",
index_type="RTREE",
name="point_rtree_reuse",
fragment_ids=[0],
index_uuid=index["uuid"],
)
assert query_ids(lance.dataset(uri), wkt) == list(range(100, 111))
# Current result: ValueError(... "failed to fill whole buffer" ...)Given the reproducible false-negative and already-published-index corruption cases, I do not think this is safe to merge yet.
861e7b3 to
db65cc5
Compare
|
Thanks for the detailed reproducers. Addressed in
Added regressions for RewriteColumns, UUID reuse, mixed-version merges with later rewrites, legacy single-item artifacts, and future-version sources. The geo test suite passes 8/8, along with focused Rust tests, clippy, formatting, and non-geo compilation. Context: #7932 (review) |
There was a problem hiding this comment.
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/rtree.rs (1)
159-194: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick win
calculate_page_offsetsbreaks backward compatibility for legacypage_size=1single-item RTree indices.
calculate_page_offsetsnow returnsVec::new()for anypage_size < 2, includingpage_size == 1withnum_items <= 1— a combinationvalidate_stored_page_sizeexplicitly declares compatible for legacy stored files. But an emptypage_offsetsbreakspage_range(0): bothstartandendfall back topages_reader.num_rows()(sinceoffsets.get(0)/offsets.get(1)are bothNone), producing an empty range even though one item exists.search_bbox/search_nullwould then silently return no results for such legacy indices — a silent correctness regression, not a crash, so nothing currently surfaces it. The original code (before this PR) computed[0]forpage_size=1, num_items=1via the normal loop (theif cur_level_items <= page_sizebranch triggers immediately and breaks, never reaching the division/step_by that can panic).The guard is overly broad: it was likely added to avoid an infinite loop (
cur_level_items.div_ceil(page_size)never shrinks whenpage_size==1 && num_items>1) or astep_by(0)panic whenpage_size==0, but neither risk applies topage_size==1 && num_items<=1. The exact safe condition is already used byvalidate_stored_page_size.No test currently exercises
RTreeMetadata::new/calculate_page_offsetsend-to-end for this legacy shape (test_stored_page_size_preserves_single_item_compatibilityonly tests the validator, not reconstruction).
As per coding guidelines, "Stable file formats must preserve backward and forward compatibility."🐛 Proposed fix
fn calculate_page_offsets(num_items: usize, page_size: u32) -> Vec<usize> { - if page_size < 2 { + if page_size == 0 || (page_size == 1 && num_items > 1) { return Vec::new(); } let mut page_offsets = vec![];🤖 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/rtree.rs` around lines 159 - 194, Update RTreeMetadata::calculate_page_offsets so page_size == 1 with num_items <= 1 follows the existing single-item calculation and returns the required offset [0], while still preventing non-terminating or invalid processing for page_size == 0 and page_size == 1 with multiple items. Preserve the validate_stored_page_size compatibility behavior and add an end-to-end test through RTreeMetadata::new for the legacy single-item case.Source: Coding guidelines
🧹 Nitpick comments (3)
python/python/tests/test_geo.py (1)
179-184: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract a shared point-schema helper to remove duplication across the 5 new RTree tests.
The same
pa.schema([pa.field("id", pa.int64()), pa.field(point(...)).with_name("point")])literal is repeated verbatim across all five new tests. A small helper would reduce copy/paste and keep future geometry-schema tweaks in one place.
python/python/tests/test_geo.py#L179-L184: replace with a call to a shared_point_dataset_schema()helper.python/python/tests/test_geo.py#L229-L234: replace with the same helper.python/python/tests/test_geo.py#L288-L293: replace with the same helper.python/python/tests/test_geo.py#L334-L339: replace with the same helper.python/python/tests/test_geo.py#L372-L377: replace with the same helper.♻️ Proposed helper
def _id_point_schema() -> pa.Schema: return pa.schema( [ pa.field("id", pa.int64()), pa.field(point("xy")).with_name("point"), ] )🤖 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 `@python/python/tests/test_geo.py` around lines 179 - 184, Extract the repeated point schema construction into a shared _point_dataset_schema() helper in python/python/tests/test_geo.py, returning the existing id and point fields unchanged. Replace the schema literals at python/python/tests/test_geo.py:179-184, 229-234, 288-293, 334-339, and 372-377 with calls to this helper.rust/lance-index/src/scalar/rtree.rs (2)
456-462: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
filter_keeps_nothinghelper across two crates. The exact same predicate is defined independently in both files, risking future divergence.
rust/lance-index/src/scalar/rtree.rs#L456-L462: make this the single canonical definition (markpub) sincelance-indexis a dependency oflance.rust/lance/src/index/scalar/rtree.rs#L16-L22: remove the local copy and import the canonical version fromlance_index::scalar::rtree.🤖 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/rtree.rs` around lines 456 - 462, Make filter_keeps_nothing in rust/lance-index/src/scalar/rtree.rs the canonical public helper by marking it pub; in rust/lance/src/index/scalar/rtree.rs, remove the duplicate local definition and import filter_keeps_nothing from lance_index::scalar::rtree instead.
67-74: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUse
Error::corrupt_fileinstead ofError::invalid_inputfor stored-metadata validation.
validate_stored_page_sizevalidates metadata read back from a persisted index file (RTreeIndex::load), not caller-supplied input. Per guidelines the error variant should reflect the root cause:Error::corrupt_filefor format/integrity issues on stored data, reservingError::invalid_inputfor caller data issues.
As per coding guidelines, "Match the Error variant to the root cause: use Error::invalid_input for caller data issues, Error::corrupt_file for format or integrity issues."♻️ Proposed fix
fn validate_stored_page_size(page_size: u32, num_items: usize) -> Result<()> { if page_size == 0 || (page_size == 1 && num_items > 1) { - return Err(Error::invalid_input(format!( + return Err(Error::corrupt_file(format!( "stored RTree page_size {page_size} cannot represent {num_items} items" ))); } Ok(()) }🤖 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/rtree.rs` around lines 67 - 74, Update validate_stored_page_size to return Error::corrupt_file for invalid persisted RTree metadata, while preserving the existing validation condition and error message.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.
Outside diff comments:
In `@rust/lance-index/src/scalar/rtree.rs`:
- Around line 159-194: Update RTreeMetadata::calculate_page_offsets so page_size
== 1 with num_items <= 1 follows the existing single-item calculation and
returns the required offset [0], while still preventing non-terminating or
invalid processing for page_size == 0 and page_size == 1 with multiple items.
Preserve the validate_stored_page_size compatibility behavior and add an
end-to-end test through RTreeMetadata::new for the legacy single-item case.
---
Nitpick comments:
In `@python/python/tests/test_geo.py`:
- Around line 179-184: Extract the repeated point schema construction into a
shared _point_dataset_schema() helper in python/python/tests/test_geo.py,
returning the existing id and point fields unchanged. Replace the schema
literals at python/python/tests/test_geo.py:179-184, 229-234, 288-293, 334-339,
and 372-377 with calls to this helper.
In `@rust/lance-index/src/scalar/rtree.rs`:
- Around line 456-462: Make filter_keeps_nothing in
rust/lance-index/src/scalar/rtree.rs the canonical public helper by marking it
pub; in rust/lance/src/index/scalar/rtree.rs, remove the duplicate local
definition and import filter_keeps_nothing from lance_index::scalar::rtree
instead.
- Around line 67-74: Update validate_stored_page_size to return
Error::corrupt_file for invalid persisted RTree metadata, while preserving the
existing validation condition and error message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 77601b83-35be-4839-b043-40d283d4412f
📒 Files selected for processing (14)
java/lance-jni/src/blocking_dataset.rspython/python/lance/dataset.pypython/python/tests/test_geo.pypython/src/dataset.rspython/src/indices.rsrust/lance-index/src/scalar/rtree.rsrust/lance/src/dataset/index.rsrust/lance/src/dataset/overlay.rsrust/lance/src/dataset/scanner.rsrust/lance/src/index.rsrust/lance/src/index/api.rsrust/lance/src/index/create.rsrust/lance/src/index/scalar.rsrust/lance/src/index/scalar/rtree.rs
2 participants
Adds fragment-scoped RTree index training and native segment consolidation.
The merge path preserves retained geometry and null rows, applies fragment-reuse remapping before filtering, and rebuilds a canonical RTree segment. Python routing and end-to-end segmented build, merge, commit, and query coverage are included.
Validated with Rust geo checks, RTree tests, clippy, formatting, Python lint, and the geo integration test.