You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/11/09 02:21:35 UTC
[GitHub] [spark] itholic opened a new pull request, #38569: [SPARK-41055][SQL] Rename `_LEGACY_ERROR_TEMP_2424` to `GROUP_BY_AGGREGATE`
itholic opened a new pull request, #38569:
URL: https://github.com/apache/spark/pull/38569
### What changes were proposed in this pull request?
This PR proposes to rename `_LEGACY_ERROR_TEMP_2424` to `GROUP_BY_AGGREGATE`
### Why are the changes needed?
To use proper error class name.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
```
./build/sbt “sql/testOnly org.apache.spark.sql.SQLQueryTestSuite*”
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on a diff in pull request #38569: [SPARK-41055][SQL] Rename `_LEGACY_ERROR_TEMP_2424` to `GROUP_BY_AGGREGATE`
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on code in PR #38569:
URL: https://github.com/apache/spark/pull/38569#discussion_r1017615605
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -469,6 +469,11 @@
"Grouping sets size cannot be greater than <maxSize>"
]
},
+ "GROUP_BY_AGGREGATE" : {
+ "message" : [
+ "aggregate functions are not allowed in GROUP BY, but found <sqlExpr>"
Review Comment:
The error message should begin from an upper case letter and ends with a dot. See other error messages in `error-classes.json`
```suggestion
"Aggregate functions are not allowed in GROUP BY, but found <sqlExpr>."
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] itholic commented on a diff in pull request #38569: [SPARK-41055][SQL] Rename `_LEGACY_ERROR_TEMP_2424` to `GROUP_BY_AGGREGATE`
Posted by GitBox <gi...@apache.org>.
itholic commented on code in PR #38569:
URL: https://github.com/apache/spark/pull/38569#discussion_r1017351734
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -469,6 +469,11 @@
"Grouping sets size cannot be greater than <maxSize>"
]
},
+ "GROUP_BY_AGGREGATE" : {
Review Comment:
I'm not pretty sure since `GROUP_BY_POS_REFERS_AGG_EXPR` requires `<index>`, but `GROUP_BY_AGGREGATE ` only cares if the aggregate function is used in the group by cause or not.
Maybe we can manually pass the missing value for `<index>` in `GROUP_BY_AGGREGATE`, but not sure how much it makes sense.
WDYT, @srielau @MaxGekk ?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #38569: [SPARK-41055][SQL] Rename `_LEGACY_ERROR_TEMP_2424` to `GROUP_BY_AGGREGATE`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #38569:
URL: https://github.com/apache/spark/pull/38569#discussion_r1018831510
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -469,6 +469,11 @@
"Grouping sets size cannot be greater than <maxSize>"
]
},
+ "GROUP_BY_AGGREGATE" : {
Review Comment:
> Have reviewed? GROUP_BY_POS_REFERS_AGG_EXPR... that's not very pretty.
Yes, this is the merged code, but we can rename it
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] itholic commented on pull request #38569: [SPARK-41055][SQL] Rename `_LEGACY_ERROR_TEMP_2424` to `GROUP_BY_AGGREGATE`
Posted by GitBox <gi...@apache.org>.
itholic commented on PR #38569:
URL: https://github.com/apache/spark/pull/38569#issuecomment-1308111274
cc @MaxGekk @srielau FYI
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] itholic commented on a diff in pull request #38569: [SPARK-41055][SQL] Rename `_LEGACY_ERROR_TEMP_2424` to `GROUP_BY_AGGREGATE`
Posted by GitBox <gi...@apache.org>.
itholic commented on code in PR #38569:
URL: https://github.com/apache/spark/pull/38569#discussion_r1017351734
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -469,6 +469,11 @@
"Grouping sets size cannot be greater than <maxSize>"
]
},
+ "GROUP_BY_AGGREGATE" : {
Review Comment:
I'm not pretty sure since `GROUP_BY_POS_REFERS_AGG_EXPR` requires <index>, but it's only care if the aggregate function is used in the group by cause.
We can manually pass the missing value for <index>, but not sure it makes sense or not.
WDYT, @srielau @MaxGekk ?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #38569: [SPARK-41055][SQL] Rename `_LEGACY_ERROR_TEMP_2424` to `GROUP_BY_AGGREGATE`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #38569:
URL: https://github.com/apache/spark/pull/38569#discussion_r1017346760
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -469,6 +469,11 @@
"Grouping sets size cannot be greater than <maxSize>"
]
},
+ "GROUP_BY_AGGREGATE" : {
Review Comment:
looks similar to `GROUP_BY_POS_REFERS_AGG_EXPR`
https://github.com/apache/spark/blob/0add57a1c0290a158666027afb3e035728d2dcee/core/src/main/resources/error/error-classes.json#L478-L483
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] itholic commented on a diff in pull request #38569: [SPARK-41055][SQL] Rename `_LEGACY_ERROR_TEMP_2424` to `GROUP_BY_AGGREGATE`
Posted by GitBox <gi...@apache.org>.
itholic commented on code in PR #38569:
URL: https://github.com/apache/spark/pull/38569#discussion_r1017351734
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -469,6 +469,11 @@
"Grouping sets size cannot be greater than <maxSize>"
]
},
+ "GROUP_BY_AGGREGATE" : {
Review Comment:
I'm not pretty sure since `GROUP_BY_POS_REFERS_AGG_EXPR` requires `<index>`, but `GROUP_BY_AGGREGATE ` only cares if the aggregate function is used in the group by cause or not.
Maybe we can manually pass the missing value for `<index>` in `GROUP_BY_AGGREGATE`, but not sure it makes sense or not.
WDYT, @srielau @MaxGekk ?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] itholic commented on a diff in pull request #38569: [SPARK-41055][SQL] Rename `_LEGACY_ERROR_TEMP_2424` to `GROUP_BY_AGGREGATE`
Posted by GitBox <gi...@apache.org>.
itholic commented on code in PR #38569:
URL: https://github.com/apache/spark/pull/38569#discussion_r1017351734
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -469,6 +469,11 @@
"Grouping sets size cannot be greater than <maxSize>"
]
},
+ "GROUP_BY_AGGREGATE" : {
Review Comment:
I'm not pretty sure since `GROUP_BY_POS_REFERS_AGG_EXPR` requires `<index>`, but `GROUP_BY_AGGREGATE ` only cares if the aggregate function is used in the group by cause.
We can manually pass the missing value for `<index>`, but not sure it makes sense or not.
WDYT, @srielau @MaxGekk ?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk closed pull request #38569: [SPARK-41055][SQL] Rename `_LEGACY_ERROR_TEMP_2424` to `GROUP_BY_AGGREGATE`
Posted by GitBox <gi...@apache.org>.
MaxGekk closed pull request #38569: [SPARK-41055][SQL] Rename `_LEGACY_ERROR_TEMP_2424` to `GROUP_BY_AGGREGATE`
URL: https://github.com/apache/spark/pull/38569
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] LuciferYang commented on a diff in pull request #38569: [SPARK-41055][SQL] Rename `_LEGACY_ERROR_TEMP_2424` to `GROUP_BY_AGGREGATE`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #38569:
URL: https://github.com/apache/spark/pull/38569#discussion_r1017347010
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -469,6 +469,11 @@
"Grouping sets size cannot be greater than <maxSize>"
]
},
+ "GROUP_BY_AGGREGATE" : {
Review Comment:
Could we re-use `GROUP_BY_POS_REFERS_AGG_EXPR ` ?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] MaxGekk commented on pull request #38569: [SPARK-41055][SQL] Rename `_LEGACY_ERROR_TEMP_2424` to `GROUP_BY_AGGREGATE`
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on PR #38569:
URL: https://github.com/apache/spark/pull/38569#issuecomment-1310561698
+1, LGTM. Merging to master.
Thank you, @itholic and @srielau @LuciferYang for review.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] itholic commented on a diff in pull request #38569: [SPARK-41055][SQL] Rename `_LEGACY_ERROR_TEMP_2424` to `GROUP_BY_AGGREGATE`
Posted by GitBox <gi...@apache.org>.
itholic commented on code in PR #38569:
URL: https://github.com/apache/spark/pull/38569#discussion_r1017351734
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -469,6 +469,11 @@
"Grouping sets size cannot be greater than <maxSize>"
]
},
+ "GROUP_BY_AGGREGATE" : {
Review Comment:
I'm not pretty sure since `GROUP_BY_POS_REFERS_AGG_EXPR` requires `<index>`, but it's only care if the aggregate function is used in the group by cause.
We can manually pass the missing value for `<index>`, but not sure it makes sense or not.
WDYT, @srielau @MaxGekk ?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] srielau commented on a diff in pull request #38569: [SPARK-41055][SQL] Rename `_LEGACY_ERROR_TEMP_2424` to `GROUP_BY_AGGREGATE`
Posted by GitBox <gi...@apache.org>.
srielau commented on code in PR #38569:
URL: https://github.com/apache/spark/pull/38569#discussion_r1017369097
##########
core/src/main/resources/error/error-classes.json:
##########
@@ -469,6 +469,11 @@
"Grouping sets size cannot be greater than <maxSize>"
]
},
+ "GROUP_BY_AGGREGATE" : {
Review Comment:
I like it that they are distinct. GROUP_BY_POS_... might be fat fingering, GROUP_BY_AGGREGATE is: You don't know how GROUP BY works...
Have reviewed? GROUP_BY_POS_REFERS_AGG_EXPR... that's not very pretty.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org