[REVIEW] Adding floating point specialization to comparators for NaNs

[rgsl888prabhu]

Added floating point specialization to comparators for NaNs and test cases to evaluate.

close #3226

[codecov]

Codecov Report

Merging #3239 into branch-0.11 will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff              @@
##           branch-0.11    #3239   +/-   ##
============================================
  Coverage        87.13%   87.13%           
============================================
  Files               49       49           
  Lines             9269     9269           
============================================
  Hits              8077     8077           
  Misses            1192     1192

---

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 6bca9f6...ef59466. Read the comment docs.

[harrism]

I have some concerns about the semantics and the overheads.

[jrhemstad]

@rgsl888prabhu a pattern like this is more along the lines of what I was envisioning:
https://wandbox.org/permlink/BEXfhMZWdwDKBprS

template <typename Element, std::enable_if_t<cudf::is_relationally_comparable<
                                  Element, Element>()>* = nullptr>
  __device__ weak_ordering operator()(size_type lhs_element_index,
                                      size_type rhs_element_index) const
      noexcept {
    if (has_nulls) {
      bool const lhs_is_null{lhs.nullable() and lhs.is_null(lhs_element_index)};
      bool const rhs_is_null{rhs.nullable() and rhs.is_null(rhs_element_index)};

      if (lhs_is_null and rhs_is_null) {  // null <? null
        return weak_ordering::EQUIVALENT;
      } else if (lhs_is_null) {  // null <? x
        return (null_precedence == null_order::BEFORE) ? weak_ordering::LESS
                                                       : weak_ordering::GREATER;
      } else if (rhs_is_null) {  // x <? null
        return (null_precedence == null_order::AFTER) ? weak_ordering::LESS
                                                      : weak_ordering::GREATER;
      }
    }

    Element const lhs_element = lhs.element<Element>(lhs_element_index);
    Element const rhs_element = rhs.element<Element>(rhs_element_index);

    return compare(lhs_element, rhs_element);
  }

This should vastly simplify the implementation you have here.

[rgsl888prabhu]

@rgsl888prabhu a pattern like this is more along the lines of what I was envisioning:
https://wandbox.org/permlink/BEXfhMZWdwDKBprS

template <typename Element, std::enable_if_t<cudf::is_relationally_comparable<
                                  Element, Element>()>* = nullptr>
  __device__ weak_ordering operator()(size_type lhs_element_index,
                                      size_type rhs_element_index) const
      noexcept {
    if (has_nulls) {
      bool const lhs_is_null{lhs.nullable() and lhs.is_null(lhs_element_index)};
      bool const rhs_is_null{rhs.nullable() and rhs.is_null(rhs_element_index)};

      if (lhs_is_null and rhs_is_null) {  // null <? null
        return weak_ordering::EQUIVALENT;
      } else if (lhs_is_null) {  // null <? x
        return (null_precedence == null_order::BEFORE) ? weak_ordering::LESS
                                                       : weak_ordering::GREATER;
      } else if (rhs_is_null) {  // x <? null
        return (null_precedence == null_order::AFTER) ? weak_ordering::LESS
                                                      : weak_ordering::GREATER;
      }
    }

    Element const lhs_element = lhs.element<Element>(lhs_element_index);
    Element const rhs_element = rhs.element<Element>(rhs_element_index);

    return compare(lhs_element, rhs_element);
  }

This should vastly simplify the implementation you have here.

@jrhemstad So, we will have two sets of functions one for floating point and other for non-floating type.

[jrhemstad]

@jrhemstad So, we will have two sets of functions one for floating point and other for non-floating type.

Yes, but it's limited to the logic that needs to be specialized between floating/non-floating types.

[jrhemstad]

One small change, otherwise looks great!

[jrhemstad]

I think you just need to merge the latest branch-0.11 into your PR to get CI to pass.

[rgsl888prabhu]

@harrism

[harrism]

Very clean solution.

[harrism]

Why does NaN appear twice in these?

[jrhemstad]

To show NaN == NaN