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: [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]
