Repository navigation
Conversation
github-actions
Bot
added
documentation
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #25858 +/- ##
==========================================
+ Coverage 82.93% 82.95% +0.02%
==========================================
Files 1154 1154
Lines 457933 458638 +705
Branches 457933 458638 +705
==========================================
+ Hits 379786 380463 +677
- Misses 55128 55136 +8
- Partials 23019 23039 +20 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
villebro
reviewed
Sep 29, 2026
villebro
left a comment
villebro
left a comment
Member
There was a problem hiding this comment.
If ETag or version are available, I feel we should IMO not need to compare against last_modified or size. WDYT?
andygrove
reviewed
Sep 29, 2026
andygrove
left a comment
andygrove
left a comment
Member
There was a problem hiding this comment.
Thanks @limadog9, this looks like the right approach to me. Putting the object store URL in the cache keys, rather than checking it on the cached value, means files at the same path in different stores get separate entries instead of evicting each other. Sharing one comparison function between both caches, with a table-driven test over every ETag × version combination, is nice too.
The keys are built from the same URL on the planning path (ListingOptions::infer_schema, ListingTable statistics) and the execution path (ParquetFormat::create_physical_plan and the proto ParquetSource path), so metadata cached during planning is still reused at execution.
I left one comment I think should be addressed before merging: datafusion-cli's metadata_cache() output changes, see the comment on Display for ObjectStorePath. The others are nits. I also added some object_store caveats to the existing thread on the ETag rule.
This is an API change. The semver check flags the FileFormat inference methods, CachedParquetFileReaderFactory::new, DFParquetMetadata::with_file_metadata_cache, ParquetFileReader::with_metadata_cache and TableScopedPath, and the upgrade guide covers all of them.
Contributor
Author
|
Thanks for the reviews, @andygrove and @villebro. I've updated the PR to address your feedback, including the CLI cache output and ETag handling. Thanks for taking another look. |
villebro
approved these changes
Sep 29, 2026
villebro
left a comment
villebro
left a comment
Member
There was a problem hiding this comment.
Validated cache_manager.rs changes, LGTM. Referring to @andygrove to review other changes.
Contributor
Author
|
Hi @andygrove, following up on the cache identity fix. The updates for the CLI cache output and ETag handling are in, and Villebro has approved the cache manager changes. Could you take another look at the remaining changes when you have a chance? The latest Actions runs are still |
Key file caches by object store URL and path. Validate cached metadata using strong ETags, versions, or size and modification time, while checking the schema independently. Carry store identity through file-format inference and Parquet readers. Expose store URLs separately from relative paths in CLI cache functions, with regression coverage and upgrade guidance.
limadog9
force-pushed
the
fix/cache-object-identity
branch
from
October 20, 2026 11:27
c6fdf87 to
2ef0653
Compare
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
4 participants
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.
Which issue does this PR close?
Closes #25841.
Rationale for this change
An overwrite can keep the same file size and modification time while changing its statistics and Parquet footer. Different object stores can also contain the same relative path with identical metadata. Reusing either cache entry can produce incorrect query results.
What changes are included in this PR?
TableScopedPathcomposes the newStoreScopedPathkey.object_store_urlin all three CLI cache functions while keeping theirpathcolumns relative to the store.Validation still depends on current object metadata; cached listings and changes between planning and execution retain the consistency limitations described in the issue.
What is the testing strategy for this PR?
Regression tests cover identifier precedence, weak and unavailable ETags, schema changes, equal-size overwrites at an unchanged timestamp, and separate stores with identical object metadata. DataFrame tests assert correct aggregates and filtered scans. Cache tests verify isolation, reuse, and listing invalidation. A CLI regression test asserts exact relative paths and store URLs in all three cache functions for matching filenames in two stores.
Passed locally:
cargo test --profile ci -p datafusion-execution -p datafusion-datasource -p datafusion-datasource-parquet -p datafusion-cli --lib --bins cache --no-fail-fast(59 tests)cargo test --profile ci -p datafusion -p datafusion-cli --test core_integration datasource::(26 tests)--lib --tests --all-features -- -D warnings) fordatafusion,datafusion-execution,datafusion-catalog-listing,datafusion-datasource,datafusion-datasource-parquet, anddatafusion-cli.cargo fmt --all -- --check./ci/scripts/doc_prettier_check.sh --write --allow-dirtyAre there any user-facing changes?
File caches no longer reuse entries from another object store or from a changed strong ETag/version when fresh metadata is available. CLI cache functions expose the store URL separately from the relative path.
API changes:
FileFormatinference methods now take an object store URL;TableScopedPathcontains aStoreScopedPath;FileMetadataCacheusesStoreScopedPath; and the cached Parquet reader factory/cache builders require the URL. Seedocs/source/library-user-guide/upgrading/56.0.0.mdfor migration details.