NoahKusaba commented on code in PR #2805:
URL:
https://github.com/apache/datafusion-ballista/pull/2805#discussion_r4504580812
##########
ballista/core/src/config.rs:
##########
@@ -560,11 +560,12 @@ static CONFIG_ENTRIES: LazyLock<HashMap<String,
ConfigEntry>> = LazyLock::new(||
.with_doc_default("(empty)"),
ConfigEntry::new(
BALLISTA_SHUFFLE_COMPRESSION_CODEC.to_string(),
- "Compression codec specification \
- used in the shuffle process. Possible values: \
- none, lz4, zstd. Defaults to lz4 to preserve current
behaviour".to_string(),
+ "Compression codec used for shuffle data written to disk and \
+ sent between executors. Possible values: none, lz4, zstd. \
+ zstd writes shuffle files about half the size of lz4 for little \
+ extra CPU, which matters most when shuffles cross the
network".to_string(),
Review Comment:
Comet's shuffle defaults to
[lz4](https://github.com/apache/datafusion-comet/blob/4de6704391c9a7832a2079fc4e8d80fc09e9b209/spark/src/main/scala/org/apache/comet/CometConf.scala#L980-L990).
Was the ~14.8 bytes/row figure Comet with lz4? If so, this compares codecs
rather than systems; Comet with zstd would be like-for-like.
##########
ballista/core/src/config.rs:
##########
@@ -560,11 +560,12 @@ static CONFIG_ENTRIES: LazyLock<HashMap<String,
ConfigEntry>> = LazyLock::new(||
.with_doc_default("(empty)"),
ConfigEntry::new(
BALLISTA_SHUFFLE_COMPRESSION_CODEC.to_string(),
- "Compression codec specification \
- used in the shuffle process. Possible values: \
- none, lz4, zstd. Defaults to lz4 to preserve current
behaviour".to_string(),
+ "Compression codec used for shuffle data written to disk and \
+ sent between executors. Possible values: none, lz4, zstd. \
+ zstd writes shuffle files about half the size of lz4 for little \
+ extra CPU, which matters most when shuffles cross the
network".to_string(),
DataType::Utf8,
- Some("lz4".to_string()),
+ Some("zstd".to_string()),
Review Comment:
This gets arrow's default zstd level,
[3](https://github.com/apache/arrow-rs/blob/59.3.2/arrow-ipc/src/compression.rs#L25).
Comet uses [level
1](https://github.com/apache/datafusion-comet/blob/4de6704391c9a7832a2079fc4e8d80fc09e9b209/spark/src/main/scala/org/apache/comet/CometConf.scala#L992-L998)
for shuffle. Could we set the level explicitly (`try_with_compression_level`)
and compare 1 vs 3?
--
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]
