NVIDIA / NVIDIA/cudf

[FEA] Support self comparisons in `cudf::experimental::row::lexicographic::two_table_comparator`

Open
#13,371 2 comments 2 reactions 0 assignees View on GitHub
1 - On Deck feature request libcudf
Dominant language
C++
Stars
9.8k
Forks
1.1k
Avg merge
3d 6m
Merged PRs (30d)
278

Description

As showcased in https://github.com/rapidsai/cudf/pull/13347, there's a need for `{lhs, lhs}` and `{rhs, rhs}` comparisons in an instance of `two_table_comparator`.

This can't simply be achieved by adding more overloads because `left` and `right` terminology is [baked into the comparator](https://github.com/rapidsai/cudf/blob/76ec53fae9dad71398c115bc405317a918036e52/cpp/include/cudf/table/experimental/row_operators.cuh#L1141-L1157) when it's constructed at the host-side. In a device function, the [strongly typed indices](https://github.com/rapidsai/cudf/blob/76ec53fae9dad71398c115bc405317a918036e52/cpp/include/cudf/table/experimental/row_operators.cuh#L1012-L1039) now work with the assumption that a `comp(i, j)` that is called in a device function operates on `{lhs, rhs}` or `{rhs, lhs}`.

We need to settle on a design that lets us refactor the row operators such that the assumption of working on two different tables can be removed.

Do we strongly type `device_row_comparator::operator()` [over here](https://github.com/rapidsai/cudf/blob/76ec53fae9dad71398c115bc405317a918036e52/cpp/include/cudf/table/experimental/row_operators.cuh#L541-L542) such that we can decide which columns of which tables to pass along to the `element_comparator` over [here](https://github.com/rapidsai/cudf/blob/76ec53fae9dad71398c115bc405317a918036e52/cpp/include/cudf/table/experimental/row_operators.cuh#L568-L575)?

I see the design looking something like this:
```
class device_row_comparator {
class element_comparator {
operator() (size_type lhs_index, size_type rhs_index);
};
dispatch_element_operator(lhs_col, rhs_col, lhs_index, rhs_index);

// these call dispatch_element_operator with the correct columns and indices
operator() (lhs_index_type lhs_index, rhs_index_type rhs_index);
operator() (lhs_index_type lhs_index, lhs_index_type rhs_index);
operator() (rhs_index_type lhs_index, rhs_index_type rhs_index);
};

// the template `Comparator` here and below will be an instance of `device_row_comparator`,
// such that the strongly type indices can be passed along directly
template
class single_table_ordering {
operator() (size_type lhs_index, size_type rhs_index) {
return comparator(lhs_index_type{lhs_index}, lhs_index_type{rhs_index});
};

template
class two_table_ordering {
// same as current version of strong_index_comparator with added overloads for {lhs, lhs} and {rhs, rhs}
};

class self_comparator {
auto less() {
return less_comparator{single_table_ordering{device_row_comparator{...}}};
}
};

class two_table_comparator {
auto less() {
return less_comparator{two_table_ordering{device_row_comparator{...}}};
}
};
```
Note: In this example, [weak_ordering_comparator_impl](https://github.com/rapidsai/cudf/blob/76ec53fae9dad71398c115bc405317a918036e52/cpp/include/cudf/table/experimental/row_operators.cuh#L612-L626) will be removed and it's functionality will instead be baked into `single_table_ordering` and `two_table_ordering`. [less_comparator](https://github.com/rapidsai/cudf/blob/76ec53fae9dad71398c115bc405317a918036e52/cpp/include/cudf/table/experimental/row_operators.cuh#L634-L644) will then be reworked with CRTP such that:
```
template
class less_comparator : Comparator
```

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.