Update is_sorted to use experimental::row::lexicographic
#12752
Conversation
divyegala
added
feature request
| if (cudf::detail::has_nested_columns(in)) { | ||
| auto const device_comparator = comparator.less<true>(has_nested_nulls(in)); |
There was a problem hiding this comment.
Not sure if it's been discussed before, I feel it makes more sense to move this branching (whether has nested cols or not) into the comparator detail.
There was a problem hiding this comment.
This is the way it was implemented for lexicographic::*_comparator, so I followed the convention. That said, there are some algorithms that dispatch different code-paths for nested and non-nested types already, so you want the granular level of control where those algorithms can select a particular version of the comparator
There was a problem hiding this comment.
Understood. It's definitely out of the scope of this PR. But if we always dispatch different code-paths, the fine control of those algorithms seems unnecessary.
There was a problem hiding this comment.
I agree. All of these if-statements scattered throughout the code seems like an anti-pattern we should try to factor out somehow. We should take a look once these are done. I'm also worried about compile time (surprise) and so that would be a reason to consider refactoring before these are complete perhaps.
There was a problem hiding this comment.
I would prefer any refactor for design patterns to be a follow up to this #12593
|
/merge |
This PR is a part of #11844.
Checklist