You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@avro.apache.org by "wgtmac (via GitHub)" <gi...@apache.org> on 2025/12/31 06:00:18 UTC
[PR] AVRO-4222: [C++] Support writer to specify compression level [avro]
wgtmac opened a new pull request, #3610:
URL: https://github.com/apache/avro/pull/3610
## What is the purpose of the change
- Add support to specify compression level to the C++ writer.
- Make all codec enums visible and add runtime availability check.
## Verifying this change
- Extended test/DataFileTests.cc to test compression levels.
## Documentation
- Does this pull request introduce a new feature? yes
- If yes, how is the feature documented? not applicable
--
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: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4222: [C++] Support writer to specify compression level [avro]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #3610:
URL: https://github.com/apache/avro/pull/3610#discussion_r2657597028
##########
lang/c++/impl/DataFile.cc:
##########
@@ -47,53 +47,183 @@ using std::array;
namespace {
const string AVRO_SCHEMA_KEY("avro.schema");
const string AVRO_CODEC_KEY("avro.codec");
-const string AVRO_NULL_CODEC("null");
-const string AVRO_DEFLATE_CODEC("deflate");
+const size_t minSyncInterval = 32;
Review Comment:
I think git is mistakenly regard this line as a new change. IIUC, this is number of bytes to check for a new sync marker.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4222: [C++] Support writer to specify compression level [avro]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #3610:
URL: https://github.com/apache/avro/pull/3610#discussion_r2657570752
##########
lang/c++/impl/DataFile.cc:
##########
@@ -64,36 +64,105 @@ const size_t maxSyncInterval = 1u << 30;
// Recommended by https://www.zlib.net/zlib_how.html
const size_t zlibBufGrowSize = 128 * 1024;
+std::string codecToString(Codec codec) {
+ switch (codec) {
+ case NULL_CODEC: return "null";
+ case DEFLATE_CODEC: return "deflate";
+ case SNAPPY_CODEC: return "snappy";
+ case ZSTD_CODEC: return "zstandard";
+ default: return "unknown";
+ }
+}
+
+void validateCodec(Codec codec, std::optional<int> level) {
+ if (!isCodecAvailable(codec)) {
+ throw Exception("Codec {} is not available.", codecToString(codec));
+ }
+
+ if (!level.has_value()) {
+ return;
+ }
+
+ int levelValue = level.value();
+ switch (codec) {
+ case NULL_CODEC:
+ case SNAPPY_CODEC:
+ // These codecs don't support compression levels, ignore
+ break;
+ case DEFLATE_CODEC:
+ if (levelValue < 0 || levelValue > 9) {
+ throw Exception("Invalid compression level {} for deflate codec. "
+ "Valid range is 0-9.",
+ levelValue);
+ }
+ break;
+ case ZSTD_CODEC:
+ if (levelValue < 1 || levelValue > 22) {
+ throw Exception("Invalid compression level {} for zstandard codec. "
+ "Valid range is 1-22.",
+ levelValue);
+ }
+ break;
+ default:
+ throw Exception("Unknown codec: {}", static_cast<int>(codec));
+ }
+}
+
} // namespace
+bool isCodecAvailable(Codec codec) {
+ switch (codec) {
+ case NULL_CODEC:
+ case DEFLATE_CODEC:
+ return true;
+ case SNAPPY_CODEC:
+#ifdef SNAPPY_CODEC_AVAILABLE
+ return true;
+#else
+ return false;
+#endif
+ case ZSTD_CODEC:
+#ifdef ZSTD_CODEC_AVAILABLE
+ return true;
+#else
+ return false;
+#endif
+ default:
+ return false;
+ }
+}
+
DataFileWriterBase::DataFileWriterBase(const char *filename, const ValidSchema &schema, size_t syncInterval,
- Codec codec, const Metadata &metadata) : filename_(filename),
- schema_(schema),
- encoderPtr_(binaryEncoder()),
- syncInterval_(syncInterval),
- codec_(codec),
- stream_(fileOutputStream(filename)),
- buffer_(memoryOutputStream()),
- sync_(makeSync()),
- objectCount_(0),
- metadata_(metadata),
- lastSync_(0) {
+ Codec codec, const Metadata &metadata,
+ std::optional<int> compressionLevel) : filename_(filename),
+ schema_(schema),
+ encoderPtr_(binaryEncoder()),
+ syncInterval_(syncInterval),
+ codec_(codec),
+ compressionLevel_(compressionLevel),
+ stream_(fileOutputStream(filename)),
Review Comment:
I agree that this is an issue. Another constructor `DataFileWriter(std::unique_ptr<OutputStream> outputStream, ...)` also have the same issue. However, this is a separate issue and fixing it may complicate this PR.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4222: [C++] Support writer to specify compression level [avro]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #3610:
URL: https://github.com/apache/avro/pull/3610#discussion_r2657518361
##########
lang/c++/impl/DataFile.cc:
##########
Review Comment:
Same as above, DataFile.hh has included it.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4222: [C++] Support writer to specify compression level [avro]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #3610:
URL: https://github.com/apache/avro/pull/3610#discussion_r2657517466
##########
lang/c++/impl/ZstdCompressWrapper.cc:
##########
Review Comment:
I think this is duplicated because ZstdCompressWrapper.hh has it.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4222: [C++] Support writer to specify compression level [avro]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on code in PR #3610:
URL: https://github.com/apache/avro/pull/3610#discussion_r2657597028
##########
lang/c++/impl/DataFile.cc:
##########
@@ -47,53 +47,183 @@ using std::array;
namespace {
const string AVRO_SCHEMA_KEY("avro.schema");
const string AVRO_CODEC_KEY("avro.codec");
-const string AVRO_NULL_CODEC("null");
-const string AVRO_DEFLATE_CODEC("deflate");
+const size_t minSyncInterval = 32;
Review Comment:
I think git is mistakenly regarding this line as a new change. IIUC, this is number of bytes to check for a new sync marker.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4222: [C++] Support writer to specify compression level [avro]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #3610:
URL: https://github.com/apache/avro/pull/3610#discussion_r2657057649
##########
lang/c++/impl/DataFile.cc:
##########
Review Comment:
```suggestion
#include <optional>
#include <random>
```
##########
lang/c++/impl/DataFile.cc:
##########
@@ -64,36 +64,105 @@ const size_t maxSyncInterval = 1u << 30;
// Recommended by https://www.zlib.net/zlib_how.html
const size_t zlibBufGrowSize = 128 * 1024;
+std::string codecToString(Codec codec) {
+ switch (codec) {
+ case NULL_CODEC: return "null";
+ case DEFLATE_CODEC: return "deflate";
+ case SNAPPY_CODEC: return "snappy";
+ case ZSTD_CODEC: return "zstandard";
+ default: return "unknown";
+ }
+}
+
+void validateCodec(Codec codec, std::optional<int> level) {
+ if (!isCodecAvailable(codec)) {
+ throw Exception("Codec {} is not available.", codecToString(codec));
+ }
+
+ if (!level.has_value()) {
+ return;
+ }
+
+ int levelValue = level.value();
+ switch (codec) {
+ case NULL_CODEC:
+ case SNAPPY_CODEC:
+ // These codecs don't support compression levels, ignore
+ break;
+ case DEFLATE_CODEC:
+ if (levelValue < 0 || levelValue > 9) {
+ throw Exception("Invalid compression level {} for deflate codec. "
+ "Valid range is 0-9.",
+ levelValue);
+ }
+ break;
+ case ZSTD_CODEC:
+ if (levelValue < 1 || levelValue > 22) {
+ throw Exception("Invalid compression level {} for zstandard codec. "
+ "Valid range is 1-22.",
+ levelValue);
+ }
+ break;
+ default:
+ throw Exception("Unknown codec: {}", static_cast<int>(codec));
+ }
+}
+
} // namespace
+bool isCodecAvailable(Codec codec) {
+ switch (codec) {
+ case NULL_CODEC:
+ case DEFLATE_CODEC:
+ return true;
+ case SNAPPY_CODEC:
+#ifdef SNAPPY_CODEC_AVAILABLE
+ return true;
+#else
+ return false;
+#endif
+ case ZSTD_CODEC:
+#ifdef ZSTD_CODEC_AVAILABLE
+ return true;
+#else
+ return false;
+#endif
+ default:
+ return false;
+ }
+}
+
DataFileWriterBase::DataFileWriterBase(const char *filename, const ValidSchema &schema, size_t syncInterval,
- Codec codec, const Metadata &metadata) : filename_(filename),
- schema_(schema),
- encoderPtr_(binaryEncoder()),
- syncInterval_(syncInterval),
- codec_(codec),
- stream_(fileOutputStream(filename)),
- buffer_(memoryOutputStream()),
- sync_(makeSync()),
- objectCount_(0),
- metadata_(metadata),
- lastSync_(0) {
+ Codec codec, const Metadata &metadata,
+ std::optional<int> compressionLevel) : filename_(filename),
+ schema_(schema),
+ encoderPtr_(binaryEncoder()),
+ syncInterval_(syncInterval),
+ codec_(codec),
+ compressionLevel_(compressionLevel),
+ stream_(fileOutputStream(filename)),
Review Comment:
nit: This seems to be an old issue: `fileOutputStream(filename)` creates/truncates the output file before running the validation checks in `init()`, i.e. an invalid codec/compression_level/syncInterval will lead to an exception but the output file will be already created.
##########
lang/c++/impl/DataFile.cc:
##########
@@ -103,20 +172,17 @@ void DataFileWriterBase::init(const ValidSchema &schema, size_t syncInterval, co
"Invalid sync interval: {}. Should be between {} and {}",
syncInterval, minSyncInterval, maxSyncInterval);
}
- setMetadata(AVRO_CODEC_KEY, AVRO_NULL_CODEC);
+ validateCodec(codec_, compressionLevel_);
+ setMetadata(AVRO_CODEC_KEY, AVRO_NULL_CODEC);
if (codec_ == NULL_CODEC) {
setMetadata(AVRO_CODEC_KEY, AVRO_NULL_CODEC);
} else if (codec_ == DEFLATE_CODEC) {
setMetadata(AVRO_CODEC_KEY, AVRO_DEFLATE_CODEC);
-#ifdef SNAPPY_CODEC_AVAILABLE
Review Comment:
The constant `AVRO_SNAPPY_CODEC` is declared only if `SNAPPY_CODEC_AVAILABLE` is defined.
##########
lang/c++/impl/DataFile.cc:
##########
@@ -103,20 +172,17 @@ void DataFileWriterBase::init(const ValidSchema &schema, size_t syncInterval, co
"Invalid sync interval: {}. Should be between {} and {}",
syncInterval, minSyncInterval, maxSyncInterval);
}
- setMetadata(AVRO_CODEC_KEY, AVRO_NULL_CODEC);
+ validateCodec(codec_, compressionLevel_);
+ setMetadata(AVRO_CODEC_KEY, AVRO_NULL_CODEC);
if (codec_ == NULL_CODEC) {
Review Comment:
This branch is not needed since line 177 sets NULL_CODEC as a default one.
##########
lang/c++/impl/ZstdCompressWrapper.cc:
##########
Review Comment:
```suggestion
#include <optional>
#include <zstd.h>
```
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4222: [C++] Support writer to specify compression level [avro]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #3610:
URL: https://github.com/apache/avro/pull/3610#issuecomment-3701598406
Could you please take a look? @thiru-mg @martin-g
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4222: [C++] Support writer to specify compression level [avro]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g merged PR #3610:
URL: https://github.com/apache/avro/pull/3610
--
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: dev-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4222: [C++] Support writer to specify compression level [avro]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #3610:
URL: https://github.com/apache/avro/pull/3610#discussion_r2657582388
##########
lang/c++/impl/DataFile.cc:
##########
@@ -47,53 +47,183 @@ using std::array;
namespace {
const string AVRO_SCHEMA_KEY("avro.schema");
const string AVRO_CODEC_KEY("avro.codec");
-const string AVRO_NULL_CODEC("null");
-const string AVRO_DEFLATE_CODEC("deflate");
+const size_t minSyncInterval = 32;
Review Comment:
What unit is used here ? ms, secs, ...
A comment would be helpful!
##########
lang/c++/impl/DataFile.cc:
##########
@@ -47,53 +47,183 @@ using std::array;
namespace {
const string AVRO_SCHEMA_KEY("avro.schema");
const string AVRO_CODEC_KEY("avro.codec");
-const string AVRO_NULL_CODEC("null");
-const string AVRO_DEFLATE_CODEC("deflate");
+const size_t minSyncInterval = 32;
+const size_t maxSyncInterval = 1u << 30;
Review Comment:
My personal preference is to use the actual value (e.g. `1073741824`) and add `// 1 << 30` as a comment. The code should be easier to read for the humans.
If `<<` is preferred then also change `32` above to `1 << 5`.
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AVRO-4222: [C++] Support writer to specify compression level [avro]
Posted by "wgtmac (via GitHub)" <gi...@apache.org>.
wgtmac commented on PR #3610:
URL: https://github.com/apache/avro/pull/3610#issuecomment-3705178074
@thiru-mg @martin-g Thanks for your review! Not sure if the latest change is what you want. Let me know what you think :)
--
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: issues-unsubscribe@avro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org