Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[C++][Substrait] Allow AGGREGATION_INVOCATION_UNSPECIFIED as valid invocation in Substrait plan #15199

Closed
ksuarez1423 opened this issue Jan 4, 2023 · 0 comments · Fixed by #15198
Closed

[C++][Substrait] Allow AGGREGATION_INVOCATION_UNSPECIFIED as valid invocation in Substrait plan #15199

ksuarez1423 opened this issue Jan 4, 2023 · 0 comments · Fixed by #15198
Assignees
@ksuarez1423
Labels
Component: C++ Component: Other Type: enhancement
Milestone
11.0.0

Comments

@ksuarez1423
Copy link
Contributor

Describe the enhancement requested

In the Substrait spec, unspecified invocations are read as AGGREGATION_INVOCATION_UNSPECIFIED, and treated as AGGREGATION_INVOCATION_ALL in practice (see: https://github.com/substrait-io/substrait/blob/main/proto/substrait/algebra.proto#L1221-L1230). Arrow's current implementation only accepts plans explicitly using AGGREGATION_INVOCATION_ALL, thus excluding valid plans using AGGREGATION_INVOCATION_UNSPECIFIED.

In order to match the expected behavior, AGGREGATION_INVOCATION_UNSPECIFIED should lead to interpretation as AGGREGATION_INVOCATION_ALL. This will allow acceptance of plans from at least the Substrait producer Ibis, which does not specify this setting.

Component(s)

C++, Other
@github-actions github-actions bot mentioned this issue Jan 5, 2023
Merged
westonpace pushed a commit that referenced this issue Jan 5, 2023
@ksuarez1423
GH-15199: [C++][Substrait] Allow AGGREGATION_INVOCATION_UNSPECIFIED a…
5fce761 
…s valid invocation (#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: #15199

Authored-by: kaesuarez <kaesuarez1423@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
@westonpace westonpace added this to the 11.0.0 milestone Jan 5, 2023
EpsilonPrime pushed a commit to EpsilonPrime/arrow that referenced this issue Jan 5, 2023
@ksuarez1423 @EpsilonPrime
apacheGH-15199: [C++][Substrait] Allow AGGREGATION_INVOCATION_UNSPECI…
0c76c31 
…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>
vibhatha pushed a commit to vibhatha/arrow that referenced this issue Jan 9, 2023
@ksuarez1423 @vibhatha
apacheGH-15199: [C++][Substrait] Allow AGGREGATION_INVOCATION_UNSPECI…
4da3fb2 
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ksuarez1423 ksuarez1423

Labels
Component: C++ Component: Other Type: enhancement
Projects
None yet
Milestone
11.0.0
3 participants
@rok @westonpace @ksuarez1423