[C++] Parquet support read page with crc32 checking

[asfimport]

Currently, C++'s Parquet support write page with checksum, but ReadPage doesn't have check any checksum. And I would like to fix it

I'd like to split this patch to different parts:

1. Implement the crc in DataPageV1, which requires a write crc config, counting crc in read mode, crc verification, migrate testing from parquet mr
2. Implement crc for DataPageV2
3. Implement crc for Dict

Reporter: Xuwei Fu / @mapleFU
Assignee: Xuwei Fu / @mapleFU

PRs and other links:

• GitHub Pull Request #14351
• GitHub Pull Request #29

_{Note: This issue was originally created as ARROW-17904. Please see the migration documentation for further details.}

[asfimport]

Antoine Pitrou / @pitrou:
That would certainly be a welcome addition!

[asfimport]

Xuwei Fu / @mapleFU:
@pitrou I'm on holidays last week. Now 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: https://github.com/apache/parquet-format#checksumming

Maybe I can use <boost/crc32.h> here?

[asfimport]

Antoine Pitrou / @pitrou:
I think we'd rather avoid a mandatory boost dependency. Surely there's a standalone CRC32 implementation somewhere under a Apache-compatible license that we can vendor?

[asfimport]

Antoine Pitrou / @pitrou:
Also cc @kou

[asfimport]

Xuwei Fu / @mapleFU:
gandiva already imports boost/crc. And I have a simple draft using it: #14351 . It's WIP because it requires some testings, and it only implement the case for DATA_PAGE v1. Mind take a look? @pitrou  

[asfimport]

Antoine Pitrou / @pitrou:
Well, the fact that Gandiva depends on Boost doesn't mean we want to depend on it for other components (many people probably don't enable Gandiva when building Arrow).

[asfimport]

Xuwei Fu / @mapleFU:
kudu/rocksdb/leveldb uses crc32c rather than standard crc32, maybe because crc32c is faster. ClickHouse even uses other checksum functions.

MySQL uses zlib's crc32 implementions ( https://github.com/mysql/mysql-server/blob/8.0/storage/innobase/ut/crc32.cc ), introducing it might need some code updates about arrow's cpuinfo.

Ganvida already includes boost/crc32, using it is quiet convinient.

Arrow includes crc32c in google, which is pasted from leveldb's crc32. But it's crc32c, which breaks the parquet standard.

I'm not familiar with other crc32 implementions, maybe there are some faster implementions.

[asfimport]

Kouhei Sutou / @kou:
It seems that there are only a few candidates...
https://github.com/search?q=crc32+simd
https://github.com/ttsuki/arkana may be usable...?

How about using zlib's crc32() as the first version?
We can replace it with optimized version in follow-up tasks.

[asfimport]

Xuwei Fu / @mapleFU:
It's ok, I'll try to use zlib. Should I extract the zlib's crc32 like mysql, or keeping ARROW_WITH_ZLIB ON when in arrow and parquet? @kou @pitrou  

[asfimport]

Kouhei Sutou / @kou:
Could you add PARQUET_REQUIRE_CRC32_CHECK or something like PARQUERT_REQUIRE_ENCRYPTION? (See also cpp/cmake_modules/DefineOptions.cmake and cpp/cmake_modules/THirdpartyToolchain.cmake.) If PARQUET_REQUIRE_CRC32_CHECK is enabled, we find zlib and enable this feature.

We don't want to bundle zlib's crc32() implementation. We use it as just a fallback implementation.

[asfimport]

Xuwei Fu / @mapleFU:
It's ok. I would using another patch to enable PARQUET_REQUIRE_CRC32_CHECK, and #14351 would be done after that patch is merged

[asfimport]

Antoine Pitrou / @pitrou:

If PARQUET_REQUIRE_CRC32_CHECK is enabled, we find zlib and enable this feature.

Isn't this overkill? We can vendor a crc32 implementation and always enable this feature. No need for zlib or an optional flag.

[asfimport]

Antoine Pitrou / @pitrou:
Or we can have Parquet always require zlib.

[asfimport]

Antoine Pitrou / @pitrou:
Also, if you're interested in performance, some numbers were published here:
https://fastmail.blog/advanced/the-search-for-a-faster-crc32/

[asfimport]

Xuwei Fu / @mapleFU:
I think parquet should always enable PARQUET_REQUIRE_CRC32_CHECK, vendoring a crc32 implemention is ok. Because crc32 is a part of standard, and just like encrptor, it should always enabled in cmake. And during using parquet, we can disable it if we want read/write to be faster

[asfimport]

Gang Wu / @wgtmac:
Vote +1 for vendoring a crc32 implementation.

Or we can do something similar to the logger implementation which uses CerrLog by default and uses a CMake option to plugin glog. We can port a simple and default crc32 implementation, and use that from zlib if ARROW_WITH_ZLIB is ON if the performance is better.

[asfimport]

Antoine Pitrou / @pitrou:
Let's keep it simple and just vendor a single "fast" implementation, such as the "slice-by-16" approach from Cyrus: https://github.com/robn/cyrus-imapd/blob/master/lib/crc32.c

[asfimport]

Kouhei Sutou / @kou:
I'm OK with the vendoring approach if we can find a fast simple CRC32 implementation.

https://github.com/robn/cyrus-imapd/blob/master/lib/crc32.c

If we use Cyrus's implementation, we should use https://github.com/cyrusimap/cyrus-imapd/blob/master/lib/crc32.c not forked one.

If we can't find a fast simple CRC32 implementation, I prefer to use (not vendor) the zlib's implementation for maintainability.

[asfimport]

Xuwei Fu / @mapleFU:
@kou @pitrou crc32 from cyrus-imapd is introduced in https://github.com/apache/arrow/pull/14351
Mind take a look?

[wjones127]

@mapleFU could you "take" this one too?

[mapleFU]

take