[REVIEW] Define and implement new search APIs #3229
Conversation
…ea_move_search_part2
|
Current status: Works for lower_bound and upper_bound. All unit tests validated. Contains requires cudf::scalar. Will start experimenting with the new PR for that. |
Codecov Report
@@ Coverage Diff @@
## branch-0.11 #3229 +/- ##
==============================================
+ Coverage 87.18% 87.3% +0.12%
==============================================
Files 49 49
Lines 9220 9214 -6
==============================================
+ Hits 8038 8044 +6
+ Misses 1182 1170 -12
Continue to review full report at Codecov.
|
ChuckHastings
changed the title
1) null_precedence is now a vector - one element for each column in table
2) Use final version of scalar
3) Includes an ugly hack to make the unit tests pass,
hack will be replaced by rapidsai#3172
|
Status - everything implemented. All unit tests pass. There is a hack in place waiting for PR 3172. Specifically, this code uses cudf::experimental::fill out of 3172, and in place of that there's some ugly code that allows the unit tests to pass. Can't be merged this way. |
|
Added string support. Had to refactor some things so this PR is now no longer dependent on PR 3172. The fill call is no longer required. |
|
rerun tests |
|
Eliminate dependency on 3255. Should be ready to review independently. |
ChuckHastings
changed the title
harrism
added
3 - Ready for Review
Co-Authored-By: Mark Harris <[email protected]>
| if (col.has_nulls()) { | ||
| auto eq_op = compare_with_value<Element, true>(*d_col, element, element_is_valid, true); | ||
|
|
||
| return thrust::any_of(rmm::exec_policy(stream)->on(stream), |
There was a problem hiding this comment.
thrust::any_of is slow, don't use it: https://github.com/thrust/thrust/issues/1016
There was a problem hiding this comment.
I will address this in a new PR.
|
|
||
| __device__ bool operator()(size_type i) noexcept { | ||
| if (nullable) { | ||
| bool const col_is_null{col.nullable() and col.is_null(i)}; |
There was a problem hiding this comment.
This nullable check is redundant. It's already a non-type parameter.
There was a problem hiding this comment.
Furthermore, the is_null can be is_null_nocheck.
There was a problem hiding this comment.
I'll look at this as I rework this to use scalar_device_view. I copied this from row_equality_comparator... but row_equality_comparator had two tables, with only one column here some of the checks are redundant.
| bool element_is_valid{value.is_valid()}; | ||
| Element element; | ||
|
|
||
| populate_element(value, element); |
There was a problem hiding this comment.
This should just use a scalar_device_view.
There was a problem hiding this comment.
I started that way and it became complicated, although I now know more about how strings and scalars work. Will revisit in a new PR.
| populate_element(value, element); | ||
|
|
||
| if (col.has_nulls()) { | ||
| auto eq_op = compare_with_value<Element, true>(*d_col, element, element_is_valid, true); |
There was a problem hiding this comment.
Comparing a value in a column against a scalar will be a common operation. This should be updated to use a scalar_device_view and made into a generic comparator and put here: https://github.com/rapidsai/cudf/blob/branch-0.11/cpp/include/cudf/table/row_operators.cuh#L147
There was a problem hiding this comment.
So it has nothing to do with tables. Should it still go in table/row_operators.cuh?
Port search logic to new cudf::column.
Closes #2946