edponce commented on a change in pull request #11132:
URL: https://github.com/apache/arrow/pull/11132#discussion_r706375290
##########
File path: cpp/src/arrow/compute/kernels/vector_sort.cc
##########
@@ -1855,19 +1854,25 @@ class ArraySelecter : public TypeVisitor {
ctx_(ctx),
array_(array),
k_(options.k),
+ order_(options.sort_keys[0].order),
Review comment:
This assumes that there is at least one key. If `sort_keys` is empty, it
would segfault. Unless the options are validated somewhere else.
I recommend to check this explicitly in the constructor.
##########
File path: cpp/src/arrow/compute/kernels/vector_sort.cc
##########
@@ -2344,22 +2343,10 @@ class SelectKUnstableMetaFunction : public MetaFunction
{
}
switch (args[0].kind()) {
case Datum::ARRAY: {
- if (select_k_options.is_top_k()) {
- return SelectKth<SortOrder::Descending>(*args[0].make_array(),
select_k_options,
- ctx);
- } else {
- return SelectKth<SortOrder::Ascending>(*args[0].make_array(),
select_k_options,
- ctx);
- }
+ return SelectKth(*args[0].make_array(), select_k_options, ctx);
Review comment:
Assumes that `args` is not empty? But can an `Array` be empty when it
reaches this code? cc @lidavidm
##########
File path: cpp/src/arrow/compute/kernels/vector_sort.cc
##########
@@ -1855,19 +1854,25 @@ class ArraySelecter : public TypeVisitor {
ctx_(ctx),
array_(array),
k_(options.k),
+ order_(options.sort_keys[0].order),
Review comment:
Similarly, for the other `XXXSelecters`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
