Skip to content

GH-34786: [C++] Fix output schema calculated by Substrait consumer for AggregateRel - #34904

Merged
westonpace merged 11 commits into
apache:mainfrom
rtpsw:GH-34786.simpler
Apr 12, 2023
Merged

GH-34786: [C++] Fix output schema calculated by Substrait consumer for AggregateRel#34904
westonpace merged 11 commits into
apache:mainfrom
rtpsw:GH-34786.simpler

Conversation

@rtpsw

@rtpswrtpsw commented Apr 5, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

@rtpsw
rtpsw requested a review from westonpace as a code ownerApril 5, 2023 08:00
@github-actions

github-actionsBot commented Apr 5, 2023

Copy link
Copy Markdown

@rtpsw

rtpsw commented Apr 5, 2023

Copy link
Copy Markdown
ContributorAuthor

@westonpace, this is a simpler PR, as compared to #34885, following a discussion with @icexelloss.

icexelloss
icexelloss reviewed Apr 5, 2023
View reviewed changes
Comment threadcpp/src/arrow/acero/aggregate_node.cc Outdated
@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Apr 5, 2023
icexelloss
icexelloss reviewed Apr 5, 2023
View reviewed changes
Comment threadcpp/src/arrow/acero/aggregate_node.cc
icexelloss
icexelloss reviewed Apr 5, 2023
View reviewed changes
Comment threadcpp/src/arrow/acero/aggregate_node.h
icexelloss
icexelloss reviewed Apr 5, 2023
View reviewed changes
Comment threadcpp/src/arrow/engine/substrait/relation_internal.h
@icexelloss

icexelloss commented Apr 5, 2023

Copy link
Copy Markdown
Contributor

@rtpsw Thanks - at high level I think this looks a lot nicer -

Are there any test we can add for this?

@rtpsw

rtpsw commented Apr 5, 2023

Copy link
Copy Markdown
ContributorAuthor

Are there any test we can add for this?

I should have an answer after experimenting with this code a bit.

@github-actionsgithub-actionsBot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Apr 7, 2023
@rtpsw

rtpsw commented Apr 7, 2023

Copy link
Copy Markdown
ContributorAuthor

Are there any test we can add for this?

I should have an answer after experimenting with this code a bit.

Added test-cases.

icexelloss
icexelloss reviewed Apr 10, 2023
View reviewed changes
Comment threadcpp/src/arrow/acero/aggregate_node.cc Outdated
icexelloss
icexelloss reviewed Apr 10, 2023
View reviewed changes
Comment threadcpp/src/arrow/acero/aggregate_node.cc Outdated
@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Apr 10, 2023
icexelloss
icexelloss reviewed Apr 10, 2023
View reviewed changes
Comment threadcpp/src/arrow/acero/aggregate_node.cc Outdated
icexelloss
icexelloss reviewed Apr 10, 2023
View reviewed changes
Comment threadcpp/src/arrow/acero/aggregate_node.cc Outdated
@icexelloss

icexelloss commented Apr 10, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

@rtpsw Looks pretty good - left a few question/comments. Please take a look.

@rtpswrtpsw mentioned this pull request Apr 10, 2023
Closed
@icexelloss

icexelloss commented Apr 10, 2023

Copy link
Copy Markdown
Contributor

@westonpace This looks pretty good to me. Can you please take a look too?

westonpace
westonpace approved these changes Apr 10, 2023
View reviewed changes

@westonpacewestonpace left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a great simplification. Thanks!

Comment threadcpp/src/arrow/acero/aggregate_node.cc Outdated
Comment threadcpp/src/arrow/acero/aggregate_node.h Outdated
Comment threadcpp/src/arrow/acero/aggregate_node.h
@github-actionsgithub-actionsBot added awaiting merge Awaiting merge and removed awaiting changes Awaiting changes labels Apr 10, 2023
rtpswand others added 5 commits April 10, 2023 17:31
@rtpsw@westonpace
Update cpp/src/arrow/acero/aggregate_node.cc
a92dbcb
Co-authored-by: Weston Pace <weston.pace@gmail.com>
@rtpsw@westonpace
Update cpp/src/arrow/acero/aggregate_node.h
9c6165c
Co-authored-by: Weston Pace <weston.pace@gmail.com>
@rtpsw@westonpace
Update cpp/src/arrow/acero/aggregate_node.h
07089d5
Co-authored-by: Weston Pace <weston.pace@gmail.com>
@rtpsw
requested fixes
4e72daa
@rtpsw
typo
aab3a05
@rtpsw
rtpsw requested a review from westonpaceApril 11, 2023 13:55
icexelloss
icexelloss reviewed Apr 11, 2023
View reviewed changes
Comment threadcpp/src/arrow/acero/aggregate_node.cc
@github-actionsgithub-actionsBot added awaiting changes Awaiting changes and removed awaiting merge Awaiting merge labels Apr 11, 2023
@westonpace
westonpace merged commit 342b74e into apache:mainApr 12, 2023
@rtpsw
rtpsw deleted the GH-34786.simpler branch April 14, 2023 06:13
@ursabot

ursabot commented Apr 14, 2023

Copy link
Copy Markdown

Benchmark runs are scheduled for baseline = 2e78adb and contender = 342b74e. 342b74e 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
[Failed] test-mac-arm
[Finished ⬇️2.3% ⬆️0.0%] ursa-i9-9960x
[Finished ⬇️0.22% ⬆️0.03%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 342b74e4 ec2-t3-xlarge-us-east-2
[Failed] 342b74e4 test-mac-arm
[Finished] 342b74e4 ursa-i9-9960x
[Finished] 342b74e4 ursa-thinkcentre-m75q
[Finished] 2e78adbe ec2-t3-xlarge-us-east-2
[Failed] 2e78adbe test-mac-arm
[Finished] 2e78adbe ursa-i9-9960x
[Finished] 2e78adbe 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

@ursabot

ursabot commented Apr 14, 2023

Copy link
Copy Markdown

['Python', 'R'] benchmarks have high level of regressions.
ursa-i9-9960x

This was referenced Apr 14, 2023
Closed
Merged
liujiacheng777 pushed a commit to LoongArch-Python/arrow that referenced this pull request May 11, 2023
@rtpsw@westonpace@liujiacheng777
apacheGH-34786: [C++] Fix output schema calculated by Substrait consu…
eceeba3
…mer for AggregateRel (apache#34904)
See apache#34786
* Closes: apache#34786
Can replace apache#34885
Lead-authored-by: Yaron Gvili <rtpsw@hotmail.com>
Co-authored-by: rtpsw <rtpsw@hotmail.com>
Co-authored-by: Weston Pace <weston.pace@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
ArgusLi pushed a commit to Bit-Quill/arrow that referenced this pull request May 15, 2023
@rtpsw@westonpace@ArgusLi
apacheGH-34786: [C++] Fix output schema calculated by Substrait consu…
977f281
…mer for AggregateRel (apache#34904)
See apache#34786
* Closes: apache#34786
Can replace apache#34885
Lead-authored-by: Yaron Gvili <rtpsw@hotmail.com>
Co-authored-by: rtpsw <rtpsw@hotmail.com>
Co-authored-by: Weston Pace <weston.pace@gmail.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@icexellossicexellossicexelloss left review comments
@westonpacewestonpaceAwaiting requested review from westonpace

Assignees

No one assigned

Labels

awaiting changesAwaiting changesComponent: C++

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[C++] Output schema calculated by Substrait consumer for aggregate rel seems incorrect.

4 participants

@rtpsw@icexelloss@ursabot@westonpace