GH-39444: [C++][Parquet] Fix Segment Fault in Modular Encryption

[tolleybot]

Rationale for this change:

This pull request addresses a critical issue (GH-39444) in the C++/Python components of Parquet, specifically a segmentation fault occurring when processing encrypted datasets over 2^15 rows. The fix involves modifications in cpp/src/parquet/encryption/internal_file_decryptor.cc, particularly in InternalFileDecryptor::GetColumnDecryptor. The caching of the Decryptor object was removed to resolve the multithreading issue causing the segmentation fault and encryption failures.

What changes are included in this PR?

• Removal of Decryptor object caching in InternalFileDecryptor::GetColumnDecryptor.
• Addition of two unit tests: large_row_parquet_encrypt_test.cc for C++ and an update to test_dataset_encryption.py with test_large_row_encryption_decryption for Python.

Are these changes tested?

Yes, the unit tests (large_row_parquet_encrypt_test.cc and test_large_row_encryption_decryption in test_dataset_encryption.py) have been added to ensure the reliability and effectiveness of these changes.

Are there any user-facing changes?

No significant user-facing changes, but the update significantly improves the backend stability and reliability of Parquet file handling. Calling DecryptionKeyRetriever::GetKey could be an expensive operation potentially involving network calls to key management servers.

• Closes: #39444
• GitHub Issue: #39444

[github-actions[bot]]

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:

• Other pull requests
• Contribution Guidelines - Contributing Overview

[github-actions[bot]]

:warning: GitHub issue #39444 has been automatically assigned in GitHub to PR creator.

[tolleybot]

Looks like I am still getting a segmentation fault randomly in InternalFileDecryptor when WipeOut is called. Ill look into seeing if I can add a mutex around all_decryptors_

[tolleybot]

It looks like all but the C++ / AMD64 Windows 2019 C++17 AVX2 build was completed without issue. I am not sure what the issue is with the C++ / AMD64 Windows 2019 C++17 AVX2, build but it seems unrelated.

[wgtmac]

The C++ side looks good to me. I have also refactored the C++ tests.

Could you take a look as well? @jorisvandenbossche @westonpace @pitrou

[wgtmac]

@adamreeve @westonpace Do you want to take another pass?

[pitrou]

@github-actions crossbow submit -g python -g cpp

[github-actions[bot]]

Revision: abf8c40392d5639cb139d91bb5cc70660a8143d9

Submitted crossbow builds: ursacomputing/crossbow @ actions-e56b1c9519

Task	Status
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-conda-python-3.10
test-conda-python-3.10-cython2
test-conda-python-3.10-hdfs-2.9.2
test-conda-python-3.10-hdfs-3.2.1
test-conda-python-3.10-pandas-latest
test-conda-python-3.10-pandas-nightly
test-conda-python-3.10-spark-v3.5.0
test-conda-python-3.10-substrait
test-conda-python-3.11
test-conda-python-3.11-dask-latest
test-conda-python-3.11-dask-upstream_devel
test-conda-python-3.11-hypothesis
test-conda-python-3.11-pandas-upstream_devel
test-conda-python-3.11-spark-master
test-conda-python-3.12
test-conda-python-3.8
test-conda-python-3.8-pandas-1.0
test-conda-python-3.8-spark-v3.5.0
test-conda-python-3.9
test-conda-python-3.9-pandas-latest
test-cuda-cpp
test-cuda-python
test-debian-11-cpp-amd64
test-debian-11-cpp-i386
test-debian-11-python-3
test-fedora-38-cpp
test-fedora-38-python-3
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-bundled
test-ubuntu-20.04-cpp-minimal-with-formats
test-ubuntu-20.04-cpp-thread-sanitizer
test-ubuntu-20.04-python-3
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-no-threading
test-ubuntu-22.04-python-3

[pitrou]

Question: the caching was probably there for a reason, is it ok to remove it? Would it be beneficial to reimplement it in a thread-safe way? (the PR description mentions network requests)

[pitrou]

Hmm, I was assuming the Cython compatibility issues were fixed? @raulcd

[raulcd]

Hmm, I was assuming the Cython compatibility issues were fixed? @raulcd

That was my understanding, isn't this the fix? https://github.com/apache/arrow/issues/40386 I've added the above to 15.0.2.

[pitrou]

Ah, that's because this PR needs rebasing. @tolleybot Can you please rebase/merge on git main?

[pitrou]

@github-actions crossbow submit -g python -g cpp

[github-actions[bot]]

Revision: c8f2cc461eeb3f4172aee0e7990bbe55b338001f

Submitted crossbow builds: ursacomputing/crossbow @ actions-c569d19f76

Task	Status
test-alpine-linux-cpp
test-build-cpp-fuzz
test-conda-cpp
test-conda-cpp-valgrind
test-conda-python-3.10
test-conda-python-3.10-cython2
test-conda-python-3.10-hdfs-2.9.2
test-conda-python-3.10-hdfs-3.2.1
test-conda-python-3.10-pandas-latest
test-conda-python-3.10-pandas-nightly
test-conda-python-3.10-spark-v3.5.0
test-conda-python-3.10-substrait
test-conda-python-3.11
test-conda-python-3.11-dask-latest
test-conda-python-3.11-dask-upstream_devel
test-conda-python-3.11-hypothesis
test-conda-python-3.11-pandas-upstream_devel
test-conda-python-3.11-spark-master
test-conda-python-3.12
test-conda-python-3.8
test-conda-python-3.8-pandas-1.0
test-conda-python-3.8-spark-v3.5.0
test-conda-python-3.9
test-conda-python-3.9-pandas-latest
test-cuda-cpp
test-cuda-python
test-debian-11-cpp-amd64
test-debian-11-cpp-i386
test-debian-11-python-3-amd64
test-debian-11-python-3-i386
test-fedora-39-cpp
test-fedora-39-python-3
test-ubuntu-20.04-cpp
test-ubuntu-20.04-cpp-bundled
test-ubuntu-20.04-cpp-minimal-with-formats
test-ubuntu-20.04-cpp-thread-sanitizer
test-ubuntu-20.04-python-3
test-ubuntu-22.04-cpp
test-ubuntu-22.04-cpp-20
test-ubuntu-22.04-cpp-no-threading
test-ubuntu-22.04-python-3
test-ubuntu-24.04-cpp
test-ubuntu-24.04-cpp-gcc-14

[pitrou]

@raulcd Not a blocker, but if you ever need to do a RC3, this would be a good candidate for inclusion :-)

[conbench-apache-arrow[bot]]

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

There were 8 benchmark results indicating a performance regression:

• Commit Run on ursa-i9-9960x at 2024-03-13 20:30:16Z
  • tpch (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-20, scale_factor=1
  • tpch (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-06, scale_factor=1
• and 6 more (see the report linked below)

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