External file decryptor bindings by cristekdatum · Pull Request #57 · protegrity/arrow

cristekdatum · 2025-08-13T19:55:26Z

Adding code for the [31] and support the external_file_decryption_properties

github-actions · 2025-08-13T19:55:45Z

Thanks for opening a pull request!

If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose

Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project.

Then could you also rename the pull request title in the following format?

GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

or

MINOR: [${COMPONENT}] ${SUMMARY}

See also:

cristekdatum · 2025-08-13T20:42:36Z

Hey @sofia-tekdatum following the pattern of file_decryption_properties I created a new SafeGetExternalFileDecryptionProperties which requires same methods at the cc side in order to fix this problem

[ 89%] Building CXX object CMakeFiles/_parquet_encryption.dir/_parquet_encryption.cpp.o
/Users/cristianguillenmendez/Documents/dev-phase2/python/build/temp.macosx-15.4-arm64-cpython-310/_parquet_encryption.cpp:22521:77: error: no member named 'SafeGetExternalFileDecryptionProperties' in 'arrow::py::parquet::encryption::PyCryptoFactory'; did you mean 'GetExternalFileDecryptionProperties'?
        __pyx_v_c_file_decryption_properties = __pyx_v_self->factory.get()->SafeGetExternalFileDecryptionProperties((*__pyx_t_4.get()), __pyx_v_c_decryption_config);
                                                                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                                                                            GetExternalFileDecryptionProperties
/Users/cristianguillenmendez/Documents/dev-phase2/cpp/build/install/include/parquet/encryption/crypto_factory.h:205:53: note: 'GetExternalFileDecryptionProperties' declared here
  std::shared_ptr<ExternalFileDecryptionProperties> GetExternalFileDecryptionProperties(
                                                    ^
1 error generated.

sofia-tekdatum

@cristekdatum You forgot to change the ParquetReader open method in _parquet.pyx

Please add.

sofia-tekdatum

Careful with copy/paste errors, they may have made you spend a lot of time debugging.

… external encryption Python test

sofia-tekdatum · 2025-08-20T02:57:23Z

I have updated the bindings code so that it forwards its property setter to the super class.

I also fixed the test, which was failing because the configs were not set up properly and were disorganized. I rewrote the fixtures to match the config settings we have in the base_app, verified everything passes.

@cristekdatum @avalerio-tkd @argmarco-tkd PTAL for final approval and merging.

sofia-tekdatum · 2025-08-20T03:05:06Z

Also, verified the values make it successfully to the C++ side by running scripts/base_app.py and printing out values on the crypto_factory.cc code.

Obviously that's not submitted, but it's the way to verify the code that originates in python makes it through to C++.

avalerio-tkd

LGTM. Just a few comments. I tried following as much as possible. Thanks very much for this.
If the comments are not relevant or covered, feel free to merge.

avalerio-tkd · 2025-08-20T03:02:42Z

-        super().__init__(cache_lifetime=cache_lifetime)
+        # Initialize the pointer first so the get/set forwards work.
        self.external_configuration.reset(new CExternalDecryptionConfiguration())
+        super().__init__(cache_lifetime=cache_lifetime)


This change of order is a bit scary. Why is there a dependency on the subclass before calling the init of the superclass? Primarily for my understanding, but let's improve the comment.

Updated comment. Because the super class constructor will invoke the setters that are overridden below, so they need the pointer to be ready, otherwise we get a null crash.

avalerio-tkd · 2025-08-20T03:03:34Z

    cdef shared_ptr[CExternalDecryptionConfiguration] external_configuration
    cdef inline shared_ptr[CExternalDecryptionConfiguration] unwrap_external(self) nogil
+
+cdef shared_ptr[CCryptoFactory] pyarrow_unwrap_cryptofactory(object crypto_factory) except *


I'm assuming moving the definitions here is just for readability and nothing functional, right?

avalerio-tkd · 2025-08-20T03:06:10Z

                               CFileEncryptionProperties,
                               CExternalFileEncryptionProperties,
                               CFileDecryptionProperties,
+                               CExternalFileDecryptionProperties,


For my understanding, are the pxd modules compiled (so the change in params cause a compilation error if done wrong) or are these interpreted? If the latter, I'm assuming this is abundantly tested by unittests, right?

They are compiled. If they're not included here, we get compilation errors.

avalerio-tkd · 2025-08-20T03:09:52Z

+        if decryption_config is None:
+            c_decryption_config = CExternalDecryptionConfiguration()
+        else:
+            c_decryption_config = deref(decryption_config.unwrap_external().get())


I lost a little track around here. If not can you add a comment at the if-else level on what this is doing.
If this is common pattern / boilerplate, please disregard.

Boilerplate check that we have even an empty decryption config to work with, in case none was sent.

avalerio-tkd · 2025-08-20T03:11:18Z

+    assert read_data_table.num_rows == data_table.num_rows
+    assert read_data_table.num_columns == data_table.num_columns
+    assert read_data_table.schema.equals(data_table.schema)
+    assert read_data_table.column_names == data_table.column_names


Thanks very much for the thorough test.

This is part of @cristekdatum's original test.

avalerio-tkd · 2025-08-20T03:12:11Z

            const CDecryptionConfiguration& decryption_config) except +*
+        shared_ptr[CExternalFileDecryptionProperties] GetExternalFileDecryptionProperties(
+            const CKmsConnectionConfig& kms_connection_config,
+            const CExternalDecryptionConfiguration& decryption_config) except +*


😮‍💨

github-actions Bot added Component: Python awaiting review labels Aug 13, 2025

cristekdatum requested review from argmarco-tkd, avalerio-tkd and sofia-tekdatum August 13, 2025 20:38

cristekdatum marked this pull request as ready for review August 13, 2025 20:38

sofia-tekdatum reviewed Aug 13, 2025

View reviewed changes

sofia-tekdatum reviewed Aug 16, 2025

View reviewed changes

Comment thread python/pyarrow/_parquet.pyx Outdated

Comment thread python/pyarrow/tests/parquet/test_external_encryption.py Outdated

Comment thread python/pyarrow/tests/parquet/test_external_encryption.py Outdated

github-actions Bot added awaiting committer review and removed awaiting review labels Aug 16, 2025

sofia-tekdatum mentioned this pull request Aug 19, 2025

Fixing problems in ExternalEncryptionConfig bindings #66

Merged

sofia-tekdatum and others added 9 commits August 19, 2025 17:18

Solved conflicts

b986ba0

solved conflicts

42f95bd

solved conflicts

6af4c5f

solved conflicts

bb658dd

code review

e10e716

code review

483ea2c

solved conflicts

1faa390

missing files

2606f06

Fixed problems with bindings for ExternalDecryptionConfig. Also fixed…

a858935

… external encryption Python test

sofia-tekdatum force-pushed the external_file_decryptor_bindings branch from 1f7542e to a858935 Compare August 20, 2025 02:36

github-actions Bot added Component: C++ Component: Parquet labels Aug 20, 2025

sofia-tekdatum added 3 commits August 19, 2025 20:54

restoring crypto_factory

03bccda

restoring crypto_factory

bf9a0ea

restoring crypto_factory

2269958

github-actions Bot removed Component: C++ Component: Parquet labels Aug 20, 2025

This was linked to issues Aug 20, 2025

Implement changes to Python bindings > Fix all existing bindings that are not propagating super class attributes #67

Closed

Implement changes to Python bindings > Create ExternalDecryptionProperties (getMethod + datatype) @Cython #31

Closed

avalerio-tkd approved these changes Aug 20, 2025

View reviewed changes

Added comment

eb30bd9

sofia-tekdatum merged commit f0599e3 into dev_phase2 Aug 20, 2025
6 of 23 checks passed

sofia-tekdatum deleted the external_file_decryptor_bindings branch August 20, 2025 15:07

Uh oh!

Conversation

cristekdatum commented Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Aug 13, 2025

Uh oh!

cristekdatum commented Aug 13, 2025

Uh oh!

sofia-tekdatum left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sofia-tekdatum left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sofia-tekdatum commented Aug 20, 2025

Uh oh!

sofia-tekdatum commented Aug 20, 2025

Uh oh!

avalerio-tkd left a comment

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cristekdatum commented Aug 13, 2025 •

edited

Loading