cloud-fan commented on code in PR #39134: URL: https://github.com/apache/spark/pull/39134#discussion_r1055044808
########## sql/core/src/test/resources/sql-tests/inputs/group-by-all.sql: ########## @@ -0,0 +1,71 @@ +-- group by all +-- see https://www.linkedin.com/posts/mosha_duckdb-firebolt-snowflake-activity-7009615821006131200-VQ0o + +create temporary view data as select * from values + ("USA", "San Francisco", "Reynold", 1, 11.0), + ("USA", "San Francisco", "Matei", 2, 12.0), + ("USA", "Berkeley", "Xiao", 3, 13.0), + ("China", "Hangzhou", "Wenchen", 4, 14.0), + ("China", "Shanghai", "Shanghaiese", 5, 15.0), + ("Korea", "Seoul", "Hyukjin", 6, 16.0), + ("UK", "London", "Sean", 7, 17.0) + as data(country, city, name, id, power); + +-- basic +select country, count(*) from data group by ALL; + +-- different case +select country, count(*) from data group by aLl; + +-- a column named "all" would still work +select all, city, count(*) from (select country as all, city, id from data) group by all, city; + +-- a column named "all" should take precedence over the normal group by all expansion +-- if all refers to the column, then the following should return 3 rows. +-- if all refers to the global aggregate, then 1 row. +SELECT count(1) FROM VALUES(1), (2), (3) AS T(all) GROUP BY all; Review Comment: I think it's better to put column resolution in one rule. Relying on the rule order is not robust. Let's look at an example: let's say the plan tree is `A -> B -> C`, and only the leaf node `A` is resolved for now. 1. `ResolveReference` resolves columns on `B`, but `B` is not fully resolved as it contains an unresolved function 2. `ResolveFunctions` resolves functions in `B`, now `B` is fully resolved 3. `ResolveGroupBy` resolves column `ALL` in `C`. This breaks the expected resolution order. Ideally, we want to run different column resolution rules in order for each plan node, not the entire plan tree. Merging them into a single rule is the most straightforward approach, or we need to design a complicated framework to achieve this. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
