Adding MAP type support for ORC Reader #9132
Conversation
…orc_struct_null_issue
github-actions
bot
added
Python
rgsl888prabhu
added
4 - Needs cuIO Reviewer
cuIO
rgsl888prabhu
requested review from
vuule and
devavret
and removed request for
harrism
|
Returning info on which columns are maps might be a part of the requirements (means a switch to |
Codecov Report
@@ Coverage Diff @@
## branch-21.10 #9132 +/- ##
===============================================
Coverage ? 10.77%
===============================================
Files ? 115
Lines ? 19138
Branches ? 0
===============================================
Hits ? 2062
Misses ? 17076
Partials ? 0 Continue to review full report at Codecov.
|
| "columns", | ||
| [None, ["lvl1_map", "lvl2_struct_map"], ["lvl2_struct_map", "lvl2_map"]], | ||
| ) | ||
| @pytest.mark.parametrize("num_rows", [0, 15, 1005, 10561, 100000]) |
There was a problem hiding this comment.
100000 is enough to have multiple stripes?
There was a problem hiding this comment.
Yes, it has 98 stripes
There was a problem hiding this comment.
I hope you mean rowgroups, 98 stripes make a ~25GB file :D
There was a problem hiding this comment.
oh, it's writing with tiny stripes, now I see :D
carry on, then :)
…me and other review changes
| case orc::DECIMAL: | ||
| if (type == type_id::DECIMAL64) { | ||
| scale = -static_cast<int32_t>(_metadata->get_types()[orc_col_id].scale.value_or(0)); | ||
| } |
There was a problem hiding this comment.
Does this need to throw an exception if type == type_id::DECIMAL32?
There was a problem hiding this comment.
In case of orc:Decimal, there are chances where they expect us to return Double. And orc supports only Decimal64 and Decimal128 if I am not wrong.
|
|
||
| schema = po.Struct(**schema) | ||
|
|
||
| lvl1_map = [ |
There was a problem hiding this comment.
Perhaps you'd like to parametrize on lvl_map types. This right now has code inside and outside the test param gen that depend on each other. Instead you could have tuples of orc file buffers and expected values as separate params.
There was a problem hiding this comment.
I am trying to avoid generating the buffer multiple times since we are looking for 100000 rows, just to execution time.
| @@ -176,7 +176,8 @@ class reader::impl { | |||
| */ | |||
| column_buffer&& assemble_buffer(const int32_t orc_col_id, | |||
| std::vector<std::vector<column_buffer>>& col_buffers, | |||
| const size_t level); | |||
| const size_t level, | |||
| rmm::cuda_stream_view stream); | |||
There was a problem hiding this comment.
Should this have a default, or will we always be calling it internally with a stream?
There was a problem hiding this comment.
Yes, this is only for internal use only.
Co-authored-by: Vyas Ramasubramani <[email protected]>
|
rerun tests |
vuule
added
5 - Ready to Merge
|
@gpucibot merge |
Since cuDF still doesn't support MAP type, this will be viewed as list of structs, and struct having key and value pair as members.
#8826 Completes Reader part of MAP support.