Refactor to move Parquet and AttributeMap logic out of encryption_sequencer #184
Conversation
avalerio-tkd
commented
Dec 7, 2025
avalerio-tkd
commented
- Renamed decoding_utils to parquet_utils
- Moved Parquet specific logic out of encryption_sequencer to parquet_utils
- Moved string-to-variant conversion logic out of encryption_sequencer to bytes_utils.h
- Small cleanups.
- Moved Parquet specific logic out of encryption_sequencer to parquet_utils - Moved string-to-variant conversion logic out of encryption_sequencer to bytes_utils.h - Small cleanups.
- Moved Parquet specific logic out of encryption_sequencer to parquet_utils - Moved string-to-variant conversion logic out of encryption_sequencer to bytes_utils.h - Small cleanups.
argmarco-tkd
left a comment
argmarco-tkd
left a comment
There was a problem hiding this comment.
Overall LGTM. Left a few nits, but I don't need a new PR.
| std::to_string(offset) + ", size=" + std::to_string(raw.size()) + ")"); | ||
| } | ||
| uint32_t len = *reinterpret_cast<const uint32_t*>(raw.data() + offset); | ||
| uint32_t len = read_u32_le(raw, offset); |
There was a problem hiding this comment.
nice. this is clearer.
| // Get the page type from the encoding attributes. | ||
| auto page_type = std::get<std::string>(encoding_attributes.at("page_type")); | ||
|
|
||
| if (page_type == "DATA_PAGE_V1") { |
There was a problem hiding this comment.
nit (for future devs) - it may be worth adding a couple of lines for each type of DATA_PAGE that describe the nuances of compression
There was a problem hiding this comment.
Comments added.
| auto result = DecompressAndSplit( | ||
| plaintext, CompressionCodec::UNCOMPRESSED, attribs_conv); |
There was a problem hiding this comment.
from a unit testing perspective, do we have tests covering the exception throwing of the non-supported compression codecs? (that can be done in a later PR, if we think it pertinent.).
There was a problem hiding this comment.
Added unittest.
| // Utility functions for creating an AttributesMap | ||
|
|
||
| // Common alias for converted encoding attributes used across modules. | ||
| using AttributesMap = std::map<std::string, std::variant<int32_t, bool, std::string>>; |
There was a problem hiding this comment.
why only int32_t (as oppossed to other int types also?) - let's add a comment.
There was a problem hiding this comment.
Done. Added comment.
|
Thanks for the review. Merging. |
2 participants
