cuio: reduce/improve kernel parms: avro #6399
Conversation
|
Please update the changelog in order to start CI tests. View the gpuCI docs here. |
Codecov Report
@@ Coverage Diff @@
## branch-0.18 #6399 +/- ##
===============================================
+ Coverage 81.92% 83.15% +1.23%
===============================================
Files 96 94 -2
Lines 16167 14779 -1388
===============================================
- Hits 13245 12290 -955
+ Misses 2922 2489 -433
Continue to review full report at Codecov.
|
harrism
changed the title
cpp/src/io/avro/avro.cpp
Outdated
| @@ -30,7 +30,7 @@ namespace avro { | |||
| * | |||
| * @returns true if successful, false if error | |||
| */ | |||
| bool container::parse(file_metadata *md, size_t max_num_rows, size_t first_row) | |||
| bool container::parse(file_metadata &md, size_t max_num_rows, size_t first_row) | |||
There was a problem hiding this comment.
The cudf convention is to take out params by pointer so as to indicate at call site that this will be modified.
There was a problem hiding this comment.
Sounds good. I just checked to see if we can turn this in to something along the lines of std::pair<bool, file_metadata> parse(int, int);, as the invocation site suggests it can, but it requires combining the reader impl constructor+read functions to make it pretty. Saving for another time, and reverting to file_metadata *md
cpp/src/io/avro/reader_impl.cu
Outdated
| @@ -425,40 +439,26 @@ table_with_metadata reader::impl::read(avro_reader_options const &options, cudaS | |||
| dict_pos += len; | |||
There was a problem hiding this comment.
If global_dictionary.host_ptr() + dict_pos is indeed correct, then you can use the offset argument global_dictionary.host_ptr(dict_pos)
cpp/src/io/avro/reader_impl.cu
Outdated
| total_dictionary_entries * sizeof(gpu::nvstrdesc_s) + dictionary_data_size); | ||
| hostdevice_vector<gpu::nvstrdesc_s> global_dictionary(total_dictionary_entries + | ||
| dictionary_data_size); | ||
|
|
||
| if (total_dictionary_entries > 0) { | ||
| size_t dict_pos = total_dictionary_entries * sizeof(gpu::nvstrdesc_s); | ||
| for (size_t i = 0; i < column_types.size(); ++i) { |
There was a problem hiding this comment.
3 lines below, there's reinterpret_cast<gpu::nvstrdesc_s *>(global_dictionary.host_ptr()) which is redundant now.
…ce-output-params-avro-2
cwharris
changed the title
|
Move to 0.17 |
harrism
added
2 - In Progress
|
@cwharris is this PR still in progress? |
|
Closing, as @vuule has picked this up, I believe. |
|
Looks like there has been a mix-up. I haven't picked this up, it's just that some of my PRs included similar parameter changes (to other cuIO components). |
|
@vuule sorry about that. moving to 0.18 seems appropriate. |
…ce-output-params-avro-2
cwharris
changed the title
|
Will finish this PR once #7156 is in. |
|
This PR has been labeled |
|
@cwharris should we close this PR? |
|
@vuule closing. |
Contributes to #5710 and #5943