Configurable NaN handling in device_row_comparators #10870
Conversation
rwlee
added
3 - Ready for Review
Codecov Report
@@ Coverage Diff @@
## branch-22.08 #10870 +/- ##
===============================================
Coverage ? 86.33%
===============================================
Files ? 144
Lines ? 22706
Branches ? 0
===============================================
Hits ? 19603
Misses ? 3103
Partials ? 0 Continue to review full report at Codecov.
|
Added the rename to |
Maybe you had java code modified before (?) 😃 |
I’m taking a final pass of review first thing this morning and will merge after. |
There was a problem hiding this comment.
Great work. I have a final pass of comments attached. I'm going to work on applying my own suggestions so that CI can run and pass. At some later point I'd like to overhaul this file to make naming conventions match. We have mismatched names across classes like lhs vs. _lhs for members and check_nulls vs. nullate for Nullate types. There's also some inconsistency about whether check_nulls goes before or after the lhs, rhs parameters.
| cudf::data_type(cudf::type_id::INT8), input_table_1.num_rows(), cudf::mask_state::UNALLOCATED); | ||
| fixed_width_column_wrapper<int8_t> expected{{1, 1, 0, 1}}; | ||
|
|
||
| row_comparison(input_table_1, |
There was a problem hiding this comment.
These tests would be a lot cleaner if the row_comparison returned a std::unique_ptr<column> (like most cudf algorithms) of bool dtype instead of accepting/mutating a mutable view. Move the make_numeric_column logic into the algorithm implementation rather than creating the object in each test.
Result would look like:
auto const got = row_comparison(input_table_1, input_table_2, column_order, lexicographic::physical_element_comparator{});There was a problem hiding this comment.
As a note, most of the functions I can think of that accept/mutate mutable views are doing so because they're a direct wrapper around a Thrust or CUB call that takes raw iterators, and they usually live in a detail namespace. User-facing algorithms generally allocate and return owning objects like std::unique_ptr<column>.
There was a problem hiding this comment.
I overhauled this file in 3a7f118. I need to make sure it compiles and will push again soon.
This reverts commit a139131.
|
@gpucibot merge |
Further splitting up #9452 -- split off at the suggestion of @bdice
Related to #10781 and #4760 -- issues and discussions related to NaN comparison behavior.