Skip to content

GH-41246: [C++][Python] Simplify nested field encryption configuration#45462

Merged
pitrou merged 40 commits into
apache:mainfrom
EnricoMi:nested-field-encryption
Dec 16, 2025
Merged

GH-41246: [C++][Python] Simplify nested field encryption configuration#45462
pitrou merged 40 commits into
apache:mainfrom
EnricoMi:nested-field-encryption

Conversation

@EnricoMi

@EnricoMi EnricoMi commented Feb 7, 2025

Copy link
Copy Markdown
Collaborator

Rationale for this change

Columns can b encrypted with individual keys. For this, the column name have to be set in EncryptionConfiguration::column_keys. This poses the following challenges for columns with nested fields like MapType, ListType, and StructType. Encrypting a column of such type requires providing an encryption key for all nested (leaf) fields. Ideally, the column name should be sufficient (as it is for any other data type) to encrypt all nested fields.

What changes are included in this PR?

The column name can be used to encrypt all nested fields of StrutType, MapType, and ListType columns with the same encryption key. The current column naming scheme can still be used for backward compatibility.

Are these changes tested?

Tested in C++ and Python.

Are there any user-facing changes?

Column encryption can be configured with less code and more intuitive naming.

Documentation and examples updated.

Fixes #41246.

@EnricoMi EnricoMi requested a review from wgtmac as a code owner February 7, 2025 11:12
@EnricoMi EnricoMi changed the title GH-41246: [C++][Python] Simplify nested field encryption GH-41246: [C++][Python] Simplify nested field encryption configuration Feb 7, 2025
@EnricoMi EnricoMi force-pushed the nested-field-encryption branch from ba57bdc to 611f7a7 Compare March 10, 2025 06:16

@AlenkaF AlenkaF left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Python tests LGTM - they are vey concise and reflect the C++ tests.

The C++ addition of FileEncryptionProperties::encrypt_schema looks good to me also but will need a check from a C++ developer. @wgtmac mind having a look?

@github-actions github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Mar 24, 2025
@AlenkaF

AlenkaF commented Mar 24, 2025

Copy link
Copy Markdown
Member

Note, CI Win failures do not look related but there is a linter fix needed:

@@ -257,11 +257,12 @@ void FileEncryptionProperties::encrypt_schema(const SchemaDescriptor& schema) {
       // encrypted_column encrypts column 'it' when 'it' is either equal to
       // encrypted_column, or 'it' starts with encrypted_column_prefix,
       // i.e. encrypted_column followed by a '.'
-      while (it != column_path_vec.end() &&
-             (it->first == encrypted_column ||
-               // C++20: can be replaced with it->first.starts_with(encrypted_column_prefix)
-              it->first.compare(0, encrypted_column_prefix_len,
-                                encrypted_column_prefix) == 0)) {
+      while (
+          it != column_path_vec.end() &&
+          (it->first == encrypted_column ||
+           // C++20: can be replaced with it->first.starts_with(encrypted_column_prefix)
+           it->first.compare(0, encrypted_column_prefix_len, encrypted_column_prefix) ==
+               0)) {

@EnricoMi EnricoMi force-pushed the nested-field-encryption branch from 25b39d0 to 0c7ff27 Compare March 25, 2025 21:13
@AlenkaF

AlenkaF commented Mar 26, 2025

Copy link
Copy Markdown
Member

The C++ failure looks related.

@AlenkaF

AlenkaF commented Mar 26, 2025

Copy link
Copy Markdown
Member

cc @pitrou, this is connected to #45411.

@EnricoMi EnricoMi force-pushed the nested-field-encryption branch 2 times, most recently from efbc7c8 to 41a3359 Compare March 27, 2025 05:40
@pitrou

pitrou commented Mar 27, 2025

Copy link
Copy Markdown
Member

This adds a user-friendly notation for nested fields:

  • Columns col.key and col.value can be used to reference they key and value nested field of a MapType column. Currently, col.key_value.key and col.key_value.value are required, respectively.

Is this useful? Is there a reasonable use case for using different encryption settings for the map key and value columns, respectively? Otherwise I think we can only keep the following items, which will also simplify the semantics and the implementation:

  • Columns col.element can be used to reference they individual list elements of a ListType column. Currently, col.list.element is required.
  • The actual column name can be used to encrypt all nested fields with the same encryption key.
  • The current column naming scheme can still be used for backward compatibility.

@EnricoMi

Copy link
Copy Markdown
Collaborator Author

Is this useful? Is there a reasonable use case for using different encryption settings for the map key and value columns, respectively?

Irrespective of it being useful, it is currently possible to do via col.key_value.key and col.key_value.value. This pull request simplifies that notation consistently with lists and structs.

@EnricoMi

Copy link
Copy Markdown
Collaborator Author

Note that I have removed encrypting the individual list elements via col.element as I cannot see a difference in semantics and encryption output between col.element and col. Both notations are equivalent.

Encrypting a single field of a list of structs column can be denoted as col.f1 rather than col.element.f1.

What do you think?

@EnricoMi

EnricoMi commented Sep 4, 2025

Copy link
Copy Markdown
Collaborator Author

@pitrou what are your thoughts on the latest logic / semantics?

@EnricoMi

Copy link
Copy Markdown
Collaborator Author

@pitrou hope you have a moment for this

@pitrou

pitrou commented Oct 30, 2025

Copy link
Copy Markdown
Member

I'll be out on vacation, so not before ~10 days I think.

@EnricoMi

Copy link
Copy Markdown
Collaborator Author

No worries, enjoy!

@pitrou

pitrou commented Nov 17, 2025

Copy link
Copy Markdown
Member

@github-actions crossbow submit preview-docs

@pitrou pitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay. This looks in general, a bunch of minor comments and suggestions below.

Comment thread cpp/src/arrow/dataset/file_parquet_encryption_test.cc Outdated
Comment thread cpp/src/arrow/dataset/file_parquet_encryption_test.cc
Comment thread cpp/src/arrow/dataset/file_parquet_encryption_test.cc Outdated
Comment thread python/pyarrow/tests/parquet/encryption.py
Comment thread python/pyarrow/tests/test_dataset_encryption.py Outdated
@github-actions

Copy link
Copy Markdown

Revision: b0ef03f

Submitted crossbow builds: ursacomputing/crossbow @ actions-151fb9c2fa

Task Status
preview-docs GitHub Actions

@pitrou

pitrou commented Dec 16, 2025

Copy link
Copy Markdown
Member

Ha, sorry, CI fails now that #48338 was merged, you need to stop passing the column path to ColumnEncryptionProperties::Builder.

@pitrou pitrou left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update @EnricoMi !

@pitrou pitrou merged commit 052e0aa into apache:main Dec 16, 2025
44 of 46 checks passed
@pitrou pitrou removed the awaiting committer review Awaiting committer review label Dec 16, 2025
@pitrou

pitrou commented Dec 16, 2025

Copy link
Copy Markdown
Member

I'm sorry that it took so long @EnricoMi . Looking forward to more contributions from you!

pitrou added a commit that referenced this pull request Dec 16, 2025
… corpus (#48558)

### Rationale for this change

In #48336 we skipped encrypted nested columns because it was too cumbersome to configure (each leaf column had to be configured independently).

Now that #45462 has been merged we can configure nested columns the same way as non-nested ones.

### Are these changes tested?

Manually and later by OSS-Fuzz.

### Are there any user-facing changes?

No.
* GitHub Issue: #48557

Authored-by: Antoine Pitrou <antoine@python.org>
Signed-off-by: Antoine Pitrou <antoine@python.org>
@conbench-apache-arrow

Copy link
Copy Markdown

After merging your PR, Conbench analyzed the 3 benchmarking runs that have been run so far on merge-commit 052e0aa.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 83 possible false positives for unstable benchmarks that are known to sometimes produce them.

pitrou added a commit that referenced this pull request Dec 17, 2025
### Rationale for this change

A `#include` required on Valgrind (due to conditional compilation) was removed in PR #45462.

### Are these changes tested?

Yes, by existing CI jobs.

### Are there any user-facing changes?

No.
* GitHub Issue: #48566

Authored-by: Antoine Pitrou <antoine@python.org>
Signed-off-by: Antoine Pitrou <antoine@python.org>
Mottl pushed a commit to Mottl/arrow that referenced this pull request May 26, 2026
…uration (apache#45462)

### Rationale for this change

Columns can b encrypted with individual keys. For this, the column name have to be set in `EncryptionConfiguration::column_keys`. This poses the following challenges for columns with nested fields like `MapType`, `ListType`, and `StructType`. Encrypting a column of such type requires providing an encryption key for all nested (leaf) fields. Ideally, the column name should be sufficient (as it is for any other data type) to encrypt all nested fields.

### What changes are included in this PR?

The column name can be used to encrypt all nested fields of `StrutType`, `MapType`, and `ListType` columns with the same encryption key. The current column naming scheme can still be used for backward compatibility.

### Are these changes tested?
Tested in C++ and Python.

### Are there any user-facing changes?
Column encryption can be configured with less code and more intuitive naming.

Documentation and examples updated.

Fixes apache#41246.
* GitHub Issue: apache#41246

Authored-by: Enrico Minack <github@enrico.minack.dev>
Signed-off-by: Antoine Pitrou <antoine@python.org>
Mottl pushed a commit to Mottl/arrow that referenced this pull request May 26, 2026
…z seed corpus (apache#48558)

### Rationale for this change

In apache#48336 we skipped encrypted nested columns because it was too cumbersome to configure (each leaf column had to be configured independently).

Now that apache#45462 has been merged we can configure nested columns the same way as non-nested ones.

### Are these changes tested?

Manually and later by OSS-Fuzz.

### Are there any user-facing changes?

No.
* GitHub Issue: apache#48557

Authored-by: Antoine Pitrou <antoine@python.org>
Signed-off-by: Antoine Pitrou <antoine@python.org>
Mottl pushed a commit to Mottl/arrow that referenced this pull request May 26, 2026
### Rationale for this change

A `#include` required on Valgrind (due to conditional compilation) was removed in PR apache#45462.

### Are these changes tested?

Yes, by existing CI jobs.

### Are there any user-facing changes?

No.
* GitHub Issue: apache#48566

Authored-by: Antoine Pitrou <antoine@python.org>
Signed-off-by: Antoine Pitrou <antoine@python.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Python][Parquet] Attempt to encrypt column of type 'list' produces OSError

3 participants