XiaoHongbo-Hope commented on PR #8731: URL: https://github.com/apache/paimon/pull/8731#issuecomment-5031831349
> One additional thought on naming: `file-format.metadata-cache.enabled` may be a better long-term public option than `parquet.metadata-cache-enabled` if we intend this to be a capability of `FormatPyArrowReader` rather than a permanently Parquet-specific feature. > > I would suggest: > > ``` > file-format.metadata-cache.enabled > file-format.metadata-cache.max-entries > ``` > > `max-entries` is preferable to `size` here because the current limit counts cached objects rather than bytes. It also avoids confusion with options such as `local-cache.max-size`, which represent an actual memory size. > > If we use the generic `file-format.*` name, I think the implementation should be generalized at the same time: > > * Route all supported formats through a `_get_or_load_file_dataset(...)` helper instead of enabling it only under `file_format == "parquet"`. > * Include `file_format` (and any format options that affect Dataset construction) in the cache key. > * Rename the implementation to something like `_FileFormatDatasetCache`, while keeping the public option named after the semantic effect rather than the cached Python object. > * Document that the exact cached metadata and benefit are format-dependent. For Parquet, Dataset reuse retains footer-derived fragment metadata; for other formats it may currently only avoid dataset/schema discovery work. > > For example, the option description could say: > > > Cache reusable PyArrow Dataset and fragment metadata across reads in the current process. The cached metadata and performance benefit depend on the file format. For Parquet, this reuses footer-derived metadata such as schema and row-group statistics. > > If this PR intentionally guarantees and tests only Parquet footer reuse, keeping a `parquet.*` public option is more precise. But if we want one stable API that can cover ORC/IPC improvements later, I prefer `file-format.metadata-cache.*`, provided the code path and cache key are made format-aware now. Thanks. This PR intentionally guarantees and tests only Parquet footer reuse, so I kept the more precise parquet.* namespace and renamed the limit to parquet.metadata-cache-max-entries. A format-aware generic cache can be introduced if ORC/IPC support is added later. -- 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]
