-
Notifications
You must be signed in to change notification settings - Fork 0
Make BOOLEAN explicitly unsupported for per-value encryption #196
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -417,6 +417,72 @@ TEST(EncryptionSequencer, ResultStorage) { | |
|
|
||
| } | ||
|
|
||
| // Test BOOLEAN type uses per-block encryption (not per-value) | ||
| TEST(EncryptionSequencer, BooleanTypeUsesPerBlockEncryption) { | ||
| // BOOLEAN is permanently unsupported for per-value operations | ||
| // and always defaults to per-block encryption | ||
| std::vector<uint8_t> boolean_data = {0xB4, 0xFF, 0x00}; // some boolean bit-packed data | ||
|
|
||
| DataBatchEncryptionSequencer sequencer( | ||
| "bool_column", | ||
| Type::BOOLEAN, | ||
| std::nullopt, | ||
| CompressionCodec::UNCOMPRESSED, | ||
| Format::PLAIN, | ||
| {{"page_type", "DICTIONARY_PAGE"}}, | ||
| CompressionCodec::UNCOMPRESSED, | ||
| "test_key", | ||
| "test_user", | ||
| "{}", | ||
| {} | ||
| ); | ||
|
|
||
| bool result = sequencer.ConvertAndEncrypt(boolean_data); | ||
| ASSERT_TRUE(result) << "BOOLEAN encryption failed: " << sequencer.error_stage_ << " - " << sequencer.error_message_; | ||
|
|
||
| // Verify it used per-block encryption mode | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. typo: "if", I believe.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed the wording. |
||
| ASSERT_TRUE(sequencer.encryption_metadata_.count("encrypt_mode_dict_page") > 0); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. assertion makes sense, but maybe just do
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
| EXPECT_EQ(sequencer.encryption_metadata_.at("encrypt_mode_dict_page"), "per_block"); | ||
|
|
||
| // Verify round-trip works | ||
| bool decrypt_result = sequencer.ConvertAndDecrypt(sequencer.encrypted_result_); | ||
| ASSERT_TRUE(decrypt_result) << "BOOLEAN decryption failed: " << sequencer.error_stage_ << " - " << sequencer.error_message_; | ||
| EXPECT_EQ(sequencer.decrypted_result_, boolean_data); | ||
| } | ||
|
|
||
| // Test RLE_DICTIONARY format uses per-block encryption (not per-value) | ||
| TEST(EncryptionSequencer, RleDictionaryFormatUsesPerBlockEncryption) { | ||
| // RLE_DICTIONARY is permanently unsupported for per-value operations | ||
| // since the values are not present in the data, only references to them | ||
| std::vector<uint8_t> rle_dict_data = {0x02, 0x00, 0x00, 0x00, 0x01}; // some RLE dictionary encoded data | ||
|
|
||
| DataBatchEncryptionSequencer sequencer( | ||
| "dict_column", | ||
| Type::INT32, | ||
| std::nullopt, | ||
| CompressionCodec::UNCOMPRESSED, | ||
| Format::RLE_DICTIONARY, | ||
| {{"page_type", "DICTIONARY_PAGE"}}, | ||
| CompressionCodec::UNCOMPRESSED, | ||
| "test_key", | ||
| "test_user", | ||
| "{}", | ||
| {} | ||
| ); | ||
|
|
||
| bool result = sequencer.ConvertAndEncrypt(rle_dict_data); | ||
| ASSERT_TRUE(result) << "RLE_DICTIONARY encryption failed: " << sequencer.error_stage_ << " - " << sequencer.error_message_; | ||
|
|
||
| // Verify it used per-block encryption mode | ||
| ASSERT_TRUE(sequencer.encryption_metadata_.count("encrypt_mode_dict_page") > 0); | ||
| EXPECT_EQ(sequencer.encryption_metadata_.at("encrypt_mode_dict_page"), "per_block"); | ||
|
|
||
| // Verify round-trip works | ||
| bool decrypt_result = sequencer.ConvertAndDecrypt(sequencer.encrypted_result_); | ||
| ASSERT_TRUE(decrypt_result) << "RLE_DICTIONARY decryption failed: " << sequencer.error_stage_ << " - " << sequencer.error_message_; | ||
| EXPECT_EQ(sequencer.decrypted_result_, rle_dict_data); | ||
| } | ||
|
|
||
| // Test FIXED_LEN_BYTE_ARRAY validation | ||
| TEST(EncryptionSequencer, FixedLenByteArrayValidation) { | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -129,6 +129,8 @@ inline static size_t GetFixedElemSizeOrThrow(Type::type datatype, const std::opt | |
| } | ||
| case Type::UNDEFINED: | ||
| return 1; | ||
| case Type::BOOLEAN: | ||
| throw InvalidInputException("BOOLEAN is bit-sized; not fixed byte-sized"); | ||
| case Type::BYTE_ARRAY: | ||
| throw InvalidInputException("BYTE_ARRAY is variable-length; not fixed-size"); | ||
| default: | ||
|
|
@@ -146,13 +148,19 @@ std::vector<RawValueBytes> SliceValueBytesIntoRawBytes( | |
| // RLE_DICTIONARY is not supported for per-value operations since the values themselves are not present in the data, | ||
| // only references to them. | ||
| if (format == Format::RLE_DICTIONARY) { | ||
| throw DBPSUnsupportedException("Unsupported format: RLE_DICTIONARY is not supported for per-value operations"); | ||
| throw DBPSUnsupportedException("Unsupported format: RLE_DICTIONARY is not supported for per-value operations " | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice clarification. |
||
| "since values are not present in the data, only references to them."); | ||
| } | ||
|
|
||
| if (format != Format::PLAIN) { | ||
| throw DBPSUnsupportedException("On SliceValueBytesIntoRawBytes, unsupported format: " + std::string(to_string(format))); | ||
| } | ||
|
|
||
| // BOOLEAN: boolean values are bit-encoded and not expanded as bytes | ||
| if (datatype == Type::BOOLEAN) { | ||
| throw DBPSUnsupportedException("On SliceValueBytesIntoRawBytes, BOOLEAN datatype is not supported for converting to bytes."); | ||
| } | ||
|
|
||
| // Variable-length BYTE_ARRAY: parse [4-byte len][bytes...] elements in order. | ||
| // This is the Parquet specific encoding for BYTE_ARRAY. | ||
| if (datatype == Type::BYTE_ARRAY) { | ||
|
|
@@ -211,6 +219,11 @@ std::vector<uint8_t> CombineRawBytesIntoValueBytes( | |
| throw DBPSUnsupportedException("On CombineRawBytesIntoValueBytes, unsupported format: " + std::string(to_string(format))); | ||
| } | ||
|
|
||
| // BOOLEAN: boolean values are bit-encoded and not expanded as bytes | ||
| if (datatype == Type::BOOLEAN) { | ||
| throw DBPSUnsupportedException("On CombineRawBytesIntoValueBytes, BOOLEAN datatype is not supported for converting to bytes."); | ||
| } | ||
|
|
||
| if (datatype == Type::BYTE_ARRAY) { | ||
| std::vector<uint8_t> out; | ||
| size_t total = 0; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in here, I'd either (a) remove the 'permanently' wording and just leave it as "boolean defaults to per-value' OR (b) add a reason as to why it's permanently unsupported.
I believe (a) is the way to go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Thanks.