Repository navigation
feat: let SpillCompression::Zstd carry a compression level - #26365
KonaeAkira wants to merge 3 commits into
Conversation
github-actions
Bot
added
documentation
…l-compression-zstd-level # Conflicts: # docs/source/library-user-guide/upgrading/56.0.0.md
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #26365 +/- ##
=======================================
Coverage 82.84% 82.84%
=======================================
Files 1151 1151
Lines 455510 455601 +91
Branches 455510 455601 +91
=======================================
+ Hits 377362 377440 +78
- Misses 55236 55243 +7
- Partials 22912 22918 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@KonaeAkira |
Brings in 363 upstream commits (through 4bf32cb). Conflicts resolved: - datafusion/common/src/config.rs: unioned the parquet_config import lists from both sides. - docs/source/library-user-guide/upgrading/56.0.0.md: both sides appended a section ending in a migration-guide code block, which interleaved. Kept upstream's sections in place and moved the SpillCompression::Zstd section, with its own migration block, to the end. - docs/source/user-guide/configs.md: regenerated via dev/update_config_docs.sh. Upstream also added spill tests that construct the now-tupled Zstd variant, so SpillCompression::Zstd became SpillCompression::Zstd(SpillCompression::DEFAULT_ZSTD_LEVEL) in datafusion/physical-plan/src/spill/lengths.rs (new upstream file, 3 sites) and in one rstest case in spill/mod.rs.
|
@kosiew Done. Sorry it's been a while :) |
There was a problem hiding this comment.
Thanks for adding configurable Zstd compression levels for spill files. This addresses a useful performance-tuning limitation while preserving the existing default behavior. The implementation is appropriately scoped, and the new tests demonstrate that non-default levels produce readable spill files.
The design keeps compression policy in the existing configuration model and delegates encoding to Arrow IPC. Carrying the level in SpillCompression is a reasonable extension, and reusing the Parquet parsing helpers avoids duplicating validation logic. The intentional Rust API change is documented with migration guidance.
I found no blocking issues. I've left two optional suggestions to strengthen boundary and decoded-value coverage.
| "zstd(3)", | ||
| ), | ||
| ("ZSTD(1)", SpillCompression::Zstd(1), "zstd(1)"), | ||
| ("zstd(22)", SpillCompression::Zstd(22), "zstd(22)"), |
There was a problem hiding this comment.
Consider adding ("zstd(0)", SpillCompression::Zstd(0), "zstd(0)") to the round-trip cases. Zero is the documented lower boundary and default-level alias, so this would verify that parsing and display preserve the explicit zero setting.
| "level {level}" | ||
| ); | ||
| validate(&spill_manager, file, num_rows, Arc::clone(&schema), 1).await?; | ||
| } |
There was a problem hiding this comment.
Consider collecting the decoded stream and comparing its batches with batches, while retaining the existing metrics and schema checks. The current validate helper checks row metrics, schema, and batch count but not decoded values, so this would strengthen round-trip coverage for levels 1 and 19.
Which issue does this PR close?
Rationale for this change
Allows for configuring the zstd compression level for spill files instead of always using arrow's default compression level (3). Possible now that arrow-rs exposes
IpcWriteOptions::try_with_compression_level.What changes are included in this PR?
SpillCompression::ZstdbecomesSpillCompression::Zstd(u32)withSpillCompression::DEFAULT_ZSTD_LEVEL = 3.datafusion.execution.spill_compressionacceptszstd(<level>)with a level in 0..=22, parsed and validated atSETtime. Plainzstdmeans level 3 to preserve current behavior.IpcWriteOptions::try_with_compression_level.What is the testing strategy for this PR?
datafusion/common/src/config.rs.datafusion/physical-plan/src/spill/mod.rsto assert that setting zstd compression level actually changes compression behavior.datafusion/sqllogictest/test_files/information_schema.slt.Are there any user-facing changes?
Yes, breaking change because
SpillCompressionis a public enum. Added an upgrade guide todocs/source/library-user-guide/upgrading/56.0.0.md.