perf: support indexed metadata for structural projections#7790
Conversation
github-actions
Bot
added
A-encoding
📝 WalkthroughWalkthroughIndexed metadata loading now supports structural projections by counting their required physical columns while excluding blob and packed-struct fields. Tests cover nested reads, physical-column remapping, metadata IO, opaque projections, and fragment heuristics. ChangesIndexed projection handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ProjectedFileReader
participant FileMetadataProvider
participant ProjectionShape
ProjectedFileReader->>FileMetadataProvider: Validate indexed projection
FileMetadataProvider->>ProjectionShape: Count structural physical columns
ProjectionShape-->>FileMetadataProvider: Return count or opaque result
FileMetadataProvider-->>ProjectedFileReader: Allow indexed loading or return NotSupported
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/lance-file/src/reader.rs (1)
3436-3474: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreamble
None-projection check is unrelated tometadata_keyand now runs redundantly per case.The block that opens with
Noneand assertsInvalidInput(zero-column projection) doesn't depend onmetadata_keyat all, yet with the new#[case::blob(...)]/#[case::packed_struct(...)]parameterization it now executes identically twice. Consider hoisting it into its own standalone test so the parameterized test only covers the opaque-projection-rejection behavior it's named for.♻️ Suggested split
- async fn test_lazy_reader_rejects_opaque_projection(#[case] metadata_key: &str) { + async fn test_lazy_reader_rejects_opaque_projection(#[case] metadata_key: &str) { let fs = FsFixture::default(); let written_file = create_some_file(&fs, LanceFileVersion::V2_1).await; let ordinary_projection = ReaderProjection::from_column_names( LanceFileVersion::V2_1, &written_file.schema, &["location.x"], ) .unwrap(); assert_eq!(ordinary_projection.schema.fields[0].children.len(), 1); assert!(ProjectedFileReader::supports_projection( &ordinary_projection, LanceFileVersion::V2_1 )); let file_scheduler = fs .scheduler .open_file(&fs.tmp_path, &CachedFileSize::unknown()) .await .unwrap(); - let err = ProjectedFileReader::try_open( - file_scheduler.clone(), - None, - Arc::<DecoderPlugins>::default(), - &test_cache(), - FileReaderOptions::default(), - ) - .await - .unwrap_err(); - assert!( - matches!(err, lance_core::Error::InvalidInput { .. }), - "expected InvalidInput, got {err:?}" - ); - let mut projection = ordinary_projection;(Move the removed block into a new, unparameterized
#[tokio::test] async fn test_lazy_reader_rejects_empty_projection().)🤖 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-file/src/reader.rs` around lines 3436 - 3474, Extract the `ProjectedFileReader::try_open` block using `None` and the `InvalidInput` assertion from `test_lazy_reader_rejects_opaque_projection` into a standalone `#[tokio::test]` named `test_lazy_reader_rejects_empty_projection`. Keep `test_lazy_reader_rejects_opaque_projection` parameterized so it only exercises behavior dependent on `metadata_key`.
🤖 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-file/src/reader.rs`:
- Around line 3436-3474: Extract the `ProjectedFileReader::try_open` block using
`None` and the `InvalidInput` assertion from
`test_lazy_reader_rejects_opaque_projection` into a standalone `#[tokio::test]`
named `test_lazy_reader_rejects_empty_projection`. Keep
`test_lazy_reader_rejects_opaque_projection` parameterized so it only exercises
behavior dependent on `metadata_key`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a8031457-e671-4b4f-8d79-c620aef8812a
📒 Files selected for processing (2)
rust/lance-file/src/reader.rsrust/lance/src/dataset/fragment.rs
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
2 participants
Summary
Indexed metadata loading currently rejects V2.1+ projections with explicit ordinary structural children. As a result, selecting a struct, list, or nested child from an ultrawide file still reads and deserializes metadata for every physical column even when the data-page projection is narrow.
ReaderProjectionalready records the selected physical leaves in DFS order, and indexed loading already compacts the selected metadata and renumbers decoder indices to0..N. The remaining blocker was the conservative eligibility check, which accepted only childless 1:1 logical-to-physical projections.This PR replaces that check with validation against the number of physical leaves required by the projected structural tree. It enables indexed metadata for ordinary full, partial, nullable, and reordered structural projections when the required physical-leaf count matches the selected column indices.
Blob and packed-struct fields remain unsupported by indexed metadata and continue to use the existing full-metadata reader. This PR does not add packed-struct projection or reconstruction logic.
Test coverage includes multi-leaf structs, partial and reordered projections, nullable structural fields, lists, metadata-subset I/O, and fixed-size-list regression cases.
Performance
The unrelated quadratic schema reconstruction cost was removed by #7766 before running these benchmarks.
The benchmarks used Criterion on an
m7i.4xlarge, with a single Lance file containing 65,536 physical columns. The measured path is the ordinary structural path retained in this PR.The metadata I/O tradeoff is:
Indexed loading still reads the complete column-metadata offset table and then issues reads for the selected metadata ranges, so the two additional logical reads are expected.
These benchmarks isolate the single-file reader path. They do not measure manifest/schema planning or sidecar-file fan-out. Scalar and primitive fixed-size-list projections were already eligible and serve only as regression controls, not as part of the newly enabled structural projection scope.