[GitHub] [arrow] jorisvandenbossche commented on a change in pull request #7318: ARROW-8917: [C++] Formalize "metafunction" concept. Add Take and Filter metafunctions, port R and Python bindings

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

jorisvandenbossche commented on a change in pull request #7318:
URL: https://github.com/apache/arrow/pull/7318#discussion_r433833418



##########
File path: cpp/src/arrow/compute/api_vector.h
##########
@@ -72,152 +72,18 @@ struct ARROW_EXPORT TakeOptions : public FunctionOptions {
 /// \param[in] values datum from which to take
 /// \param[in] indices which values to take
 /// \param[in] options options
-/// \param[in] context the function execution context, optional
+/// \param[in] ctx the function execution context, optional
 /// \return the resulting datum
 ARROW_EXPORT
 Result<Datum> Take(const Datum& values, const Datum& indices,
                    const TakeOptions& options = TakeOptions::Defaults(),
-                   ExecContext* context = NULLPTR);
+                   ExecContext* ctx = NULLPTR);
 
-/// \brief Take from an array of values at indices in another array
-///
-/// The output array will be of the same type as the input values
-/// array, with elements taken from the values array at the given
-/// indices. If an index is null then the taken element will be null.
-///
-/// For example given values = ["a", "b", "c", null, "e", "f"] and
-/// indices = [2, 1, null, 3], the output will be
-/// = [values[2], values[1], null, values[3]]
-/// = ["c", "b", null, null]

Review comment:
       should we keep this explanation+example in the docstring of the Datum-based Take above?

##########
File path: cpp/src/arrow/compute/api_vector.h
##########
@@ -304,5 +170,41 @@ Result<std::shared_ptr<Array>> ValueCounts(const Datum& value,
 ARROW_EXPORT
 Result<Datum> DictionaryEncode(const Datum& data, ExecContext* ctx = NULLPTR);
 
+// ----------------------------------------------------------------------
+// Deprecated functions
+
+// TODO: Add deprecation warnings to these functions

Review comment:
       TODO in this PR or is there already a JIRA?

##########
File path: cpp/src/arrow/compute/function.h
##########
@@ -230,5 +234,19 @@ class ARROW_EXPORT ScalarAggregateFunction
       const std::vector<ValueDescr>& values) const;
 };
 
+/// \brief A function that dispatches to other functions. Must override
+/// Function::Execute.

Review comment:
       does this mean that it must handle the different Datum kinds itself? (such as dealing with combination of array and chuned array?)

##########
File path: cpp/src/arrow/compute/kernels/vector_filter.cc
##########
@@ -176,6 +239,9 @@ void RegisterVectorFilter(FunctionRegistry* registry) {
     AddKernel(InputType::Array(value_ty->id()), *value_ty);
   }
   DCHECK_OK(registry->AddFunction(std::move(filter)));
+
+  // Add take metafunction

Review comment:
       ```suggestion
     // Add filter metafunction
   ```

##########
File path: python/pyarrow/array.pxi
##########
@@ -921,81 +921,15 @@ cdef class Array(_PandasConvertible):
 
     def take(self, object indices):
         """
-        Take elements from an array.
-
-        The resulting array will be of the same type as the input array, with
-        elements taken from the input array at the given indices. If an index
-        is null then the taken element will be null.

Review comment:
       Can you keep this "If an index is null then the taken element will be null." sentence in the docstring in the compute module?

##########
File path: python/pyarrow/array.pxi
##########
@@ -921,81 +921,15 @@ cdef class Array(_PandasConvertible):
 
     def take(self, object indices):
         """
-        Take elements from an array.
-
-        The resulting array will be of the same type as the input array, with
-        elements taken from the input array at the given indices. If an index
-        is null then the taken element will be null.

Review comment:
       and maybe copy paste the example below as well




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