icexelloss commented on code in PR #34627:
URL: https://github.com/apache/arrow/pull/34627#discussion_r1151057012
##########
cpp/proto/substrait/extension_rels.proto:
##########
@@ -58,3 +58,14 @@ message NamedTapRel {
// If empty, field names will be automatically generated.
repeated string columns = 3;
}
+
+message SegmentedAggregateRel {
+ // Grouping keys of the aggregation
+ repeated substrait.Expression.ReferenceSegment grouping_keys = 2;
Review Comment:
I see. I am leaning towards using `FieldReference` just because it feels it
is a more standard way to refer to a field. In the consumer we can easily check
and raises for anything that is not `ReferenceSegment` but using
`FieldReference` makes it more consistent with other Substrait messages and
probably makes it easier if we want to upstream this in the future. IMO the
protobuf message should use more general types and the consumer/backend can
decide what it supports and not support under that general type. And as an
extension message, it is still good try to keep consist with existing
standard/non-extension messages instead going our own way unless there is a
good reason (such as allowing multiple right tables in asof join for
performance).
I would +1 to making this `FieldReference` if the code complexity is similar.
--
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]
