NVIDIA / NVIDIA/cudf

[FEA] aggregation_request should view rather than own its aggregations

Open
#12,656 0 comments 0 reactions 0 assignees View on GitHub
0 - Backlog feature request libcudf
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 `aggregation_request` struct contains a `std::vector>`. This forces the request to take ownership of the `groupy_aggregation`s, which leads to cumbersome code filled with unnecessary moves (unless the aggregations are temporaries, in which case ownership transfer is not an issue).

**Describe the solution you'd like**
The request should instead contain a vector of aggregations so that it can copy the aggregations when the request is constructed. That will allow calling code to pass local objects or temporaries equally easily when building up an aggregation request.

**Describe alternatives you've considered**
In an ideal world the request would not own aggregations at all and would instead own const references to the aggregations (const so that C++ lifetime extension rules would allow passing rvalues when building up the vector). However, container types cannot directly hold references, so this isn't an option.

An alternative would be to instead store instances of `std::reference_wrapper`, but since those create an extra level of indirection they won't bind directly to a temporary and will therefore prohibit usage of rvalues, at which point they are no better than storing by value.

**Additional context**
Currently `aggregation_request`s are typically built up by manually inserting elements into the vector of aggregations. A constructor accepting a (host) span may be a reasonable alternative, but it's not necessary to make the other changes proposed above.

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.