westonpace commented on code in PR #13613:
URL: https://github.com/apache/arrow/pull/13613#discussion_r938343153
##########
cpp/src/arrow/engine/substrait/expression_internal.cc:
##########
@@ -41,6 +41,70 @@ namespace internal {
using ::arrow::internal::make_unique;
} // namespace internal
+Status DecodeArg(const substrait::FunctionArgument& arg, int idx,
SubstraitCall* call,
+ const ExtensionSet& ext_set,
+ const ConversionOptions& conversion_options) {
+ if (arg.has_enum_()) {
+ const substrait::FunctionArgument::Enum& enum_val = arg.enum_();
+ if (enum_val.has_specified()) {
+ call->SetEnumArg(static_cast<uint32_t>(idx), enum_val.specified());
+ } else {
+ call->SetEnumArg(idx, util::nullopt);
+ }
+ } else if (arg.has_value()) {
+ ARROW_ASSIGN_OR_RAISE(compute::Expression expr,
+ FromProto(arg.value(), ext_set, conversion_options));
+ call->SetValueArg(idx, std::move(expr));
+ } else if (arg.has_type()) {
+ return Status::NotImplemented("Type arguments not currently supported");
+ } else {
+ return Status::NotImplemented("Unrecognized function argument class");
+ }
+ return Status::OK();
+}
+
+Result<SubstraitCall> DecodeScalarFunction(
+ Id id, const substrait::Expression::ScalarFunction& scalar_fn,
+ const ExtensionSet& ext_set, const ConversionOptions& conversion_options) {
+ ARROW_ASSIGN_OR_RAISE(auto output_type_and_nullable,
+ FromProto(scalar_fn.output_type(), ext_set,
conversion_options));
+ SubstraitCall call(id, output_type_and_nullable.first,
output_type_and_nullable.second);
+ for (int i = 0; i < scalar_fn.arguments_size(); i++) {
+ ARROW_RETURN_NOT_OK(
+ DecodeArg(scalar_fn.arguments(i), i, &call, ext_set,
conversion_options));
+ }
+ return std::move(call);
+}
+
+Result<SubstraitCall> FromProto(const substrait::AggregateFunction& func, bool
is_hash,
+ const ExtensionSet& ext_set,
+ const ConversionOptions& conversion_options) {
+ if (func.phase() !=
substrait::AggregationPhase::AGGREGATION_PHASE_INITIAL_TO_RESULT) {
+ return Status::NotImplemented(
+ "Unsupported aggregation phase. Only INITIAL_TO_RESULT is supported");
Review Comment:
I added an `EnumToString` method and used it to print out the invalid
invocation / phase that the user provided. The error looks like this:
```
NotImplemented: Unsupported aggregation invocation
'AGGREGATION_INVOCATION_DISTINCT'. Only AGGREGATION_INVOCATION_ALL is supported
```
If the user provides a completely invalid value I thought I would get
"unknown" but it looks like the google protobuf converter changes it to
unspecified so I get:
```
NotImplemented: Unsupported aggregation invocation
'AGGREGATION_INVOCATION_UNSPECIFIED'. Only AGGREGATION_INVOCATION_ALL is
supported
```
##########
cpp/src/arrow/engine/substrait/expression_internal.cc:
##########
@@ -41,6 +41,70 @@ namespace internal {
using ::arrow::internal::make_unique;
} // namespace internal
+Status DecodeArg(const substrait::FunctionArgument& arg, int idx,
SubstraitCall* call,
+ const ExtensionSet& ext_set,
+ const ConversionOptions& conversion_options) {
+ if (arg.has_enum_()) {
+ const substrait::FunctionArgument::Enum& enum_val = arg.enum_();
+ if (enum_val.has_specified()) {
+ call->SetEnumArg(static_cast<uint32_t>(idx), enum_val.specified());
+ } else {
+ call->SetEnumArg(idx, util::nullopt);
+ }
+ } else if (arg.has_value()) {
+ ARROW_ASSIGN_OR_RAISE(compute::Expression expr,
+ FromProto(arg.value(), ext_set, conversion_options));
+ call->SetValueArg(idx, std::move(expr));
+ } else if (arg.has_type()) {
+ return Status::NotImplemented("Type arguments not currently supported");
+ } else {
+ return Status::NotImplemented("Unrecognized function argument class");
+ }
+ return Status::OK();
+}
+
+Result<SubstraitCall> DecodeScalarFunction(
+ Id id, const substrait::Expression::ScalarFunction& scalar_fn,
+ const ExtensionSet& ext_set, const ConversionOptions& conversion_options) {
+ ARROW_ASSIGN_OR_RAISE(auto output_type_and_nullable,
+ FromProto(scalar_fn.output_type(), ext_set,
conversion_options));
+ SubstraitCall call(id, output_type_and_nullable.first,
output_type_and_nullable.second);
+ for (int i = 0; i < scalar_fn.arguments_size(); i++) {
+ ARROW_RETURN_NOT_OK(
+ DecodeArg(scalar_fn.arguments(i), i, &call, ext_set,
conversion_options));
+ }
+ return std::move(call);
+}
+
+Result<SubstraitCall> FromProto(const substrait::AggregateFunction& func, bool
is_hash,
+ const ExtensionSet& ext_set,
+ const ConversionOptions& conversion_options) {
+ if (func.phase() !=
substrait::AggregationPhase::AGGREGATION_PHASE_INITIAL_TO_RESULT) {
+ return Status::NotImplemented(
+ "Unsupported aggregation phase. Only INITIAL_TO_RESULT is supported");
+ }
+ if (func.invocation() !=
+ substrait::AggregateFunction::AggregationInvocation::
+ AggregateFunction_AggregationInvocation_AGGREGATION_INVOCATION_ALL) {
+ return Status::NotImplemented(
+ "Unsupported aggregation invocation. Only AGGREGATION_INVOCATION_ALL
is "
+ "supported");
Review Comment:
Added `EnumToString` here too
--
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]
