[GitHub] [arrow] westonpace commented on a diff in pull request #13069: ARROW-15901: [C++] Support custom output field names in Substrait

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

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


##########
cpp/src/arrow/compute/exec/options.h:
##########
@@ -232,10 +232,13 @@ class ARROW_EXPORT SinkNodeConsumer {
 /// \brief Add a sink node which consumes data within the exec plan run
 class ARROW_EXPORT ConsumingSinkNodeOptions : public ExecNodeOptions {
  public:
-  explicit ConsumingSinkNodeOptions(std::shared_ptr<SinkNodeConsumer> consumer)
-      : consumer(std::move(consumer)) {}
+  explicit ConsumingSinkNodeOptions(std::shared_ptr<SinkNodeConsumer> consumer,
+                                    std::vector<std::string> names = {})
+      : consumer(std::move(consumer)), names(std::move(names)) {}
 
   std::shared_ptr<SinkNodeConsumer> consumer;
+  /// \brief Names to rename the sink's schema fields to
+  std::vector<std::string> names;

Review Comment:
   In this case I think the naming scheme is correct.  There are two categories of objects we deal with, passive data objects and more complex stateful objects.  The google style guide likes to use structs for the former and classes for the latter and so it describes this as [structs vs. classes](https://google.github.io/styleguide/cppguide.html#Structs_vs._Classes).
   
   In Arrow we are pretty inconsistent about applying `struct` and `class`.  Many of our "options" objects are `class` when they should be `struct` for example.  However, we are pretty consistent about the naming.  For a passive data object all properties should be public and [should not have trailing underscores](https://google.github.io/styleguide/cppguide.html#Variable_Names).
   
   Options objects should always be passive data objects.  Things that travel back and forth across library boundaries should generally be either passive data objects or pure virtual interfaces when possible.  So I do not think a change is needed here.  This options object should be a passive data object and it is following the correct rules for a passive data object.



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