Replacing vector-based TypedList with zero-copy TypedBuffer on top-level EncryptionSequencer#227
Conversation
avalerio-tkd
commented
Mar 10, 2026
avalerio-tkd
commented
- Updating EncryptionSequencer to use TypedValuesBuffer instead of TypedListValues (zero-copy optimization).
- Updating Encryptor interface to use reimplementation of BasicEncryptor using the zero-copy TypedValuesBuffer.
…pedListValues (zero-copy optimization). - Updating Encryptor interface to use reimplementation of BasicEncryptor using the zero-copy TypedValuesBuffer.
- Adding new helper functions for testing only, to generate sample Parquet payloads for testing.
|
@argmarco-tkd thanks for the review. I went ahead an removed most of the dead code, to make sure there were no hidden/dangling dependencies to it that could affect performance. Could you PTAL? |
…t payloads for testing. - Fixing performance_test.cpp and dbpa_remote_testapp.cpp to use the new helper functions.
argmarco-tkd
left a comment
argmarco-tkd
left a comment
There was a problem hiding this comment.
Thanks! Overall LGTM. Left a suggestion for the *_forTesting new method. No need for a new PR if/when addressed.
| std::vector<uint8_t> CombineRawBytesIntoValueBytesForTesting( | ||
| const std::vector<RawValueBytes>& elements, | ||
| Type::type datatype, | ||
| const std::optional<int>& datatype_length, | ||
| Encoding::type encoding); | ||
|
|
||
| std::vector<uint8_t> BuildByteArrayValueBytesForTesting(const std::string& payload); | ||
|
|
||
| std::vector<std::string> ParseByteArrayListValueBytesForTesting(const std::vector<uint8_t>& bytes); |
There was a problem hiding this comment.
nit: wouldn't these be better placed in a test_utils or similar file? (OK to keep as-is, but had to ask).
There was a problem hiding this comment.
Thought so, but it's kinda nice having all parquet formatting stuff in a single place.
Will move them to a header-only file. No, not gonna unittest the functions needed only for unittests. :)
---> Such a unitest would be called: parquet_testing_utils_test.cpp so, no.. :)
| } | ||
|
|
||
| std::vector<uint8_t> CombineRawBytesIntoValueBytes( | ||
| std::vector<uint8_t> CombineRawBytesIntoValueBytesForTesting( |
There was a problem hiding this comment.
nit: also a candidate for test_utils or similar? (again, I'm ok to keep as-is)
There was a problem hiding this comment.
See above. After the move, will merge it.
2 participants
