andygrove opened a new issue, #25841: URL: https://github.com/apache/datafusion/issues/25841
### Is your feature request related to a problem or challenge? The file statistics cache and the file metadata (Parquet footer) cache treat a cached entry as valid if the file's size and last modified time still match (plus the schema fingerprint, for statistics): - [`CachedFileMetadata::is_valid_for`](https://github.com/apache/datafusion/blob/991fd23dd0be0046af5945b8c3905612859b657a/datafusion/execution/src/cache/cache_manager.rs#L127-L137), used by `ListingTable` when [collecting statistics](https://github.com/apache/datafusion/blob/991fd23dd0be0046af5945b8c3905612859b657a/datafusion/catalog-listing/src/table.rs#L1132-L1143) - [`CachedFileMetadataEntry::is_valid_for`](https://github.com/apache/datafusion/blob/991fd23dd0be0046af5945b8c3905612859b657a/datafusion/execution/src/cache/cache_manager.rs#L284-L287), used when [fetching Parquet metadata](https://github.com/apache/datafusion/blob/991fd23dd0be0046af5945b8c3905612859b657a/datafusion/datasource-parquet/src/metadata.rs#L272-L275) `ObjectMeta` also has `e_tag` and `version`, but neither is compared. S3 reports `Last-Modified` with one-second precision, so a file overwritten in place at the same size within the same second passes the check, and the entry for the old contents is used. Stale statistics give wrong answers for `COUNT(*)`, `MIN` and `MAX`, which can be answered from statistics alone. A stale footer would make the reader decode the new contents with the old file's row group and page offsets. The keys don't include the object store either. Statistics are keyed by [`TableScopedPath`](https://github.com/apache/datafusion/blob/991fd23dd0be0046af5945b8c3905612859b657a/datafusion/execution/src/cache/mod.rs#L150-L153), a table reference plus a store-relative path, and footers by the store-relative [`Path`](https://github.com/apache/datafusion/blob/991fd23dd0be0046af5945b8c3905612859b657a/datafusion/execution/src/cache/cache_manager.rs#L90) alone. So `s3://bucket-a/data/part-0.parquet` and `s3://bucket-b/data/part-0.parquet` share a footer cache entry. They share a statistics entry too when they're read through the same table name, or anonymously through `read_parquet`, which [uses the shared cache](https://github.com/apache/datafusion/blob/991fd23dd0be0046af5945b8c3905612859b657a/datafusion/core/src/execution/context/mod.rs#L1839-L1840) with no table reference. Only size and modification time tell them apart. Both cases need a size and a modification time to match exactly, so they're rare. But the exposure grows with the lifetime of the `RuntimeEnv`. A stronger check only helps where the file listing itself is fresh, for example with `list_files_cache_ttl` set or the list files cache turned off. This came up in Ballista, where apache/datafusion-ballista#2498 shares one file statistics cache across all the sessions a scheduler creates, while each job still lists its own files. @comphead pointed out the gap in review. ### Describe the solution you'd like 1. In both `is_valid_for` methods, also compare `e_tag` and `version` when the cached and the current `ObjectMeta` both have them, and fall back to size and modification time otherwise. S3 listings include the ETag, and `LocalFileSystem` derives one from the inode, modification time and size. Two different objects with the same content can have the same ETag, but then their statistics and footers are the same too. 2. Include the object store URL in the cache keys, or store it in each entry and check it. `TableScopedPath` is public, so this is an API change. With (1) in place it mainly matters for stores that don't report ETags. ### Describe alternatives you've considered Documenting the limitation instead. That's cheaper, but it's hard for users to make sure a rewrite always changes the size or the second-granularity modification time. ### Additional context - #23072 (done in #23201) made cached statistics schema-aware. This extends the same idea to the file's identity. - #18211 asks to pin scans to a specific `e_tag` or `version`. That's about consistency between planning and execution rather than cache validity, but it would use the same `ObjectMeta` fields. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
