You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "andygrove (via GitHub)" <gi...@apache.org> on 2026/07/14 20:58:32 UTC
[PR] feat: support gzip compression in native Parquet writes [datafusion-comet]
andygrove opened a new pull request, #4930:
URL: https://github.com/apache/datafusion-comet/pull/4930
## Which issue does this PR close?
Closes #4929. Part of #1625.
## Rationale for this change
Comet's native Parquet writer supported `none`, `snappy`, `lz4`, and `zstd`, so a write configured with `spark.sql.parquet.compression.codec=gzip` fell back to Spark's writer. gzip is a valid Spark Parquet codec and is widely used, so this was an avoidable loss of native write coverage.
Reviewing that code path surfaced two related gaps, both fixed here:
- `parseCompressionCodec` did not follow Spark's codec precedence. Spark's `ParquetOptions` resolves `compression` > `parquet.compression` (`ParquetOutputFormat.COMPRESSION`) > `spark.sql.parquet.compression.codec`, but Comet checked only the first and third. `df.write.option("parquet.compression", "gzip")` with the default SQLConf was therefore written natively as snappy, silently substituting the user's requested codec.
- Spark accepts `uncompressed` as a spelling of `none`, but the supported-codec set listed only `none`, so `uncompressed` fell back to Spark's writer unnecessarily.
`native/core` also requested the `parquet` crate with `default-features = false` while naming only `experimental` and `arrow`. The codec features (`snap`, `lz4`, `zstd`, and gzip's `flate2`) were enabled only incidentally, through Cargo feature unification with other crates that pull `parquet` with default features. A dependency bump that changed that would have left the writer compiling fine but failing at execution time with "Disabled feature at compile time", so the features are now requested explicitly.
## What changes are included in this PR?
- Add `Gzip = 4` to the protobuf `CompressionCodec` enum.
- Give the native Parquet writer its own `ParquetCompression` enum instead of borrowing the shuffle crate's `CompressionCodec`. gzip is a Parquet codec, not a shuffle codec, and the shuffle path has no business carrying it. The shuffle branch of the planner is unchanged and still rejects any codec it does not map. gzip maps to `Compression::GZIP(GzipLevel::default())`, i.e. level 6, matching parquet-mr's zlib default.
- Request the `parquet` codec features explicitly in `native/core/Cargo.toml`.
- Emit the gzip codec from `CometDataWritingCommand`, honor Spark's `parquet.compression` precedence, and accept `uncompressed` as an alias of `none`.
Compression levels remain non-configurable (gzip and zstd use fixed defaults), so parquet-mr's `parquet.compression.codec.{gzip,zstd}.level` configs are still ignored. That is out of scope here and noted in the issue.
## How are these changes tested?
`CometParquetWriterSuite` had no compression coverage at all, so this adds it:
- A codec matrix test over `none`, `uncompressed`, `snappy`, `lz4`, `zstd`, and `gzip`. For each, it asserts `CometNativeWriteExec` is in the executed plan (so a silent fallback to Spark's writer cannot make the test pass vacuously), reads the data back and compares against the source, and opens the Parquet footer to assert every column chunk reports the expected codec (so a write that ignored the requested codec cannot pass either).
- A test that `.option("parquet.compression", "gzip")` beats a `snappy` SQLConf default and produces a natively written GZIP file.
- A fallback test using `lz4_raw`, a codec Spark writes but Comet does not support: the plan must contain no `CometNativeWriteExec`, and the data must still round-trip. This catches a codec being added to the supported set without a matching serde case.
A Rust unit test covers the `ParquetCompression` to `parquet::basic::Compression` mapping for all five variants.
This work was scaffolded with the brainstorming, writing-plans, and subagent-driven-development skills.
--
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: support gzip compression in native Parquet writes [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #4930:
URL: https://github.com/apache/datafusion-comet/pull/4930#discussion_r3591708350
##########
native/core/src/execution/operators/parquet_writer.rs:
##########
@@ -52,11 +51,37 @@ use datafusion::{
use futures::TryStreamExt;
use parquet::{
arrow::ArrowWriter,
- basic::{Compression, ZstdLevel},
+ basic::{Compression, GzipLevel, ZstdLevel},
file::properties::WriterProperties,
};
use url::Url;
+/// Compression codecs supported by the native Parquet writer.
+///
+/// This is deliberately separate from the shuffle `CompressionCodec`: gzip is a valid Parquet
+/// codec but is not a codec we compress shuffle blocks with.
+#[derive(Debug, Clone, PartialEq)]
+pub enum ParquetCompression {
+ None,
+ Snappy,
+ Lz4,
+ Zstd(i32),
+ Gzip,
+}
+
+impl ParquetCompression {
Review Comment:
in this case we prob can just add GZIP variant into `CompressionCodec` and reuse, however if we follow Apache Spark there are 2 enums indeed, for parquet and internal compression like shuffle.
--
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: support gzip compression in native Parquet writes [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #4930:
URL: https://github.com/apache/datafusion-comet/pull/4930#discussion_r3599422422
##########
spark/src/test/scala/org/apache/comet/parquet/CometParquetWriterSuite.scala:
##########
@@ -138,6 +142,70 @@ class CometParquetWriterSuite extends CometTestBase {
}
}
+ test("parquet write with each supported compression codec") {
Review Comment:
I added this test
--
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: support gzip compression in native Parquet writes [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #4930:
URL: https://github.com/apache/datafusion-comet/pull/4930#issuecomment-4983844815
Spark 3.4 failure:
```
- parquet write with unsupported compression codec falls back to Spark *** FAILED *** (33 milliseconds)
java.lang.IllegalArgumentException: The value of spark.sql.parquet.compression.codec should be one of brotli, uncompressed, lz4, gzip, lzo, snappy, none, zstd, but was lz4_raw
at org.apache.spark.internal.config.TypedConfigBuilder.$anonfun$checkValues$1(ConfigBuilder.scala:119)
```
--
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: support gzip compression in native Parquet writes [datafusion-comet]
Posted by "parthchandra (via GitHub)" <gi...@apache.org>.
parthchandra commented on code in PR #4930:
URL: https://github.com/apache/datafusion-comet/pull/4930#discussion_r3583931993
##########
spark/src/test/scala/org/apache/comet/parquet/CometParquetWriterSuite.scala:
##########
@@ -138,6 +142,70 @@ class CometParquetWriterSuite extends CometTestBase {
}
}
+ test("parquet write with each supported compression codec") {
Review Comment:
Not a requirement but do you think it might be useful to add a test to cover codec precedence? I.e. specify zstd in the conf, but then override it to gzip
--
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: support gzip compression in native Parquet writes [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #4930:
URL: https://github.com/apache/datafusion-comet/pull/4930#discussion_r3599081724
##########
native/core/src/execution/operators/parquet_writer.rs:
##########
@@ -52,11 +51,37 @@ use datafusion::{
use futures::TryStreamExt;
use parquet::{
arrow::ArrowWriter,
- basic::{Compression, ZstdLevel},
+ basic::{Compression, GzipLevel, ZstdLevel},
file::properties::WriterProperties,
};
use url::Url;
+/// Compression codecs supported by the native Parquet writer.
+///
+/// This is deliberately separate from the shuffle `CompressionCodec`: gzip is a valid Parquet
+/// codec but is not a codec we compress shuffle blocks with.
+#[derive(Debug, Clone, PartialEq)]
+pub enum ParquetCompression {
+ None,
+ Snappy,
+ Lz4,
+ Zstd(i32),
+ Gzip,
+}
+
+impl ParquetCompression {
Review Comment:
I think it would be better to maintain separate enums for shuffle vs parquet writes, since they support different codecs.
--
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: support gzip compression in native Parquet writes [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #4930:
URL: https://github.com/apache/datafusion-comet/pull/4930#discussion_r3590537571
##########
native/core/src/execution/operators/parquet_writer.rs:
##########
@@ -52,11 +51,37 @@ use datafusion::{
use futures::TryStreamExt;
use parquet::{
arrow::ArrowWriter,
- basic::{Compression, ZstdLevel},
+ basic::{Compression, GzipLevel, ZstdLevel},
file::properties::WriterProperties,
};
use url::Url;
+/// Compression codecs supported by the native Parquet writer.
+///
+/// This is deliberately separate from the shuffle `CompressionCodec`: gzip is a valid Parquet
+/// codec but is not a codec we compress shuffle blocks with.
+#[derive(Debug, Clone, PartialEq)]
+pub enum ParquetCompression {
+ None,
+ Snappy,
+ Lz4,
+ Zstd(i32),
+ Gzip,
+}
+
+impl ParquetCompression {
Review Comment:
do we need this enum, cant we reuse `Compression` ?
--
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: support gzip compression in native Parquet writes [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #4930:
URL: https://github.com/apache/datafusion-comet/pull/4930#discussion_r3590591431
##########
native/core/src/execution/operators/parquet_writer.rs:
##########
@@ -52,11 +51,37 @@ use datafusion::{
use futures::TryStreamExt;
use parquet::{
arrow::ArrowWriter,
- basic::{Compression, ZstdLevel},
+ basic::{Compression, GzipLevel, ZstdLevel},
file::properties::WriterProperties,
};
use url::Url;
+/// Compression codecs supported by the native Parquet writer.
+///
+/// This is deliberately separate from the shuffle `CompressionCodec`: gzip is a valid Parquet
+/// codec but is not a codec we compress shuffle blocks with.
+#[derive(Debug, Clone, PartialEq)]
+pub enum ParquetCompression {
+ None,
+ Snappy,
+ Lz4,
+ Zstd(i32),
+ Gzip,
+}
+
+impl ParquetCompression {
Review Comment:
From the PR description:
"Give the native Parquet writer its own ParquetCompression enum instead of borrowing the shuffle crate's CompressionCodec. gzip is a Parquet codec, not a shuffle codec, and the shuffle path has no business carrying it."
--
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: support gzip compression in native Parquet writes [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove merged PR #4930:
URL: https://github.com/apache/datafusion-comet/pull/4930
--
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