bkietz commented on a change in pull request #11810:
URL: https://github.com/apache/arrow/pull/11810#discussion_r763129850
##########
File path: cpp/src/arrow/util/compression_lz4.cc
##########
@@ -313,6 +335,13 @@ class Lz4FrameCodec : public Codec {
class Lz4Codec : public Codec {
public:
+ Lz4Codec() : compression_level_(kUseDefaultCompressionLevel) {}
Review comment:
> I'm not able to find kDefaultCompressionLevel, also not in the code
for other compressors.
First of all, sorry for the confusion: Instead of `kDefaultCompressionLevel`
I should have written `kLz4DefaultCompressionLevel`. My intent was to indicate
that all of these should result in an equivalent codec:
1. `Lz4Codec()`
2. `Lz4Codec(kUseDefaultCompressionLevel)`
3. `Lz4Codec(kLz4DefaultCompressionLevel)`
However as you have it written `1` results in an Lz4Codec with
`compression_level_ == kUseDefaultCompressionLevel` whereas `2,3` result in
`compression_level_ == kLz4DefaultCompressionLevel`.
---
My stylistic preference would be to delete the default constructor of
`Lz4Codec` and write `Lz4HadoopCodec`'s constructor to explicitly pass
`kUseDefaultCompressionLevel` to the base class' constructor like so:
```c++
Lz4HadoopCodec() : Lz4Codec(kUseDefaultCompressionLevel) {}
```
--
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]
