GH-33115: [C++] Parquet Implement crc in reading and writing Page for DATA_PAGE (v1)

[mapleFU]

This patch add crc in writing and reading DATA_PAGE. And crc for dictionary, DATA_PAGE_V2 will be added in comming patches.

• Implement crc in writing DATA_PAGE
• Implement crc in reading DATA_PAGE
• Adding config for write crc page and checking
• Testing DATA_PAGE with crc, the testing maybe borrowed from parquet-mr
• Using crc library in https://issues.apache.org/jira/browse/ARROW-17904

And there is some questions, I found that in thirdparty, arrow imports crc32c, which is extracted from leveldb's crc library. But seems that our standard uses crc32, which has a different magic number. So I vendor implementions mentioned in https://issues.apache.org/jira/browse/ARROW-17904 .

The default config of enable crc in parquet-mr for writer is true, but here I use false, because set it true may slow down writer.

• Closes: [C++] Parquet support read page with crc32 checking #33115

[github-actions]

https://issues.apache.org/jira/browse/ARROW-17904

[github-actions]

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

[pitrou]

Thanks for the PR @mapleFU . I haven't looked in detail yet.

IMHO The testing approach should be to add reference files in https://github.com/apache/parquet-testing/tree/master/data .
Ideally these files should provide the given feature matrix:

• correct and incorrect CRCs
• v1 and v2 data pages
• compressed and uncompressed columns

This would probably be spread over two files, one with correct CRCs, one with incorrect CRCs.
What do you think?

[mapleFU]

I'd like to follow the testing here: https://github.com/apache/parquet-mr/blob/master/parquet-hadoop/src/test/java/org/apache/parquet/hadoop/TestDataPageV1Checksums.java
@pitrou

[pitrou]

I'd like to follow the testing here: https://github.com/apache/parquet-mr/blob/master/parquet-hadoop/src/test/java/org/apache/parquet/hadoop/TestDataPageV1Checksums.java

This actually seems more complicated to implement (you have to replicate details of the file format in the tests).
Also, without reference files we wouldn't test that different implementations actually agree on the CRC computation.

[mapleFU]

This actually seems more complicated to implement (you have to replicate details of the file format in the tests). Also, without reference files we wouldn't test that different implementations actually agree on the CRC computation.

Sure, let me generate some data files in parquet-mr following TestDataPageV1Checksums first.

[mapleFU]

@pitrou @kou Hi, so should I put crc to vendored or util?

[pitrou]

Since the code is simple and the algorithm well-know, my vote is to own it and keep it in util.

[kou]

I like cpp/src/arrow/vendored/ but it's not a strong opinion. So I'm OK with util.

[pitrou]

@mapleFU Is this still WIP?

[mapleFU]

Some comment fixed, test would be added later

[mapleFU]

I add a crc32 test which crc "123456789". But I found it hard to find some authority crc tests:

1. boost crc only crc "123456789"
2. folly crc uses boost crc to do some cross verification

So, I wonder where should I copy more crc tests? Or just test "123456789" is enough? @pitrou @kou

[pitrou]

As I said before:

Also, we need unit tests for the CRC32 functionality using well-known test vectors (or generated from a well-known implementation, such as Python's).

[pitrou]

And, yes, one or two test vectors is enough. But make sure to include bytes >= 128 (to check for signedness issues).

[mapleFU]

https://github.com/python/cpython/blob/3ceb4b8d3ad48101e67da8fb8f581d3377863bfa/Lib/test/test_binascii.py#L237-L242
Seems that it's also short, lol...

[mapleFU]

@pitrou I decide to add boost crc32 for testing, though we don't introduce it in our code, but I think using it to test is a goot idea.

[mapleFU]

C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Tools\MSVC\14.29.30133\include\random(1839): error C2338: invalid template argument for uniform_int_distribution: N4659 29.6.1.1 [rand.req.genl]/1e requires one of short, int, long, long long, unsigned short, unsigned int, unsigned long, or unsigned long long
D:/a/arrow/arrow/cpp/src/arrow/util/crc32_test.cc(55): note: see reference to class template instantiation 'std::uniform_int_distribution<uint8_t>' being compiled
C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\VC\Tools\MSVC\14.29.30133\include\random(1839): error C2338: note: char, signed char, unsigned char, char8_t, int8_t, and uint8_t are not allowed

Seems windows cannot use random uint8_t...

[mapleFU]

Oh, seems #34038 cause the error... Let's waiting it to be merged...
@pitrou @wjones127 and comments are fixed, PTAL again

[wjones127]

This looks good. We use boost for testing in other places, so I think that's reasonable.