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.

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

Group by Alias #215

Closed
jychen7 opened this issue Apr 28, 2021 · 2 comments · Fixed by #485
Closed

Group by Alias #215

jychen7 opened this issue Apr 28, 2021 · 2 comments · Fixed by #485
Labels
bug Something isn't working

Comments

@jychen7
Copy link
Contributor

jychen7 commented Apr 28, 2021

Describe the bug
#110 suggests we can group by alias, however, I modify the existing test case select_simple_aggregate_with_groupby_with_aliases a bit, then it fails

To Reproduce

SELECT state AS a, MIN(age) AS b FROM person GROUP BY state

shows correct answer

SELECT state AS a, MIN(age) AS b FROM person GROUP BY a

raises error Plan("Projection references non-aggregate values")

Expected behavior

SELECT state AS a, MIN(age) AS b FROM person GROUP BY state

SELECT state AS a, MIN(age) AS b FROM person GROUP BY a

returns same answer

@jychen7 jychen7 added the bug Something isn't working label Apr 28, 2021
@jychen7
Copy link
Contributor Author

jychen7 commented Apr 28, 2021

I think we could resolve_aliases_to_exprs for select.group_by
similar to
datafusion/blob/e86ad26a678a32e2743bc031ae5cb81e6931a231/datafusion/src/sql/planner.rs#L563-L566

However, as a rust learner, I am still on the way struggling the rust typing system when deal with the array mapping

@alamb
Copy link
Contributor

alamb commented Apr 28, 2021

Thanks @jychen7 -- the expected behavior looks correct here 👍 Let us know if you need any help with the Rust

jychen7 added a commit to jychen7/arrow-datafusion that referenced this issue Jun 3, 2021
@alamb alamb closed this as completed in #485 Jun 4, 2021
alamb pushed a commit that referenced this issue Jun 4, 2021
* #215 resolve aliases for group by exprs

* cargo fmt --all
ozankabak added a commit that referenced this issue Nov 23, 2023
…mns) (#8281)

* Complex exprs requirement support (#215)

* Discover ordering of complex expressions in group by and window partition by

* Remove unnecessary tests

* Update comments

* Minor changes

* Better projection support complex expression support

* Fix failing test

* Simplifications

* Simplifications

* Add is end flag

* Simplifications

* Simplifications

* Simplifications

* Minor changes

* Minor changes

* Minor changes

* All tests pass

* Change implementation of find_longest_permutation

* Minor changes

* Minor changes

* Remove projection section

* Remove projection implementation

* Fix linter errors

* Remove projection sections

* Minor changes

* Add docstring comments

* Add comments

* Minor changes

* Minor changes

* Add comments

* simplifications

* Minor changes

* Review Part 1

* Add new tests

* Review Part 2

* Address review feedback

* Remove error message check in the test

---------

Co-authored-by: Mehmet Ozan Kabak <ozankabak@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants