NVIDIA / NVIDIA/cudf

[FEA] Alternate design for owning/non-owning comparators

Open
#11,040 10 comments 0 reactions 0 assignees View on GitHub
feature request libcudf proposal
Dominant language
C++
Stars
9.8k
Forks
1.1k
Avg merge
3d 6m
Merged PRs (30d)
278

Description

**Is your feature request related to a problem? Please describe.**

The new experimental row operators require non-trivial preprocessing that involves new allocations whose lifetime must be maintained while attempting to do any row-wise operations on the specified data.

To manage this, we introduced owning and non-owning comparator types.

For example, `self_comparator` is an owning type that handles doing row-wise operations on a single table.

`self_comparator` is not a binary callable object (it doesn't have an `operator()`). Creating the actual non-owning callable object is currently done through a member factory function of the owning type (renamed in https://github.com/rapidsai/cudf/pull/10870).

```
table_view input_table{...};
self_comparator s{input_table}; // "Expensive" construction that does necessary pre-processing and allocations
auto callable = s.less(); // "Cheap" factory that returns a binary callable suitable for passing to algorithms like thrust::sort
thrust::sort(..., callable);
```
After reviewing code using this functionality I've noticed that the callable being returned from a member of the owning type is a bit awkward. For instance the `s.less()` call above isn't immediately obvious that this is actually a factory returning a callable function object. One way to remedy that could be to call it `s.make_less()` instead, but I think there's an all together better way.

**Describe the solution you'd like**
Inspired by `std::` function objects like `std::less/std::equal_to`, I think we should make the callable objects currently being returned from functions like `less()/less_equivalent()/equal_to()` to instead be freestanding types that are _constructible from_ the owning types instead of being returned from a factory of the owning type.

I think this would simplify the owning type as well as make the owner/viewer relationship more clear and explicit.

For example, the code above would become:
```
table_view input_table{...};
self_comparator s{input_table}; // "Expensive" construction that does necessary pre-processing and allocations
auto callable = cudf::row::less{s}; // "Cheap" construction that _views_ the internals of self_comparator
thrust::sort(..., callable);
```

Here's a high level sketch of how this idea could be implemented: https://godbolt.org/z/5fbK7PTb6

Salient points:
- `less` is a standalone type constructible from the `owning` type
- `less` is a `friend` of the owning type to access internals
- `less` deletes constructions from an r-value ref of the owning type to prevent construction from a temporary of the owning type. If allowed, this would lead to dangling references.
- For simplicity, this sketch has `less::operator()` just invoke the `PhysicalComparator`. The actual implementation would have more layers. It would be roughly equivalent to what the internals of the `self_comparator::less` factory above does today.

**Additional Thoughts**

I've come to believe that "comparator" is probably an inappropriate name for the owning types. It's not a comparator (not invokable), it just preprocesses and holds data needed by the actual comparators.

I don't have a good suggestion for a different name yet.

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.