[GitHub] [arrow] westonpace commented on a diff in pull request #13527: ARROW-16990: [C++] Typos/copy-paste mistakes in Substrait API docstrings

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

westonpace commented on code in PR #13527:
URL: https://github.com/apache/arrow/pull/13527#discussion_r915263591


##########
cpp/src/arrow/engine/substrait/serde.h:
##########
@@ -79,150 +79,151 @@ Result<compute::ExecPlan> DeserializePlan(
 /// produced by each toplevel Substrait relation when deserializing a Substrait Plan.
 using WriteOptionsFactory = std::function<std::shared_ptr<dataset::WriteNodeOptions>()>;
 
-/// \brief Deserializes a Substrait Plan message to a list of ExecNode declarations
+/// \brief Deserializes a Substrait Plan message to a list of ExecNode declarations.
 ///
 /// The output of each top-level Substrait relation will be written to a filesystem.
 /// `write_options_factory` can be used to control write behavior.
 ///
 /// \param[in] buf a buffer containing the protobuf serialization of a Substrait Plan
-/// message
+/// message.
 /// \param[in] write_options_factory factory function for generating the write options of
-/// a node consuming the batches produced by each toplevel Substrait relation
+/// a node consuming the batches produced by each toplevel Substrait relation.
 /// \param[in] registry an extension-id-registry to use, or null for the default one.
 /// \param[out] ext_set_out if non-null, the extension mapping used by the Substrait
 /// Plan is returned here.
 /// \return a vector of ExecNode declarations, one for each toplevel relation in the
-/// Substrait Plan
+/// Substrait Plan.
 ARROW_ENGINE_EXPORT Result<std::vector<compute::Declaration>> DeserializePlans(
     const Buffer& buf, const WriteOptionsFactory& write_options_factory,
     const ExtensionIdRegistry* registry = NULLPTR, ExtensionSet* ext_set_out = NULLPTR);
 
-/// \brief Deserializes a single-relation Substrait Plan message to an execution plan
+/// \brief Deserializes a single-relation Substrait Plan message to an execution plan.
 ///
 /// The output of the single Substrait relation will be written to a filesystem.
 /// `write_options_factory` can be used to control write behavior.
 ///
 /// \param[in] buf a buffer containing the protobuf serialization of a Substrait Plan
-/// message
-/// \param[in] write_options write options of a node consuming the batches produced by
-/// each toplevel Substrait relation
+/// message.
+/// \param[in] write_options write options for the node that will consume the batches
+/// produced by the toplevel Substrait relation.

Review Comment:
   ```suggestion
   /// produced by the top level Substrait relation.
   ```
   Or top-level?



##########
cpp/src/arrow/engine/substrait/serde.h:
##########
@@ -39,38 +39,38 @@ namespace engine {
 /// each toplevel Substrait relation when deserializing a Substrait Plan.
 using ConsumerFactory = std::function<std::shared_ptr<compute::SinkNodeConsumer>()>;
 
-/// \brief Deserializes a Substrait Plan message to a list of ExecNode declarations
+/// \brief Deserializes a Substrait Plan message to a list of ExecNode declarations.
 ///
 /// The output of each top-level Substrait relation will be sent to a caller supplied
-/// consumer function provided by consumer_factory
+/// consumer function provided by consumer_factory.
 ///
 /// \param[in] buf a buffer containing the protobuf serialization of a Substrait Plan
-/// message
+/// message.
 /// \param[in] consumer_factory factory function for generating the node that consumes
-/// the batches produced by each toplevel Substrait relation
+/// the batches produced by each toplevel Substrait relation.
 /// \param[in] registry an extension-id-registry to use, or null for the default one.
 /// \param[out] ext_set_out if non-null, the extension mapping used by the Substrait
 /// Plan is returned here.
 /// \return a vector of ExecNode declarations, one for each toplevel relation in the
-/// Substrait Plan
+/// Substrait Plan.
 ARROW_ENGINE_EXPORT Result<std::vector<compute::Declaration>> DeserializePlans(
     const Buffer& buf, const ConsumerFactory& consumer_factory,
     const ExtensionIdRegistry* registry = NULLPTR, ExtensionSet* ext_set_out = NULLPTR);
 
-/// \brief Deserializes a single-relation Substrait Plan message to an execution plan
+/// \brief Deserializes a single-relation Substrait Plan message to an execution plan.
 ///
-/// The output of each top-level Substrait relation will be sent to a caller supplied
-/// consumer function provided by consumer_factory
+/// The output of each top-level Substrait relation will be sent to the given consumer
+/// node.

Review Comment:
   ```suggestion
   /// The output of each top-level Substrait relation will be sent to the given consumer.
   ```
   Minor nit: A `SinkNodeConsumer` isn't actually an exec node.  An exec node has to worry about various lifecycle concerns but a consumer just receives batches.



-- 
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: github-unsubscribe@arrow.apache.org

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