icexelloss commented on code in PR #33909:
URL: https://github.com/apache/arrow/pull/33909#discussion_r1095139749
##########
cpp/src/arrow/engine/substrait/options.h:
##########
@@ -83,6 +84,14 @@ class ARROW_ENGINE_EXPORT ExtensionProvider {
ARROW_ENGINE_EXPORT std::shared_ptr<ExtensionProvider>
default_extension_provider();
+struct ARROW_ENGINE_EXPORT NamedTapNodeOptions : public
compute::ExecNodeOptions {
+ NamedTapNodeOptions(const std::string& name, std::shared_ptr<Schema> schema)
+ : name(name), schema(std::move(schema)) {}
+
+ std::string name;
+ std::shared_ptr<Schema> schema;
+};
Review Comment:
IFWIW, agree a specific structure can be useful however in this case I
don't feel it is much more useful than a general one in this case. For example,
the specific structure for the NamedTapNode make function returns a
NamedTapNode option, and doesn't not allow the user to return a declaration
with a custom node option (e.g, in our case, a WriteSmoothNodeOption), so we
need to handle that in our code after calling this NamedTapNode make function
here (or totally by pass NamedTapNode function here). This an be done but I
feel that is unnecessary indirection.
##########
cpp/src/arrow/engine/substrait/options.h:
##########
@@ -83,6 +84,14 @@ class ARROW_ENGINE_EXPORT ExtensionProvider {
ARROW_ENGINE_EXPORT std::shared_ptr<ExtensionProvider>
default_extension_provider();
+struct ARROW_ENGINE_EXPORT NamedTapNodeOptions : public
compute::ExecNodeOptions {
+ NamedTapNodeOptions(const std::string& name, std::shared_ptr<Schema> schema)
+ : name(name), schema(std::move(schema)) {}
+
+ std::string name;
+ std::shared_ptr<Schema> schema;
+};
Review Comment:
I agree a specific structure can be useful however in this case I don't feel
it is much more useful than a general one in this case. For example, the
specific structure for the NamedTapNode make function returns a NamedTapNode
option, and doesn't not allow the user to return a declaration with a custom
node option (e.g, in our case, a WriteSmoothNodeOption), so we need to handle
that in our code after calling this NamedTapNode make function here (or totally
by pass NamedTapNode function here). This an be done but I feel that is
unnecessary indirection.
--
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]
