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-33616: [C++] Reorder group_by so that keys/segment keys come before aggregates #34551

Merged
merged 10 commits into from
Apr 5, 2023
Merged

GH-33616: [C++] Reorder group_by so that keys/segment keys come before aggregates #34551

merged 10 commits into from
Apr 5, 2023

Conversation

westonpace
Copy link
Member

@westonpace westonpace commented Mar 13, 2023 •
edited by github-actions bot
Loading

@github-actions
Copy link

@github-actions
Copy link

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

@jorisvandenbossche
Copy link
Member

The failures in the doc build are because the docstring examples in TableGroupBy.aggregate in table.pxi need to be updated as well (those examples are validated)

@westonpace
Copy link
Member Author

@nealrichardson R is currently failing, I think, because it is already reordering columns to get the desired behavior. Would you have a chance to take a look and back out that code? If not, I can try and look at it later.

@nealrichardson
Copy link
Member

Sure, I can take a look

@westonpace westonpace requested a review from kou as a code owner March 14, 2023 21:41
@kou
Copy link
Member

kou commented Mar 15, 2023

It seems that we need to update more tests for GLib/Ruby.
Can I push changes for it to this branch directly?

@westonpace
Copy link
Member Author

westonpace commented Mar 17, 2023 •
edited
Loading

It seems that we need to update more tests for GLib/Ruby.
Can I push changes for it to this branch directly?

@kou

Yes, I would be very grateful. If you want me to fix this though I can. My ruby fixes just tend to be a little "guess and check" with the CI.

@kou
Copy link
Member

kou commented Mar 17, 2023

Done!

@westonpace westonpace force-pushed the feature/GH-33616--reorder-groupby branch from 433f84c to 836e022 Compare March 22, 2023 14:04
@amol-
Copy link
Member

amol- commented Apr 3, 2023

@westonpace is there anything blocking this one? It looks also it needs to be rebased.

@rtpsw rtpsw mentioned this pull request Apr 3, 2023
Closed
@westonpace westonpace force-pushed the feature/GH-33616--reorder-groupby branch from 836e022 to 700384b Compare April 4, 2023 06:52
@westonpace westonpace force-pushed the feature/GH-33616--reorder-groupby branch from 700384b to 947060c Compare April 4, 2023 06:53
@westonpace westonpace force-pushed the feature/GH-33616--reorder-groupby branch from b8f84c2 to 653a051 Compare April 4, 2023 07:30
@westonpace
Copy link
Member Author

CI failures are unrelated (and these tests were passing before the acero refactor was rebased). I'm going to go ahead and merge this.

@westonpace westonpace merged commit 379c1fb into apache:main Apr 5, 2023
@ursabot
Copy link

ursabot commented Apr 5, 2023

Benchmark runs are scheduled for baseline = 8824aca and contender = 379c1fb. 379c1fb 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 ⬇️0.0% ⬆️1.11%] test-mac-arm
[Finished ⬇️0.26% ⬆️0.0%] ursa-i9-9960x
[Failed ⬇️0.0% ⬆️0.0%] ursa-thinkcentre-m75q
Buildkite builds:
[Finished] 379c1fb0 ec2-t3-xlarge-us-east-2
[Failed] 379c1fb0 test-mac-arm
[Finished] 379c1fb0 ursa-i9-9960x
[Failed] 379c1fb0 ursa-thinkcentre-m75q
[Finished] 8824acac ec2-t3-xlarge-us-east-2
[Failed] 8824acac test-mac-arm
[Finished] 8824acac ursa-i9-9960x
[Failed] 8824acac 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

@heronshoes heronshoes mentioned this pull request Apr 6, 2023
Closed
7 tasks
ArgusLi pushed a commit to Bit-Quill/arrow that referenced this pull request May 15, 2023
@westonpace @nealrichardson
@kou @ArgusLi
apacheGH-33616: [C++] Reorder group_by so that keys/segment keys come…
f2978f9 
… before aggregates (apache#34551)

* Closes: apache#33616

Lead-authored-by: Weston Pace <weston.pace@gmail.com>
Co-authored-by: Neal Richardson <neal.p.richardson@gmail.com>
Co-authored-by: Sutou Kouhei <kou@clear-code.com>
Signed-off-by: Weston Pace <weston.pace@gmail.com>
rtpsw pushed a commit to rtpsw/arrow that referenced this pull request May 16, 2023
@westonpace @nealrichardson
@kou @rtpsw
apacheGH-33616: [C++] Reorder group_by so that keys/segment keys come…
d270a3d 
… before aggregates (apache#34551)

* Closes: apache#33616

Lead-authored-by: Weston Pace <weston.pace@gmail.com>
Co-authored-by: Neal Richardson <neal.p.richardson@gmail.com>
Co-authored-by: Sutou Kouhei <kou@clear-code.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

@AlenkaF AlenkaF Awaiting requested review from AlenkaF

@paleolimbot paleolimbot Awaiting requested review from paleolimbot

@thisisnic thisisnic Awaiting requested review from thisisnic thisisnic is a code owner

@kou kou Awaiting requested review from kou kou is a code owner

Assignees
No one assigned
Labels
awaiting committer review Awaiting committer review Component: C++ Component: GLib Component: Python Component: R Component: Ruby
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[C++] Group_By from Substrait Shuffles Column Names
6 participants
@westonpace @jorisvandenbossche @nealrichardson @kou @amol- @ursabot