[REVIEW] Raise temporary error for decimal128 types in parquet reader

[galipremsagar]

This PR adds a decimal128 type validation in parquet reader. This is put in-place to unblock libcudf changes: #9765 and this validation will soon be removed once python side of decimal128 changes are merged(blocked by libcudf from_arrow bug).

[codecov]

Codecov Report

Merging #9804 (6014e1b) into branch-22.02 (967a333) will decrease coverage by 0.05%.
The diff coverage is 5.24%.

@@               Coverage Diff                @@
##           branch-22.02    #9804      +/-   ##
================================================
- Coverage         10.49%   10.43%   -0.06%     
================================================
  Files               119      119              
  Lines             20305    20447     +142     
================================================
+ Hits               2130     2133       +3     
- Misses            18175    18314     +139     

Impacted Files	Coverage Δ
python/cudf/cudf/__init__.py	0.00% <0.00%> (ø)
python/cudf/cudf/core/_base_index.py	0.00% <0.00%> (ø)
python/cudf/cudf/core/column/column.py	0.00% <0.00%> (ø)
python/cudf/cudf/core/column/string.py	0.00% <ø> (ø)
python/cudf/cudf/core/frame.py	0.00% <0.00%> (ø)
python/cudf/cudf/core/groupby/groupby.py	0.00% <0.00%> (ø)
python/cudf/cudf/core/index.py	0.00% <ø> (ø)
python/cudf/cudf/core/indexed_frame.py	0.00% <0.00%> (ø)
python/cudf/cudf/core/multiindex.py	0.00% <0.00%> (ø)
python/cudf/cudf/io/json.py	0.00% <ø> (ø)
... and 11 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8002cbd...6014e1b. Read the comment docs.

[vuule]

Looks good, thanks for taking care of this!

[vyasr]

Do we know anything about the performance implications of this? I don't know how expensive functions like to_arrow_schema and pq.read_metadata are, but it does seem like significant error-checking work to be done up front. Would it be possible to try reading the file, catching whatever error we get back from that, and then checking for 128-bit columns after the fact?

[galipremsagar]

Do we know anything about the performance implications of this? I don't know how expensive functions like to_arrow_schema and pq.read_metadata are, but it does seem like significant error-checking work to be done up front. Would it be possible to try reading the file, catching whatever error we get back from that, and then checking for 128-bit columns after the fact?

Here is a benchmark of reading 1000 columns dataframe:

In [3]: df = cudf.DataFrame(columns=range(1, 1000))

In [4]: df
Out[4]: d
Empty DataFrame
Columns: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 980, 981, 982, 983, 984, 985, 986, 987, 988, 989, 990, 991, 992, 993, 994, 995, 996, 997, 998, 999]
Index: []

In [5]: df[1] = ['a', 'b', 'c']

In [6]: df
Out[6]: 
  1     2     3     4     5     6     7     8     9     10    11    12    13    14    15   ...   985   986   987   988   989   990   991   992   993   994   995   996   997   998   999
0   a  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  ...  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>
1   b  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  ...  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>
2   c  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  ...  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>  <NA>

[3 rows x 999 columns]

In [10]: df.columns = df.columns.astype('str')

In [11]: df.to_parquet('long.parquet')

# Before
In [12]: %timeit cudf.read_parquet("long.parquet")
151 ms ± 1.61 ms per loop (mean ± std. dev. of 7 runs, 10 loops each)

# This PR
In [3]: %timeit cudf.read_parquet("long.parquet")
171 ms ± 4.53 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

I thought it was OK to introduce this overhead since this is a temporary one and will soon be gone once python support is enabled for Decimal128Column. But feel free to let me know your thoughts on this as I could be wrong too.

[vyasr]

I thought it was OK to introduce this overhead since this is a temporary one and will soon be gone once python support is enabled for Decimal128Column. But feel free to let me know your thoughts on this as I could be wrong too.

Thanks for the context, I missed that. A 10-15% performance regression is pretty undesirable, but not as big a deal if it's temporary. I think that as long as we anticipate the Decimal128 issues being resolved in this release it should be fine. I wouldn't want to release 22.02 with this performance regression in it though. Do you or @vuule know what the timeline is?

[galipremsagar]

I wouldn't want to release 22.02 with this performance regression in it though. Do you or @vuule know what the timeline is?

Yup, it is being targeted for 22.02

[vuule]

A 10-15% performance regression is pretty undesirable, but not as big a deal if it's temporary

IMO it's fine, as this is 15% with an empty dataframe, and the overhead does not increase with the number of rows. So it should be negligible in most cases.

[vyasr]

A 10-15% performance regression is pretty undesirable, but not as big a deal if it's temporary
IMO it's fine, as this is 15% with an empty dataframe, and the overhead does not increase with the number of rows. So it should be negligible in most cases.

It's not actually empty, right? @galipremsagar added enough data that it will actually write out a meaningful file with many columns, so it's not a trivial read. If the overhead doesn't scale with the number of rows then I agree that it should become negligible for a reasonable size though.

In any case, as long as we're targeting this for 22.02 the discussion is moot and I'm fine with this solution.

[galipremsagar]

@gpucibot merge