Test velox parquet using presto unit test

[qqibrow]

Overview

We’ve developed a test solution to test velox velox native parquet reader using presto unit tests. we want to discuss whether we can add that in CI in either velox or presto project.

Why

1. Test coverage in velox parquet is not enough.
2. The unit tests in presto is comprehensive, covering all data types, including complex types like struct, array, and map. Each unit test includes forward ordering, backward ordering, and randomly inserted nulls testing. To facilitate complex type testing, A custom hive parquet writer (e.g, SingleLevelArraySchemaConverter is used to surface issues. (this is the reason why #7002 was blocked at first because it's hard to reproduce)
3. Extensibility: Each unit test verifies a pair of writer and reader, such as native Parquet writer and native Parquet reader or Hive Parquet writer and native Parquet reader. The unit test is easy to extend when implementing native parquet writer in velox.
4. Better Debuggability: The unit test inputs can be examined to understand failures, and the native reader in Presto can be referred to understand the correct logic.

High level implementation

1. Implement a standalone parquet reader in velox that takes a Parquet file as input and outputs a binary file with data in SerializedPage format. Draft PR
2. In Presto unit test in AbstractParquetReader, during each test, call the Velox Parquet reader with the Parquet file input, read the generated output file, decode it back into pages, and compare the data against expected results. Draft PR

Testing result

1. The failure rate reduced significantly from 53/82 in Dec 2023 to 13/82 in Apr 2024.
2. Findings in rest of the failures:
   1. the failure from testLongXXX and testSmallIntXXX might because the implementation of schema conversion from parquet type is different. e.g, presto side code https://github.com/prestodb/presto/blob/fd4c51758ba8c120c3decb8e5c4850c98b20c9cf/presto-hive/src/main/java/com/facebook/presto/hive/parquet/ParquetPageSourceFactory.java#L389
   2. testTimestampMillisBackedByINT64 failure is supposed to be resolved by https://github.com/facebookincubator/velox/pull/4680/files
   3. the rest of failures are testSingleLevelSchemaArrayOfArrayOfStructOfArray, testSingleLevelSchemaNestedArrays and testStructOfTwoNestedSingleLevelSchemaArrays. the root cause is Backward-compatibility rule support in velox , leading to incorrect schema.

Test Location Options

1. velox CI: the test program can be a standalone java program in velox CI. This will ensure all PRs are tested before landing.
2. prestodb CI: This will enable Velox Parquet verification each time Velox is upgraded. However, developers have to address issues reactively.

Long term Solution

The long term solution for testing is to using fuzzer to test velox parquet. Using presto unit test will be an interim solution before such implementation.

[qqibrow]

@xiaoxmeng @mbasmanova @pedroerp @czentgr @majetideepak @yingsu00 @aditi-pandit @FelixYBW @hitarth @jaystarshot

[FelixYBW]

Can you list the PR and its status somewhere we can track?

We can test the Spark UT with complex datatype enabled once all current PRs are merged. Later we can test the parquet files of parquet-mr UT.

[qqibrow]

we are working on merging outstanding PRs into velox. will keep you posted. thanks!

[mbasmanova]

The long term solution for testing is to using fuzzer to test velox parquet. Using presto unit test will be an interim solution before such implementation.

@qqibrow What's blocking this solution? Why can't we implement it now?

[qqibrow]

@mbasmanova we had a sync back at veloxcon. seems no group has the bandwidth now. Also even after the fuzzer approach is implemented, we still need this method to validate.

[mbasmanova]

@qqibrow

we had a sync back at veloxcon

Would you summarize the discussion here?

Is there a GitHub issue that describes the long term solution? If not, let's create one. Do you have a rough estimate of the effort required to implement it? What's the major difficultly? What are the pros / cons of the two solutions you have in mind?

[qqibrow]

Groups from Pinterest, Uber, IBM, and Intel had an discussion about complex type support in Parquet:

1. All participants recognize the strong need for Parquet.
2. Pinterest and Uber are actively engaged in fixing the complex type in Parquet, with support from IBM in code review. Currently, we are utilizing Presto unit tests.
3. Intel will setup testing pipeline in gluten using parquet-mr unit test to test velox parquet reader.
4. no groups have the bandwidth for now to implement the fuzzer approach for testing velox parquet.

Ying has created #8103 to track the the long term solution. I don’t have a rough estimate now but it will take some time to support all features supported in presto parquet test framework. More details can be found within the issue thread.

Here are the pros and cons:

Presto Unit Test:

Pros:

1. convenient and practical
2. Guarantees the Velox Parquet reader aligns with Presto's native Parquet reader

Cons:

1. mix java & c++ env in testing.

Fuzzer Test:

Pros:

1. in c++
2. Consistency with existing Velox testing infrastructure.

Cons:

1. Implementation will take time.
2. hard to estimate the effort required to achieve parity with the Presto Parquet unit test framework.

LMKWDYT @mbasmanova @FelixYBW @yingsu00

[mbasmanova]

@qqibrow It seems to me that you are proposing an end-to-end test for Velox Parquet reader using Presto's testing framework. This seems useful and feels like it belongs to PrestoDB repo / CI. Velox still needs to have its own comprehensive testing for Parquet and it would be great to start working on that ASAP. I noticed that there are no updates on the issue Ying created 4 months ago in December.

[FelixYBW]

@mbasmanova Rui is working on a Spark Fuzzer test framework for Velox now. With it we can compare existing parquet and autogenerated parquet against with Spark.

Presto fuzzer test in Velox can use the same parquet and do the test also.

[qqibrow]

sounds good. started a discussion in #presto-cpp in prestoDB slack. Let me know whether I shall drive the discussion somewhere else.