Fix alignment of compressed blocks in ORC writer #12077

vuule · 2022-11-04T21:05:50Z

Description

Closes #11812
Fixed alignment of compressed blocks in ORC writer - impacted ZLIB compression.
Re-enabled nvCOMP DEFLATE compression in ORC - nvCOMP 2.5+ only.

Refactored the nvCOMP feature status(enabled/disabled in cuDF) checks to include reason why features are not enabled (if not enabled).
Refactored call sites to return the detailed error message if an operation fails because of nvCOMP integration config.
Refactored nvCOMP adapter macros to allow mocking of the parameters that determine if an nvCOMP feature is enabled (env var, GPU compute capability, nvCOMP version).
Added tests to verify the logic of the newly refactored feature status checks (allowed by the mocking above).
Fix a Parquet test that was calling ORC reader/writer 😬

Checklist

I am familiar with the Contributing Guidelines.
New or existing tests cover these changes.
The documentation is up to date with these changes.

…bug-nvcomp-deflate-align

codecov · 2022-11-04T23:37:46Z

Codecov Report

Base: 87.47% // Head: 88.08% // Increases project coverage by +0.60% 🎉

Coverage data is based on head (593a594) compared to base (f817d96).
Patch has no changes to coverable lines.

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

Additional details and impacted files

@@               Coverage Diff                @@
##           branch-22.12   #12077      +/-   ##
================================================
+ Coverage         87.47%   88.08%   +0.60%     
================================================
  Files               133      135       +2     
  Lines             21826    22100     +274     
================================================
+ Hits              19093    19466     +373     
+ Misses             2733     2634      -99

Impacted Files	Coverage Δ
python/cudf/cudf/core/column/interval.py	`85.45% <0.00%> (-9.10%)`	⬇️
python/cudf/cudf/io/text.py	`91.66% <0.00%> (-8.34%)`	⬇️
python/cudf/cudf/core/_base_index.py	`81.28% <0.00%> (-4.27%)`	⬇️
python/cudf/cudf/io/json.py	`92.06% <0.00%> (-2.68%)`	⬇️
python/cudf/cudf/utils/utils.py	`89.91% <0.00%> (-0.69%)`	⬇️
python/cudf/cudf/core/column/timedelta.py	`90.17% <0.00%> (-0.58%)`	⬇️
python/cudf/cudf/core/column/datetime.py	`89.21% <0.00%> (-0.51%)`	⬇️
python/cudf/cudf/core/column/column.py	`87.96% <0.00%> (-0.46%)`	⬇️
python/dask_cudf/dask_cudf/core.py	`73.72% <0.00%> (-0.41%)`	⬇️
python/cudf/cudf/io/parquet.py	`90.45% <0.00%> (-0.39%)`	⬇️
... and 41 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

…bug-nvcomp-deflate-align

vuule · 2022-11-07T23:59:25Z

cpp/src/io/orc/writer_impl.cu

+      util::round_up_unsafe<size_t>(max_compressed_block_size, compressed_block_align);
    auto const padded_block_header_size =
-      util::round_up_unsafe<size_t>(block_header_size, uncomp_block_align);
+      util::round_up_unsafe<size_t>(block_header_size, compressed_block_align);


Actual fix for the alignment. Everything else is to safely re-enable DEFLATE compression.

jbrennan333

Looks good! I just had a few minor comments.

cpp/src/io/comp/nvcomp_adapter.cpp

cpp/tests/io/comp/decomp_test.cpp

…bug-nvcomp-deflate-align

jbrennan333

+1 looks good to me

bdice

I'm glad we have tests now. Do we actually cover a good portion of the version/hardware cases in our testing (manual or CI)?

bdice · 2022-11-09T23:47:36Z

cpp/src/io/comp/nvcomp_adapter.cpp

-      return NVCOMP_HAS_ZSTD_COMP and detail::nvcomp_integration::is_stable_enabled();
-    default: return false;
+    case compression_type::DEFLATE: {
+      if (not NVCOMP_HAS_DEFLATE(


I suppose this expands to a valid runtime expression as well as being useful at compile time in #if macros? Not super familiar with using macros like this.

It is simply a compile time text substitution, so this one will translate to if (not (MAJOR > 2 or (MAJOR == 2 and MINOR >= 5)))

Not super familiar with using macros like this.

Yeah, this is tech from the dark ages, thankfully not commonly used any more. I unfortunately could not avoid it here :(

I think you have the right idea, this is pretty much a type-unsafe constexpr function that can be used in #if conditions. By the means of text substitution, as Mike said :D

bdice · 2022-11-09T23:52:26Z

cpp/src/io/comp/nvcomp_adapter.hpp

+
+  feature_status_parameters();
+  feature_status_parameters(
+    int major, int minor, int patch, bool all_enabled, bool stable_enabled, int cc)


Compute capabilities are not really integral, there are lots of values like 8.6 or 6.1. Does this get some kind of normalized integral value like 86? How does this interact with params.compute_capability == 6?

Looks like my naming is bad here.
This is just the major capability number.
cudaDeviceGetAttribute(&compute_capability, cudaDevAttrComputeCapabilityMajor, device)) returns an int so this should be fine.
I'll fix the name to avoid confusion.

Renamed to reflect that this is the major component of compute capability.

vuule · 2022-11-10T00:19:13Z

Do we actually cover a good portion of the version/hardware cases in our testing (manual or CI)?

I don't know about HW, but we definitely only cover a single nvCOMP version.

; branch 'branch-22.12' of https://github.com/rapidsai/cudf into bug-nvcomp-deflate-align

vuule · 2022-11-10T23:19:40Z

rerun tests

vuule · 2022-11-11T06:40:25Z

@gpucibot merge

vuule added 6 commits November 2, 2022 12:17

use correct compressed buffer alignment

31df6c7

Merge branch 'branch-22.12' of https://github.com/rapidsai/cudf into …

5581b80

…bug-nvcomp-deflate-align

refactor decompression support check

951c817

error strings everywhere!

80bd7f4

test fix

08fb316

Merge branch 'branch-22.12' of https://github.com/rapidsai/cudf into …

71893fe

…bug-nvcomp-deflate-align

vuule added bug Something isn't working cuIO cuIO issue non-breaking Non-breaking change labels Nov 4, 2022

vuule self-assigned this Nov 4, 2022

github-actions bot added Python Affects Python cuDF API. libcudf Affects libcudf (C++/CUDA) code. labels Nov 4, 2022

vuule added 3 commits November 4, 2022 15:00

nvcomp version mockable

aec6e0c

compute capability mockable

bdfcdb7

integration config mock; tests

3cb56b4

vuule added 4 commits November 4, 2022 16:41

test fix

063679b

remove extra line break

b1c1bdd

Merge branch 'branch-22.12' of https://github.com/rapidsai/cudf into …

6c696ce

…bug-nvcomp-deflate-align

group mockable params into a single object

20f5ed1

vuule commented Nov 7, 2022

View reviewed changes

vuule marked this pull request as ready for review November 8, 2022 00:00

vuule requested review from a team as code owners November 8, 2022 00:00

vuule requested review from bdice, brandon-b-miller and mythrocks November 8, 2022 00:00

jbrennan333 reviewed Nov 8, 2022

View reviewed changes

cpp/src/io/comp/nvcomp_adapter.cpp Outdated Show resolved Hide resolved

cpp/tests/io/comp/decomp_test.cpp Outdated Show resolved Hide resolved

cpp/tests/io/comp/decomp_test.cpp Show resolved Hide resolved

Merge branch 'branch-22.12' of https://github.com/rapidsai/cudf into …

cdfecb3

…bug-nvcomp-deflate-align

code review

44b97dc

vuule requested a review from jbrennan333 November 9, 2022 01:14

jbrennan333 approved these changes Nov 9, 2022

View reviewed changes

hyperbolic2346 approved these changes Nov 9, 2022

View reviewed changes

bdice reviewed Nov 9, 2022

View reviewed changes

vuule added 2 commits November 10, 2022 11:47

Merge branch 'bug-nvcomp-deflate-align' of https://github.com/vuule/cudf

f4f808f

; branch 'branch-22.12' of https://github.com/rapidsai/cudf into bug-nvcomp-deflate-align

cc -> cc_major rename

a6ad48b

vuule requested a review from bdice November 10, 2022 19:50

bdice approved these changes Nov 10, 2022

View reviewed changes

rapids-bot bot merged commit d335aa3 into rapidsai:branch-22.12 Nov 11, 2022

vuule deleted the bug-nvcomp-deflate-align branch November 11, 2022 06:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix alignment of compressed blocks in ORC writer #12077

Fix alignment of compressed blocks in ORC writer #12077

vuule commented Nov 4, 2022 •

edited

Loading

codecov bot commented Nov 4, 2022 •

edited

Loading

vuule Nov 7, 2022

jbrennan333 left a comment

jbrennan333 left a comment

bdice left a comment

bdice Nov 9, 2022

hyperbolic2346 Nov 10, 2022

vuule Nov 10, 2022

bdice Nov 9, 2022

vuule Nov 10, 2022

vuule Nov 10, 2022

vuule commented Nov 10, 2022

vuule commented Nov 10, 2022

vuule commented Nov 11, 2022

Fix alignment of compressed blocks in ORC writer #12077

Fix alignment of compressed blocks in ORC writer #12077

Conversation

vuule commented Nov 4, 2022 • edited Loading

Description

Checklist

codecov bot commented Nov 4, 2022 • edited Loading

Codecov Report

vuule Nov 7, 2022

Choose a reason for hiding this comment

jbrennan333 left a comment

Choose a reason for hiding this comment

jbrennan333 left a comment

Choose a reason for hiding this comment

bdice left a comment

Choose a reason for hiding this comment

bdice Nov 9, 2022

Choose a reason for hiding this comment

hyperbolic2346 Nov 10, 2022

Choose a reason for hiding this comment

vuule Nov 10, 2022

Choose a reason for hiding this comment

bdice Nov 9, 2022

Choose a reason for hiding this comment

vuule Nov 10, 2022

Choose a reason for hiding this comment

vuule Nov 10, 2022

Choose a reason for hiding this comment

vuule commented Nov 10, 2022

vuule commented Nov 10, 2022

vuule commented Nov 11, 2022

vuule commented Nov 4, 2022 •

edited

Loading

codecov bot commented Nov 4, 2022 •

edited

Loading