GH-39645: [Python] Fix read_table for encrypted parquet #39438

tmct · 2024-01-03T11:39:46Z

Rationale for this change

Currently, if you try to read a decrypted parquet with read_table, passing decryption_properties - in the happy path (pyarrow.data available for import) the reading/decryption of the file fails, as the decryption properties are missing.

What changes are included in this PR?

Pass through the argument that was intended to have been passed.

Are these changes tested?

We have tested this locally on an encrypted parquet dataset - please advise on any further testing you would like beyond that and the standard CI.

Are there any user-facing changes?

Not in any cases where their code was previously working? The intended behaviour for encrypted dataset decryption should start working.

Closes: [Python] Issue with Decryption Properties in read_table for Encrypted Parquet Files #39645

github-actions · 2024-01-03T11:40:11Z

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}

In the case of PARQUET issues on JIRA the title also supports:

PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}

See also:

kou · 2024-01-03T22:41:59Z

Could you add a test for this case and open a new issue for this?
This is not a MINOR change: https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes

raulcd · 2024-01-15T13:52:03Z

@tmct do you want to open an issue and follow it up as suggested by @kou or do you want us to follow it up?
We should create an issue, link it to the PR and create a test. cc @AlenkaF

AlenkaF · 2024-01-15T14:16:30Z

Thank you for fixing this!
We should add a test to the pyarrow/tests/parquet/test_encryption.py::read_encrypted_parquet.

tolleybot · 2024-01-16T19:23:47Z

I have added the issue ##39645 Issue with Decryption Properties in read_table for Encrypted Parquet Files for this PR
@tmct let me know if there are any details you want to add to the issue.

github-actions · 2024-01-17T06:55:08Z

⚠️ GitHub issue #39645 has been automatically assigned in GitHub to PR creator.

tolleybot · 2024-01-22T15:20:11Z

I have added new test test_encrypted_parquet_read_table

AlenkaF · 2024-01-22T15:31:00Z

The failures look connected.

tolleybot · 2024-01-26T14:37:39Z

I provided updates, and it looks like two tests are failing. I am not sure if they are related, however.

kou · 2024-01-26T14:54:57Z

Could you rebase on main?

github-actions · 2024-01-26T15:01:08Z

⚠️ GitHub issue #39645 has been automatically assigned in GitHub to PR creator.

python/pyarrow/parquet/core.py

AlenkaF · 2024-01-29T08:49:13Z

The failures can be found on other builds also so they are not related to this PR.

tolleybot · 2024-02-26T17:04:53Z

Hello everyone, anyway to get a reviewer on this PR?

AlenkaF

Adding two comments, thank you for your patience! =)

python/pyarrow/_dataset_parquet.pyx

python/pyarrow/parquet/core.py

python/pyarrow/tests/parquet/test_encryption.py

pitrou · 2024-02-27T14:37:55Z

python/pyarrow/_dataset_parquet.pyx

@@ -808,6 +815,18 @@ cdef class ParquetFragmentScanOptions(FragmentScanOptions):
            raise ValueError("size must be larger than zero")
        self.reader_properties().set_thrift_container_size_limit(size)

+    def set_file_decryption_properties(self, FileDecryptionProperties decryption_properties):


Several things:

will this work if Parquet encryption is not enabled at compile time? (see how parquet_decryption_config works)

can you make this a read/write property like the other options here?

can it also be added to the constructor like the other options?

can you add unit tests for it? see existing test in

arrow/python/pyarrow/tests/test_dataset_encryption.py

Line 131 in 40a8a68

# set decryption config for parquet fragment scan options

how does this interact with decryption_config?

Ill look into this

I think 1 through 4 are addressed now.
As for 5 decryption_config and file_decryption_properties currently don't interact in Python code base. But as per @wgtmac they do in c++. Perhaps @tolleybot has more insight.

arrow/cpp/src/arrow/dataset/file_parquet_encryption_test.cc

Lines 220 to 233 in 318d22a

// Create the ReaderProperties object using the FileDecryptionProperties object

auto decryption_config =

std::make_shared<parquet::encryption::DecryptionConfiguration>();

auto file_decryption_properties = crypto_factory_->GetFileDecryptionProperties(

*kms_connection_config_, *decryption_config);

auto reader_properties = parquet::default_reader_properties();

reader_properties.file_decryption_properties(file_decryption_properties);

// Read entire file as a single Arrow table

parquet::arrow::FileReaderBuilder reader_builder;

ASSERT_OK(reader_builder.Open(input, reader_properties));

ASSERT_OK_AND_ASSIGN(auto arrow_reader, reader_builder.Build());

std::shared_ptr<Table> table;

ASSERT_OK(arrow_reader->ReadTable(&table));

It seems DecryptionConfiguration is meant for scanning, while FileDecryptionProperties is to be used when reading single files with ReadTable. Both seem to be public.

python/pyarrow/_dataset_parquet.pyx

pitrou · 2024-02-27T14:41:45Z

@wgtmac Would you like to take a look at this? I'm not entirely sure which kind of decryption arguments should be exposed in the public Parquet reading APIs. It seems we already take a decryption_config for Parquet datasets, but this is adding decryption_properties.

also cc @jorisvandenbossche

wgtmac

@pitrou I'm not that familiar with the python code base. AFAIK, ParquetDecryptionConfig is used by parquet dataset reader to create FileDecryptionProperties for each file like this:

arrow/cpp/src/arrow/dataset/file_parquet.cc

Lines 82 to 86 in 40a8a68

    
           if (parquet_decrypt_config != nullptr) { 
        
             auto file_decryption_prop = 
        
                 parquet_decrypt_config->crypto_factory->GetFileDecryptionProperties( 
        
                     *parquet_decrypt_config->kms_connection_config, 
        
                     *parquet_decrypt_config->decryption_config, path, filesystem);

python/pyarrow/parquet/core.py

rok · 2024-05-08T21:57:39Z

@pitrou could you do another pass here please?

AlenkaF

LGTM
Thank you for working on this!

rok · 2024-05-09T13:24:34Z

@wgtmac could you please do a quick sanity check over the decryption_config / decryption_properties part?

wgtmac

+1

wgtmac · 2024-05-09T14:31:19Z

python/pyarrow/_dataset_parquet.pyx

@@ -715,6 +714,9 @@ cdef class ParquetFragmentScanOptions(FragmentScanOptions):
    decryption_config : pyarrow.dataset.ParquetDecryptionConfig, default None
        If not None, use the provided ParquetDecryptionConfig to decrypt the
        Parquet file.
+    decryption_properties : pyarrow.dataset.FileDecryptionProperties, default None


Is this correct? I only see C++ class FileDecryptionProperties under parquet namespace.

Nice catch! Changed to:

decryption_properties : pyarrow.parquet.FileDecryptionProperties, default None

I'll wait for the CI and merge.

Refactor Dataset Loading to Utilize ds.dataset for Single File Handling Changed the dataset loading logic to directly use ds.dataset for both single files and directories. Removed the conditional check for single_file and the manual creation of FileSystemDataset from a single fragment. Rationale: - The updated approach leverages the flexibility of ds.dataset, which, according to its documentation, can handle a single file scenario. As per the docstring: "Path pointing to a single file: Open a FileSystemDataset from a single file." - This simplification leads to cleaner and more maintainable code, as ds.dataset internally manages the distinctions between a single file and a directory, removing the need for explicit conditional checks and fragment creation. This refactor enhances code clarity and relies on the robustness of PyArrow's dataset handling, making the code more streamlined. Commit Summary: - Removed `decryption_properties` from `scan_args` to avoid passing it to `ParquetFragmentScanOptions`. - Added `set_file_decryption_properties` function to apply decryption properties directly to `default_fragment_scan_options`. - Applied decryption properties using the new function, ensuring compatibility with `ParquetFragmentScanOptions`. Rationale: - Eliminating `decryption_properties` from `scan_args` prevents errors since `ParquetFragmentScanOptions` does not support this parameter. Its set through the CReaderProperties property - Introducing `set_file_decryption_properties` provides a clear and focused method for handling decryption settings, by updating the CReaderProperties Add test for writing and reading encrypted Parquet files with parquet.read_table I change the name of test_encrypted_parquet_write_read_table. Updated some formatting.

moved name from single_file back to buffer_reader. updated some of the tests in test_encryption to use some utility functions to help with the repetitiveness updates to test_encryption.py to try and re-use common code through utility functions Changed the variable buffer_reader back to single_file

conbench-apache-arrow · 2024-05-09T19:41:39Z

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

None of the specified runs were found on the Conbench server.

The full Conbench report has more details.

…#39438) ### Rationale for this change Currently, if you try to read a decrypted parquet with read_table, passing decryption_properties - in the happy path (pyarrow.data available for import) the reading/decryption of the file fails, as the decryption properties are missing. ### What changes are included in this PR? Pass through the argument that was intended to have been passed. ### Are these changes tested? We have tested this locally on an encrypted parquet dataset - please advise on any further testing you would like beyond that and the standard CI. ### Are there any user-facing changes? Not in any cases where their code was previously working? The intended behaviour for encrypted dataset decryption should start working. * Closes: apache#39645 Lead-authored-by: Tom McTiernan <[email protected]> Co-authored-by: Don <[email protected]> Co-authored-by: Rok Mihevc <[email protected]> Signed-off-by: Rok Mihevc <[email protected]>

github-actions bot added Component: Python awaiting review Awaiting review labels Jan 3, 2024

tmct changed the title ~~Pass forgotten keyword argument~~ GH-39438: [${pyarrow}] ${Pass forgotten keyword argument} Jan 3, 2024

tmct changed the title ~~GH-39438: [${pyarrow}] ${Pass forgotten keyword argument}~~ GH-39438: [pyarrow] Pass forgotten keyword argument Jan 3, 2024

tmct changed the title ~~GH-39438: [pyarrow] Pass forgotten keyword argument~~ GH-39438: [Python] Pass forgotten keyword argument Jan 3, 2024

tmct changed the title ~~GH-39438: [Python] Pass forgotten keyword argument~~ MINOR: [Python] Pass forgotten keyword argument Jan 3, 2024

AlenkaF changed the title ~~MINOR: [Python] Pass forgotten keyword argument~~ GH-39645: [Python] Pass forgotten keyword argument Jan 17, 2024

tmct changed the title ~~GH-39645: [Python] Pass forgotten keyword argument~~ GH-39645: [Python] Fix read_table for encrypted parquet Jan 26, 2024

AlenkaF reviewed Jan 29, 2024

View reviewed changes

python/pyarrow/parquet/core.py Show resolved Hide resolved

github-actions bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jan 29, 2024

AlenkaF reviewed Feb 27, 2024

View reviewed changes

python/pyarrow/_dataset_parquet.pyx Outdated Show resolved Hide resolved

python/pyarrow/parquet/core.py Show resolved Hide resolved

pitrou reviewed Feb 27, 2024

View reviewed changes

wgtmac reviewed Feb 27, 2024

View reviewed changes

python/pyarrow/parquet/core.py Outdated Show resolved Hide resolved

python/pyarrow/parquet/core.py Show resolved Hide resolved

tolleybot force-pushed the patch-1 branch from 0656c92 to d713dc0 Compare March 19, 2024 17:58

tmct requested a review from assignUser as a code owner March 27, 2024 15:06

github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels May 8, 2024

rok force-pushed the patch-1 branch from d92d557 to 0d4b9a0 Compare May 8, 2024 19:19

github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels May 8, 2024

AlenkaF approved these changes May 9, 2024

View reviewed changes

rok requested a review from wgtmac May 9, 2024 13:22

wgtmac approved these changes May 9, 2024

View reviewed changes

github-actions bot added awaiting change review Awaiting change review awaiting changes Awaiting changes and removed awaiting changes Awaiting changes awaiting change review Awaiting change review labels May 9, 2024

rok force-pushed the patch-1 branch from c7447bb to 4eb807d Compare May 9, 2024 15:51

tmct and others added 3 commits May 9, 2024 20:48

Add decryption_properties to ParquetFragmentScanOptions arguments

de6887b

rok force-pushed the patch-1 branch from 4eb807d to de6887b Compare May 9, 2024 18:49

rok merged commit bd44410 into apache:main May 9, 2024
10 of 12 checks passed

rok removed the awaiting change review Awaiting change review label May 9, 2024

bkurtz mentioned this pull request Oct 30, 2024

[Python] Accessing parquet files with parquet.read_table in google cloud storage fails, but works with dataset, works in 16.1.0 fails in 17.0.0 #43574

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-39645: [Python] Fix read_table for encrypted parquet #39438

GH-39645: [Python] Fix read_table for encrypted parquet #39438

tmct commented Jan 3, 2024 •

edited by github-actions bot

Loading

github-actions bot commented Jan 3, 2024

kou commented Jan 3, 2024

raulcd commented Jan 15, 2024

AlenkaF commented Jan 15, 2024

tolleybot commented Jan 16, 2024 •

edited

Loading

github-actions bot commented Jan 17, 2024

tolleybot commented Jan 22, 2024

AlenkaF commented Jan 22, 2024

tolleybot commented Jan 26, 2024

kou commented Jan 26, 2024

github-actions bot commented Jan 26, 2024

AlenkaF commented Jan 29, 2024

tolleybot commented Feb 26, 2024

AlenkaF left a comment

pitrou Feb 27, 2024

tolleybot Apr 16, 2024

This comment was marked as outdated.

This comment was marked as outdated.

rok May 8, 2024 •

edited

Loading

rok May 8, 2024

pitrou commented Feb 27, 2024

wgtmac left a comment •

edited

Loading

rok commented May 8, 2024

AlenkaF left a comment

rok commented May 9, 2024

wgtmac left a comment

wgtmac May 9, 2024

rok May 9, 2024

rok May 9, 2024

conbench-apache-arrow bot commented May 9, 2024

	// Create the ReaderProperties object using the FileDecryptionProperties object
	auto decryption_config =
	std::make_shared<parquet::encryption::DecryptionConfiguration>();
	auto file_decryption_properties = crypto_factory_->GetFileDecryptionProperties(
	kms_connection_config_, decryption_config);
	auto reader_properties = parquet::default_reader_properties();
	reader_properties.file_decryption_properties(file_decryption_properties);

	// Read entire file as a single Arrow table
	parquet::arrow::FileReaderBuilder reader_builder;
	ASSERT_OK(reader_builder.Open(input, reader_properties));
	ASSERT_OK_AND_ASSIGN(auto arrow_reader, reader_builder.Build());
	std::shared_ptr<Table> table;
	ASSERT_OK(arrow_reader->ReadTable(&table));

	if (parquet_decrypt_config != nullptr) {
	auto file_decryption_prop =
	parquet_decrypt_config->crypto_factory->GetFileDecryptionProperties(
	*parquet_decrypt_config->kms_connection_config,
	*parquet_decrypt_config->decryption_config, path, filesystem);

GH-39645: [Python] Fix read_table for encrypted parquet #39438

GH-39645: [Python] Fix read_table for encrypted parquet #39438

Conversation

tmct commented Jan 3, 2024 • edited by github-actions bot Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

github-actions bot commented Jan 3, 2024

kou commented Jan 3, 2024

raulcd commented Jan 15, 2024

AlenkaF commented Jan 15, 2024

tolleybot commented Jan 16, 2024 • edited Loading

github-actions bot commented Jan 17, 2024

tolleybot commented Jan 22, 2024

AlenkaF commented Jan 22, 2024

tolleybot commented Jan 26, 2024

kou commented Jan 26, 2024

github-actions bot commented Jan 26, 2024

AlenkaF commented Jan 29, 2024

tolleybot commented Feb 26, 2024

AlenkaF left a comment

Choose a reason for hiding this comment

pitrou Feb 27, 2024

Choose a reason for hiding this comment

tolleybot Apr 16, 2024

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

rok May 8, 2024 • edited Loading

Choose a reason for hiding this comment

rok May 8, 2024

Choose a reason for hiding this comment

pitrou commented Feb 27, 2024

wgtmac left a comment • edited Loading

Choose a reason for hiding this comment

rok commented May 8, 2024

AlenkaF left a comment

Choose a reason for hiding this comment

rok commented May 9, 2024

wgtmac left a comment

Choose a reason for hiding this comment

wgtmac May 9, 2024

Choose a reason for hiding this comment

rok May 9, 2024

Choose a reason for hiding this comment

rok May 9, 2024

Choose a reason for hiding this comment

conbench-apache-arrow bot commented May 9, 2024

tmct commented Jan 3, 2024 •

edited by github-actions bot

Loading

tolleybot commented Jan 16, 2024 •

edited

Loading

rok May 8, 2024 •

edited

Loading

wgtmac left a comment •

edited

Loading