rtpsw commented on code in PR #33909:
URL: https://github.com/apache/arrow/pull/33909#discussion_r1091118571
##########
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:
This is a design part to discuss. The current PR implementation takes the
`kind field (i.e., `named_tap_rel.kind()`) as the factory name in the
declaration. There is no need for a new `NamedTapProvider` concept when the
factory registry is the provider of taps. OTOH, I agree with your mapping idea
- adding a mapping from the `kind` field to the factory name would improve.
So, the plan here is:
- `NamedTapRel`: the `kind` field selects the tap's kind, the `name` field
configures the tap's action, and the `columns` field configures the tap's
output schema.
- `NamedTapNodeOptions`: the `name` field configured the tap's action as in
`NamedTapRel`, and the `schema` field is the output schema configured for the
tap. The selection of the tap kind is not done via options, so it's not
included in them.
I could add a description along these lines in docstrings.
The test code shows a vanilla example of this. The `AddPassFactory` tap is
registered in the factory registry. It's configuration is not the focus of the
test. In a (future) test case of a specific tap, we would cast the options
argument of the tap's `Make` function to `NamedTapNodeOptions` and use the
fields there to configure it.
--
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]
