Adding Snappy to API server flow#171
Conversation
avalerio-tkd
commented
Nov 30, 2025
avalerio-tkd
commented
- Added Snappy compression/decompression support
- Added tests for compression/decompression library
- Added tests for compression/decompression library
argmarco-tkd
left a comment
argmarco-tkd
left a comment
There was a problem hiding this comment.
Thanks for this. Overall LGTM. Left a few comments.
| throw DBPSUnsupportedException( | ||
| "Unsupported compression codec: " + std::string(to_string(compression))); | ||
| } | ||
| std::vector<uint8_t> Compress(const std::vector<uint8_t>& bytes, CompressionCodec::type compression); |
There was a problem hiding this comment.
no namespace? (I'm OK either way.)
There was a problem hiding this comment.
Done. Added a namespace, thanks for the suggestion.
| if (bytes.empty()) { | ||
| return bytes; | ||
| } | ||
| std::string decompressed; |
There was a problem hiding this comment.
wait, string? why is this? It seems odd - specially given that just below (before returning) we're convering it to a byte array.
There was a problem hiding this comment.
Ah! This is because string has native support for resizing so Snappy uses it while compressing/decompressing. Otherwise it would need to calculate a sized buffer beforehand. So used for simplicity.
There was a problem hiding this comment.
got it, makes sense - string being used as an auto-resize buffer. let's add a quick note to the code. Have we checked is there risk for side effects for using strings? (e.g. UTF-8 encoding doing something 'magic' but undesired with the bytes ?) If this is not an issue, using string makes sense.
There was a problem hiding this comment.
Added an inline comment for this. Also added (then removed) a unittest to Compress/Decompress special utf-8 chars. It feels still wrong to merge code with special chars, so won't merge that specific test, but it works.
| TEST(CompressionUtils, CompressDecompress_Snappy_RoundTrip) { | ||
| std::vector<uint8_t> original = {0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08}; | ||
| std::vector<uint8_t> compressed = Compress(original, CompressionCodec::SNAPPY); | ||
| std::vector<uint8_t> decompressed = Decompress(compressed, CompressionCodec::SNAPPY); |
There was a problem hiding this comment.
this test should also validate than compressed != original (at least). Else, if the impl of 'snappy' was switched with the one from Uncompressed, the test would succeed.
There was a problem hiding this comment.
Done. Added the verification.
| TEST(CompressionUtils, CompressDecompress_Snappy_SingleByte) { | ||
| std::vector<uint8_t> original = {0x42}; | ||
| std::vector<uint8_t> compressed = Compress(original, CompressionCodec::SNAPPY); | ||
| std::vector<uint8_t> decompressed = Decompress(compressed, CompressionCodec::SNAPPY); |
There was a problem hiding this comment.
similar to above. let's at least verify that 'original' was transformed (and that 'original' != 'compressed')
| set(BUILD_TESTING ON CACHE BOOL "" FORCE) | ||
|
|
||
| # Disable Snappy tests to avoid conflicts with our GoogleTest | ||
| # Snappy includes its own GoogleTest which conflicts with our FetchContent GoogleTest |
There was a problem hiding this comment.
this seems familiar :)
| # Suppress warnings from Snappy's source code | ||
| if(TARGET snappy) | ||
| target_compile_options(snappy PRIVATE | ||
| -Wno-sign-compare |
There was a problem hiding this comment.
why do we need this?
There was a problem hiding this comment.
There are a bunch of compilation warnings that make the compilation output too noisy, so added this to suppress the warnings.
| target_include_directories(decoding_utils_test PRIVATE src/server) | ||
|
|
||
| # Compression utils tests | ||
| add_executable(compression_utils_test src/server/compression_utils_test.cpp) |
There was a problem hiding this comment.
do we need this now that we have GTEST?
There was a problem hiding this comment.
This is the same as the other test make targets, no? Following the same pattern.
avalerio-tkd
left a comment
avalerio-tkd
left a comment
There was a problem hiding this comment.
Thanks for the review @argmarco-tkd . Could you PTAL?
argmarco-tkd
left a comment
argmarco-tkd
left a comment
There was a problem hiding this comment.
Overall LGTM. Left an additional question on the usage of string as a buffer. I don't need a new PR if addressed.
Thanks for the pickiness. Comment addressed. Merging. |
2 participants
