SteNicholas opened a new issue, #417: URL: https://github.com/apache/paimon-cpp/issues/417
### Search before asking - [x] I searched in the [issues](https://github.com/apache/paimon-cpp/issues) and found nothing similar. ### Motivation After a Parquet data file is closed, Paimon C++ gets the file's column statistics by opening the file it has just written and reading its footer back: - `DataFileWriter::GetResult()` → `GetFieldStats()` → `stats_extractor_->Extract(fs_, path_, pool_)` (`src/paimon/core/io/data_file_writer.cpp:57`, `:82`). `KeyValueDataFileWriter::GetFieldStats()` does the same for primary-key tables (`src/paimon/core/io/key_value_data_file_writer.cpp:200`). - `ParquetStatsExtractor::ExtractWithFileInfo()` (`src/paimon/format/parquet/parquet_stats_extractor.cpp:257`) calls `FileSystem::Open(path)` and `InputStream::Length()`, then opens a `parquet::arrow::FileReader`. That reader reads the file tail (Arrow 17 reads a fixed 64 KiB tail, plus a second read when the footer is larger) and Thrift-decodes the whole `FileMetaData` again. The writer already holds exactly this metadata. After `parquet::arrow::FileWriter::Close()`, `FileWriter::metadata()` returns the `FileMetaData` that was serialized into the footer. Re-reading it costs, for every written file: - An open plus a length lookup. On Jindo/OSS this includes the `getFileStatus` round trip that #331 avoids on the read side when the length is known. - One or more ranged reads of the footer. - A full Thrift decode of the footer, whose cost grows with columns × row groups. This happens for every data file produced by writes and by compaction, on both append and primary-key tables, before the file's `DataFileMeta` can be built. On high-latency storage it adds round trips for every rolled file, and the overhead weighs most when many small files are produced, for example level-0 files or many partitions/buckets each writing a file. Velox's Parquet writer uses the in-memory metadata instead: `Writer::close()` returns a `ParquetFileMetadata` wrapping `arrowContext_->writer->metadata()` (`velox/dwio/parquet/writer/Writer.cpp`), and statistics are derived from it without re-reading the file. ### Solution Derive the stats from the metadata the writer already holds, and keep the re-read path as a fallback. 1. **Keep the metadata.** In `ParquetFormatWriter::Finish()`, after `writer_->Close()`, retain `writer_->metadata()` (`std::shared_ptr<parquet::FileMetaData>`). 2. **Share the conversion.** Factor the part of `ParquetStatsExtractor::ExtractWithFileInfo()` that runs after the footer is obtained into a helper taking `const parquet::FileMetaData&`. That covers the per-row-group merge through `MergeStats`, `ConvertStatsToColumnStats`, nested-field handling and `FileInfo(num_rows)`. The re-read path and the in-memory path then produce stats through the same code. 3. **Use it from the data file writers.** `DataFileWriter` and `KeyValueDataFileWriter` take the stats from the format writer when it can provide them, and fall back to `stats_extractor_->Extract(fs_, path_, pool_)` otherwise, for example for other formats. `FormatWriter` and `FormatStatsExtractor` are exported under `include/paimon/format/`, so the plumbing should preferably stay internal, e.g. an internal interface that the Parquet writer implements and the data file writer checks for. A new virtual method on the public classes would change their ABI; if that route is preferred, the impact should be called out. 4. **Out of scope.** Callers that extract stats from files they did not write keep re-reading the footer, e.g. migration (`src/paimon/core/migrate/file_meta_utils.cpp:83`). ORC, Avro and Lance can adopt the same hook later. ### Anything else? **The in-memory `FileMetaData` is not built the same way as a decoded footer**, so equivalence has to be tested rather than assumed. In Arrow 17, `FileMetaDataBuilder::Finish()` (`cpp/src/parquet/metadata.cc`) default-constructs the `FileMetaData` and only calls `InitSchema()` and `InitKeyValueMetadata()`. As a result: - `writer_version_` is not parsed from `created_by` and stays a default `ApplicationVersion` with an empty application name. - `InitColumnOrders()` is not called. Reading `ColumnChunkMetaData::is_stats_set()` and `ApplicationVersion::HasCorrectStatistics()`, both paths should accept the statistics of every column with a known sort order. An empty application name is neither `parquet-cpp`, `parquet-mr` nor `unknown`, and the PARQUET-251 check compares application names before versions. A test should still pin this down: write files that cover - every supported primitive type, including DECIMAL stored as INT32/INT64, TIMESTAMP including INT96, STRING/BINARY, and FLOAT/DOUBLE with NaN; - all-null columns; - nested types; - multiple row groups; and assert that the in-memory stats equal the stats obtained by re-reading the footer. Suggested validation: - Unit tests for the equivalence above. - A test with a counting `FileSystem` asserting that closing a Parquet data file no longer opens it for reading. - Existing write and compaction integration tests pass unchanged. The `SimpleStats` stored in `DataFileMeta` must stay identical. No storage format or protocol change. ### Are you willing to submit a PR? - [ ] I'm willing to submit a PR! -- 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]
