-
Notifications
You must be signed in to change notification settings - Fork 4k
ARROW-9520: [Rust] [DataFusion] Add support for aliased aggregate exprs #8322
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.
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
Conversation
alamb
left a comment
There was a problem hiding this 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
left a comment
There was a problem hiding this 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
|
Can you run locally |
|
Thanks @returnString this looks great but you'll need to run |
|
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? |
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
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks @returnString
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!