GH-37242: [Python][Parquet] Parquet Support write and validate Page CRC

[frazar]

Rationale for this change

The C++ Parquet API already supports enabling CRC checksum for read and write operations.

CRC checksum are optional and can detect data corruption due to, for example, file storage issues or cosmic rays.

It would then be beneficial to expose this optional functionality to the Python API too.

This PR is based on a previous PR which became stale: #37439

What changes are included in this PR?

The PyArrow interface is expanded to include a page_checksum_enabled flag.

Are these changes tested?

[ ] NOT YET!

Are there any user-facing changes?

The change is backward compatible. An additional, optional keyword argument is added to some interfaces.

Closes #37242
Supersedes #37439

• Closes: [Python][Parquet] Parquet Support write and validate CRC #37242

[mapleFU]

General LGTM

It's a pity that we don't have testing for parquet-testing ( https://github.com/apache/parquet-testing/tree/master/data ). So that it's a bit hard to verify the case the crc is corrupt. Maybe the case can be added later.

[mapleFU]

Would you mind rebase master and retrigger the CI? I don't know why continuous-integration/appveyor/pr failed :-(

@frazar

[AlenkaF]

AppVeyor failure is not connected, there is already an issue opened: #38431

[frazar]

It's a pity that we don't have testing for parquet-testing ( https://github.com/apache/parquet-testing/tree/master/data ). So that it's a bit hard to verify the case the crc is corrupt.

I'm actually working on a very dumb unit test to verify CRC checks. However, my test passes instead of failing.. Will soon push my draft unit test so that I can get some feedback on that too.

[mapleFU]

You can have a try at datapage_v1-corrupt-checksum.parquet and rle-dict-uncompressed-corrupt-checksum.parquet in parquet-testing repo. These would help.

When detect a Parquet CRC error, the underlying C++ will throw an Exception ( see https://github.com/apache/arrow/blob/main/cpp/src/parquet/column_reader.cc#L500 ).

[frazar]

I have pushed my changes for the unit test. In particular, the test test_page_checksum_verification() of "python/pyarrow/tests/parquet/test_basic.py" does not pass because reading a corrupted file does not raise an exception!

I also tried reading the following corrupted files, as suggested:

• datapage_v1-corrupt-checksum.parquet
• rle-dict-uncompressed-corrupt-checksum.parquet.

But I had the same results, no exception is raised!

Am I missing something?

[mapleFU]

But I had the same results, no exception is raised!

Have you finish it now? Let me take a look

[mapleFU]

I've open an pr to test with debug log. Let me dive into it tonight

[mapleFU]

I've add some logs in #38501

The ParquetFragmentScanOptions is built with crc, but Parquet reader didn't. Would @jorisvandenbossche mind help and take a look?

[frazar]

Added even more logs in this branch, and got a surprising result:

dataset: read_options_args: {'coerce_int96_timestamp_unit': None} scan_args {'pre_buffer': True, 'thrift_string_size_limit': None, 'thrift_container_size_limit': None, 'page_checksum_verification': True}
read_options: None
default_fragment_scan_options: None
build scanOptions with  {'pre_buffer': True, 'thrift_string_size_limit': None, 'thrift_container_size_limit': None, 'page_checksum_verification': True}
set_page_checksum_verification() called with: check_crc=true     <--- Here the C++ setter is called with argument true
page_checksum_verification_ is now: true
page_checksum_verification() called, returning: false            <--- Here the C++ getter is called, but returns false!
Open with crc: false
current page type is: 2, isset crc is: true
page_checksum_verification() called, returning: false
properties_.page_checksum_verification(): false
current_page_header_.__isset.crc: true
PageCanUseChecksum(page_type): true
page_checksum_verification() called, returning: false
current page type is: 0, isset crc is: true
page_checksum_verification() called, returning: false
properties_.page_checksum_verification(): false
current_page_header_.__isset.crc: true
PageCanUseChecksum(page_type): true
page_checksum_verification() called, returning: false

I see 2 possible explanations:

• Either something is zero-ing page_checksum_verification_ without using the setter,
• Or we are looking at methods call for two different instances

[mapleFU]

set_page_checksum_verification() called with: check_crc=true <--- Here the C++ setter is called with argument true
page_checksum_verification_ is now: true

Sorry for late reply because I'm so busy in these days. I'll try to find the reason out this weekend. I guess this is C++'s dataset's problem. Let me find it out.

[AlenkaF]

Great, thank you for the changes! I think it is looking very good, I just have two small nits. Will try to have one last detailed look again today 👍

[AlenkaF]

Thanks for the quick change! I have some suggestions about the tests, but we are close to ready 👍

[AlenkaF]

Thank you so much for quick iterations and patience with our suggestions 👍

I have re-run the integration job. The R failures do not seem to be related.

[frazar]

It seems like the "Integration / AMD64 Conda Integration Test (pull_request)" jobs is cancelled after 60m due to a timeout. Not sure why it's taking so long, it took only 35m on other PRs that were recently closed.

[AlenkaF]

There are some PRs that also have the integration job time close to 60min (57min for example) and some PRs are also timing out https://github.com/apache/arrow/actions/workflows/integration.yml?query=is%3Afailure. I do not think it is related but will run the job one more time just to see if it changes.

[mapleFU]

I've rerun the failed job. Would you mind move forward if ci passed? @AlenkaF

[AlenkaF]

@github-actions crossbow submit -g python

[github-actions]

Revision: b25c124

Submitted crossbow builds: ursacomputing/crossbow @ actions-e7d4f3b41c

Task	Status
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-python
test-debian-11-python-3
test-fedora-38-python-3
test-ubuntu-20.04-python-3
test-ubuntu-22.04-python-3

[AlenkaF]

Thank you!

[AlenkaF]

@frazar would you mind adding "take" to the issue #37242 before I merge?

[frazar]

Thank you very much for all your guidance and reviews. I felt welcomed and supported in all steps of the process.

One question: which could be the first official release of the pyarrow package including this feature? Not asking for an ETA, just a broad estimate. Thanks!!

[mapleFU]

One question: which could be the first official release of the pyarrow package including this feature? Not asking for an ETA, just a broad estimate. Thanks!!

It would be in 15.0.0 . Currently we're just released 14.0.0, 14 only accept severe bugfixes.

[mapleFU]

Thanks @frazar !

[conbench-apache-arrow]

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

There were no benchmark performance regressions. 🎉

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