Refactor to Create typed_list_values module and related library cleanup#182
Conversation
avalerio-tkd
commented
Dec 5, 2025
avalerio-tkd
commented
- Refactor to move Type bit-handling to TypedListValues
- Refined common methods that converted between value bytes and typed list values and centralized in TypedListValues.
- Cleanup nits of DecodingUtils related to bit shifting and byte slicing.
- Rewrote ValueBytes encoding so that (1) Parquet specific encoding lives on decoding_utils and (2) TypedListValues logic is on typed_list_values.
- Refined common methods that converted between value bytes and typed list values and centralized in TypedListValues. - Cleanup nits of DecodingUtils related to bit shifting and byte slicing. - Rewrote ValueBytes encoding so that (1) Parquet specific encoding lives on decoding_utils and (2) TypedListValues logic is on typed_list_values.
- Refined common methods that converted between value bytes and typed list values and centralized in TypedListValues. - Cleanup nits of DecodingUtils related to bit shifting and byte slicing. - Rewrote ValueBytes encoding so that (1) Parquet specific encoding lives on decoding_utils and (2) TypedListValues logic is on typed_list_values.
argmarco-tkd
left a comment
argmarco-tkd
left a comment
There was a problem hiding this comment.
Thank you for this refactor. Things are much cleaner now.
Overall everything looks good, but the specifics (logic) inside decoding_utils are a bit unknown to me. Happy to chat if you need an in-depth review of those (but I think the tests help ensure correctness there)
|
|
||
| # Bytes utils tests | ||
| add_executable(bytes_utils_test src/server/bytes_utils_test.cpp) | ||
| add_executable(bytes_utils_test src/common/bytes_utils_test.cpp) |
|
|
||
| } // namespace | ||
|
|
||
| std::string PrintTypedList(const TypedListValues& list) { |
There was a problem hiding this comment.
nit: technically this is a "TypedListToString", instead of a "PrintTypedList" (there is no actual printing) :)
There was a problem hiding this comment.
Updated. In a previous life it was a print, then changed. Thanks for that.
| /** | ||
| * Type alias for raw value bytes. | ||
| */ | ||
| using RawValueBytes = std::vector<uint8_t>; |
| /** | ||
| * Simple container for an encrypted payload and its size. | ||
| */ | ||
| struct EncryptedValue { |
There was a problem hiding this comment.
why would not do the alias thing similar to what happened with RawValueBytes? :)
| int32_t rep_level_length = std::get<int32_t>(encoding_attribs.at("page_v2_repetition_levels_byte_length")); | ||
| total_level_bytes = def_level_length + rep_level_length; | ||
| std::cout << "CalculateLevelBytesLength DATA_PAGE_V2: total_level_bytes=" << total_level_bytes << std::endl; | ||
| std::cout << "CalculateLevelBytesLength DATA_PAGE_V2: total_level_bytes=" |
There was a problem hiding this comment.
I'm guessing this is meant to be temporary? (same for other printouts in this file) if so, let's add the TODO/link to task to clean up.
| std::vector<T> DecodeFixedSizeType(const uint8_t* raw_data, size_t raw_size, const char* name) { | ||
| if ((raw_size % sizeof(T)) != 0) { | ||
| throw InvalidInputException(std::string("Invalid data size for ") + name + " decoding"); | ||
| static size_t GetFixedElemSizeOrThrow(Type::type datatype, const std::optional<int>& datatype_length) { |
There was a problem hiding this comment.
is there a reason this static vs. some of the other utils which have been inlined?
There was a problem hiding this comment.
Update. Yes, much better as inlined. Thanks.
| int fixed_length = datatype_length.value(); | ||
| if ((bytes.size() % fixed_length) != 0) { | ||
|
|
||
| // Variable-length BYTE_ARRAY: parse [4-byte len][bytes...] elements in order. |
There was a problem hiding this comment.
just for clarity - is this how this data type is encoded in parquet?
There was a problem hiding this comment.
Correct. This is Parquet specific. Added a comment inline and also to the header file. The next PR will rename the module this to make more explicit that this is specific Parquet encode/decode and move to parquet_utils. But that's next PR.
argmarco-tkd
left a comment
argmarco-tkd
left a comment
There was a problem hiding this comment.
(no need for a new PR if/when the comments/nits are addressed)
- Switched EncryptedValue to std::vector<uint8_t> in value_encryption_utils.h
|
Thanks for the comments. Merging.. |
2 participants
