feat: expose cached file metadata APIs on FileFragment#7820
Conversation
Make the full and indexed file metadata lookup methods public so callers can reuse the fragment's dataset-level metadata cache instead of loading metadata independently through FileReader.
📝 WalkthroughWalkthrough
ChangesFileFragment Metadata Access
Estimated code review effort: 1 (Trivial) | ~2 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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 (3)
rust/lance/src/dataset/fragment.rs (3)
1623-1637: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winComplete the rustdoc for both newly public APIs.
get_file_metadatalacks an example and links to its related types, whileget_file_metadata_indexhas no rustdoc at all. Document the scheduler/file relationship, cache behavior,known_schema, and return types with examples matching the actual signatures.As per coding guidelines, public Rust APIs must include examples and links to relevant structs and methods.
Also applies to: 1640-1663
🤖 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/fragment.rs` around lines 1623 - 1637, The newly public methods get_file_metadata and get_file_metadata_index need complete rustdoc. Add documentation describing the FileScheduler relationship, metadata caching, known_schema behavior, and each method’s actual return type, with links to relevant structs and methods and examples that compile against their signatures. Keep the implementation unchanged.Source: Coding guidelines
1623-1663: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd regression coverage for the public accessors.
Add tests that invoke both methods through their public API and verify full/indexed metadata retrieval, including reuse of the dataset metadata cache.
As per coding guidelines, every 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 `@rust/lance/src/dataset/fragment.rs` around lines 1623 - 1663, Add regression tests for the public get_file_metadata and get_file_metadata_index accessors, covering complete and indexed metadata retrieval and confirming repeated calls reuse the dataset metadata cache. Use the existing fragment/dataset test utilities and exercise both methods through their public API, including the known_schema path where applicable.Source: Coding guidelines
1623-1628: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGuard the file-scheduler path before caching metadata. Both helpers trust
file_scheduler.reader().path()and will read/cache metadata for whatever handle they receive, even if it is not one ofself.metadata.files. Add a fragment-path check or make the precondition explicit in the API docs.🤖 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/fragment.rs` around lines 1623 - 1628, Update get_file_metadata and the related metadata helper to validate that file_scheduler.reader().path() belongs to self.metadata.files before reading or caching metadata. Reject handles for paths outside the fragment, or explicitly document this as a required API precondition if validation is intentionally omitted; ensure the cache is never accessed for an unrelated path.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/src/dataset/fragment.rs`:
- Around line 1623-1637: The newly public methods get_file_metadata and
get_file_metadata_index need complete rustdoc. Add documentation describing the
FileScheduler relationship, metadata caching, known_schema behavior, and each
method’s actual return type, with links to relevant structs and methods and
examples that compile against their signatures. Keep the implementation
unchanged.
- Around line 1623-1663: Add regression tests for the public get_file_metadata
and get_file_metadata_index accessors, covering complete and indexed metadata
retrieval and confirming repeated calls reuse the dataset metadata cache. Use
the existing fragment/dataset test utilities and exercise both methods through
their public API, including the known_schema path where applicable.
- Around line 1623-1628: Update get_file_metadata and the related metadata
helper to validate that file_scheduler.reader().path() belongs to
self.metadata.files before reading or caching metadata. Reject handles for paths
outside the fragment, or explicitly document this as a required API precondition
if validation is intentionally omitted; ensure the cache is never accessed for
an unrelated path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 09dd1f4a-e5dc-4823-ac05-7ddc4b35ef8d
📒 Files selected for processing (1)
rust/lance/src/dataset/fragment.rs
Lance row groups currently report memory_size as a placeholder value of 1, which gives cache and scheduling callers no useful idea of how much decoded memory a chunk may occupy. We need a best-effort estimate that also works for externally referenced Lance files, without relying on extra information being recorded when the file is written. This change derives the estimate from the existing schema and footer metadata. Fixed-width leaf columns use the logical row count multiplied by their value width. Variable-width and nested columns use Lance page metadata: completed pages follow the 8 MiB writer page target, the final page is prorated from its row count, and encoded page bytes are only used as a lower bound or single-page fallback instead of being mistaken for decoded memory. Deletions are handled by applying the live-to-physical row ratio once, and physical columns belonging to nested fields are grouped back into their top-level schema columns. The Rust bridge now provides estimates for each top-level column and keeps the fragment-level API as the saturating sum of those values. LanceTableReader distributes the fragment estimate across logical chunks according to their row ranges, so the chunk values add back up to the fragment total and remain consistent across projections and cached reader creation. Estimation stays best effort on the C++ side, with a zero fallback when metadata cannot be estimated, including unsupported legacy files. The estimator resolves both normal dataset files and files referenced through external base paths. It only reads schema, column, and page metadata from the footer and never reads or decompresses data pages. It also relies on Lance's normalized ColumnInfo and PageInfo representation for storage formats 2.1 through 2.3, avoiding encoding-specific logic that would need to be maintained whenever the file format changes. The current Lance version does not expose get_file_metadata publicly, so the implementation temporarily uses the public read_all_metadata path without sharing Lance's metadata cache; after upgrading to a version containing lance-format/lance#7820, this can switch to cached metadata and remove the duplicate footer I/O. Signed-off-by: jiaqizho <[email protected]>
hi @BubbleCal , no sure why this python CI failed. could u retrigger it? thanks |
Seems flaky, just reran it |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Make the full and indexed file metadata lookup methods public so callers can reuse the fragment's dataset-level metadata cache instead of loading metadata independently through FileReader. I ran into this while trying to inspect the file footer of a fragment that had already been opened. The obvious option was to call FileReader::read_all_metadata, but that API reads directly from storage and does not reuse the dataset’s file metadata cache. This can result in another metadata I/O even though the fragment has already loaded the same information. FileFragment already has cache-aware helpers for loading the full metadata or metadata index. This change makes get_file_metadata and get_file_metadata_index public so other fragment-level operations can reuse the existing cached metadata instead of reading it again through FileReader. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Exposed file metadata and metadata index access for broader integration and tooling. <!-- end of auto-generated comment: release notes by coderabbit.ai --> (cherry picked from commit f23b8d6)
Make the full and indexed file metadata lookup methods public so callers can reuse the fragment's dataset-level metadata cache instead of loading metadata independently through FileReader.
I ran into this while trying to inspect the file footer of a fragment that had already been opened. The obvious option was to call FileReader::read_all_metadata, but that API reads directly from storage and does not reuse the dataset’s file metadata cache. This can result in another metadata I/O even though the fragment has already loaded the same information.
FileFragment already has cache-aware helpers for loading the full metadata or metadata index. This change makes get_file_metadata and get_file_metadata_index public so other fragment-level operations can reuse the existing cached metadata instead of reading it again through FileReader.
Summary by CodeRabbit