ARROW-14003: [C++][Python] Not providing a sort_key in the "select_k_unstable" kernel crashes

[aocsa]

This is a minor fix for issue ARROW-14003. Moreover I added some function (topK/bottomK) specialization in arrow/compute.py.
@jorisvandenbossche

[github-actions]

https://issues.apache.org/jira/browse/ARROW-14003

[edponce]

If it does not makes sense to invoke select_k_unstable without providing a k and sort_keys, then also make these parameters mandatory in the SelectKOptions.

Although, client code can still provide an invalid value of k and an empty vector to sort_keys, it expresses the desired intent.

[aocsa]

If it does not makes sense to invoke select_k_unstable without providing a k and sort_keys, then also make these parameters mandatory in the SelectKOptions.

Although, client code can still provide an invalid value of k and an empty vector to sort_keys, it expresses the desired intent.

You are right, however there is a requirement to have a default constructor enabled to implement copy here:

arrow/cpp/src/arrow/compute/function_internal.h

Line 634 in 4fde206

std::unique_ptr<FunctionOptions> Copy(const FunctionOptions& options) const override {

I think this was a a design decision to implement a general copy method with the registered properties.

[edponce]

@aocsa I see...thanks for the response. Maybe at some point in the future a more flexible form of FunctionOptions is used.

[jorisvandenbossche]

I am personally fine with the "top_k" / "bottom_k" specializations of "select_k" (it makes it more convenient to use), but I am wondering whether, if we want those, we shouldn't rather add them in C++? Because now those kernels are python specific, and eg also won't work in a query execution context.

[aocsa]

I am personally fine with the "top_k" / "bottom_k" specializations of "select_k" (it makes it more convenient to use), but I am wondering whether, if we want those, we shouldn't rather add them in C++? Because now those kernels are python specific, and eg also won't work in a query execution context.

In previous review at #11019, there was a common agreement to expose just one general API. However we can add these specialization in a follow-up PR which implements SelectKStable https://issues.apache.org/jira/browse/ARROW-13969

[aocsa]

I Think this PR is ready to be merged

[lidavidm]

Thanks for the fixes.