Skip to content

ARROW-9520: [Rust] [DataFusion] Add support for aliased aggregate exprs#8322

Closed
returnString wants to merge 2 commits intoapache:masterfrom
returnString:allow_aliased_aggregates
Closed

ARROW-9520: [Rust] [DataFusion] Add support for aliased aggregate exprs#8322
returnString wants to merge 2 commits intoapache:masterfrom
returnString:allow_aliased_aggregates

Conversation

@returnString
Copy link
Contributor

@returnString returnString commented Oct 1, 2020

First attempt at contributing to Arrow/DataFusion, just fixing this small issue around aliased aggregate columns. Please let me know if this isn't an ideal implementation!

@github-actions
Copy link

github-actions bot commented Oct 1, 2020

https://issues.apache.org/jira/browse/ARROW-9520

alamb
alamb approved these changes Oct 1, 2020
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 to me. FYI @jorgecarleitao / @andygrove

jorgecarleitao
jorgecarleitao approved these changes Oct 2, 2020
View reviewed changes
Copy link
Member

@jorgecarleitao jorgecarleitao left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your first contribution to DataFusion!

This is great: really neat and clean solution to this, @returnString !

LGTM

@jorgecarleitao
Copy link
Member

jorgecarleitao commented Oct 2, 2020

Can you run locally cargo fmt, so that we fix all linting issues shown in the CI pipeline?

@andygrove
Copy link
Member

andygrove commented Oct 2, 2020

Thanks @returnString this looks great but you'll need to run cargo +stable fmt --all to fix the linter check in CI before we can merge.

@returnString
Copy link
Contributor Author

returnString commented Oct 2, 2020

That's interesting, I've got rustfmt set to run on save whilst editing in vscode but I was having some issues with code completion in the project too; I'll try and figure out what's up with that for any future contributions.

Are squash merges accepted for this project or would you prefer that I rebase the PR down to a single commit?

@alamb
Copy link
Contributor

alamb commented Oct 2, 2020

Are squash merges accepted for this project or would you prefer that I rebase the PR down to a single commit?

There is some sort of automation script that merges arrow PRs -- I don't think there is any need to rebase to a single commit

andygrove
andygrove approved these changes Oct 2, 2020
View reviewed changes
Copy link
Member

@andygrove andygrove left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @returnString

@andygrove andygrove closed this in 334e33e Oct 3, 2020
@returnString returnString deleted the allow_aliased_aggregates branch October 3, 2020 17:14
@asfimport asfimport mentioned this pull request Oct 3, 2020
Closed
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

@andygrove andygrove andygrove approved these changes

@jorgecarleitao jorgecarleitao jorgecarleitao approved these changes

Assignees

No one assigned

Labels

Component: Rust - DataFusion Component: Rust

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@returnString @jorgecarleitao @andygrove @alamb