[GitHub] [arrow] kiszk commented on a change in pull request #7264: ARROW-8926: [C++] Improve arrow/compute/*.h comments, correct typos and outdated language

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

kiszk commented on a change in pull request #7264:
URL: https://github.com/apache/arrow/pull/7264#discussion_r429998394



##########
File path: cpp/src/arrow/compute/kernel.h
##########
@@ -345,13 +388,13 @@ class ARROW_EXPORT KernelSignature {
                                                bool is_varargs = false);
 
   /// \brief Return true if the signature if compatible with the list of input
-  /// value descriptors
+  /// value descriptorsl

Review comment:
       Is this change necessary?

##########
File path: cpp/src/arrow/compute/kernel.h
##########
@@ -345,13 +388,13 @@ class ARROW_EXPORT KernelSignature {
                                                bool is_varargs = false);
 
   /// \brief Return true if the signature if compatible with the list of input
-  /// value descriptors
+  /// value descriptorsl

Review comment:
       nit: Is this change necessary?

##########
File path: cpp/src/arrow/compute/kernel.h
##########
@@ -386,47 +435,75 @@ struct SimdLevel {
   enum type { NONE, SSE4_2, AVX, AVX2, AVX512, NEON };
 };
 
+/// \brief The strategy to use for propagating or otherwise populating the
+/// validity bitmap of a kernel output.
 struct NullHandling {
   enum type {
     /// Compute the output validity bitmap by intersecting the validity bitmaps
-    /// of the arguments. Kernel does not do anything with the bitmap
+    /// of the arguments. Kernel generally need not touch the bitmap
+    /// thereafter, but a kernel's exec function is permitted to alter the
+    /// bitmap after the null intersection is computed if it needs to.
     INTERSECTION,
 
-    /// Kernel expects a pre-allocated buffer to write the result bitmap into
+    /// Kernel expects a pre-allocated buffer to write the result bitmap
+    /// into. The preallocated memory is not zeroed (except for the last byte),
+    /// so the kernel should ensure to completely populate the bitmap.
     COMPUTED_PREALLOCATE,
 
-    /// Kernel allocates and populates the validity bitmap of the output
+    /// Kernel allocates and set the validity bitmap of the output.

Review comment:
       nit: `set` -> `sets`

##########
File path: cpp/src/arrow/compute/exec.h
##########
@@ -146,22 +158,53 @@ class ARROW_EXPORT SelectionVector {
 /// Array and Scalar values and an optional SelectionVector indicating that
 /// there is an unmaterialized filter that either must be materialized, or (if
 /// the kernel supports it) pushed down into the kernel implementation.
+///
+/// ExecBatch is semantically similar to RecordBatch in that in a SQL context
+/// it represents a collection of records, but constant "columns" are
+/// represented by Scalar values rather than having to be converted into arrays
+/// with repeated values.
+///
+/// TOOD: Datum uses arrow/util/variant.h which may be a bit heavier-weight

Review comment:
       nit: `TOOD` -> `TODO`

##########
File path: cpp/src/arrow/compute/kernel.h
##########
@@ -324,16 +365,18 @@ class ARROW_EXPORT OutputType {
   // For FIXED resolution
   std::shared_ptr<DataType> type_;
 
+  /// \brief The shape of the output type to return when using Resolve. If ANY
+  /// will promote the input shapes.
   ValueDescr::Shape shape_ = ValueDescr::ANY;
 
   // For COMPUTED resolution
   Resolver resolver_;
 };
 
-/// \brief Holds the input types and output type of the kernel
+/// \brief Holds the input types and output type of the kernel.
 ///
-/// Varargs functions should pass a single input type to be used to validate
-/// the the input types of a function invocation
+/// VarArgs functions should pass a single input type to be used to validate
+/// the the input types of a function invocation.

Review comment:
       nit: `the the` -> `the`




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