You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "felipecrv (via GitHub)" <gi...@apache.org> on 2023/05/25 00:52:54 UTC
[GitHub] [arrow] felipecrv commented on a diff in pull request #35514: GH-35515: [C++][Python] Add non decomposable aggregation UDF
felipecrv commented on code in PR #35514:
URL: https://github.com/apache/arrow/pull/35514#discussion_r1204876452
##########
cpp/src/arrow/engine/substrait/extension_set.cc:
##########
@@ -979,23 +981,22 @@ ExtensionIdRegistry::SubstraitAggregateToArrow DecodeBasicAggregate(
options = std::make_shared<compute::VarianceOptions>(ddof);
}
}
- fixed_arrow_func += arrow_function_name;
- ARROW_ASSIGN_OR_RAISE(compute::Expression arg, call.GetValueArg(0));
- const FieldRef* arg_ref = arg.field_ref();
- if (!arg_ref) {
- return Status::Invalid("Expected an aggregate call ", call.id().uri, "#",
- call.id().name, " to have a direct reference");
+ fixed_arrow_func += arrow_function_name;
Review Comment:
This line was glued to the previous block before it's part of the function name building process.
##########
cpp/src/arrow/engine/substrait/extension_set.cc:
##########
@@ -979,23 +981,22 @@ ExtensionIdRegistry::SubstraitAggregateToArrow DecodeBasicAggregate(
options = std::make_shared<compute::VarianceOptions>(ddof);
}
}
- fixed_arrow_func += arrow_function_name;
- ARROW_ASSIGN_OR_RAISE(compute::Expression arg, call.GetValueArg(0));
- const FieldRef* arg_ref = arg.field_ref();
- if (!arg_ref) {
- return Status::Invalid("Expected an aggregate call ", call.id().uri, "#",
- call.id().name, " to have a direct reference");
+ fixed_arrow_func += arrow_function_name;
+ std::vector<FieldRef> target;
+ for (int i = 0; i < call.size(); i++) {
+ ARROW_ASSIGN_OR_RAISE(compute::Expression arg, call.GetValueArg(i));
+ const FieldRef* arg_ref = arg.field_ref();
Review Comment:
Remove `const` so it can be moved below.
##########
cpp/src/arrow/engine/substrait/extension_set.cc:
##########
@@ -979,23 +981,22 @@ ExtensionIdRegistry::SubstraitAggregateToArrow DecodeBasicAggregate(
options = std::make_shared<compute::VarianceOptions>(ddof);
}
}
- fixed_arrow_func += arrow_function_name;
- ARROW_ASSIGN_OR_RAISE(compute::Expression arg, call.GetValueArg(0));
- const FieldRef* arg_ref = arg.field_ref();
- if (!arg_ref) {
- return Status::Invalid("Expected an aggregate call ", call.id().uri, "#",
- call.id().name, " to have a direct reference");
+ fixed_arrow_func += arrow_function_name;
+ std::vector<FieldRef> target;
+ for (int i = 0; i < call.size(); i++) {
+ ARROW_ASSIGN_OR_RAISE(compute::Expression arg, call.GetValueArg(i));
+ const FieldRef* arg_ref = arg.field_ref();
+ if (!arg_ref) {
+ return Status::Invalid("Expected an aggregate call ", call.id().uri, "#",
+ call.id().name, " to have a direct reference");
+ }
+ target.emplace_back(std::move(*arg_ref));
}
-
return compute::Aggregate{std::move(fixed_arrow_func),
- options ? std::move(options) : nullptr, *arg_ref, ""};
+ options ? std::move(options) : nullptr, target, ""};
Review Comment:
Is `std::move(target)` possible here?
##########
cpp/src/arrow/engine/substrait/extension_set.cc:
##########
@@ -954,7 +954,9 @@ ExtensionIdRegistry::SubstraitAggregateToArrow DecodeBasicAggregate(
return Status::Invalid("Expected aggregate call ", call.id().uri, "#",
call.id().name, " to have at least one argument");
}
- case 1: {
+ default: {
Review Comment:
Yes
--
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