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

GH-15199: [C++][Substrait] Allow AGGREGATION_INVOCATION_UNSPECIFIED as valid invocation #15198

Merged
merged 3 commits into from
Jan 5, 2023
Merged

GH-15199: [C++][Substrait] Allow AGGREGATION_INVOCATION_UNSPECIFIED as valid invocation #15198

merged 3 commits into from
Jan 5, 2023

Conversation

ksuarez1423
Copy link
Contributor

@ksuarez1423 ksuarez1423 commented Jan 4, 2023 •
edited by github-actions bot
Loading

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.

@kou
Copy link
Member

kou commented Jan 4, 2023 •
edited
Loading

Could you create a GitHub issue because this is not a minor change?

https://github.com/apache/arrow/blob/master/CONTRIBUTING.md#minor-fixes

Any functionality change should have a GitHub issue opened. For minor changes that affect documentation, you do not need to open up a GitHub issue. Instead you can prefix the title of your PR with "MINOR: " if meets the following guidelines:

  • Grammar, usage and spelling fixes that affect no more than 2 files
  • Documentation updates affecting no more than 2 files and not more than 500 words.

@ksuarez1423 ksuarez1423 changed the title MINOR: [C++][Substrait] Allow AGGREGATION_INVOCATION_UNSPECIFIED as valid invocation GH-15199: [C++][Substrait] Allow AGGREGATION_INVOCATION_UNSPECIFIED as valid invocation Jan 5, 2023
@github-actions
Copy link

github-actions bot commented Jan 5, 2023

@github-actions
Copy link

github-actions bot commented Jan 5, 2023

⚠️ GitHub issue #15199 has been automatically assigned in GitHub to PR creator.

@ksuarez1423
Copy link
Contributor Author

Put together a quick issue at #15199.

@github-actions
Copy link

github-actions bot commented Jan 5, 2023

⚠️ GitHub issue #15199 has no components, please add labels for components.

westonpace
westonpace approved these changes Jan 5, 2023
View reviewed changes
Copy link
Member

@westonpace westonpace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching this spec discrepancy!

@westonpace westonpace merged commit 5fce761 into apache:master Jan 5, 2023
EpsilonPrime pushed a commit to EpsilonPrime/arrow that referenced this pull request 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>
@ursabot
Copy link

ursabot commented Jan 6, 2023

Benchmark runs are scheduled for baseline = d4a0c9e and contender = 5fce761. 5fce761 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished ⬇️0.0% ⬆️0.0%] ec2-t3-xlarge-us-east-2
[Finished ⬇️2.94% ⬆️1.2%] test-mac-arm
[Finished ⬇️0.0% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.3% ⬆️0.2%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 5fce7618 ec2-t3-xlarge-us-east-2
[Finished] 5fce7618 test-mac-arm
[Finished] 5fce7618 ursa-i9-9960x
[Finished] 5fce7618 ursa-thinkcentre-m75q
[Finished] d4a0c9e8 ec2-t3-xlarge-us-east-2
[Finished] d4a0c9e8 test-mac-arm
[Finished] d4a0c9e8 ursa-i9-9960x
[Finished] d4a0c9e8 ursa-thinkcentre-m75q
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

vibhatha pushed a commit to vibhatha/arrow that referenced this pull request 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
Reviewers

@westonpace westonpace westonpace approved these changes

Assignees
No one assigned
Labels
Component: C++
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[C++][Substrait] Allow AGGREGATION_INVOCATION_UNSPECIFIED as valid invocation in Substrait plan
4 participants
@ksuarez1423 @kou @ursabot @westonpace