[GitHub] [arrow] jorgecarleitao commented on a change in pull request #9602: ARROW-11630: [Rust] Introduce limit option for sort kernel

[GitBox <gi...@apache.org>]

jorgecarleitao commented on a change in pull request #9602:
URL: https://github.com/apache/arrow/pull/9602#discussion_r584928250



##########
File path: rust/arrow/Cargo.toml
##########
@@ -51,6 +51,8 @@ flatbuffers = "^0.8"
 hex = "0.4"
 prettytable-rs = { version = "0.8.0", optional = true }
 lexical-core = "^0.7"
+partial_sort = "0.1.1"
+pdqsort = "1.0.3"

Review comment:
       I am -1 on this. Either add a feature gate or something else.

##########
File path: rust/arrow/src/compute/kernels/sort.rs
##########
@@ -36,8 +36,12 @@ use TimeUnit::*;
 ///
 /// Returns an `ArrowError::ComputeError(String)` if the array type is either unsupported by `sort_to_indices` or `take`.
 ///
-pub fn sort(values: &ArrayRef, options: Option<SortOptions>) -> Result<ArrayRef> {
-    let indices = sort_to_indices(values, options)?;
+pub fn sort(
+    values: &ArrayRef,
+    options: Option<SortOptions>,
+    limit: Option<usize>,

Review comment:
       I think that this should be done on a separate function instead of adding another parameter to this one. It would also allow to feature-gate said function with the two new dependencies.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org