Workaround for nvcomp zstd overwriting blocks for orc due to underestimate of sizes by jbrennan333 · Pull Request #11288 · NVIDIA/cudf

jbrennan333 · 2022-07-18T16:52:56Z

This is a possible workaround for issue #11280. We have a goal to support NVCOMP ZSTD in 22.08, so a short-term fix is desired.

There is a heuristic in gpuParseCompressedStripeData to estimate the size of the decompress buffer for very small compressed blocks. For ZSTD, it is possible to have a high enough compression ratio that this heuristic underestimates the needed decompress size.

This pr adds a boolean parameter to allow us to disable the block size estimate for ZSTD. When the estimate is disabled, it falls back to the maximum block size, which is guaranteed to be big enough.

cc: @devavret, @vuule

…imate of sizes

jbrennan333 · 2022-07-18T18:20:21Z

I think I might put up a less hacky alternative that just adds a boolean allow_block_size_estimate parameter to ParseCompressedStripeData. I can set it to true for all but ZSTD.

codecov · 2022-07-18T18:47:15Z

Codecov Report

Merging #11288 (e9a9688) into branch-22.08 (edc5062) will increase coverage by 0.06%.
The diff coverage is 100.00%.

❗ Current head e9a9688 differs from pull request most recent head f496f8d. Consider uploading reports for the commit f496f8d to get more accurate results

@@               Coverage Diff                @@
##           branch-22.08   #11288      +/-   ##
================================================
+ Coverage         86.34%   86.40%   +0.06%     
================================================
  Files               144      143       -1     
  Lines             22826    22753      -73     
================================================
- Hits              19708    19659      -49     
+ Misses             3118     3094      -24

Impacted Files	Coverage Δ
python/cudf/cudf/_lib/__init__.py	`100.00% <ø> (ø)`
python/cudf/cudf/core/column/column.py	`89.77% <100.00%> (+0.07%)`	⬆️
python/cudf/cudf/io/parquet.py	`90.93% <100.00%> (+0.09%)`	⬆️
python/cudf/cudf/core/dataframe.py	`93.57% <0.00%> (+0.04%)`	⬆️
python/cudf/cudf/core/column/string.py	`88.80% <0.00%> (+0.12%)`	⬆️
python/cudf/cudf/core/groupby/groupby.py	`91.02% <0.00%> (+0.21%)`	⬆️
python/cudf/cudf/core/column/numerical.py	`96.19% <0.00%> (+0.29%)`	⬆️
python/cudf/cudf/core/tools/datetimes.py	`84.49% <0.00%> (+0.30%)`	⬆️
python/cudf/cudf/core/column/lists.py	`91.70% <0.00%> (+0.97%)`	⬆️

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 edc5062...f496f8d. Read the comment docs.

…sion_ratio

jbrennan333 · 2022-07-18T21:30:58Z

The build error looks like it is unrelated to this change. I think it is related to #11025.

15:53:20 [13/229] Building CXX object cudf/_lib/CMakeFiles/binaryop.dir/binaryop.cxx.o
15:53:20 FAILED: cudf/_lib/CMakeFiles/binaryop.dir/binaryop.cxx.o 
15:53:20 $BUILD_PREFIX/bin/aarch64-conda-linux-gnu-c++ -DCUDF_VERSION=22.08.00 -DJITIFY_USE_CACHE -DSPDLOG_ACTIVE_LEVEL=SPDLOG_LEVEL_INFO -DTHRUST_DEVICE_SYSTEM=THRUST_DEVICE_SYSTEM_CUDA -DTHRUST_HOST_SYSTEM=THRUST_HOST_SYSTEM_CPP -Dbinaryop_EXPORTS -I$PREFIX/include/python3.8 -I$PREFIX/include/rapids/libcudacxx -I$PREFIX/include/libcudf/Thrust -I$PREFIX/include/libcudf/Thrust/dependencies -fvisibility-inlines-hidden -std=c++17 -fmessage-length=0 -ftree-vectorize -fPIC -fstack-protector-strong -fno-plt -O3 -pipe -isystem $PREFIX/include -fdebug-prefix-map=$SRC_DIR=/usr/local/src/conda/cudf-22.08.00a -fdebug-prefix-map=$PREFIX=/usr/local/src/conda-prefix -fvisibility-inlines-hidden -std=c++17 -fmessage-length=0 -ftree-vectorize -fPIC -fstack-protector-strong -fno-plt -O3 -pipe -isystem /opt/conda/envs/rapids/include -isystem /usr/local/cuda/include -O3 -DNDEBUG -fPIC -MD -MT cudf/_lib/CMakeFiles/binaryop.dir/binaryop.cxx.o -MF cudf/_lib/CMakeFiles/binaryop.dir/binaryop.cxx.o.d -o cudf/_lib/CMakeFiles/binaryop.dir/binaryop.cxx.o -c $SRC_DIR/python/cudf/_skbuild/linux-aarch64-3.8/cmake-build/cudf/_lib/binaryop.cxx
15:53:20 $SRC_DIR/python/cudf/_skbuild/linux-aarch64-3.8/cmake-build/cudf/_lib/binaryop.cxx: In function 'int __pyx_pymod_exec_binaryop(PyObject*)':
15:53:20 $SRC_DIR/python/cudf/_skbuild/linux-aarch64-3.8/cmake-build/cudf/_lib/binaryop.cxx:23027:129: error: 'INT_POW' is not a member of 'cudf::binary_operator'
15:53:20 23027 |   __pyx_t_4 = __Pyx_PyInt_From_int32_t(((__pyx_t_4cudf_4_lib_8binaryop_underlying_type_t_binary_operator)cudf::binary_operator::INT_POW)); if (unlikely(!__pyx_t_4)) __PYX_ERR(0, 60, __pyx_L1_error)
15:53:20       |                                                                                                                                 ^~~~~~~

jbrennan333 · 2022-07-18T21:47:04Z

rerun tests

devavret

Looks good. Just needs a test

jbrennan333 · 2022-07-20T17:19:14Z

@devavret I have added a test and verified it fails without this fix and passes with it.

One issue with the current test is that it assumes ZSTD compression is supported for ORC.
All of the checks for ZSTD support are in nvcomp_adapter.cpp and they are not currently exposed.
How do you recommend I check for ZSTD support in the test?

Currently, it will fail if you build with a version of nvcomp that does not support ZSTD. Maybe that is acceptable for a test?

I tried it by compiling with nvcomp-2.2, and it failed with this:

/home/jimb/dev/rapids/cudf/cpp/tests/io/orc_test.cpp:1121: Failure
Failed
statement:cudf_io::read_orc(in_opts)
reason: cuDF failure at: /home/jimb/dev/rapids/cudf/cpp/src/io/comp/nvcomp_adapter.cpp:163: nvCOMP 2.3 or newer is required for Zstandard compression

jbrennan333 · 2022-07-20T21:20:56Z

rerun tests

jbrennan333 · 2022-07-20T21:55:35Z

rerun tests

devavret · 2022-07-21T11:33:50Z

Seems like we want the NVCOMP_HAS_ZSTD macro or a function that can return a bool in a header. I'd like @vuule's opinion on this.
If we had one of those, we could GTEST_SKIP() the test.

jbrennan333 · 2022-07-21T14:27:14Z

Another alternative is to replicate this code from nvcomp_adapter.cpp in orc_test.cpp:

#define NVCOMP_ZSTD_HEADER <nvcomp/zstd.h>
#if __has_include(NVCOMP_ZSTD_HEADER)
#include NVCOMP_ZSTD_HEADER
#define NVCOMP_HAS_ZSTD 1
#else
#define NVCOMP_HAS_ZSTD 0
#endif

Let me know what you guys prefer.
Note that the build failures appear to be unrelated to my change. I'm going to hold off rerunning tests until we resolve how we want to handle this.

jbrennan333 · 2022-07-21T18:15:01Z

rerun tests

vuule

Looks good, just a few suggestions.

vuule · 2022-07-21T21:44:25Z

Another alternative is to replicate this code from nvcomp_adapter.cpp in orc_test.cpp:
#define NVCOMP_ZSTD_HEADER <nvcomp/zstd.h>
#if __has_include(NVCOMP_ZSTD_HEADER)
#include NVCOMP_ZSTD_HEADER
#define NVCOMP_HAS_ZSTD 1
#else
#define NVCOMP_HAS_ZSTD 0
#endif
Let me know what you guys prefer. Note that the build failures appear to be unrelated to my change. I'm going to hold off rerunning tests until we resolve how we want to handle this.

I slightly prefer moving NVCOMP_HAS_ZSTD and NVCOMP_HAS_DEFLATE to nvcomp_adapter.hpp. Not perfect because no namespace, but it's not a practical concern IMO.
Longer term we should add a new exception type for unsupported compression; then the test can catch or check the result.

vuule

Changed my mind about the macros. This looks good 👍

jbrennan333 · 2022-07-21T22:23:04Z

Thanks for the review @vuule! I was in the process of putting up a change where I just added the include check to orc_test.cpp while you were reviewing, so I didn't see this comment. I can definitely change it to move the defines if you prefer that.

jbrennan333 · 2022-07-21T22:24:10Z

I did it again - comments crossing. Thanks for the reviews @vuule and @devavret!

vyasr

LGTM, thanks

vyasr · 2022-07-22T16:56:08Z

@gpucibot merge

Stale review

vyasr · 2022-07-22T16:56:57Z

@gpucibot merge

Workaround for nvcomp zstd overwriting blocks for orc due to underest…

99fcbf0

…imate of sizes

jbrennan333 added bug Something isn't working 4 - Needs cuIO Reviewer non-breaking Non-breaking change labels Jul 18, 2022

jbrennan333 self-assigned this Jul 18, 2022

github-actions Bot added the libcudf Affects libcudf (C++/CUDA) code. label Jul 18, 2022

Change workaround to use a new bool parameter instead of log2_compres…

b29989e

…sion_ratio

jbrennan333 marked this pull request as ready for review July 19, 2022 14:06

jbrennan333 requested a review from a team as a code owner July 19, 2022 14:06

jbrennan333 requested review from devavret and trxcllnt July 19, 2022 14:06

devavret previously requested changes Jul 20, 2022

View reviewed changes

Add unit test

4d528f4

Add additional verification to test

ef3eb36

Merge branch 'branch-22.08' into jtb-cudf-orc-workaround

e1dfa47

jbrennan333 requested a review from vuule July 21, 2022 14:34

vuule reviewed Jul 21, 2022

View reviewed changes

Comment thread cpp/src/io/orc/orc_gpu.hpp Outdated

Comment thread cpp/src/io/orc/reader_impl.cu Outdated

Comment thread cpp/tests/io/orc_test.cpp

Add check for zstd header to orc_test

842b7ce

minor fixes from review

f496f8d

vuule approved these changes Jul 21, 2022

View reviewed changes

vyasr approved these changes Jul 22, 2022

View reviewed changes

rapids-bot Bot merged commit 3f7bb6b into NVIDIA:branch-22.08 Jul 22, 2022

vyasr mentioned this pull request Jul 22, 2022

[BUG] Decompressing an orc file with ZSTD can fail due to an inaccurate uncompress block size #11280

Closed

vuule mentioned this pull request Oct 21, 2022

Remove uncompressed block size estimate from ORC reader #11964

Closed

3 tasks

vyasr added 4 - Needs Review Waiting for reviewer to review or respond and removed 4 - Needs cuIO Reviewer labels Feb 23, 2024

Uh oh!

Conversation

jbrennan333 commented Jul 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jbrennan333 commented Jul 18, 2022

Uh oh!

codecov Bot commented Jul 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jbrennan333 commented Jul 18, 2022

Uh oh!

jbrennan333 commented Jul 18, 2022

Uh oh!

devavret left a comment

Choose a reason for hiding this comment

Uh oh!

jbrennan333 commented Jul 20, 2022

Uh oh!

jbrennan333 commented Jul 20, 2022

Uh oh!

jbrennan333 commented Jul 20, 2022

Uh oh!

devavret commented Jul 21, 2022

Uh oh!

jbrennan333 commented Jul 21, 2022

Uh oh!

jbrennan333 commented Jul 21, 2022

Uh oh!

vuule left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vuule commented Jul 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vuule left a comment

Choose a reason for hiding this comment

Uh oh!

jbrennan333 commented Jul 21, 2022

Uh oh!

jbrennan333 commented Jul 21, 2022

Uh oh!

vyasr left a comment

Choose a reason for hiding this comment

Uh oh!

vyasr commented Jul 22, 2022

Uh oh!

vyasr commented Jul 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jbrennan333 commented Jul 18, 2022 •

edited

Loading

codecov Bot commented Jul 18, 2022 •

edited

Loading

vuule commented Jul 21, 2022 •

edited

Loading