JingsongLi commented on PR #8731: URL: https://github.com/apache/paimon/pull/8731#issuecomment-5019196442
Thanks for working on this. I checked how similar systems handle repeated Parquet reads, and this is a well-established optimization rather than a Paimon-specific workaround: - PyArrow's `ParquetFileFragment` already caches metadata across scans when the fragment/dataset object is reused (`cache_metadata=True` by default): https://arrow.apache.org/docs/python/generated/pyarrow.dataset.ParquetFileFragment.html - DataFusion 50 added a production-ready Parquet metadata cache for footers, statistics, and page indexes. It uses a memory-bounded LRU (50 MB by default), validates entries using file identity metadata, and reports a 12x improvement for point-read microbenchmarks: https://datafusion.apache.org/blog/output/2025/09/29/datafusion-50.0.0/#parquet-metadata-cache - DuckDB has cached Parquet metadata since 2020 (https://github.com/duckdb/duckdb/pull/1103) and later changed its object cache to a memory-bounded LRU after unbounded metadata entries caused excessive memory usage: https://github.com/duckdb/duckdb/pull/20157 So the overall direction here looks sound, especially for immutable Paimon data files and long-lived PyTorch workers. Reusing the PyArrow Dataset is also a pragmatic way to extend the lifetime of PyArrow's built-in fragment metadata cache. The per-`FileIO` / filesystem isolation, single-flight loading, fork reset, disabled-by-default behavior, and the test that verifies reduced footer I/O are all useful safeguards. A few suggestions: 1. **Consider bounding by estimated bytes, not only entry count.** Footer metadata size can vary substantially with the number of columns and row groups, so 256 entries does not correspond to a predictable memory budget. DataFusion and current DuckDB both use byte-based limits. This could be a follow-up if estimating the retained PyArrow Dataset size is difficult. 2. **Please make the immutability/staleness contract explicit.** A path-only key is safe for Paimon-managed immutable data files, but could return stale schema/statistics if this reader is ever used for files overwritten in place. More general caches include size, last-modified time, ETag, or object version in the identity. 3. **A remote-storage benchmark would make the benefit easier to evaluate.** For example: repeated filtered reads over OSS/S3 across multiple epochs, reporting latency, remote request count, metadata size, and cache hit rate. The current tests prove that footer I/O is reduced, but not how much it changes an expected training workload. 4. **Some observability would help production tuning.** Hit/miss/load/eviction counters and estimated retained metadata bytes would make it easier to choose a cache size and confirm that a workload actually benefits. This is particularly relevant because separately deserialized `FileIO` instances (for example, separate Ray tasks) intentionally do not share entries. Overall, this has strong precedent in other Parquet engines. My main design concern is the predictability of memory usage; the cache scope and immutable-file assumption should also be documented clearly. -- 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]
