Skip to content

Covert grouping to udaf #11147

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
Merged
merged 9 commits into from
Jul 2, 2024
Merged

Covert grouping to udaf #11147

merged 9 commits into from
Jul 2, 2024

Conversation

@Rachelint
Copy link
Contributor

@Rachelint Rachelint commented Jun 27, 2024 •
edited
Loading

Which issue does this PR close?

Closes #10906

Rationale for this change

What changes are included in this PR?

  • Converts grouping to UDAF
  • Clean the codes related to the old buil_in grouping

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates labels Jun 27, 2024
@Rachelint Rachelint changed the title Change grouping to udaf Covert grouping to udaf Jun 27, 2024
@Rachelint Rachelint force-pushed the change-grouping-to-udaf branch from a9b0f66 to 99559b0 Compare June 29, 2024 03:28
@Rachelint Rachelint force-pushed the change-grouping-to-udaf branch from 99559b0 to 7461211 Compare June 29, 2024 03:40
@github-actions github-actions bot added the sql SQL Planner label Jul 1, 2024
@Rachelint Rachelint marked this pull request as ready for review July 1, 2024 17:12
@Rachelint
Copy link
Contributor Author

Rachelint commented Jul 1, 2024 •
edited
Loading

@jayzhan211 Hi, mind taking a quick look? Sorry for the long delay due to being a bit busy last month.

dharanad
dharanad approved these changes Jul 1, 2024
View reviewed changes
Copy link
Contributor

@dharanad dharanad left a comment

Choose a reason for hiding this comment

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

LGTM

@jayzhan211
Copy link
Contributor

jayzhan211 commented Jul 2, 2024

Remember to add the test in roundtrip_expr_api in datafusion/proto/tests/cases/roundtrip_logical_plan.rs, others LGTM.

@alamb
Copy link
Contributor

alamb commented Jul 2, 2024

Thanks @Rachelint -- this is great.

Remember to add the test in roundtrip_expr_api in datafusion/proto/tests/cases/roundtrip_logical_plan.rs, others LGTM.

As I am slightly panicng about the size of the review queue, I took the liberty of adding this test myself and merging up from main

alamb
alamb approved these changes Jul 2, 2024
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thanks @Rachelint and @dharanad and @jayzhan211 for the reviews

@Rachelint
Copy link
Contributor Author

Rachelint commented Jul 2, 2024 •
edited
Loading

Thanks for

Thanks @Rachelint -- this is great.

Remember to add the test in roundtrip_expr_api in datafusion/proto/tests/cases/roundtrip_logical_plan.rs, others LGTM.

As I am slightly panicng about the size of the review queue, I took the liberty of adding this test myself and merging up from main

Thanks @alamb for helping, and sorry for the delay.
Thanks all for the reviews @jayzhan211 @dharanad

@jayzhan211
Copy link
Contributor

jayzhan211 commented Jul 2, 2024

🚀

@jayzhan211 jayzhan211 merged commit 4bc3228 into apache:main Jul 2, 2024
@alamb
Copy link
Contributor

alamb commented Jul 2, 2024 •
edited
Loading

Thanks @alamb for helping, and sorry for the delay.
Thanks all for the reviews @jayzhan211 @dharanad

No worries -- thank you for the contributions

findepi pushed a commit to findepi/datafusion that referenced this pull request Jul 16, 2024
@Rachelint @alamb @findepi
Covert grouping to udaf (apache#11147)
73b56e3 
* define Grouping udf and impl AggregateUDFImpl for it.

* add `grouping` to default list.

* remove the old grouping related codes.

* continue to remove codes.

* regen pbs in proto.

* remove built-in grouping in proto codes.

* fix sql it.

* Add test + export fn

---------

Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alamb alamb alamb approved these changes

+1 more reviewer

@dharanad dharanad dharanad approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

logical-expr Logical plan and expressions physical-expr Changes to the physical-expr crates sql SQL Planner

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Convert Grouping to UDAF

4 participants

@Rachelint @jayzhan211 @alamb @dharanad