Skip to content

Commit

Permalink
apacheGH-15199: [C++][Substrait] Allow AGGREGATION_INVOCATION_UNSPECI…
Browse files Browse the repository at this point in the history
…FIED as valid invocation (apache#15198)

In our use of our Substrait, we only accept AGGREGATION_INVOCATION_ALL for aggregation invocations. This is fine, but the spec (as per: https://github.com/substrait-io/substrait/blob/main/proto/substrait/algebra.proto#L1221-L1230) automatically reads AGGREGATION_INVOCATION_UNSPECIFIED as AGGREGATION_INVOCATION_ALL, while we reject it. 

This PR accepts AGGREGATION_INVOCATION_UNSPECIFIED past our error checking. Any requested AGGREGATION_INVOCATION_UNSPECIFIED is thus processed as AGGREGATION_INVOCATION_ALL, with no further functionality changes. The basic aggregation test is modified to test this. 
* Closes: apache#15199

Authored-by: kaesuarez <kaesuarez1423@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
  • Loading branch information
ksuarez1423 authored and EpsilonPrime committed Jan 5, 2023
1 parent 2c87fdc commit 0c76c31
Show file tree
Hide file tree
Showing 2 changed files with 3 additions and 4 deletions.
6 changes: 3 additions & 3 deletions cpp/src/arrow/engine/substrait/expression_internal.cc
Original file line number Diff line number Diff line change
Expand Up @@ -147,9 +147,9 @@ Result<SubstraitCall> FromProto(const substrait::AggregateFunction& func, bool i
EnumToString(func.phase(), *substrait::AggregationPhase_descriptor()),
"'. Only INITIAL_TO_RESULT is supported");
}
if (func.invocation() !=
substrait::AggregateFunction::AggregationInvocation::
AggregateFunction_AggregationInvocation_AGGREGATION_INVOCATION_ALL) {
if (func.invocation() != substrait::AggregateFunction::AGGREGATION_INVOCATION_ALL &&
func.invocation() !=
substrait::AggregateFunction::AGGREGATION_INVOCATION_UNSPECIFIED) {
return Status::NotImplemented(
"Unsupported aggregation invocation '",
EnumToString(func.invocation(),
Expand Down
1 change: 0 additions & 1 deletion cpp/src/arrow/engine/substrait/serde_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1708,7 +1708,6 @@ TEST(Substrait, AggregateBasic) {
}],
"sorts": [],
"phase": "AGGREGATION_PHASE_INITIAL_TO_RESULT",
"invocation": "AGGREGATION_INVOCATION_ALL",
"outputType": {
"i64": {}
}
Expand Down

0 comments on commit 0c76c31

Please sign in to comment.