Implement ORC chunked reader - #15094
Merged
rapids-bot[bot] merged 356 commits intoMay 2, 2024
Merged
Conversation
ttnghia
added
feature request
ttnghia
force-pushed
the
chunked_orc_reader
branch
from
February 20, 2024 19:30
c0ed5c3 to
c75daeb
Compare
Signed-off-by: Nghia Truong <nghiat@nvidia.com>
Signed-off-by: Nghia Truong <nghiat@nvidia.com>
Signed-off-by: Nghia Truong <nghiat@nvidia.com>
Signed-off-by: Nghia Truong <nghiat@nvidia.com>
Signed-off-by: Nghia Truong <nghiat@nvidia.com>
Signed-off-by: Nghia Truong <nghiat@nvidia.com>
# Conflicts: # cpp/src/io/orc/aggregate_orc_metadata.hpp # cpp/src/io/orc/reader_impl.cu # cpp/src/io/orc/reader_impl.hpp # cpp/src/io/orc/reader_impl_preprocess.cu
Signed-off-by: Nghia Truong <nghiat@nvidia.com>
Signed-off-by: Nghia Truong <nghiat@nvidia.com>
Signed-off-by: Nghia Truong <nghiat@nvidia.com>
Signed-off-by: Nghia Truong <nghiat@nvidia.com>
Signed-off-by: Nghia Truong <nghiat@nvidia.com>
Signed-off-by: Nghia Truong <nghiat@nvidia.com>
Signed-off-by: Nghia Truong <nghiat@nvidia.com>
mythrocks
reviewed
Feb 29, 2024
mythrocks
left a comment
mythrocks
left a comment
Contributor
There was a problem hiding this comment.
Sorry, this is going to take me a little while to traverse. :]
Comment on lines
125
to
133
| // NVBENCH_BENCH_TYPES(BM_orc_read_data, | ||
| // NVBENCH_TYPE_AXES(d_type_list, | ||
| // nvbench::enum_type_list<cudf::io::io_type::DEVICE_BUFFER>)) | ||
| // .set_name("orc_read_decode") | ||
| // .set_type_axes_names({"data_type", "io"}) | ||
| // .set_min_samples(4) | ||
| // .add_int64_axis("cardinality", {0, 1000}) | ||
| // .add_int64_axis("run_length", {1, 32}); | ||
|
|
Contributor
There was a problem hiding this comment.
Do we intend to remove this, or un-comment it?
Contributor
Author
There was a problem hiding this comment.
Sorry I still need to clean up this and much more 😄
| * @brief Destructor explicitly declared to avoid inlining in header | ||
| */ | ||
| ~reader(); | ||
| virtual ~reader(); |
| * | ||
| * @param output_size_limit Limit on total number of bytes to be returned per read, | ||
| * or `0` if there is no limit | ||
| * @param data_read_limit Limit on memory usage for the purposes of decompression and processing |
Contributor
There was a problem hiding this comment.
If this is in bytes, data_read_limit_bytes might be a more descriptive name.
Contributor
Author
There was a problem hiding this comment.
The doxygen should clarify its name thus I'm inclined to keep the name shorter. I even prefer read_limit but that seems too short and less descriptive so maybe data_read_limit is better.
| * @param stream CUDA stream used for device memory operations and kernel launches | ||
| * @param mr Device memory resource to use for device memory allocation | ||
| */ | ||
| explicit chunked_reader(std::size_t output_size_limit, |
Contributor
There was a problem hiding this comment.
Silly question: Why is this ctor explicit? This isn't a single argument ctor, so I don't know if we run the risk of accidental conversion into a chunked_reader.
Contributor
There was a problem hiding this comment.
I take my question back. From scripture, this might be to prevent construction like:
chunked_reader const my_reader = { 10, 20, std::vector{...}, orc_reader_options{...} };Do I have this right?
Contributor
Author
There was a problem hiding this comment.
Exactly, the explicit keyword here is just to prevent such construction. Technically we would not have any issue with it, just the matter of code cleaness.
Signed-off-by: Nghia Truong <nghiat@nvidia.com>
Signed-off-by: Nghia Truong <nghiat@nvidia.com>
Signed-off-by: Nghia Truong <nghiat@nvidia.com>
Contributor
Author
|
/ok to test |
vuule
self-requested a review
April 26, 2024 20:27
vuule
reviewed
Apr 26, 2024
vuule
left a comment
vuule
left a comment
Collaborator
There was a problem hiding this comment.
Another set of small suggestions
Looks pretty good overall
vuule
reviewed
Apr 26, 2024
Signed-off-by: Nghia Truong <nghiat@nvidia.com>
Signed-off-by: Nghia Truong <nghiat@nvidia.com>
Signed-off-by: Nghia Truong <nghiat@nvidia.com>
Signed-off-by: Nghia Truong <nghiat@nvidia.com>
Signed-off-by: Nghia Truong <nghiat@nvidia.com>
Signed-off-by: Nghia Truong <nghiat@nvidia.com>
Signed-off-by: Nghia Truong <nghiat@nvidia.com>
Signed-off-by: Nghia Truong <nghiat@nvidia.com>
Signed-off-by: Nghia Truong <nghiat@nvidia.com>
Contributor
Author
|
/ok to test |
vuule
self-requested a review
April 30, 2024 23:25
nvdbaranec
approved these changes
May 1, 2024
vuule
reviewed
May 2, 2024
vuule
left a comment
vuule
left a comment
Collaborator
There was a problem hiding this comment.
finally finished a full pass
bunch of small questions/suggestions, nothing really blocking
Signed-off-by: Nghia Truong <nghiat@nvidia.com>
Signed-off-by: Nghia Truong <nghiat@nvidia.com>
Signed-off-by: Nghia Truong <nghiat@nvidia.com>
vuule
approved these changes
May 2, 2024
Contributor
Author
|
/ok to test |
Contributor
Author
|
/merge |
rapids-bot Bot
pushed a commit
that referenced
this pull request
May 6, 2024
This adds JNI implementation for chunked ORC reader, allowing to read ORC files by an iterative manner. Depends on: * #15094 Closes #12228. Authors: - Nghia Truong (https://github.com/ttnghia) Approvers: - Jason Lowe (https://github.com/jlowe) URL: #15446
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This implements ORC chunked reader, to support reading ORC such that:
read_chunk(), and has limited size which stays within a givenoutput_limitparameter.data_read_limitparameter, allowing to read very large ORC files without OOM.2^31rows).Depends on:
segmented_row_bit_countfor computing row sizes by segments of rows #15169hostdevice_vectorand add more APIs #15252Partially contribute to #12228.
Benchmarks
Due to some small optimizations in ORC reader, reading ORC files all-at-once (reading the entire file into just one output table) can be a little bit faster. For example, with the benchmark
orc_read_io_compression:When memory is limited, chunked read can help avoiding OOM but with some sort of performance trade-off. For example, for reading a table of size 500MB from file using 64MB output limits and 640 MB data read limit:
And if memory is too limited, chunked read with 8MB output limit/80MB data read limit: