xanderbailey commented on code in PR #54:
URL: https://github.com/apache/datafusion-iceberg/pull/54#discussion_r4228229896
##########
crates/datafusion/src/physical_plan/write.rs:
##########
@@ -103,9 +105,21 @@ impl IcebergWriteExec {
))
}
- // Create a record batch with serialized data files
- fn make_result_batch(data_files: Vec<String>) -> Result<RecordBatch> {
- let files_array = Arc::new(StringArray::from(data_files)) as ArrayRef;
+ // Each row holds one Avro container, amortizing its schema over all files
from a task.
+ fn make_result_batch(
+ data_files: Vec<DataFile>,
+ partition_type: &StructType,
+ format_version: FormatVersion,
+ ) -> Result<RecordBatch> {
+ if data_files.is_empty() {
+ return Ok(RecordBatch::new_empty(Self::make_result_schema()));
+ }
+
+ let mut buffer = Vec::new();
+ write_data_files_to_avro(&mut buffer, data_files, partition_type,
format_version)
+ .map_err(to_datafusion_error)?;
+ let files_array =
+ Arc::new(LargeBinaryArray::from_vec(vec![buffer.as_slice()])) as
ArrayRef;
Review Comment:
Claude's finding not my own - `LargeBinaryArray::from_vec` copies the entire
byte slice into a new Arrow values buffer. With all of a task's file metadata
now in `buffer`, this temporarily keeps two complete payloads alive and does an
extra O(payload size) copy. Since this function owns the `Vec<u8>`, consider
constructing the one-value array with
`OffsetBuffer::from_lengths([buffer.len()])` and `Buffer::from_vec(buffer)` so
Arrow takes ownership of the allocation. This is an optimization opportunity,
not a newly introduced correctness problem.
##########
crates/datafusion/src/physical_plan/write.rs:
##########
@@ -488,6 +490,60 @@ mod tests {
.build()
}
+ #[test]
+ fn test_float_partition_metadata_roundtrip() {
Review Comment:
Non-blocking test suggestion: this verifies the serialization helper, maying
consider adding an integration case that inserts REAL and DOUBLE finite
values/NaN/infinities/null into an identity-partitioned table, forces multiple
files into a task payload, and asserts the insert count, manifest file/row
counts, and unfiltered scans. This would help us from regressing in the future.
--
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]
