JingsongLi commented on PR #686: URL: https://github.com/apache/paimon-rust/pull/686#issuecomment-5235383721
Thanks for revising the object-store behavior. I traced the change from `Storage` through `FileIO` and `FileRead` into the Parquet metadata reader. I did not find a correctness issue, but I think two points should be addressed before merging. First, `supports_cheap_range_reads = true` does not necessarily mean that exact metadata reads are faster. The new tests show the request trade-off clearly: a small file on the prefetch path can be loaded with one read, while the exact path uses two serialized reads without page indexes and three when `OffsetIndex` is requested. The HDFS native reader reuses a file-scoped handle and block locations, but each range still creates a block stream and sends another read-block operation. This may be beneficial because it reads fewer bytes, but it can also increase per-file latency, especially when scanning many small Parquet files. Issue #687 says the reproducible HDFS measurements should be available before this PR is ready, but those numbers are not present yet. Could we add the file count, footer/index size distribution, request counts, and repeated wall-clock results before deciding whether this should be enabled unconditionally (or needs a file-size threshold)? Second, the policy currently expands a public, format-agnostic API for a single private Parquet consumer. `FileRead::supports_cheap_range_reads` is only consumed by `ArrowFileReader`, but propagating the value requires changes across `Storage -> FileIO -> InputFile/OutputFile -> InputFileReader -> FileRead`. Storing a read policy on `OutputFile` is a sign that the policy has crossed its natural boundary. A smaller design would keep the classification crate-private: 1. Keep `Storage::supports_cheap_range_reads()` as the backend decision point. 2. Expose it through a crate-private method on `FileIO`. 3. Have `DataFileReader` pass the policy to `create_format_reader_with_budget`. 4. Store the resulting metadata prefetch policy on `ParquetFormatReader` / `ArrowFileReader`. 5. Remove the public `FileRead` method and the capability fields on `InputFile`, `OutputFile`, and `InputFileReader`. `DataFileReader` already owns the `FileIO` and is the only production caller of `create_format_reader_with_budget`, so this keeps the same behavior for HDFS/local/memory, object stores, and `CustomFs` without widening the public I/O contract. -- 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]
