xudong963 opened a new pull request, #27027: URL: https://github.com/apache/datafusion/pull/27027
## Which issue does this PR close? - Closes #27026. - Related: #23472 (exposing per-file metadata from `ParquetSink`). ## Rationale for this change A `COPY ... TO ... STORED AS PARQUET` or `INSERT INTO` that writes many files keeps growing in memory until the statement finishes, outside the memory pool. `ParquetSink` kept every written file's `ParquetMetaData` (all column chunk statistics plus page indexes) until the sink was dropped, in two places: the writer tasks returned `(Path, ParquetMetaData)` through a `JoinSet` that is only drained after the demux loop has announced the last file, and the join loop then moved each one into `written: HashMap<Path, ParquetMetaData>`. Nothing in the engine read that map; the only caller of `ParquetSink::written()` was the `datafusion-core` unit tests. The join loop itself only needs two numbers per file for the `rows_written` / `bytes_written` metrics. #27026 has the production numbers: a ~700-file partition write grew RSS by roughly 1 GiB per 10 GB written until it was OOM-killed. ## What changes are included in this PR? - Writer tasks reduce the closed file's metadata to `(rows, compressed bytes)` (`written_file_counts`) and drop the `ParquetMetaData` inside the task, so the sink holds no per-file state once a file is closed. Metrics are unchanged. - The `written` map and `ParquetSink::written()` are removed. #27026 floated deprecating `written()` instead, but a deprecated method that returns an empty map would be a silent behavior change, and keeping it working means keeping the retention. Removing it is the honest option; an opt-in way to receive each file's metadata as it is written can be added for #23472 without carrying this cost by default. - The duplicate-path check that rode on the map is gone with it. Output paths are generated by the demuxer (`<write_id>_<n>`), so it had no way to fire. - `parking_lot` is dropped from `datafusion-datasource-parquet`; the mutex was its only use. - The `datafusion-core` `ParquetSink` tests get a `SinkTestStore` fixture (temp dir + `LocalFileSystem`) and read each written file's footer back from the store instead of asking the sink. As a side effect the temp directory now lives for the whole test instead of being deleted right after creation and silently recreated by the object store on the first write. - Upgrade guide entry for 56.0.0. ## What is the testing strategy for this PR? Existing coverage, now exercised through the written files rather than the sink's map: `cargo test -p datafusion --lib -- datasource::file_format::parquet::tests::parquet_sink` (8 tests: single, partitioned and parallel writes, arrow-schema metadata, extension handling, memory reservation). `cargo clippy --all-targets --all-features -D warnings` on `datafusion-datasource-parquet` and `cargo clippy --tests` on `datafusion` are clean. ## Are there any user-facing changes? `ParquetSink::written()` is removed (`api change`). A GitHub code search finds no callers outside copies of DataFusion's own test file. The upgrade guide points at `ParquetMetaDataReader` / `ParquetObjectReader` for reading a written file's metadata back and at #23472 for a sink-side API. -- 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]
