Nested struct binop comparison #9452

rwlee · 2021-10-15T18:02:48Z

Partial solution to #8964

Adding column to column nested struct comparison for binary comparison operations. The existing binops cannot currently handle structs or nested types -- this PR does not attempt to add support for list types. It also does not include scalar to column and column to scalar comparison support.

Assumptions

Code currently assumes that the nested structs have the same type structure, if this is a bad assumption they can be added with a CUDF_FAIL result.

Alternatives considered

Using existing binops for the nested child columns and then merging the results was considered and abandoned because of its performance and its handling of nulls within the child column comparisons. The child column comparisons would evaluate a comparison involving null on either side as null, causing invalid results for the struct struct(2, null) == struct(2, null) should be true and struct(2, null) == struct(2, 1) should be false. However the child comparisons for both would result in [true, null] which could not be combined unambiguously.

Design

Leverages the row_equality_comparator and row_lexicographic_comparator to compare flattened tables of the struct column. To support the output type and different operator arguments, a type dispatch and a case switch are used. I'm hopeful that there is a better way to dispatch on the operator type than the case switch that's currently used. Right now each operator is a unique function, though with some cleanup I think they can be merged.

Open questions that need to be answered

Is there a better way to dispatch on the operator type than the case switch currently being used?
Currently this sits separate from the binop operators, but can be integrated into binary_ops.cu. How should this be integrated and should the code be part of struct utilities or the existing binops code?
Should nested null equality be configurable?

Follow ons

Add column to scalar and scalar to columnar support
Add null_equal operator support
JNI support for the new functionality

codecov · 2021-10-15T19:49:53Z

Codecov Report

Merging #9452 (1dd1159) into branch-22.06 (8d861ce) will decrease coverage by 0.09%.
The diff coverage is 94.84%.

@@               Coverage Diff                @@
##           branch-22.06    #9452      +/-   ##
================================================
- Coverage         86.40%   86.31%   -0.10%     
================================================
  Files               143      144       +1     
  Lines             22448    22648     +200     
================================================
+ Hits              19396    19548     +152     
- Misses             3052     3100      +48

Impacted Files	Coverage Δ
python/cudf/cudf/core/column/string.py	`88.78% <ø> (-0.31%)`	⬇️
python/cudf/cudf/core/frame.py	`93.41% <ø> (ø)`
python/cudf/cudf/core/indexed_frame.py	`91.70% <ø> (ø)`
python/cudf/cudf/io/__init__.py	`100.00% <ø> (ø)`
python/cudf/cudf/testing/testing.py	`81.81% <50.00%> (+0.12%)`	⬆️
python/cudf/cudf/io/parquet.py	`90.83% <86.60%> (-1.87%)`	⬇️
python/cudf/cudf/core/index.py	`92.06% <88.88%> (-0.25%)`	⬇️
python/cudf/cudf/core/scalar.py	`89.01% <90.90%> (-0.31%)`	⬇️
python/cudf/cudf/core/dataframe.py	`93.78% <96.36%> (+0.08%)`	⬆️
python/cudf/cudf/__init__.py	`90.69% <100.00%> (+0.22%)`	⬆️
... and 29 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0cc29a0...1dd1159. Read the comment docs.

cpp/src/structs/utilities.hpp

cpp/src/structs/utilities.cu

cpp/include/cudf/detail/structs/utilities.hpp

cpp/src/structs/utilities.cpp

cpp/src/binaryop/compiled/binary_ops.hpp

This PR adds some struct utility functions. This change is needed for the eventual support of structs in binary operations. See also: PR #9452. Authors: - Bradley Dice (https://github.com/bdice) Approvers: - Vyas Ramasubramani (https://github.com/vyasr) - Jake Hemstad (https://github.com/jrhemstad) URL: #10776

…#10780) This PR changes the internal APIs used for binary operations to use `column_view` objects instead of `column_device_view` objects. This change is needed for the eventual support of structs in binary operations. See also: PR #9452. Authors: - Bradley Dice (https://github.com/bdice) Approvers: - Ryan Lee (https://github.com/rwlee) - Nghia Truong (https://github.com/ttnghia) - Jake Hemstad (https://github.com/jrhemstad) URL: #10780

Co-authored-by: Bradley Dice <[email protected]>

…_compare

…/row_op_split

jrhemstad · 2022-05-10T15:42:46Z

cpp/include/cudf/table/experimental/row_operators.cuh

+template <typename Comparator, weak_ordering... values>
+struct weak_ordering_comparator_impl {
+  __device__ bool operator()(size_type const& lhs, size_type const& rhs)


We can actually static_assert this requirement to be sure it doesn't happen.

Double check that I'm using the right names here.

Suggested change

template <typename Comparator, weak_ordering... values>

struct weak_ordering_comparator_impl {

__device__ bool operator()(size_type const& lhs, size_type const& rhs)

template <typename Comparator, weak_ordering... values>

struct weak_ordering_comparator_impl {

static_assert( not (sizeof...(values) == 1 and values == weak_ordering::equivalent), "weak_ordering_comparator should not be used for pure equality comparisons. The `row_equality_comparator` should be used instead");

__device__ bool operator()(size_type const& lhs, size_type const& rhs)

For reference, the compiler complains about values == weak_ordering::equivalent in the context of the static assert. This forces an expansion --> ((weak_ordering::EQUIVALENT == values) && ...), which subsequently makes the sizeof...(values) == 1 check irrelevant.

bdice · 2022-05-10T17:30:58Z

cpp/src/binaryop/compiled/equality_ops.cu

-               binary_op_device_dispatcher<ops::NotEqual>{
-                 *common_dtype, *outd, *lhsd, *rhsd, is_lhs_scalar, is_rhs_scalar});
-    }
+  if (is_struct(lhs.type()) && is_struct(rhs.type())) {


This function is about 65 lines long. Could we separate out the struct logic into another function and call it here / return early, to reduce the long if/else branches in this function body?

void dispatch_equality_op(...) { CUDF_EXPECTS(...); if (is_struct(lhs.type()) && is_struct(rhs.type())) { // Handle struct data dispatch_equality_op_structs(...); return; } // existing function body. }

bdice · 2022-05-10T17:34:58Z

cpp/src/binaryop/compiled/util.cpp

@@ -190,6 +192,26 @@ std::optional<data_type> get_common_type(data_type out, data_type lhs, data_type

 bool is_supported_operation(data_type out, data_type lhs, data_type rhs, binary_operator op)
 {
-  return double_type_dispatcher(lhs, rhs, is_supported_operation_functor{}, out, op);
+  return double_type_dispatcher(lhs, rhs, is_supported_operation_functor{}, out, op) ||
+         (is_struct(lhs) && is_struct(rhs) &&


This logic should be baked into the is_supported_operation_functor rather than treated as a special case here.

bdice · 2022-05-10T17:37:11Z

cpp/src/binaryop/compiled/util.cpp

+bool is_supported_operation(data_type out,
+                            column_view const& lhs,
+                            column_view const& rhs,
+                            binary_operator op)


I would prefer to keep only one API for is_supported_operation, rather than one for data types and one for column views. If the above function can be refactored to remove the special case for struct types, then we can replace the call to is_supported_operation(data_type out, data_type lhs, data_type rhs, binary_operator op) with that double type dispatcher call to the impl.

bdice · 2022-05-10T17:38:37Z

cpp/src/binaryop/compiled/binary_ops.cuh

@@ -284,27 +290,71 @@ void apply_binary_op(mutable_column_view& out,
                     bool is_rhs_scalar,
                     rmm::cuda_stream_view stream)
 {
-  auto common_dtype = get_common_type(out.type(), lhs.type(), rhs.type());
+  if (is_struct(lhs.type()) && is_struct(rhs.type())) {


Same as below: This function is about 65 lines long. Could we separate out the struct logic into another function and call it here / return early, to reduce the long if/else branches in this function body?

bdice · 2022-05-10T17:40:05Z

cpp/include/cudf/table/experimental/row_operators.cuh

 */
-template <typename Nullate>
+template <typename Nullate, bool NanConfig = false>


Let's try to pull out the NanConfig and related changes into a separate PR.

…t_col_compare

This PR changes the experimental `device_row_comparator` to return `weak_ordering` instead of `bool`. Originally part of PR #9452. Aids PR #10730, which builds strongly-typed two table comparators and should return a `weak_ordering`. Authors: - Ryan Lee (https://github.com/rwlee) Approvers: - Bradley Dice (https://github.com/bdice) - Nghia Truong (https://github.com/ttnghia) URL: #10793

@bdice

Further splitting up #9452 -- split off at the suggestion of @bdice Related to #10781 and #4760 -- issues and discussions related to NaN comparison behavior. Authors: - Ryan Lee (https://github.com/rwlee) - Bradley Dice (https://github.com/bdice) Approvers: - Jake Hemstad (https://github.com/jrhemstad) - Bradley Dice (https://github.com/bdice) - Nghia Truong (https://github.com/ttnghia) URL: #10870

rwlee · 2022-06-27T20:10:50Z

Closing in favor of #11153

Replaces #9452 Takes the core cudf work from 9452 and integrates it into the JNI while removing struct support integration into cudf's binary operator API. Includes scalar comparison support in addition to vector to vector comparison support. Follow on needed Add null_equal operator support Authors: - Ryan Lee (https://github.com/rwlee) - Bradley Dice (https://github.com/bdice) Approvers: - Raza Jafri (https://github.com/razajafri) - Bradley Dice (https://github.com/bdice) - Nghia Truong (https://github.com/ttnghia) URL: #11153

rwlee added 2 commits September 30, 2021 11:25

struct binop first pass

262c2a2

vector-vector nested struct comparison

a865fe0

rwlee added 2 - In Progress Currently a work in progress libcudf Affects libcudf (C++/CUDA) code. Spark Functionality that helps Spark RAPIDS labels Oct 15, 2021

github-actions bot added the CMake CMake build issue label Oct 15, 2021

rwlee added this to the List and Struct data types and operations milestone Oct 15, 2021

rwlee added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Oct 15, 2021

cleanup and simplify core code

ce4440c

jrhemstad reviewed Oct 16, 2021

View reviewed changes

cpp/src/structs/utilities.hpp Outdated Show resolved Hide resolved

jrhemstad reviewed Oct 16, 2021

View reviewed changes

cpp/src/structs/utilities.cu Outdated Show resolved Hide resolved

jrhemstad reviewed Oct 16, 2021

View reviewed changes

cpp/src/structs/utilities.cu Outdated Show resolved Hide resolved

jrhemstad reviewed Oct 16, 2021

View reviewed changes

cpp/src/structs/utilities.cu Outdated Show resolved Hide resolved

sameerz linked an issue Oct 19, 2021 that may be closed by this pull request

[FEA] Add nested struct support for comparison operations #8964

Closed

rwlee added 6 commits October 19, 2021 17:30

remove type dispatch and other code cleanup

1472c5f

move struct comparison to compiled binops code

4a31fb6

improved testing, type checks, and skipped null value calculations

ce2d727

cleanup

10c95f9

Merge branch 'branch-21.12' into rwlee/struct_col_compare

12cd09e

fix upmerge issues

b6fa590

rwlee marked this pull request as ready for review October 21, 2021 23:07

rwlee requested a review from a team as a code owner October 21, 2021 23:07

rwlee requested review from hyperbolic2346 and vuule October 21, 2021 23:07

rwlee added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Oct 21, 2021

karthikeyann requested changes Oct 22, 2021

View reviewed changes

bdice mentioned this pull request May 4, 2022

Two-table comparators with strong index types #10730

Merged

split off weak ordering row operator changes

bf1c6ee

device_row_comparator private with friend class

5d87db2

rwlee mentioned this pull request May 4, 2022

Return weak orderings from device_row_comparator. #10793

Merged

Merge remote-tracking branch 'pub/branch-22.06' into rwlee/row_op_split

fd716b9

rwlee and others added 8 commits May 6, 2022 14:36

device_less conversion to templated struct

2dd2045

fold parameter pack

7ba960e

Apply suggestions from code review

84833e7

Co-authored-by: Bradley Dice <[email protected]>

Merge remote-tracking branch 'pub/branch-22.06' into rwlee/struct_col…

a944b4f

…_compare

fix code style

4d197ea

Merge branch 'rwlee/row_op_split' of github.com:rwlee/cudf into rwlee…

08092fe

…/row_op_split

Merge branch 'rwlee/row_op_split' into rwlee/struct_col_compare

d8986c5

fix code format

548dcf1

jrhemstad reviewed May 10, 2022

View reviewed changes

bdice reviewed May 10, 2022

View reviewed changes

Merge remote-tracking branch 'upstream/branch-22.06' into rwlee/struc…

1dd1159

…t_col_compare

rwlee mentioned this pull request May 17, 2022

Configurable NaN handling in device_row_comparators #10870

Merged

revans2 mentioned this pull request May 20, 2022

[FEA] Add GpuMapConcat support for nested (array, struct, map) types. NVIDIA/spark-rapids#5559

Closed

2 tasks

bdice mentioned this pull request Jun 6, 2022

[REVIEW] Add support for Series.between #11051

Merged

rwlee mentioned this pull request Jun 25, 2022

Struct binary comparison op functionality for spark rapids #11153

Merged

rwlee closed this Jun 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nested struct binop comparison #9452

Nested struct binop comparison #9452

rwlee commented Oct 15, 2021 •

edited

Loading

codecov bot commented Oct 15, 2021 •

edited

Loading

jrhemstad May 10, 2022

rwlee May 17, 2022

bdice May 10, 2022

bdice May 10, 2022

bdice May 10, 2022

bdice May 10, 2022

bdice May 10, 2022

rwlee May 17, 2022

rwlee commented Jun 27, 2022

Nested struct binop comparison #9452

Nested struct binop comparison #9452

Conversation

rwlee commented Oct 15, 2021 • edited Loading

Assumptions

Alternatives considered

Design

Open questions that need to be answered

Follow ons

codecov bot commented Oct 15, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwlee commented Jun 27, 2022

rwlee commented Oct 15, 2021 •

edited

Loading

codecov bot commented Oct 15, 2021 •

edited

Loading