icexelloss commented on code in PR #34627:
URL: https://github.com/apache/arrow/pull/34627#discussion_r1152274277
##########
cpp/src/arrow/engine/substrait/options.cc:
##########
@@ -165,6 +170,74 @@ class DefaultExtensionProvider : public
BaseExtensionProvider {
named_tap_rel.name(),
renamed_schema));
return RelationInfo{{std::move(decl), std::move(renamed_schema)},
std::nullopt};
}
+
+ Result<RelationInfo> MakeSegmentedAggregateRel(
+ const ConversionOptions& conv_opts, const std::vector<DeclarationInfo>&
inputs,
+ const substrait_ext::SegmentedAggregateRel& seg_agg_rel,
+ const ExtensionSet& ext_set) {
+ if (inputs.size() != 1) {
+ return Status::Invalid(
+ "substrait_ext::SegmentedAggregateRel requires a single input but
got: ",
+ inputs.size());
+ }
+ if (seg_agg_rel.segment_keys_size() == 0) {
+ return Status::Invalid(
+ "substrait_ext::SegmentedAggregateRel requires at least one segment
key");
+ }
+
+ auto input_schema = inputs[0].output_schema;
+
+ // store key fields to be used when output schema is created
+ std::vector<int> key_field_ids;
+ std::vector<FieldRef> keys;
+ for (auto& key_refseg : seg_agg_rel.grouping_keys()) {
+ ARROW_ASSIGN_OR_RAISE(auto field_ref,
+ DirectReferenceFromProto(&key_refseg, ext_set,
conv_opts));
+ ARROW_ASSIGN_OR_RAISE(auto match, field_ref.FindOne(*input_schema));
+ key_field_ids.emplace_back(std::move(match[0]));
+ keys.emplace_back(std::move(field_ref));
+ }
+
+ // store segment key fields to be used when output schema is created
+ std::vector<int> segment_key_field_ids;
+ std::vector<FieldRef> segment_keys;
+ for (auto& key_refseg : seg_agg_rel.segment_keys()) {
+ ARROW_ASSIGN_OR_RAISE(auto field_ref,
+ DirectReferenceFromProto(&key_refseg, ext_set,
conv_opts));
+ ARROW_ASSIGN_OR_RAISE(auto match, field_ref.FindOne(*input_schema));
+ segment_key_field_ids.emplace_back(std::move(match[0]));
+ segment_keys.emplace_back(std::move(field_ref));
+ }
+
+ std::vector<compute::Aggregate> aggregates;
+ aggregates.reserve(seg_agg_rel.measures_size());
+ std::vector<std::vector<int>> agg_src_fieldsets;
+ agg_src_fieldsets.reserve(seg_agg_rel.measures_size());
+ for (auto agg_measure : seg_agg_rel.measures()) {
+ ARROW_ASSIGN_OR_RAISE(
+ auto parsed_measure,
+ internal::ParseAggregateMeasure(agg_measure, ext_set, conv_opts,
+ /*is_hash=*/!keys.empty(),
input_schema));
+ aggregates.push_back(std::move(parsed_measure.aggregate));
+ agg_src_fieldsets.push_back(std::move(parsed_measure.fieldset));
+ }
+
+ ARROW_ASSIGN_OR_RAISE(auto decl_info,
+ internal::MakeAggregateDeclaration(
+ std::move(inputs[0].declaration),
std::move(input_schema),
Review Comment:
My C++ knowledge is probably lacking here but do these std::move do
anything? It looks like MakeAggregateDeclaration doesn't take a `&&` "move"
paramter (rvalue reference parameter) which IIUC is how std::move is supposed
to work with.
Similarly I noticed the std::move is used with `EmitProcess` which also
doesn't take `&&` move parameter. Perhaps @westonpace can shed some C++ light?
--
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]
