icexelloss commented on code in PR #34627:
URL: https://github.com/apache/arrow/pull/34627#discussion_r1151921611
##########
cpp/src/arrow/engine/substrait/options.cc:
##########
@@ -165,6 +170,69 @@ 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");
+ }
Review Comment:
> We could fallback to a regular aggregation if there are no segment keys
right?
from the producer side we will never create a segmented aggregate rel
without segment key so I think this is fine to leave as is.
##########
cpp/src/arrow/engine/substrait/options.cc:
##########
@@ -165,6 +170,69 @@ 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");
+ }
Review Comment:
> We could fallback to a regular aggregation if there are no segment keys
right?
from the producer side we will never create a segmented aggregate rel
without segment key so I think this is fine to leave as is.
--
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]
