kosiew commented on code in PR #24924: URL: https://github.com/apache/datafusion/pull/24924#discussion_r4036881000
########## datafusion/common/src/config.rs: ########## Review Comment: Could we also add the matching `SET datafusion.execution.soft_max_bytes_per_output_file = 0` error case in `set_variable.slt`? `ConfigNonZeroUsize` already rejects zero, but adding coverage through the public SQL configuration path would keep this consistent with the sibling row-limit setting. ########## datafusion/datasource/src/write/demux.rs: ########## Review Comment: `get_array_memory_size()` measures the pre-serialization Arrow allocation rather than the number of bytes actually written to the output file. This is especially noticeable with Parquet, since the default compression is `zstd(3)`. A highly compressible blob batch could occupy hundreds of MiB or even GiB in Arrow memory while producing a very small Parquet row group. In that case, the demuxer can rotate before each following batch and create much smaller files than the configured `soft_max_bytes_per_output_file` target. That seems to conflict with the documented output-file-byte semantics and also affects the large-blob use case this option is intended to help with. Could we move the accounting to a serialized or emitted-byte boundary, with feedback to the demuxer if needed? Alternatively, the option would need to be narrowed and renamed to make it clear that it is based on input memory size. It would also be useful to add a regression test using a large, highly compressible `Binary` column with compressed Parquet. The current parameterized test forces `UNCOMPRESSED`, so it would not catch this difference. -- 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]
