bkietz commented on code in PR #14415:
URL: https://github.com/apache/arrow/pull/14415#discussion_r999646728
##########
cpp/src/arrow/engine/substrait/extension_set.cc:
##########
@@ -156,15 +156,33 @@ Result<compute::Expression>
SubstraitCall::GetValueArg(uint32_t index) const {
return value_arg_it->second;
}
-bool SubstraitCall::HasValueArg(uint32_t index) const {
+bool SubstraitCall::HasValueArg(int index) const {
return value_args_.find(index) != value_args_.end();
}
-void SubstraitCall::SetValueArg(uint32_t index, compute::Expression value_arg)
{
+void SubstraitCall::SetValueArg(int index, compute::Expression value_arg) {
size_ = std::max(size_, index + 1);
value_args_[index] = std::move(value_arg);
}
+std::optional<std::vector<std::string> const*> SubstraitCall::GetOption(
+ std::string_view option_name) const {
+ auto opt = options_.find(std::string(option_name));
+ if (opt == options_.end()) {
+ return std::nullopt;
+ }
+ return &opt->second;
+}
+
+void SubstraitCall::SetOption(std::string_view option_name,
+ const std::vector<std::string_view>&
option_preferences) {
Review Comment:
Should this assert `!option_preferences.empty()`?
##########
cpp/src/arrow/engine/substrait/extension_set.cc:
##########
@@ -645,22 +663,19 @@ struct ExtensionIdRegistryImpl : ExtensionIdRegistry {
};
template <typename Enum>
-using EnumParser =
std::function<Result<Enum>(std::optional<std::string_view>)>;
+using EnumParser = std::function<Result<Enum>(std::string_view)>;
template <typename Enum>
EnumParser<Enum> GetEnumParser(const std::vector<std::string>& options) {
std::unordered_map<std::string, Enum> parse_map;
for (std::size_t i = 0; i < options.size(); i++) {
parse_map[options[i]] = static_cast<Enum>(i + 1);
Review Comment:
It's probably not necessary to reify "UNSPECIFIED" in all the enumerations.
We will always either coerce to a default or error because a real value was
required. That could be expressed in EnumParser as:
```c++
template <typename Enum>
using EnumParser = std::function<Result<Enum>(std::string_view repr,
std::optional<Enum> default)>;
```
... and it would save you some one-based indexing headache.
Probably out of scope for this PR
--
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]
