Use nvcomp's snappy compressor in ORC writer #9242
Conversation
Cmake changes (excluding changes needed in nvcomp's cmake) Replace cuIO's snappy compressor with nvcomp
…or rather than a hardcoded value
When writing statistics, there's not enough space allocated in chunk's compressed buffer. This results in the compressed buffer being written into another chunk's memory.
Codecov Report
@@ Coverage Diff @@
## branch-21.10 #9242 +/- ##
================================================
- Coverage 10.85% 10.84% -0.01%
================================================
Files 115 116 +1
Lines 19158 19171 +13
================================================
Hits 2080 2080
- Misses 17078 17091 +13
Continue to review full report at Codecov.
|
vuule
added
cuIO
| device_span<gpu_inflate_input_s> comp_in, | ||
| device_span<gpu_inflate_status_s> comp_out, |
There was a problem hiding this comment.
Isn't it mildly worse to pass a span here if you don't ever use the size? In the sense of having to hypothetically push another argument on the stack.
There was a problem hiding this comment.
As I see it, the trade-off is that with span it's clear that the parameter is an array, not a pointer to a single object. Now, whether this is a good trade-off is up for debate :D
| thrust::transform(rmm::exec_policy(stream), | ||
| compressed_bytes_written.begin(), | ||
| compressed_bytes_written.end(), | ||
| comp_out.begin(), | ||
| [] __device__(size_t size) { | ||
| gpu_inflate_status_s status{}; | ||
| status.bytes_written = size; | ||
| return status; |
There was a problem hiding this comment.
Could this be replaced with a thrust::transform_output_iterator in the call to nvcompBatchedSnappyCompressAsync, basically to safe allocating and materializing the compressed_bytes_written? [1]
[1] https://thrust.github.io/doc/classthrust_1_1transform__output__iterator.html
There was a problem hiding this comment.
nvcompBatchedSnappyCompressAsync is a C API and doesn't take iterators. 😞
There was a problem hiding this comment.
Oh, I see. Too bad! Thanks for clarifying 👍
|
@gpucibot merge |
This reverts commit 08cbbcd.
vyasr
added
4 - Needs Review
Issue #9205
depends on #9235