[limadog9]

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?

• Validate cached statistics and Parquet metadata using strong ETags when both are available, otherwise shared versions, otherwise size and modification time. Schema validation still applies independently. Weak HTTP ETags are treated as missing, and opaque ETag values are compared verbatim.
• Include the object store URL in statistics, listing, and Parquet metadata cache keys, and carry it through schema inference, statistics inference, and execution readers. TableScopedPath composes the new StoreScopedPath key.
• Keep anonymous listing-cache invalidation within the affected object store, and compute the store URL once per scan.
• Expose object_store_url in all three CLI cache functions while keeping their path columns relative to the store.
• Document the public API and CLI changes in the upgrade guide and CLI reference.
• Remove a redundant semicolon flagged by strict Windows linting.

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)
• Strict Clippy (--lib --tests --all-features -- -D warnings) for datafusion, datafusion-execution, datafusion-catalog-listing, datafusion-datasource, datafusion-datasource-parquet, and datafusion-cli.
• cargo fmt --all -- --check
• ./ci/scripts/doc_prettier_check.sh --write --allow-dirty

Are 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: FileFormat inference methods now take an object store URL; TableScopedPath contains a StoreScopedPath; FileMetadataCache uses StoreScopedPath; and the cached Parquet reader factory/cache builders require the URL. See docs/source/library-user-guide/upgrading/56.0.0.md for migration details.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 95.36266% with 39 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.95%. Comparing base (d89a85d) to head (2ef0653).
⚠️ Report is 131 commits behind head on main.

Files with missing lines	Patch %	Lines
...afusion/core/src/datasource/file_format/parquet.rs	91.17%	0 Missing and 9 partials ⚠️
datafusion/execution/src/cache/cache_manager.rs	94.30%	9 Missing ⚠️
datafusion/datasource/src/url.rs	86.88%	0 Missing and 8 partials ⚠️
datafusion/core/src/datasource/file_format/csv.rs	54.54%	0 Missing and 5 partials ⚠️
datafusion/catalog-listing/src/table.rs	90.62%	0 Missing and 3 partials ⚠️
datafusion/datasource-arrow/src/file_format.rs	0.00%	0 Missing and 2 partials ⚠️
datafusion/datasource-parquet/src/metadata.rs	98.13%	0 Missing and 2 partials ⚠️
datafusion/datasource-parquet/src/source.rs	80.00%	0 Missing and 1 partial ⚠️

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[villebro]

If ETag or version are available, I feel we should IMO not need to compare against last_modified or size. WDYT?

[andygrove]

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.

[limadog9]

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]

Validated cache_manager.rs changes, LGTM. Referring to @andygrove to review other changes.

[limadog9]

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 action_required; could a maintainer approve CI? GitHub is also reporting merge conflicts with main, so a rebase is needed before merging. Thanks!

[andygrove]

Thanks for tackling this. Scoping the metadata, statistics and listing caches by object store and validating by ETag is the right direction, and parquet_caches_separate_object_stores and parquet_caches_refresh_changed_etag are good tests. I have one correctness gap and a few smaller points.

1. Re-registering a store under the same URL keeps the old store's entries valid

The caches are keyed on ObjectStoreUrl, not on the store instance. If register_object_store replaces the store for a URL (new credentials, a different endpoint, a repointed mem://), the old entries stay in the cache. If the new store has a file with the same path, size, mtime and ETag, it gets the old store's footer and statistics.

This reproduces on the PR head (2ef06534). The test below fails with the old data (expected min=7 max=9, actual min=1 max=3):

Test (add to cache_object_identity.rs)

#[tokio::test]
async fn parquet_caches_reregistered_object_store() {
    let first = Arc::new(FixedTimeStore::default());
    let second = Arc::new(FixedTimeStore::default());
    let path = Path::from("data.parquet");
    let first_bytes = parquet_bytes(vec![1, 2, 3]);
    let second_bytes = parquet_bytes(vec![7, 8, 9]);
    assert_eq!(first_bytes.len(), second_bytes.len());
    first.put(&path, first_bytes.into()).await.unwrap();
    second.put(&path, second_bytes.into()).await.unwrap();
    // The two stores are indistinguishable by ObjectMeta, ETag included.
    assert_eq!(
        first.head(&path).await.unwrap(),
        second.head(&path).await.unwrap()
    );

    let ctx = SessionContext::new();
    let url = Url::parse("mem://reused").unwrap();
    ctx.runtime_env().register_object_store(&url, first);
    let first_result = aggregate_file(&ctx, "mem://reused/data.parquet").await;
    assert_batches_eq!(
        [
            "+-----+-----+-------+",
            "| min | max | count |",
            "+-----+-----+-------+",
            "| 1   | 3   | 3     |",
            "+-----+-----+-------+",
        ],
        &first_result
    );

    // Point the same URL at a different backend holding different bytes.
    ctx.runtime_env().register_object_store(&url, second);
    let second_result = aggregate_file(&ctx, "mem://reused/data.parquet").await;
    assert_batches_eq!(
        [
            "+-----+-----+-------+",
            "| min | max | count |",
            "+-----+-----+-------+",
            "| 7   | 9   | 3     |",
            "+-----+-----+-------+",
        ],
        &second_result
    );
}

The setup is narrow, since the ETag and size must also match. Still, the PR title says it isolates object stores, and this is the remaining way for one store's cache entries to reach another. I haven't checked the base commit; I'd expect the same stale result there, so this is probably a gap rather than a regression. Possible fixes: evict entries for the URL when register_object_store replaces a store, or include a per-registration identity in the key.

2. A matching ETag skips the size and mtime check (cache_manager.rs, file_metadata_matches)

This is a design question, not a bug I can show. A non-weak ETag match now returns true without looking at size or mtime, and a matching version does the same. That is correct for stores whose ETags are content or version based (S3, GCS, Azure), but it trusts every store to return unique ETags, which the old check did not. Requiring size and mtime to agree as well would cost nothing in correct cases and keep the old safety net. I'm not asking for a change if you'd rather keep this.

3. Public API changes

• FileFormat::infer_stats and infer_ordering gain an &ObjectStoreUrl parameter. Only Parquet uses it (the other formats take _object_store_url), and every downstream FileFormat implementation stops compiling. Could the store scope be applied where the cache handle is created instead, so the trait doesn't change?
• FileMetadataCache is now keyed on StoreScopedPath, and TableScopedPath.path became store_path.path. A TableScopedPath::new(...) constructor or path() accessor would ease migration. The 56.0.0 upgrade guide helps, but source-level compatibility is lost.
• datafusion-cli: object_store_url is inserted right after path in the metadata_cache() and statistics_cache() schemas (and the list-files one), which shifts columns for positional consumers. Appending it would keep them working.

4. Minor

• Each cache get and put builds a StoreScopedPath, cloning the URL and path, per file, and list_with_cache re-parses the URL string twice. Holding an Arc<ObjectStoreUrl> computed once would avoid this. I haven't measured it.
• with_metadata_cache and with_file_metadata_cache require a URL even when the cache is None. Passing the pair as Option<(Arc<cache>, ObjectStoreUrl)> would make that invalid state unrepresentable.

Item 1 is the one I'd like resolved or explicitly scoped out before merge. The rest are suggestions.

[github-actions]

Marked as draft after 14 days without activity: merge conflicts.

Please address these items and mark Ready for review when ready.

[villebro]

@limadog9 it seems this PR has gone stale, would you mind rebasing it to resolve the conflicts and taking a look at @andygrove 's review?

[villebro]

@limadog9 this still appears to be marked as Draft. Is this ready for re-review?