Datasource: Add file_metadata() to DataSink for per-file write metadata - #23656
Datasource: Add file_metadata() to DataSink for per-file write metadata#23656qzyu999 wants to merge 1 commit into
Conversation
ed75a23 to
a4d415e
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #23656 +/- ##
==========================================
+ Coverage 80.66% 80.68% +0.01%
==========================================
Files 1086 1086
Lines 366694 367027 +333
Branches 366694 367027 +333
==========================================
+ Hits 295806 296121 +315
- Misses 53265 53277 +12
- Partials 17623 17629 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Add FileWriteMetadata struct to datafusion-datasource along with a new default method on the DataSink trait: - file_metadata(): Post-hoc accessor returning per-file path, row count, byte size, and optional format-specific metadata bytes after write_all completes. The default implementation returns an empty Vec, so existing DataSink implementations are unaffected. ParquetSink overrides file_metadata() to expose path, row_count, and byte_size for each written file using the existing self.written HashMap that already collects ParquetMetaData during writes. The typed ParquetMetaData remains accessible via ParquetSink::written() for Rust consumers who need column-level statistics directly. DataSinkExec gains a file_metadata() convenience accessor that delegates to the underlying sink. This enables external table formats (Iceberg, Delta Lake, Hudi) to obtain per-file metadata after a DataFusion write without re-reading file footers or bypassing the write pipeline entirely. Closes apache#23472
a4d415e to
f9c560a
Compare
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Adds a standardized way for DataSink implementations to expose per-file write metadata (e.g., file path, row count, size, and optional format-specific bytes) so external consumers (Iceberg/Delta/Hudi) can commit written files without re-reading footers or downcasting to ParquetSink.
Changes:
- Introduces
FileWriteMetadataand adds a defaultDataSink::file_metadata()accessor (returns empty by default). - Implements
file_metadata()forParquetSinkusing already-collectedwrittenmetadata (no extra I/O). - Adds a
DataSinkExec::file_metadata()convenience method and new unit tests in both crates.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| datafusion/datasource/src/sink.rs | Adds FileWriteMetadata, extends DataSink/DataSinkExec API, and adds trait-level tests. |
| datafusion/datasource-parquet/src/sink.rs | Implements ParquetSink::file_metadata() and adds parquet-specific tests validating behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn build_test_ctx(store_url: &ObjectStoreUrl) -> Arc<TaskContext> { | ||
| let tmp_dir = tempfile::TempDir::new().unwrap(); | ||
| let local = Arc::new( | ||
| LocalFileSystem::new_with_prefix(&tmp_dir) | ||
| .expect("should create object store"), | ||
| ); | ||
|
|
||
| let session = SessionConfig::default(); | ||
| let runtime = RuntimeEnv::default(); | ||
| runtime | ||
| .object_store_registry | ||
| .register_store(store_url.as_ref(), local); | ||
|
|
||
| Arc::new( | ||
| TaskContext::default() | ||
| .with_session_config(session) | ||
| .with_runtime(Arc::new(runtime)), | ||
| ) | ||
| } |
| let file_sink_config = FileSinkConfig { | ||
| original_url: String::default(), | ||
| object_store_url: object_store_url.clone(), | ||
| file_group: FileGroup::new(vec![PartitionedFile::new("/tmp".to_string(), 1)]), | ||
| table_paths: vec![ListingTableUrl::parse("file:///tmp/test/").unwrap()], | ||
| output_schema: Arc::clone(&schema), | ||
| table_partition_cols: vec![], | ||
| insert_op: InsertOp::Overwrite, | ||
| keep_partition_by_columns: false, | ||
| file_extension: "parquet".into(), | ||
| file_output_mode: FileOutputMode::Automatic, | ||
| }; |
| let row_count = parquet_meta.file_metadata().num_rows() as u64; | ||
| let byte_size: u64 = parquet_meta | ||
| .row_groups() | ||
| .iter() | ||
| .map(|rg| rg.compressed_size() as u64) | ||
| .sum(); |
| /// Sum of compressed row group sizes in bytes. | ||
| /// | ||
| /// Note: this may differ slightly from the actual on-disk file size as it | ||
| /// excludes the Parquet footer, page indexes, and other metadata overhead. | ||
| pub byte_size: u64, |
timsaucer
left a comment
There was a problem hiding this comment.
This looks like a good addition. I have a couple of suggestions and a question. Also I think the copilot suggestions look like they should be addressed.
| fn make_test_batch(schema: &SchemaRef) -> RecordBatch { | ||
| let col_a: ArrayRef = Arc::new(StringArray::from(vec!["foo", "bar", "baz"])); | ||
| let col_b: ArrayRef = Arc::new(StringArray::from(vec!["one", "two", "three"])); | ||
| RecordBatch::try_new(Arc::clone(schema), vec![col_a, col_b]).unwrap() |
There was a problem hiding this comment.
There is a record_batch! macro that makes this a little more pleasant.
| let batch = | ||
| RecordBatch::try_new(Arc::clone(&schema), vec![col_a, col_b]).unwrap(); |
There was a problem hiding this comment.
Same comment as above about reusing the record_batch! macro
| /// Sum of compressed row group sizes in bytes. | ||
| /// | ||
| /// Note: this may differ slightly from the actual on-disk file size as it | ||
| /// excludes the Parquet footer, page indexes, and other metadata overhead. |
There was a problem hiding this comment.
FileWriteMetada should be format agnostic, right? Here we're talking about row group sizes, which isn't a concept in all of the datasources. I think we just need a comment update here, but if this is specific to parquet then it probably needs to go into the format_metadata.
| /// reconstruct column statistics without re-reading the file. | ||
| /// | ||
| /// For formats that do not produce file-level metadata this is `None`. | ||
| pub format_metadata: Option<Bytes>, |
There was a problem hiding this comment.
I suppose the other option, which matches some metadata approaches is that these be key/value pairs. That has the advantage of being human readable but the downside of potential data loss on conversion, such as f64 -> String. Here we have to pay a serialization/deserialization cost so parsing strings is likely not too much less performant. It's probably not a hot path anyway.
Which issue does this PR close?
Closes #23472.
Rationale for this change
External table formats (Apache Iceberg, Delta Lake, Apache Hudi) require per-file column statistics (column_sizes, null_counts, lower/upper bounds, split_offsets) when committing written files to their catalogs. Currently,
ParquetSinkcomputes and stores this metadata internally (self.written) but theDataSinktrait only returns a row count forcing external consumers to either:DataSinkExec.sink()toParquetSinkand callwritten()(fragile, undocumented)This PR adds a standard, non-breaking mechanism to expose per-file metadata through the
DataSinktrait.What changes are included in this PR?
New type in
datafusion-datasource:FileWriteMetadata: path + row_count + byte_size + optional format-specific metadata bytesTrait extension (non-breaking, default implementation):
DataSink::file_metadata()Vec<FileWriteMetadata>(default: empty)ParquetSink implementation:
file_metadata()using the existingself.writtenHashMap that already collectsParquetMetaDataduring writes zero additional I/ODataSinkExec convenience:
file_metadata()method that delegates to the inner sink, avoiding the downcast danceThe typed
ParquetMetaDataremains accessible via the existingParquetSink::written()method for Rust consumers who need full column-level statistics directly. Theformat_metadata: Option<Bytes>field onFileWriteMetadatais reserved for future Thrift serialization when FFI consumers (e.g. Python via PyO3) need it.Are these changes tested?
Yes 14 new tests across both crates:
datafusion-datasource(7 tests):FileWriteMetadataequality and clone behaviorfile_metadata()returns empty afterwrite_allfile_metadata()returns correct entriesDataSinkExec::file_metadata()delegates correctlydatafusion-datasource-parquet(7 tests):write_allcount == sum of per-file row_counts)ParquetSink::written()accessorDataSinkExecwrapper withArc<dyn DataSink>dispatchAll existing
parquet_sink_write*tests continue to pass unchanged.Are there any user-facing changes?
New public API surface (additive only, no breaking changes):
DataSink::file_metadata()default methodFileWriteMetadatastructDataSinkExec::file_metadata()convenience method