alamb commented on code in PR #5333:
URL: https://github.com/apache/arrow-rs/pull/5333#discussion_r1467868987


##########
arrow-select/src/take.rs:
##########
@@ -769,11 +769,25 @@ to_indices_reinterpret!(Int32Type, UInt32Type);
 to_indices_identity!(UInt64Type);
 to_indices_reinterpret!(Int64Type, UInt64Type);
 
+/// Take rows by index from [`RecordBatch`] and returns a new [`RecordBatch`] 
from those indexes.
+pub fn take_record_batch(
+    record_batch: &RecordBatch,
+    indices: &[u64],

Review Comment:
   Maybe it could be like 
   ```rust
   pub fn take_record_batch(
       record_batch: &RecordBatch,
       indices: &UInt64Array,
   ..
   ```
   Then an example use might be
   
   ```rust
   let half1 = take_record_batch(&batch, &UInt64Array::from(all[0..len(all) / 
2])))
   ```
   
   Perhaps with some good doc examples this would be easy to use
   
   Another thought might be to use the 
[`UInt64::From`](https://docs.rs/arrow/latest/arrow/array/array/type.UInt64Array.html#impl-From%3CVec%3COption%3C%3CUInt64Type+as+ArrowPrimitiveType%3E::Native%3E%3E%3E-for-PrimitiveArray%3CUInt64Type%3E)
 impls like:
   
   ```
   /// Take rows by index from [`RecordBatch`] and returns a new 
[`RecordBatch`] from those indexes.
   pub fn take_record_batch(
       record_batch: &RecordBatch,
       indices: impl Into<UInt64Array>,
   )
   ```
   
   though then that needs an owned array which seems less than ideal 🤔 



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

Reply via email to