fix: preserve RTree row id semantics#7666
Conversation
github-actions
Bot
added
A-index
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
27bfc21 to
f4972ab
Compare
|
@Xuanwo Could you help review this? thanks. |
f4972ab to
8821a30
Compare
|
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:
📝 WalkthroughWalkthroughThe R-tree scalar index now tracks nulls and query results by absolute row ID, tightens row-ID schema validation, propagates the representation through updates and training, and expands geo-index tests for stable and unstable row IDs. ChangesR-tree row ID tracking
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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-index/src/scalar/rtree.rs (1)
641-653: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winInclude the actual field name/type found in the schema-validation error.
The error message states the required shape but omits what was actually found, making failures harder to diagnose.
♻️ Proposed fix
let row_id_field = schema.field(1); if row_id_field.name() != ROW_ID || *row_id_field.data_type() != DataType::UInt64 { return Err(Error::invalid_input_source( - "Second field in RTree index schema must be _rowid with type UInt64".into(), + format!( + "Second field in RTree index schema must be {} with type UInt64, found field '{}' with type {:?}", + ROW_ID, row_id_field.name(), row_id_field.data_type() + ), )); }As per coding guidelines: "Include full context in error messages such as variable names, values, sizes, types, and indices; avoid generic messages like
"Invalid chunk 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-index/src/scalar/rtree.rs` around lines 641 - 653, Update validate_schema’s invalid-input error for the second field to include the actual row_id_field.name() and data_type() values alongside the expected _rowid/UInt64 requirements; preserve the existing schema validation and error type.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 480-482: Replace the panic in RTreeIndex::calculate_included_frags
with the existing Error::not_supported(...) error and preserve its Result return
type. Add a test covering this method that asserts it returns Result::Err
without panicking, including the expected unsupported-error behavior.
---
Outside diff comments:
In `@rust/lance-index/src/scalar/rtree.rs`:
- Around line 641-653: Update validate_schema’s invalid-input error for the
second field to include the actual row_id_field.name() and data_type() values
alongside the expected _rowid/UInt64 requirements; preserve the existing schema
validation and error type.
🪄 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: ddff190f-b0b7-4777-98b3-1b41774f6ae8
📒 Files selected for processing (2)
rust/lance-index/src/scalar/rtree.rsrust/lance/src/dataset/tests/dataset_geo.rs
8821a30 to
2c42824
Compare
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-index/src/scalar/rtree.rs (1)
717-717: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winPreallocate the per-batch selection buffer.
non_null_indexescan contain up tonum_rowsentries and is rebuilt for every batch.Proposed fix
- let mut non_null_indexes = vec![]; + let mut non_null_indexes = Vec::with_capacity(num_rows);As per coding guidelines, use
Vec::with_capacity()when size is known or estimable.🤖 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` at line 717, Update the per-batch non_null_indexes allocation in the R-tree batch processing logic to use Vec::with_capacity(num_rows), since the buffer can hold up to num_rows entries and is rebuilt for each batch.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/src/dataset/tests/dataset_geo.rs`:
- Line 211: Update the test’s rows vector initialization near the exact-two-row
assertion to use Vec::with_capacity with capacity 2 instead of Vec::new(),
preserving the existing row collection and assertions.
---
Outside diff comments:
In `@rust/lance-index/src/scalar/rtree.rs`:
- Line 717: Update the per-batch non_null_indexes allocation in the R-tree batch
processing logic to use Vec::with_capacity(num_rows), since the buffer can hold
up to num_rows entries and is rebuilt for each batch.
🪄 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: b271a3bf-4065-4bda-9c11-9613d7f65ebe
📒 Files selected for processing (2)
rust/lance-index/src/scalar/rtree.rsrust/lance/src/dataset/tests/dataset_geo.rs
386c76e to
5b37c05
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
rust/lance-index/src/scalar/rtree.rs (1)
480-482:⚠️ Potential issue | 🔴 CriticalPanic on manifest migration path still unresolved.
calculate_included_fragsstill panics viaunimplemented!(). Per the PR objective this method should now reject fragment coverage recalculation from row-ID payloads, not abort.commit.rscalls this method to rebuild stale fragment bitmaps, so this can crash manifest migration for any pre-existing RTree index. This was already flagged in a prior review round and remains unfixed; noResult::Errtest was added either.🛡️ Proposed fix
async fn calculate_included_frags(&self) -> Result<RoaringBitmap> { - unimplemented!() + // RTree indexes store logical row IDs, not physical row addresses, so + // fragment coverage cannot be derived from the payload alone. + Err(Error::not_supported( + "RTree index does not support fragment bitmap recalculation from row-ID payloads" + .into(), + )) }Add a
#[tokio::test]asserting this returnsResult::Errwith the expected message, per the "test withResult::Errassertions instead of#[should_panic]" guideline.🤖 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 480 - 482, Replace the unimplemented!() body in calculate_included_frags with a Result::Err carrying the expected rejection message, so manifest migration returns an error instead of panicking. Add a #[tokio::test] covering this method and assert it returns Err with the expected message, without using #[should_panic].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/src/dataset/tests/dataset_geo.rs`:
- Around line 156-158: Update the rstest configuration for test_geo_rtree_index
to replace #[values(true, false)] with named #[case::stable_row_ids(true)] and
#[case::unstable_row_ids(false)] entries, and change the parameter annotation to
#[case] while preserving the existing test behavior.
---
Duplicate comments:
In `@rust/lance-index/src/scalar/rtree.rs`:
- Around line 480-482: Replace the unimplemented!() body in
calculate_included_frags with a Result::Err carrying the expected rejection
message, so manifest migration returns an error instead of panicking. Add a
#[tokio::test] covering this method and assert it returns Err with the expected
message, without using #[should_panic].
🪄 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: 37c0cadf-30a1-4bf5-acf7-4e6cfa264ee5
📒 Files selected for processing (2)
rust/lance-index/src/scalar/rtree.rsrust/lance/src/dataset/tests/dataset_geo.rs
5b37c05 to
63ee5dc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/dataset/tests/dataset_geo.rs`:
- Around line 229-234: Update the row validation loop in the dataset geo test so
the use_stable_row_ids false branch asserts row_id equals row_addr, while
preserving the existing assert_ne! check for stable IDs and the fragment ID
assertion.
🪄 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: c8f24940-438c-4637-8ef1-c67e671461cc
📒 Files selected for processing (2)
rust/lance-index/src/scalar/rtree.rsrust/lance/src/dataset/tests/dataset_geo.rs
e828af1 to
d130677
Compare
d130677 to
020d4bc
Compare
|
Duplicate of #7932 |
1 participant
Summary
Testing
Summary by CodeRabbit
search_nullconsistently use global row IDs, including when row-id remapping is enabled._rowidand beUInt64(with clearer expected vs actual details).(row_id, row_addr)pairs, including additional stable-row-id checks.