rapidsai / cudf

cuDF - GPU DataFrame Library
https://docs.rapids.ai/api/cudf/stable/
Apache License 2.0
8.42k stars 902 forks source link

cudf::rank not passed enough parameters in list/struct rank benchmarks #16624

Closed harrism closed 2 months ago

harrism commented 2 months ago

The rank lists and rank structs benchmarks do not pass the bool percentage parameter to cudf::rank. But they do pass a MR*, and since pointers can be implicitly converted to bool, the MR goes unused and percentage is taken as true.

https://github.com/rapidsai/cudf/blob/58799d698d861866b5650d368f5195174fc9644e/cpp/benchmarks/sort/rank_lists.cpp#L35-L40

and

https://github.com/rapidsai/cudf/blob/58799d698d861866b5650d368f5195174fc9644e/cpp/benchmarks/sort/rank_structs.cpp#L33-L38

The prototype for rank is:

std::unique_ptr<column> rank(
  column_view const& input,
  rank_method method,
  order column_order,
  null_policy null_handling,
  null_order null_precedence,
  bool percentage,
  rmm::cuda_stream_view stream      = cudf::get_default_stream(),
  rmm::device_async_resource_ref mr = rmm::mr::get_current_device_resource());

Note that passing a MR without passing a stream should not compile, and if the parameter before stream were something that a pointer can't be converted to, it wouldn't.

This is another good reason to avoid bool parameters. :)

davidwendt commented 2 months ago

Closed by #16666