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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
