Implementing BasicEncryptor with the optimized TypedBuffers by avalerio-tkd · Pull Request #226 · protegrity/DataBatchProtectionService

avalerio-tkd · 2026-03-08T01:36:20Z

Adding new BasicXOREncryptor implementation (transitional name before renaming to BasicEncryptor).
Implementing BasicXOREncryptor with typed buffer optimizations.
Added methods to TypedBuffer to get the write span for an element during SetElement calls
Added mehtods to TypedBuffer for getters of attributes.

…re renaming to BasicEncryptor). - Implementing BasicXOREncryptor with typed buffer optimizations. - Added methods to TypedBuffer to get the write span for an element during SetElement calls - Added mehtods to TypedBuffer for getters of attributes.

- Updating unittests - Including a temporary XorEncryptorInterface during the migration.

argmarco-tkd

Thank you for this. Overall the code seems correct, but I do have concerns around code complexity (left a few specific comments)

argmarco-tkd · 2026-03-08T15:24:56Z

+              << " user=" << user_id_ << " key=" << key_id_
+              << " datatype=" << dbps::enum_utils::to_string(datatype_) << std::endl;
+
+    return std::visit([&](const auto& input_buffer) -> std::vector<uint8_t> {


can we simplify this code? (e.g. remove the use of lambdas?) BasicEncryptor is supposed to be an example encryptor - this implementation makes legibility a bit hard.

Great comment. Totally agree on keeping BasicEncryptor as readable as possible.

However, we can't remove this one since it's a needed visit due to TypeBuffer overloading. There are workarounds but all end up doing a visit somewhere, just placed somewhere else. This is in general a known boilerplate pattern for accessing variant types in cpp.

Discussed offline. Added a TODO note in-place for this and will address it in a followup cleanup.

argmarco-tkd · 2026-03-08T15:29:55Z

+            encrypted_bytes, kFixedHeaderLength,
+            RawBytesFixedSizedCodec{header.element_size}};
+
+        auto decrypt_fixed_into = [&](auto output) {


simillar than for the encrypt. Can we optimize for legibility here?

Moved it to a separate helper function.

argmarco-tkd · 2026-03-08T15:31:10Z

+            return output;
+        };
+
+        // TODO: This is leaking Parquet-specific types into the encryptor, which should be agnostic of Parquet.


thanks for the call out. why was this not needed before?

The previous implementation had the same dependency, just that it was harder to read because it was used indirectly by a helper function on parquet_utils, so harder to detect. I didn't realize it either.

I have a possible solution in mind that we can discuss offline. The gist is we can add a type annotation to the output. This can come from the Codec that generates it, could be as simple as a unique byte value. This would be protected by the version check if the Codec code changes.

Discussed offline. The TODO note capture the pending item. Will leave as-is for this PR and will address in a followup.

argmarco-tkd · 2026-03-08T17:52:13Z

+    void SetRawElement(size_t position, tcb::span<const uint8_t> raw);
+
+    // Getters for immediately available properties.
+    size_t GetSpanSize() const { return elements_span_.size(); }


GetSpanSize seems like leakage of impl details - can a simple 'GetSize()` do?

Renamed to GetRawBufferSize(). GetSize() is ambiguous/too close to other values related to ElementSize.

avalerio-tkd

Thanks for the review and the comments. Will followup to chat offline and make decisions of code complexity VS efficiencies trade-offs, all valid concerns on the review.

avalerio-tkd · 2026-03-08T23:44:22Z

+              << " user=" << user_id_ << " key=" << key_id_
+              << " datatype=" << dbps::enum_utils::to_string(datatype_) << std::endl;
+
+    return std::visit([&](const auto& input_buffer) -> std::vector<uint8_t> {


Great comment. Totally agree on keeping BasicEncryptor as readable as possible.

However, we can't remove this one since it's a needed visit due to TypeBuffer overloading. There are workarounds but all end up doing a visit somewhere, just placed somewhere else. This is in general a known boilerplate pattern for accessing variant types in cpp.

avalerio-tkd · 2026-03-08T23:53:48Z

+            encrypted_bytes, kFixedHeaderLength,
+            RawBytesFixedSizedCodec{header.element_size}};
+
+        auto decrypt_fixed_into = [&](auto output) {


Moved it to a separate helper function.

avalerio-tkd · 2026-03-08T23:57:26Z

+            return output;
+        };
+
+        // TODO: This is leaking Parquet-specific types into the encryptor, which should be agnostic of Parquet.


The previous implementation had the same dependency, just that it was harder to read because it was used indirectly by a helper function on parquet_utils, so harder to detect. I didn't realize it either.

I have a possible solution in mind that we can discuss offline. The gist is we can add a type annotation to the output. This can come from the Codec that generates it, could be as simple as a unique byte value. This would be protected by the version check if the Codec code changes.

avalerio-tkd · 2026-03-09T00:03:22Z

+    void SetRawElement(size_t position, tcb::span<const uint8_t> raw);
+
+    // Getters for immediately available properties.
+    size_t GetSpanSize() const { return elements_span_.size(); }


Renamed to GetRawBufferSize(). GetSize() is ambiguous/too close to other values related to ElementSize.

- Added protection to GetWriteSpanForElement to prevent an object copy by misbehaved callers.

avalerio-tkd · 2026-03-09T19:51:37Z

Updated after offline discussions. @argmarco-tkd could you PTAL?

argmarco-tkd

Thanks for the discussion and the changes. LGTM, Ship it!

- Added cached current_element_size_ to TypedBuffer iterator.

avalerio-tkd added 4 commits March 7, 2026 19:33

- Updating CMakeLists.txt to include new BasicXOREncryptor library.

df77cdd

- Updating unittests - Including a temporary XorEncryptorInterface during the migration.

- Revert DBPSEncryptor interface to original state.

f891206

- Full revert of DBPSEncryptor interface to original state.

349db98

avalerio-tkd requested review from argmarco-tkd and sofia-tekdatum March 8, 2026 03:00

argmarco-tkd reviewed Mar 8, 2026

View reviewed changes

avalerio-tkd commented Mar 9, 2026

View reviewed changes

avalerio-tkd added 2 commits March 8, 2026 18:27

- Fixes and updates to address review comments.

abce28a

- Added protection to GetWriteSpanForElement to prevent an object copy by misbehaved callers.

- Addressing code review comments.

34b186d

- Fixed compilation error.

cb5b655

argmarco-tkd approved these changes Mar 9, 2026

View reviewed changes

- Added more test to Encrypt variable-sized buffers.

38a5879

- Added cached current_element_size_ to TypedBuffer iterator.

avalerio-tkd merged commit 9afea2d into main Mar 9, 2026
2 checks passed

avalerio-tkd deleted the av_typelist_optimizing_067 branch March 17, 2026 02:31

avalerio-tkd mentioned this pull request Mar 19, 2026

Optimize memory buffers on DBPS EncryptionSequencer libraries #218

Closed

Conversation

avalerio-tkd commented Mar 8, 2026

Uh oh!

argmarco-tkd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avalerio-tkd Mar 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avalerio-tkd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

avalerio-tkd Mar 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

avalerio-tkd commented Mar 9, 2026

Uh oh!

argmarco-tkd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

avalerio-tkd Mar 8, 2026 •

edited

Loading

avalerio-tkd Mar 8, 2026 •

edited

Loading