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

support large-utf8 in groupby #35

Merged
merged 2 commits into from Apr 24, 2021
Merged

support large-utf8 in groupby #35

merged 2 commits into from Apr 24, 2021

Conversation

ritchie46
Copy link
Contributor

@ritchie46 ritchie46 commented Apr 23, 2021 •
edited by alamb

This PR adds support for LargeUtf8 in groupby operations.

Administrative question, Do I need to create an addtional issue?

Closes #38

@jorgecarleitao jorgecarleitao added the enhancement New feature or request label Apr 23, 2021
alamb
alamb reviewed Apr 23, 2021
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.

Looks good @ritchie46 -- thank you!

I think all that is missing is some sort of test that exercises this codepath (to ensure that we don't accidentally break / regress in this functionality in the future)

Perhaps you might following the model of group_by_dictionary: https://github.com/apache/arrow-datafusion/blob/master/datafusion/src/execution/context.rs#L1650

@alamb
Copy link
Contributor

alamb commented Apr 23, 2021

Administrative question, Do I need to create an addtional issue?

Yes, please do -- we are hoping to use issues to create the changelog for each release

@ritchie46
Copy link
Contributor Author

Looks good @ritchie46 -- thank you!

I think all that is missing is some sort of test that exercises this codepath (to ensure that we don't accidentally break / regress in this functionality in the future)

Perhaps you might following the model of group_by_dictionary: https://github.com/apache/arrow-datafusion/blob/master/datafusion/src/execution/context.rs#L1650

Thanks, for pointing me in the direction. I still need to get a bit more familiar with this code base.

@ritchie46 ritchie46 mentioned this pull request Apr 23, 2021
Closed
alamb
alamb approved these changes Apr 24, 2021
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.

Looks good - thanks @ritchie46

@alamb alamb merged commit 245f0b8 into apache:master Apr 24, 2021
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

@returnString returnString returnString approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support large-utf8 in groupby operations.
4 participants
@ritchie46 @alamb @returnString @jorgecarleitao