zeroshade commented on code in PR #1376:
URL: https://github.com/apache/arrow-go/pull/1376#discussion_r4271421807
##########
arrow/ipc/writer.go:
##########
@@ -392,6 +397,7 @@ func newRecordEncoder(
maxDepth int64,
allow64b bool,
codec flatbuf.CompressionType,
+ zstdLevel int,
Review Comment:
Nit: `newRecordEncoder` now has nine positional parameters with `codec`,
`zstdLevel`, and `compressNP` adjacent, which is easy to transpose (every test
call site had to be edited). Consider bundling the compression settings into
one value.
##########
arrow/ipc/ipc.go:
##########
@@ -165,6 +166,21 @@ func WithZstd() Option {
}
}
+// WithZstdLevel tells the writer to use ZSTD compression at the given
Review Comment:
`zstd.EncoderLevelFromZstd` maps levels to only four encoder speeds (`<3` →
`SpeedFastest`, `3–5` → `SpeedDefault`, `6–9` → `SpeedBetterCompression`, `≥10`
→ `SpeedBestCompression`), and `level <= 0` falls back to the default. Please
document that mapping here instead of promising the full 1–22 Zstandard range.
##########
arrow/ipc/ipc.go:
##########
@@ -165,6 +166,21 @@ func WithZstd() Option {
}
}
+// WithZstdLevel tells the writer to use ZSTD compression at the given
+// compression level (1-22, as defined by the Zstandard specification).
+// Level 1 is the fastest but produces the least compression, while higher
+// levels produce smaller output at the cost of slower compression.
+// The default level (when WithZstd is used without WithZstdLevel) is 3,
+// which balances speed and compression ratio.
+//
+// This option only has an effect when ZSTD compression is enabled
+// (via WithZstd) and is ignored otherwise.
+func WithZstdLevel(level int) Option {
Review Comment:
This is silently ignored unless `WithZstd()` is also passed. Since a level
only makes sense for ZSTD, consider having `WithZstdLevel` also set `cfg.codec
= flatbuf.CompressionTypeZSTD`.
--
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]
