fix: avoid panic when filtering on a nested Blob v2 extension field#7738
Open
mansiverma897993 wants to merge 1 commit into
Open
fix: avoid panic when filtering on a nested Blob v2 extension field#7738mansiverma897993 wants to merge 1 commit into
mansiverma897993 wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthrough
ChangesNested blob projection
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/lance-core/src/datatypes/field.rs (1)
302-308: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing descriptive message on
assert!.The touched
assert!still lacks a descriptive message. Per coding guidelines, assertions guarding non-corruption invariants should carry a message explaining what failed, especially since this now has four disjuncts covering distinct scenarios (map fields, projection membership, leaf fields, blobs).♻️ Suggested message
assert!( // One of the following must be true !children.is_empty() // Some children were projected || !projection.contains_field_id(self.id) // Caller is not asking for this field || self.children.is_empty() // This isn't a nested field - || self.is_blob() // This is a blob field + || self.is_blob(), // This is a blob field + "field '{}' (id {}) is a nested, selected field but all children were projected away", + self.name, self.id );As per coding guidelines, "Prefer
debug_assert!overassert!for non-safety invariants; reserveassert!for conditions preventing data corruption, and always include descriptive messages."🤖 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-core/src/datatypes/field.rs` around lines 302 - 308, Add a descriptive failure message to the assertion guarding the projected children invariant in the relevant field projection logic. Keep all four existing conditions unchanged, and explain that a nested non-blob field requested by the projection must retain projected children.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-core/src/datatypes/field.rs`:
- Around line 310-320: Update Field::apply_projection so the unloaded_mut call
only applies to blob-v2 fields or fields that originally had children, not
legacy LargeBinary fields with no children. Preserve loaded legacy blobs when
blob_handling requests raw binary values, while retaining existing projection
behavior for applicable blob-v2 fields.
In `@rust/lance-core/src/datatypes/schema.rs`:
- Around line 1638-1676: Strengthen repro_nested_blob_v2_projection_panic by
inspecting the resulting schema after scan_projection.into_schema() instead of
only checking that it completes. Locate image.image_bytes and assert it is
represented as the unloaded blob descriptor, with non-empty descriptor children
and the expected logical blob-v2 type, preventing a blob-typed struct with zero
children from passing.
---
Outside diff comments:
In `@rust/lance-core/src/datatypes/field.rs`:
- Around line 302-308: Add a descriptive failure message to the assertion
guarding the projected children invariant in the relevant field projection
logic. Keep all four existing conditions unchanged, and explain that a nested
non-blob field requested by the projection must retain projected children.
🪄 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: e84bd122-31c7-40b7-be08-d890d78a8f55
📒 Files selected for processing (2)
rust/lance-core/src/datatypes/field.rsrust/lance-core/src/datatypes/schema.rs
Comment on lines
310
to
320
| if children.is_empty() && !projection.contains_field_id(self.id) { | ||
| None | ||
| } else { | ||
| let mut new_field = self.clone(); | ||
| new_field.children = children; | ||
| Some(projection.blob_handling.unload_if_needed(new_field)) | ||
| let mut new_field = projection.blob_handling.unload_if_needed(new_field); | ||
| if new_field.is_blob() && new_field.children.is_empty() { | ||
| new_field.unloaded_mut(); | ||
| } | ||
| Some(new_field) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate BlobHandling::should_unload and any tests exercising legacy (non-v2) blob loaded/unloaded behavior.
rg -n -C5 'fn should_unload' rust/lance-core/src/datatypes/schema.rs
rg -n -C3 'BlobHandling' rust/lance-core/src/datatypes/schema.rs
rg -nP --type=rust -C3 '\bBLOB_META_KEY\b' rust/lance-core
rg -nP --type=rust -C5 'blob_handling.*all_binary|all_binary' rustRepository: lance-format/lance
Length of output: 10990
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the projection logic and related blob helpers.
sed -n '280,330p' rust/lance-core/src/datatypes/field.rs
printf '\n---\n'
sed -n '540,580p' rust/lance-core/src/datatypes/field.rs
printf '\n---\n'
sed -n '1070,1095p' rust/lance-core/src/datatypes/schema.rs
printf '\n---\n'
sed -n '1848,1945p' rust/lance-core/src/datatypes/field.rs
printf '\n---\n'
sed -n '10260,10310p' rust/lance/src/dataset/scanner.rsRepository: lance-format/lance
Length of output: 10828
Preserve loaded legacy blobs in Field::apply_projection
new_field.is_blob() && new_field.children.is_empty() also matches legacy LargeBinary blob fields, since they naturally have no children. That forces unloaded_mut() even when blob_handling wants the raw binary value, so AllBinary/SomeBinary can վերադարձ the descriptor struct instead of bytes. Narrow this to blob-v2 fields or to fields that originally had children.
🤖 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-core/src/datatypes/field.rs` around lines 310 - 320, Update
Field::apply_projection so the unloaded_mut call only applies to blob-v2 fields
or fields that originally had children, not legacy LargeBinary fields with no
children. Preserve loaded legacy blobs when blob_handling requests raw binary
values, while retaining existing projection behavior for applicable blob-v2
fields.
Comment on lines
+1638
to
+1676
| #[test] | ||
| fn repro_nested_blob_v2_projection_panic() { | ||
| use arrow_schema::{Field as ArrowField, Schema as ArrowSchema}; | ||
| use lance_arrow::{ARROW_EXT_NAME_KEY, BLOB_V2_EXT_NAME}; | ||
|
|
||
| // image: struct<image_bytes: extension<lance.blob.v2>, error: utf8> | ||
| let blob_meta: HashMap<String, String> = | ||
| [(ARROW_EXT_NAME_KEY.to_string(), BLOB_V2_EXT_NAME.to_string())] | ||
| .into_iter() | ||
| .collect(); | ||
| let image_bytes = ArrowField::new( | ||
| "image_bytes", | ||
| ArrowDataType::Struct(ArrowFields::from(vec![ | ||
| ArrowField::new("data", ArrowDataType::LargeBinary, true), | ||
| ArrowField::new("uri", ArrowDataType::Utf8, true), | ||
| ])), | ||
| true, | ||
| ) | ||
| .with_metadata(blob_meta); | ||
| let error = ArrowField::new("error", ArrowDataType::Utf8, true); | ||
| let image = ArrowField::new( | ||
| "image", | ||
| ArrowDataType::Struct(ArrowFields::from(vec![image_bytes, error])), | ||
| true, | ||
| ); | ||
|
|
||
| let mut schema = Schema::try_from(&ArrowSchema::new(vec![image])).unwrap(); | ||
| schema.set_field_id(None); | ||
| let base = Arc::new(schema); | ||
|
|
||
| // Mirrors Scanner::filtered_projection for `image.image_bytes IS NOT NULL` | ||
| let filter_schema = Projection::empty(base.clone()) | ||
| .union_column("image.image_bytes", OnMissing::Error) | ||
| .unwrap() | ||
| .into_schema(); | ||
| let scan_projection = Projection::empty(base.clone()).union_schema(&filter_schema); | ||
| let _read_schema = scan_projection.into_schema(); // panics in Field::apply_projection | ||
| } | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Strengthen the regression test beyond "doesn't panic".
The test only verifies that into_schema() doesn't panic; it doesn't assert what the resulting image.image_bytes field actually looks like. Given the fix in field.rs forces blob fields with empty projected children into their descriptor view, this test should also assert that the resulting field is correctly unloaded (e.g., non-empty descriptor children, expected logical type) so a future regression that produces a structurally-invalid field (e.g., blob-typed struct with zero children) would be caught, not just one that panics.
✅ Suggested strengthening
let scan_projection = Projection::empty(base.clone()).union_schema(&filter_schema);
- let _read_schema = scan_projection.into_schema(); // panics in Field::apply_projection
+ let read_schema = scan_projection.into_schema(); // used to panic in Field::apply_projection
+ let image_bytes = read_schema
+ .field("image.image_bytes")
+ .expect("image_bytes should survive projection");
+ assert!(!image_bytes.children.is_empty(), "blob field must not end up with empty children");📝 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.
Suggested change
| #[test] | |
| fn repro_nested_blob_v2_projection_panic() { | |
| use arrow_schema::{Field as ArrowField, Schema as ArrowSchema}; | |
| use lance_arrow::{ARROW_EXT_NAME_KEY, BLOB_V2_EXT_NAME}; | |
| // image: struct<image_bytes: extension<lance.blob.v2>, error: utf8> | |
| let blob_meta: HashMap<String, String> = | |
| [(ARROW_EXT_NAME_KEY.to_string(), BLOB_V2_EXT_NAME.to_string())] | |
| .into_iter() | |
| .collect(); | |
| let image_bytes = ArrowField::new( | |
| "image_bytes", | |
| ArrowDataType::Struct(ArrowFields::from(vec![ | |
| ArrowField::new("data", ArrowDataType::LargeBinary, true), | |
| ArrowField::new("uri", ArrowDataType::Utf8, true), | |
| ])), | |
| true, | |
| ) | |
| .with_metadata(blob_meta); | |
| let error = ArrowField::new("error", ArrowDataType::Utf8, true); | |
| let image = ArrowField::new( | |
| "image", | |
| ArrowDataType::Struct(ArrowFields::from(vec![image_bytes, error])), | |
| true, | |
| ); | |
| let mut schema = Schema::try_from(&ArrowSchema::new(vec![image])).unwrap(); | |
| schema.set_field_id(None); | |
| let base = Arc::new(schema); | |
| // Mirrors Scanner::filtered_projection for `image.image_bytes IS NOT NULL` | |
| let filter_schema = Projection::empty(base.clone()) | |
| .union_column("image.image_bytes", OnMissing::Error) | |
| .unwrap() | |
| .into_schema(); | |
| let scan_projection = Projection::empty(base.clone()).union_schema(&filter_schema); | |
| let _read_schema = scan_projection.into_schema(); // panics in Field::apply_projection | |
| } | |
| #[test] | |
| fn repro_nested_blob_v2_projection_panic() { | |
| use arrow_schema::{Field as ArrowField, Schema as ArrowSchema}; | |
| use lance_arrow::{ARROW_EXT_NAME_KEY, BLOB_V2_EXT_NAME}; | |
| // image: struct<image_bytes: extension<lance.blob.v2>, error: utf8> | |
| let blob_meta: HashMap<String, String> = | |
| [(ARROW_EXT_NAME_KEY.to_string(), BLOB_V2_EXT_NAME.to_string())] | |
| .into_iter() | |
| .collect(); | |
| let image_bytes = ArrowField::new( | |
| "image_bytes", | |
| ArrowDataType::Struct(ArrowFields::from(vec![ | |
| ArrowField::new("data", ArrowDataType::LargeBinary, true), | |
| ArrowField::new("uri", ArrowDataType::Utf8, true), | |
| ])), | |
| true, | |
| ) | |
| .with_metadata(blob_meta); | |
| let error = ArrowField::new("error", ArrowDataType::Utf8, true); | |
| let image = ArrowField::new( | |
| "image", | |
| ArrowDataType::Struct(ArrowFields::from(vec![image_bytes, error])), | |
| true, | |
| ); | |
| let mut schema = Schema::try_from(&ArrowSchema::new(vec![image])).unwrap(); | |
| schema.set_field_id(None); | |
| let base = Arc::new(schema); | |
| // Mirrors Scanner::filtered_projection for `image.image_bytes IS NOT NULL` | |
| let filter_schema = Projection::empty(base.clone()) | |
| .union_column("image.image_bytes", OnMissing::Error) | |
| .unwrap() | |
| .into_schema(); | |
| let scan_projection = Projection::empty(base.clone()).union_schema(&filter_schema); | |
| let read_schema = scan_projection.into_schema(); // used to panic in Field::apply_projection | |
| let image_bytes = read_schema | |
| .field("image.image_bytes") | |
| .expect("image_bytes should survive projection"); | |
| assert!( | |
| !image_bytes.children.is_empty(), | |
| "blob field must not end up with empty children" | |
| ); | |
| } |
🤖 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-core/src/datatypes/schema.rs` around lines 1638 - 1676, Strengthen
repro_nested_blob_v2_projection_panic by inspecting the resulting schema after
scan_projection.into_schema() instead of only checking that it completes. Locate
image.image_bytes and assert it is represented as the unloaded blob descriptor,
with non-empty descriptor children and the expected logical blob-v2 type,
preventing a blob-typed struct with zero children from passing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a Rust assertion panic in
Field::apply_projectionwhen a filter is applied on a nested Blob v2 extension field (e.g.,image.image_bytes IS NOT NULL), resolving issue #7707.Root Cause
When a filter is applied to a Blob v2 field, the query filter is parsed and evaluated. The filter schema unloads the blob to read its metadata/description, replacing its physical child fields (e.g.,
data,uri) with descriptor children that have a field ID of-1.When this filter schema is unioned into the scan projection:
field_idsset.Field::apply_projectioncall on the base schema, the physical children are filtered out, leaving the projected children list empty.!children.is_empty() || !projection.contains_field_id(self.id) || self.children.is_empty().Changes
Field::apply_projectioninrust/lance-core/src/datatypes/field.rsto allow blob fields to bypass the empty-children assertion since they can be dynamically unloaded.unloaded_mut(). This guarantees the field correctly holds descriptor children instead of an empty struct list.repro_nested_blob_v2_projection_panicinrust/lance-core/src/datatypes/schema.rsto prevent regressions.Summary by CodeRabbit
Bug Fixes
Tests