You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "Smotrov (via GitHub)" <gi...@apache.org> on 2025/11/26 20:17:13 UTC
[PR] feat: add compression level configuration for JSON/CSV writers [datafusion]
Smotrov opened a new pull request, #18954:
URL: https://github.com/apache/datafusion/pull/18954
## Which issue does this PR close?
Closes #18947
## Rationale for this change
Currently, DataFusion uses default compression levels when writing compressed JSON and CSV files. For ZSTD, this means level 3, which prioritizes speed over compression ratio. Users working with large datasets who want to optimize for storage costs or network transfer have no way to increase the compression level.
This is particularly important for cloud data lake scenarios where storage and egress costs can be significant.
## What changes are included in this PR?
- Add `compression_level: Option<u32>` field to `JsonOptions` and `CsvOptions` in `config.rs`
- Add `convert_async_writer_with_level()` method to `FileCompressionType` (non-breaking API extension)
- Keep original `convert_async_writer()` as a convenience wrapper for backward compatibility
- Update `JsonWriterOptions` and `CsvWriterOptions` with `compression_level` field
- Update `ObjectWriterBuilder` to support compression level
- Update JSON and CSV sinks to pass compression level through the write pipeline
- Update proto definitions and conversions for serialization support
- Fix unrelated unused import warning in `udf.rs` (conditional compilation for debug-only imports)
## Are these changes tested?
The changes follow the existing patterns used throughout the codebase. The implementation was verified by:
- Building successfully with `cargo build`
- Running existing tests with `cargo test --package datafusion-proto`
- All 131 proto integration tests pass
## Are there any user-facing changes?
Yes, users can now specify compression level when writing JSON/CSV files:
```rust
use datafusion::common::config::JsonOptions;
use datafusion::common::parsers::CompressionTypeVariant;
let json_opts = JsonOptions {
compression: CompressionTypeVariant::ZSTD,
compression_level: Some(9), // Higher compression
..Default::default()
};
```
**Supported compression levels:**
- ZSTD: 1-22 (default: 3)
- GZIP: 0-9 (default: 6)
- BZIP2: 1-9 (default: 9)
- XZ: 0-9 (default: 6)
**This is a non-breaking change** - the original `convert_async_writer()` method signature is
## Are these changes testedatibility.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add compression level configuration for JSON/CSV writers [datafusion]
Posted by "Smotrov (via GitHub)" <gi...@apache.org>.
Smotrov commented on code in PR #18954:
URL: https://github.com/apache/datafusion/pull/18954#discussion_r2567626930
##########
datafusion/common/src/file_options/csv_writer.rs:
##########
@@ -41,6 +43,20 @@ impl CsvWriterOptions {
Self {
writer_options,
compression,
+ compression_level: None,
+ }
+ }
+
+ /// Create a new `CsvWriterOptions` with the specified compression level.
+ pub fn new_with_level(
+ writer_options: WriterBuilder,
+ compression: CompressionTypeVariant,
+ compression_level: Option<u32>,
Review Comment:
Looking at `flate2::Compression`, it uses `new(level: u32)` + `default()` rather than `Option`.
My rationale was config system integration. `CsvOptions.compression_level` is `Option<u32>` because users may or may not specify it in config. The `new_with_level(..., Option<u32>)` signature makes the `TryFrom<&CsvOptions>` impl straightforward.
But I agree the public API could be cleaner. I could:
1. Keep `new_with_level(..., compression_level: u32)` as you suggest (non-optional)
2. Let `TryFrom` internally call `new()` when `compression_level` is `None`, or `new_with_level()` when `Some`
Would you prefer that approach?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add compression level configuration for JSON/CSV writers [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #18954:
URL: https://github.com/apache/datafusion/pull/18954#discussion_r2628663395
##########
datafusion/common/src/config.rs:
##########
@@ -2933,6 +2950,14 @@ config_namespace! {
/// Options controlling JSON format
pub struct JsonOptions {
pub compression: CompressionTypeVariant, default = CompressionTypeVariant::UNCOMPRESSED
+ /// Compression level for the output file. The valid range depends on the
+ /// compression algorithm:
+ /// - ZSTD: 1 to 22 (default: 3)
+ /// - GZIP: 0 to 10 (default: varies by implementation)
+ /// - BZIP2: 0 to 9 (default: 6)
+ /// - XZ: 0 to 9 (default: 6)
+ /// If not specified, the default level for the compression algorithm is used.
+ pub compression_level: Option<u32>, default = None
Review Comment:
Maybe a good follow on PR to add
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add compression level configuration for JSON/CSV writers [datafusion]
Posted by "Smotrov (via GitHub)" <gi...@apache.org>.
Smotrov commented on code in PR #18954:
URL: https://github.com/apache/datafusion/pull/18954#discussion_r2586384722
##########
datafusion/common/src/config.rs:
##########
@@ -2786,6 +2787,14 @@ config_namespace! {
/// The default behaviour depends on the `datafusion.catalog.newlines_in_values` setting.
pub newlines_in_values: Option<bool>, default = None
pub compression: CompressionTypeVariant, default = CompressionTypeVariant::UNCOMPRESSED
+ /// Compression level for the output file. The valid range depends on the
+ /// compression algorithm:
+ /// - ZSTD: 1 to 22 (default: 3)
+ /// - GZIP: 0 to 10 (default: varies by implementation)
Review Comment:
Updated
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add compression level configuration for JSON/CSV writers [datafusion]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #18954:
URL: https://github.com/apache/datafusion/pull/18954#discussion_r2570017971
##########
datafusion/common/src/config.rs:
##########
@@ -2933,6 +2950,14 @@ config_namespace! {
/// Options controlling JSON format
pub struct JsonOptions {
pub compression: CompressionTypeVariant, default = CompressionTypeVariant::UNCOMPRESSED
+ /// Compression level for the output file. The valid range depends on the
+ /// compression algorithm:
+ /// - ZSTD: 1 to 22 (default: 3)
+ /// - GZIP: 0 to 10 (default: varies by implementation)
+ /// - BZIP2: 0 to 9 (default: 6)
+ /// - XZ: 0 to 9 (default: 6)
+ /// If not specified, the default level for the compression algorithm is used.
+ pub compression_level: Option<u32>, default = None
Review Comment:
I just realize that there is no `impl JsonOptions` with all `with_xyz(mut self, ...)` setters like the CsvOptions.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add compression level configuration for JSON/CSV writers [datafusion]
Posted by "Smotrov (via GitHub)" <gi...@apache.org>.
Smotrov commented on PR #18954:
URL: https://github.com/apache/datafusion/pull/18954#issuecomment-3583088763
Hi @andygrove, @Dandandan, @viirya!
This is my first contribution to DataFusion. Could a maintainer please approve the CI workflows? Thank you!
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add compression level configuration for JSON/CSV writers [datafusion]
Posted by "Smotrov (via GitHub)" <gi...@apache.org>.
Smotrov commented on code in PR #18954:
URL: https://github.com/apache/datafusion/pull/18954#discussion_r2567581119
##########
datafusion/common/src/config.rs:
##########
@@ -2786,6 +2787,14 @@ config_namespace! {
/// The default behaviour depends on the `datafusion.catalog.newlines_in_values` setting.
pub newlines_in_values: Option<bool>, default = None
pub compression: CompressionTypeVariant, default = CompressionTypeVariant::UNCOMPRESSED
+ /// Compression level for the output file. The valid range depends on the
+ /// compression algorithm:
+ /// - ZSTD: 1 to 22 (default: 3)
+ /// - GZIP: 0 to 10 (default: varies by implementation)
Review Comment:
Good catch @Jefffrey ! You're right that "varies by implementation" is vague. Let me clarify:
The GZIP compression in async-compression uses flate2 under the hood. Looking at the flate2 source code, the default is level 6:
```Rust
// From https://github.com/rust-lang/flate2-rs/blob/main/src/lib.rs#L220-L224
impl Default for Compression {
fn default() -> Compression {
Compression(6)
}
}
```
This is the standard zlib/gzip default (going back to the original zlib implementation). The valid range is 0-9 (not 0-10 as I incorrectly wrote).
I'll update the comment to be more precise:
```Rust
/// - GZIP: 0 to 9 (default: 6)
```
The reason I was initially cautious is that some compression **libraries allow you to swap backends** (e.g., flate2 can use miniz_oxide, zlib-rs, or native zlib), but they all follow the same 0-9 range and default to 6 for compatibility.
Would you like me to also update the code to fix the comment and rerun CI workflows?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add compression level configuration for JSON/CSV writers [datafusion]
Posted by "Smotrov (via GitHub)" <gi...@apache.org>.
Smotrov commented on code in PR #18954:
URL: https://github.com/apache/datafusion/pull/18954#discussion_r2566439318
##########
datafusion/common/src/config.rs:
##########
@@ -2786,6 +2787,14 @@ config_namespace! {
/// The default behaviour depends on the `datafusion.catalog.newlines_in_values` setting.
pub newlines_in_values: Option<bool>, default = None
pub compression: CompressionTypeVariant, default = CompressionTypeVariant::UNCOMPRESSED
+ /// Compression level for the output file. The valid range depends on the
+ /// compression algorithm:
+ /// - ZSTD: 1 to 22 (default: 3)
Review Comment:
You [right](https://facebook.github.io/zstd/zstd_manual.html) :-)
But than it will be not compressed which is not really useful in this context.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add compression level configuration for JSON/CSV writers [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #18954:
URL: https://github.com/apache/datafusion/pull/18954
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add compression level configuration for JSON/CSV writers [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #18954:
URL: https://github.com/apache/datafusion/pull/18954#discussion_r2567110618
##########
datafusion/common/src/file_options/csv_writer.rs:
##########
@@ -41,6 +43,20 @@ impl CsvWriterOptions {
Self {
writer_options,
compression,
+ compression_level: None,
+ }
+ }
+
+ /// Create a new `CsvWriterOptions` with the specified compression level.
+ pub fn new_with_level(
+ writer_options: WriterBuilder,
+ compression: CompressionTypeVariant,
+ compression_level: Option<u32>,
Review Comment:
Is it better to just have `compression_level: u32` and direct users to use `new` if they want default (`None`) compression level? Thoughts? 🤔
##########
datafusion/common/src/config.rs:
##########
@@ -2786,6 +2787,14 @@ config_namespace! {
/// The default behaviour depends on the `datafusion.catalog.newlines_in_values` setting.
pub newlines_in_values: Option<bool>, default = None
pub compression: CompressionTypeVariant, default = CompressionTypeVariant::UNCOMPRESSED
+ /// Compression level for the output file. The valid range depends on the
+ /// compression algorithm:
+ /// - ZSTD: 1 to 22 (default: 3)
+ /// - GZIP: 0 to 10 (default: varies by implementation)
Review Comment:
What does `varies by implementation` mean here? Depends on system library, depends on rust crate dependency (in which case ideally we'd know which it is)?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add compression level configuration for JSON/CSV writers [datafusion]
Posted by "Smotrov (via GitHub)" <gi...@apache.org>.
Smotrov commented on code in PR #18954:
URL: https://github.com/apache/datafusion/pull/18954#discussion_r2586273603
##########
datafusion/common/src/file_options/csv_writer.rs:
##########
@@ -41,6 +43,20 @@ impl CsvWriterOptions {
Self {
writer_options,
compression,
+ compression_level: None,
+ }
+ }
+
+ /// Create a new `CsvWriterOptions` with the specified compression level.
+ pub fn new_with_level(
+ writer_options: WriterBuilder,
+ compression: CompressionTypeVariant,
+ compression_level: Option<u32>,
Review Comment:
I'm not an LLM :-)) at least so far.
Actually I was refereeing `datafusion/datafusion/common/src/file_options/csv_writer.rs` which returns `CsvWriterOptions` with optional compression level.
But you are correct. It constructs directly not with `new_with()` or `new()`. Looked at the wrong place, sorry for misleading comment.
But coming back to the original question: do you prefer `compression_level: u32` or `Option<u32>`?
Perhaps if someone is calling `new_with_level()` there is not so much sense to pass `None` as a compression level.
How do you think?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add compression level configuration for JSON/CSV writers [datafusion]
Posted by "Smotrov (via GitHub)" <gi...@apache.org>.
Smotrov commented on PR #18954:
URL: https://github.com/apache/datafusion/pull/18954#issuecomment-3608558000
> I think we need a roundtrip test for proto (enhancing an existing one if possible) and possibly and end-to-end test to show this new config in use when writing a file
Added. Please check if this is what you were asking for or should be tuned somehow.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add compression level configuration for JSON/CSV writers [datafusion]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #18954:
URL: https://github.com/apache/datafusion/pull/18954#discussion_r2566415414
##########
datafusion/common/src/config.rs:
##########
@@ -2786,6 +2787,14 @@ config_namespace! {
/// The default behaviour depends on the `datafusion.catalog.newlines_in_values` setting.
pub newlines_in_values: Option<bool>, default = None
pub compression: CompressionTypeVariant, default = CompressionTypeVariant::UNCOMPRESSED
+ /// Compression level for the output file. The valid range depends on the
+ /// compression algorithm:
+ /// - ZSTD: 1 to 22 (default: 3)
Review Comment:
I remember zstd compression level could be negative, no?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add compression level configuration for JSON/CSV writers [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #18954:
URL: https://github.com/apache/datafusion/pull/18954#issuecomment-3667503421
Thanks again @Smotrov and @Jefffrey
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add compression level configuration for JSON/CSV writers [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #18954:
URL: https://github.com/apache/datafusion/pull/18954#discussion_r2571050758
##########
datafusion/common/src/config.rs:
##########
@@ -2786,6 +2787,14 @@ config_namespace! {
/// The default behaviour depends on the `datafusion.catalog.newlines_in_values` setting.
pub newlines_in_values: Option<bool>, default = None
pub compression: CompressionTypeVariant, default = CompressionTypeVariant::UNCOMPRESSED
+ /// Compression level for the output file. The valid range depends on the
+ /// compression algorithm:
+ /// - ZSTD: 1 to 22 (default: 3)
+ /// - GZIP: 0 to 10 (default: varies by implementation)
Review Comment:
Yes we should specify the default if known; having it left as `implementation specific` is very confusing to users
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add compression level configuration for JSON/CSV writers [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on code in PR #18954:
URL: https://github.com/apache/datafusion/pull/18954#discussion_r2571074369
##########
datafusion/common/src/file_options/csv_writer.rs:
##########
@@ -41,6 +43,20 @@ impl CsvWriterOptions {
Self {
writer_options,
compression,
+ compression_level: None,
+ }
+ }
+
+ /// Create a new `CsvWriterOptions` with the specified compression level.
+ pub fn new_with_level(
+ writer_options: WriterBuilder,
+ compression: CompressionTypeVariant,
+ compression_level: Option<u32>,
Review Comment:
Is this response LLM generated? It doesn't make sense with regards to the codebase; `TryFrom<&CsvOptions>` doesn't use `new()` or `new_with_level()`
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add compression level configuration for JSON/CSV writers [datafusion]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on PR #18954:
URL: https://github.com/apache/datafusion/pull/18954#issuecomment-3583228453
> A tiny `fmt` update. @viirya would appreciate your CI workflows approval.
I wanted to do it but I think @andygrove triggered it before I did. 🙂
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add compression level configuration for JSON/CSV writers [datafusion]
Posted by "Smotrov (via GitHub)" <gi...@apache.org>.
Smotrov commented on PR #18954:
URL: https://github.com/apache/datafusion/pull/18954#issuecomment-3583200993
A tiny `fmt` update.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add compression level configuration for JSON/CSV writers [datafusion]
Posted by "2010YOUY01 (via GitHub)" <gi...@apache.org>.
2010YOUY01 commented on code in PR #18954:
URL: https://github.com/apache/datafusion/pull/18954#discussion_r2571230489
##########
datafusion/common/src/config.rs:
##########
@@ -2786,6 +2787,14 @@ config_namespace! {
/// The default behaviour depends on the `datafusion.catalog.newlines_in_values` setting.
pub newlines_in_values: Option<bool>, default = None
pub compression: CompressionTypeVariant, default = CompressionTypeVariant::UNCOMPRESSED
+ /// Compression level for the output file. The valid range depends on the
+ /// compression algorithm:
+ /// - ZSTD: 1 to 22 (default: 3)
+ /// - GZIP: 0 to 10 (default: varies by implementation)
+ /// - BZIP2: 0 to 9 (default: 6)
+ /// - XZ: 0 to 9 (default: 6)
+ /// If not specified, the default level for the compression algorithm is used.
+ pub compression_level: Option<u32>, default = None
Review Comment:
Should we include level inside compression type `CompressionTypeVariant`, like
```
pub enum CompressionTypeVariant {
/// Gzip-ed file, level 1–9
Gzip { level: u32 },
....
```
This introduces some API changes, but I think it's cleaner and better for the long term 🤔
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add compression level configuration for JSON/CSV writers [datafusion]
Posted by "Smotrov (via GitHub)" <gi...@apache.org>.
Smotrov commented on code in PR #18954:
URL: https://github.com/apache/datafusion/pull/18954#discussion_r2586273603
##########
datafusion/common/src/file_options/csv_writer.rs:
##########
@@ -41,6 +43,20 @@ impl CsvWriterOptions {
Self {
writer_options,
compression,
+ compression_level: None,
+ }
+ }
+
+ /// Create a new `CsvWriterOptions` with the specified compression level.
+ pub fn new_with_level(
+ writer_options: WriterBuilder,
+ compression: CompressionTypeVariant,
+ compression_level: Option<u32>,
Review Comment:
I'm not an LLM :-)) at least so far.
Actually I was refereeing datafusion/datafusion/common/src/file_options/csv_writer.rs which returns `CsvWriterOptions` with optional compression level.
But you are correct. It constructs directly not with `new_with()` or 'new()'. Looked at the wrong place, sorry for misleading comment.
But coming back to the original question: do you prefer `compression_level: u32` or `Option<u32>`?
Perhaps if someone is calling `new_with_level()` there is not so much sense to pass `None` as a compression level.
How do you think?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add compression level configuration for JSON/CSV writers [datafusion]
Posted by "Smotrov (via GitHub)" <gi...@apache.org>.
Smotrov commented on code in PR #18954:
URL: https://github.com/apache/datafusion/pull/18954#discussion_r2586295957
##########
datafusion/common/src/config.rs:
##########
@@ -2786,6 +2787,14 @@ config_namespace! {
/// The default behaviour depends on the `datafusion.catalog.newlines_in_values` setting.
pub newlines_in_values: Option<bool>, default = None
pub compression: CompressionTypeVariant, default = CompressionTypeVariant::UNCOMPRESSED
+ /// Compression level for the output file. The valid range depends on the
+ /// compression algorithm:
+ /// - ZSTD: 1 to 22 (default: 3)
+ /// - GZIP: 0 to 10 (default: varies by implementation)
+ /// - BZIP2: 0 to 9 (default: 6)
+ /// - XZ: 0 to 9 (default: 6)
+ /// If not specified, the default level for the compression algorithm is used.
+ pub compression_level: Option<u32>, default = None
Review Comment:
Perhaps in some other PR. :-)
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add compression level configuration for JSON/CSV writers [datafusion]
Posted by "Smotrov (via GitHub)" <gi...@apache.org>.
Smotrov commented on code in PR #18954:
URL: https://github.com/apache/datafusion/pull/18954#discussion_r2586380643
##########
datafusion/common/src/file_options/csv_writer.rs:
##########
@@ -41,6 +43,20 @@ impl CsvWriterOptions {
Self {
writer_options,
compression,
+ compression_level: None,
+ }
+ }
+
+ /// Create a new `CsvWriterOptions` with the specified compression level.
+ pub fn new_with_level(
+ writer_options: WriterBuilder,
+ compression: CompressionTypeVariant,
+ compression_level: Option<u32>,
Review Comment:
Switched to `compression_level: u32`
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org