Add a test for ORC write with more than one stripe #11743
Conversation
Signed-off-by: Yan Feng <[email protected]>
There was a problem hiding this comment.
Some questions
This new test function ensures that enough rows are written to generate more than one stripe during ORC write process, allowing us to catch the #11736 bug.
This bug hasn't been fixed, why does no test seem to fail after adding this test case, since it can catch this bug?
| assert_gpu_and_cpu_writes_are_equal_collect( | ||
| # Generate a large enough dataframe to produce more than one stripe(typically 64MB) | ||
| # Preferably use only one partition to avoid splitting the data | ||
| lambda spark, path: gen_df(spark, gen_list, 12800, num_slices=1).write.orc(path), |
There was a problem hiding this comment.
Question: Where does the 12800 number come from? Do we know it will be greater than 64m (orc stripe size) for all the datagens you tested?
There was a problem hiding this comment.
This number comes from my experiment.(
There was a problem hiding this comment.
In general CUDF will split the data by rows and by size.
In parquet the split rows in 20,000 but for ORC it is 1,000,000. I am not sure how 12,800 booelan values produces a more than one stripe. I would really like to understand this better because I would expect that to be no where close to the row group count we expect to cause multiple slices.
| @pytest.mark.parametrize('orc_gens', orc_write_gens_list, ids=idfn) | ||
| @pytest.mark.parametrize('orc_impl', ["native", "hive"]) | ||
| @allow_non_gpu(*non_utc_allow) | ||
| def test_write_more_than_one_stripe_round_trip(spark_tmp_path, orc_gens, orc_impl): |
There was a problem hiding this comment.
If the difference is just to generate more data than test_write_round_trip, why not just make this case generate more?
And another question is if we want to generate more than one stripe for other cases in this file or just this case.
There was a problem hiding this comment.
If the difference is just to generate more data than test_write_round_trip, why not just make this case generate more?
Well, if the new case will fail I think it makes sense to add it and keep the previous one so we can test the two behaviours at the same time.
Another option might be to add the length to the @pytest.mark.parametrize too, but not sure if this will bring some tricky if else in orc_gens to xfail the failed cases.
Signed-off-by: Yan Feng <[email protected]>
| @@ -91,6 +91,20 @@ def test_write_round_trip(spark_tmp_path, orc_gens, orc_impl): | |||
| data_path, | |||
| conf={'spark.sql.orc.impl': orc_impl, 'spark.rapids.sql.format.orc.write.enabled': True}) | |||
|
|
|||
| @pytest.mark.parametrize('orc_gen', [pytest.param(boolean_gen, marks=pytest.mark.xfail(reason='https://github.com/NVIDIA/spark-rapids/issues/11736'))], ids=idfn) | |||
There was a problem hiding this comment.
In my understanding, we also need to test other kinds of data, not just the ones that failed?
There was a problem hiding this comment.
I would like to see more data types so that we are not concerned about what other errors we might be seeing. I would also like to see parquet tests with more that one row group. I am fine if that is a follow on issue too.
There was a problem hiding this comment.
My main concern is the time this is going to take to run and the memory requirements to run it. But a lot of that is going to depend on what we learn about how to get multiple slices to happen in CUDF, and why they are acting this way.
The slowness comes down to how we validate the data. It is done by a single python thread that is comparing the CPU and GPU data row by row.
The memory issues are from the fact that we slice the GPU very small and I am concerned that if we have too much data both the GPU will run out of memory and the python process trying to do the comparison might also run into some limits on CI.
It might be better to expose the row and size limits from CUDF as configs in the plugin which we could use for testing.
| @@ -91,6 +91,20 @@ def test_write_round_trip(spark_tmp_path, orc_gens, orc_impl): | |||
| data_path, | |||
| conf={'spark.sql.orc.impl': orc_impl, 'spark.rapids.sql.format.orc.write.enabled': True}) | |||
|
|
|||
| @pytest.mark.parametrize('orc_gen', [pytest.param(boolean_gen, marks=pytest.mark.xfail(reason='https://github.com/NVIDIA/spark-rapids/issues/11736'))], ids=idfn) | |||
There was a problem hiding this comment.
I would like to see more data types so that we are not concerned about what other errors we might be seeing. I would also like to see parquet tests with more that one row group. I am fine if that is a follow on issue too.
| assert_gpu_and_cpu_writes_are_equal_collect( | ||
| # Generate a large enough dataframe to produce more than one stripe(typically 64MB) | ||
| # Preferably use only one partition to avoid splitting the data | ||
| lambda spark, path: gen_df(spark, gen_list, 12800, num_slices=1).write.orc(path), |
There was a problem hiding this comment.
In general CUDF will split the data by rows and by size.
In parquet the split rows in 20,000 but for ORC it is 1,000,000. I am not sure how 12,800 booelan values produces a more than one stripe. I would really like to understand this better because I would expect that to be no where close to the row group count we expect to cause multiple slices.
closes #11735
This new test function ensures that enough rows are written to generate more than one stripe during ORC write process, allowing us to catch the "ORC writes don't fully support Booleans with nulls" bug.