You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "MaxGekk (via GitHub)" <gi...@apache.org> on 2023/08/06 18:27:42 UTC
[GitHub] [spark] MaxGekk opened a new pull request, #42365: [WIP][SPARK-44680][SQL] Improve the error of parameters in `DEFAULT`
MaxGekk opened a new pull request, #42365:
URL: https://github.com/apache/spark/pull/42365
### What changes were proposed in this pull request?
In the PR, I propose to check that `DEFAULT` clause contains a parameter. If so, raise appropriate error about the feature is not supported.
### Why are the changes needed?
This improves user experience with Spark SQL by saying about the root cause of the issue.
### Does this PR introduce _any_ user-facing change?
No if user's code doesn't depend on error message.
### How was this patch tested?
By running new test:
```
$ build/sbt "test:testOnly *ParametersSuite"
```
--
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] cloud-fan commented on a diff in pull request #42365: [SPARK-44680][SQL] Improve the error for parameters in `DEFAULT`
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42365:
URL: https://github.com/apache/spark/pull/42365#discussion_r1286777682
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -3170,15 +3174,15 @@ class AstBuilder extends DataTypeAstBuilder with SQLConfHelper with Logging {
*/
override def visitDefaultExpression(ctx: DefaultExpressionContext): String =
withOrigin(ctx) {
- verifyAndGetExpression(ctx.expression())
+ verifyAndGetExpression(ctx.expression(), "DEFAULT")
}
/**
* Create a generation expression string.
*/
override def visitGenerationExpression(ctx: GenerationExpressionContext): String =
withOrigin(ctx) {
- verifyAndGetExpression(ctx.expression())
+ verifyAndGetExpression(ctx.expression(), "GENERATED")
Review Comment:
`generated expression`?
--
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] melihsozdinler commented on a diff in pull request #42365: [SPARK-44680][SQL] Improve the error for parameters in `DEFAULT`
Posted by "melihsozdinler (via GitHub)" <gi...@apache.org>.
melihsozdinler commented on code in PR #42365:
URL: https://github.com/apache/spark/pull/42365#discussion_r1285746360
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -3170,15 +3174,15 @@ class AstBuilder extends DataTypeAstBuilder with SQLConfHelper with Logging {
*/
override def visitDefaultExpression(ctx: DefaultExpressionContext): String =
withOrigin(ctx) {
- verifyAndGetExpression(ctx.expression())
+ verifyAndGetExpression(ctx.expression(), "DEFAULT")
Review Comment:
I think using enums is better for future usage, and the code is more robust against changes with typo problems. And later we can reuse this enum at another place as well, when needed.
--
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 #42365: [SPARK-44680][SQL] Improve the error for parameters in `DEFAULT`
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #42365:
URL: https://github.com/apache/spark/pull/42365#issuecomment-1667518258
@cloud-fan Could you review the PR, please.
--
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 #42365: [SPARK-44680][SQL] Improve the error for parameters in `DEFAULT`
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk closed pull request #42365: [SPARK-44680][SQL] Improve the error for parameters in `DEFAULT`
URL: https://github.com/apache/spark/pull/42365
--
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] cloud-fan commented on a diff in pull request #42365: [SPARK-44680][SQL] Improve the error for parameters in `DEFAULT`
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42365:
URL: https://github.com/apache/spark/pull/42365#discussion_r1286777682
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -3170,15 +3174,15 @@ class AstBuilder extends DataTypeAstBuilder with SQLConfHelper with Logging {
*/
override def visitDefaultExpression(ctx: DefaultExpressionContext): String =
withOrigin(ctx) {
- verifyAndGetExpression(ctx.expression())
+ verifyAndGetExpression(ctx.expression(), "DEFAULT")
}
/**
* Create a generation expression string.
*/
override def visitGenerationExpression(ctx: GenerationExpressionContext): String =
withOrigin(ctx) {
- verifyAndGetExpression(ctx.expression())
+ verifyAndGetExpression(ctx.expression(), "GENERATED")
Review Comment:
`generated expression`?
--
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 #42365: [SPARK-44680][SQL] Improve the error for parameters in `DEFAULT`
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #42365:
URL: https://github.com/apache/spark/pull/42365#issuecomment-1669150336
Merging to master/3.5. Thank you, @srielau @melihsozdinler @cloud-fan 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] MaxGekk commented on a diff in pull request #42365: [SPARK-44680][SQL] Improve the error for parameters in `DEFAULT`
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #42365:
URL: https://github.com/apache/spark/pull/42365#discussion_r1286166383
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala:
##########
@@ -3170,15 +3174,15 @@ class AstBuilder extends DataTypeAstBuilder with SQLConfHelper with Logging {
*/
override def visitDefaultExpression(ctx: DefaultExpressionContext): String =
withOrigin(ctx) {
- verifyAndGetExpression(ctx.expression())
+ verifyAndGetExpression(ctx.expression(), "DEFAULT")
Review Comment:
I believe new enum is overkill in this case while the value is used only once. You can make a typo in enum and in string literal with the same probability. I would propose to introduce such enum later when its value is used at least twice.
--
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