timsaucer commented on code in PR #23656:
URL: https://github.com/apache/datafusion/pull/23656#discussion_r4376680137
##########
datafusion/datasource-parquet/src/sink.rs:
##########
@@ -456,6 +456,36 @@ impl DataSink for ParquetSink {
physical_plan_type:
Some(PhysicalPlanType::ParquetSink(Box::new(node))),
}))
}
+
+ fn file_metadata(&self) -> Vec<FileWriteMetadata> {
+ let written = self.written.lock();
+ written
+ .iter()
+ .map(|(path, parquet_meta)| {
+ let row_count: u64 = parquet_meta
+ .file_metadata()
+ .num_rows()
+ .try_into()
+ .unwrap_or(0);
+ let byte_size: u64 = parquet_meta
Review Comment:
Adding up `compressed_size()` over the row groups doesn't give the file
size, because it leaves out the footer, the page indexes and the magic bytes.
That matters for the consumers this PR is aimed at. Iceberg's
`file_size_in_bytes` has to be exact, since readers use it to find the footer,
and Delta's `size` works the same way. A committer using this value would write
broken metadata.
We already track the exact number. `file_metadata.size` is the `AtomicU64`
passed to `ObjectWriterBuilder::with_bytes_written_counter` in
`create_async_arrow_writer`, and `output_single_parquet_file_parallelized` uses
it too. Could we store that next to the `ParquetMetaData` in `written` and
report it here? Then the `byte_size` docs on `FileWriteMetadata` could just say
"bytes written for this file" and drop the "approximation" caveat.
##########
datafusion/datasource/src/sink.rs:
##########
@@ -87,6 +116,21 @@ pub trait DataSink: Any + DisplayAs + Debug + Send + Sync {
) -> Result<Option<datafusion_proto_models::protobuf::PhysicalPlanNode>> {
Ok(None)
}
+
+ /// Returns metadata for files written during the most recent
Review Comment:
This says the metadata covers the files from the *most recent* `write_all`
call, but `ParquetSink` never clears `self.written`. It only does `try_insert`
in `write_all`, so entries pile up across calls. If the same sink is written to
twice, `file_metadata()` returns the files from both writes.
Could we either clear `written` at the start of `write_all` so the behavior
matches this doc, or change the doc to say it returns every file written by
this sink instance? Whichever we pick, a test that calls `write_all` twice on
the same sink would lock it in.
--
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]
