Fix STRUCT comparison between Pandas and Spark dataframes in fastparquet tests #9418

res-life · 2023-10-11T10:02:30Z

closes #9399

Add converter from fastparquet result to cpu type result
The fastparquet reading parquet result is flatted, and is not the same as CPU type result.
e.g.: fastparquet Row(a.b1 = 1, a.b2 = 2) vs CPU type Row(a = Row(b1 = 1, b2 = 2))
Modify assert_gpu_and_cpu_are_equal_collect to add a converter function parameter.
update the test case in Add tests to check compatibility with fastparquet #9366

Signed-off-by: Chong Gao <res_life@163.com>

revans2

Looks good to me.

integration_tests/src/main/python/asserts.py

integration_tests/src/main/python/fastparquet_utils.py

mythrocks · 2023-10-11T21:40:52Z

Thanks for working on this, @res-life. I only had a couple of minor questions.

integration_tests/src/main/python/asserts.py

winningsix · 2023-10-11T22:48:54Z

integration_tests/src/main/python/asserts.py

+        (from_cpu, from_gpu) = result_converter_func_before_compare(from_cpu, from_gpu)
+        # after the above conversion, the result is changed,
+        # so should sort result locally before compare, add @ignore_order(local=True) to the test case
+        assert should_sort_locally()


nit: we can add some assert message e.g. assert should_sort_locally() "local sort needed since the result is changed"

# after the above conversion, the result is changed,

This is true for the fastparquet case here, but may be false for some cases. We'd better not enforce the local sort. Callers should add it when they know it is necessary.

integration_tests/src/main/python/fastparquet_utils.py

integration_tests/src/main/python/asserts.py

firestarman · 2023-10-12T01:42:06Z

integration_tests/src/main/python/asserts.py

+        (from_cpu, from_gpu) = result_converter_func_before_compare(from_cpu, from_gpu)
+        # after the above conversion, the result is changed,
+        # so should sort result locally before compare, add @ignore_order(local=True) to the test case
+        assert should_sort_locally()


# after the above conversion, the result is changed,

This is true for the fastparquet case here, but may be false for some cases. We'd better not enforce the local sort. Callers should add it when they know it is necessary.

integration_tests/src/main/python/asserts.py

integration_tests/src/main/python/fastparquet_utils.py

res-life · 2023-10-12T02:19:36Z

This is true for the fastparquet case here, but may be false for some cases. We'd better not enforce the local sort. Callers should add it when they know it is necessary.

The assert will help developer fast find the failure reason.
If developer use @ignore_order(local=False) and passed in the result convert func, the result will be change, the previous sort no longer take effect, so need a new sort.

firestarman · 2023-10-12T02:35:18Z

This is true for the fastparquet case here, but may be false for some cases. We'd better not enforce the local sort. Callers should add it when they know it is necessary.

The assert will help developer fast find the failure reason. If developer use @ignore_order(local=False) and passed in the result convert func, the result will be change, the previous sort no longer take effect, so need a new sort.

If users need the non-local sort, then it will conflict with the requirement of local sort. If local sort is always needed when this conversion func is specfied, we may need to forcely do the local sort instead of an assertion. And I am not sure, will the order of the output always be changed when giving a conversion func ? This is confusing me and i think it depends on the implementation of this func.

res-life · 2023-10-12T10:11:22Z

New changes:

Now flatten the GPU result like fastparquet does to compare, in this way it's more simple and reliable.

res-life · 2023-10-12T10:58:31Z

If users need the non-local sort, then it will conflict with the requirement of local sort. If local sort is always needed when this conversion func is specfied, we may need to forcely do the local sort instead of an assertion. And I am not sure, will the order of the output always be changed when giving a conversion func ? This is confusing me and i think it depends on the implementation of this func.

I tested, first sort on Spark, then convert(do not change order), then sort on local, and the order does not change for the result, so I removed the sort requirement. It's depending caller to sort the data or not.

firestarman · 2023-10-13T01:33:44Z

Looks good to me

winningsix

LGTM except 1 last nit.

winningsix · 2023-10-13T03:12:13Z

integration_tests/src/main/python/fastparquet_utils.py

+
+from pyspark.sql import Row
+
+def get_fastparquet_result_converter():


The name may be misleading. It's not returning the convertor but actual the converted result.

Not sure how we plan to use this. Probably you was meaning get_fastparquet_canonicalized_result.

Yes, we plan to use this. The functions with "_" as the prefix are internal functions. This retuns a function and will be used like cpu,gpu = get_fastparquet_result_converter()(cpu, gpu). So will change the name to "get_fastparquet_result_canonicalizer".

res-life · 2023-10-25T04:04:49Z

build

mythrocks

LGTM. Shall we merge this?

res-life · 2023-10-26T01:47:43Z

Test passed on my local Spark311,320,330,340,350, so I'll merge it.
Thanks all for your review.

Add converter from fastparquet result to cpu type result

995f4ac

Signed-off-by: Chong Gao <res_life@163.com>

res-life requested review from winningsix and mythrocks October 11, 2023 10:02

revans2 reviewed Oct 11, 2023

View reviewed changes

mythrocks reviewed Oct 11, 2023

View reviewed changes

integration_tests/src/main/python/asserts.py Outdated Show resolved Hide resolved

mythrocks reviewed Oct 11, 2023

View reviewed changes

integration_tests/src/main/python/fastparquet_utils.py Outdated Show resolved Hide resolved

winningsix reviewed Oct 11, 2023

View reviewed changes

firestarman reviewed Oct 12, 2023

View reviewed changes

Fix

f42533b

Chong Gao added 2 commits October 12, 2023 17:31

change to flatten gpu result

9da9e69

Resort the result if specified an canonicalize func

2dadf7e

Remove the sort

b4836e8

mythrocks previously approved these changes Oct 12, 2023

View reviewed changes

winningsix reviewed Oct 13, 2023

View reviewed changes

Rename function name

3769c6f

res-life dismissed mythrocks’s stale review via 3769c6f October 13, 2023 05:17

winningsix previously approved these changes Oct 15, 2023

View reviewed changes

sameerz added the test Only impacts tests label Oct 16, 2023

Chong Gao added 2 commits October 25, 2023 11:44

Merge branch 'branch-23.12' into fastparquet-converter

788b7b5

Enable xfail case

9f1292e

res-life dismissed winningsix’s stale review via 9f1292e October 25, 2023 04:03

res-life changed the title ~~[WIP] Fix STRUCT comparison between Pandas and Spark dataframes in fastparquet tests~~ Fix STRUCT comparison between Pandas and Spark dataframes in fastparquet tests Oct 25, 2023

res-life marked this pull request as ready for review October 25, 2023 07:04

mythrocks approved these changes Oct 25, 2023

View reviewed changes

res-life merged commit e815dcf into NVIDIA:branch-23.12 Oct 26, 2023
29 of 30 checks passed

res-life deleted the fastparquet-converter branch October 26, 2023 01:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix STRUCT comparison between Pandas and Spark dataframes in fastparquet tests #9418

Fix STRUCT comparison between Pandas and Spark dataframes in fastparquet tests #9418

res-life commented Oct 11, 2023 •

edited

Loading

revans2 left a comment

mythrocks commented Oct 11, 2023

winningsix Oct 11, 2023

firestarman Oct 12, 2023

firestarman Oct 12, 2023

res-life commented Oct 12, 2023

firestarman commented Oct 12, 2023 •

edited

Loading

res-life commented Oct 12, 2023 •

edited

Loading

res-life commented Oct 12, 2023

firestarman commented Oct 13, 2023

winningsix left a comment

winningsix Oct 13, 2023

res-life Oct 13, 2023

res-life commented Oct 25, 2023

mythrocks left a comment

res-life commented Oct 26, 2023


		from pyspark.sql import Row

		def get_fastparquet_result_converter():

Fix STRUCT comparison between Pandas and Spark dataframes in fastparquet tests #9418

Fix STRUCT comparison between Pandas and Spark dataframes in fastparquet tests #9418

Conversation

res-life commented Oct 11, 2023 • edited Loading

revans2 left a comment

Choose a reason for hiding this comment

mythrocks commented Oct 11, 2023

winningsix Oct 11, 2023

Choose a reason for hiding this comment

firestarman Oct 12, 2023

Choose a reason for hiding this comment

firestarman Oct 12, 2023

Choose a reason for hiding this comment

res-life commented Oct 12, 2023

firestarman commented Oct 12, 2023 • edited Loading

res-life commented Oct 12, 2023 • edited Loading

res-life commented Oct 12, 2023

firestarman commented Oct 13, 2023

winningsix left a comment

Choose a reason for hiding this comment

winningsix Oct 13, 2023

Choose a reason for hiding this comment

res-life Oct 13, 2023

Choose a reason for hiding this comment

res-life commented Oct 25, 2023

mythrocks left a comment

Choose a reason for hiding this comment

res-life commented Oct 26, 2023

res-life commented Oct 11, 2023 •

edited

Loading

firestarman commented Oct 12, 2023 •

edited

Loading

res-life commented Oct 12, 2023 •

edited

Loading