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/02/03 13:16:22 UTC
[GitHub] [spark] senthh opened a new pull request #35390: [SPARK-37936][SQL] Use error classes in the parsing errors of intervals
senthh opened a new pull request #35390:
URL: https://github.com/apache/spark/pull/35390
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'core/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
In this PR, We propose to throw ParseException from below methods with the error classes:
moreThanOneFromToUnitInIntervalLiteralError
invalidIntervalLiteralError
invalidIntervalFormError
invalidFromToUnitValueError
fromToIntervalUnsupportedError
mixedIntervalUnitsError
MORE_THAN_ONE_FROM_TO_UNIT_IN_INTERVAL_LITERAL - moreThanOneFromToUnitInIntervalLiteralError
INVALID_INTERVAL_LITERAL - invalidIntervalLiteralError
INVALID_INTERVAL_FORM - invalidIntervalFormError
INVALID_FROM_TO_UNIT_VALUE - invalidFromToUnitValueError
UNSUPPORTED_FROM_TO_INTERVAL - fromToIntervalUnsupportedError
MIXED_INTERVAL_UNITS - mixedIntervalUnitsError
New error classes are added to error-classes.json.
New test suite QueryParsingErrorsSuite for checking the errors has been created.
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
Please refer [SPARK-37935](https://issues.apache.org/jira/browse/SPARK-37935) - Migrate onto error classes
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
No
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
New Test suite QueryParsingErrorsSuite created
--
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] senthh commented on a change in pull request #35390: [SPARK-37936][SQL] Use error classes in the parsing errors of intervals
Posted by GitBox <gi...@apache.org>.
senthh commented on a change in pull request #35390:
URL: https://github.com/apache/spark/pull/35390#discussion_r799100834
##########
File path: core/src/main/resources/error/error-classes.json
##########
@@ -138,6 +153,9 @@
"UNABLE_TO_ACQUIRE_MEMORY" : {
"message" : [ "Unable to acquire %s bytes of memory, got %s" ]
},
+ "UNSUPPORTED_FROM_TO_INTERVAL" : {
Review comment:
@MaxGekk Test queries are throwing messages similar to "Intervals FROM month TO second are not supported." and hence using UNSUPPORTED_FEATURE is leading to test failures in many places. So I think using UNSUPPORTED_FROM_TO_INTERVAL will be a good option and also it matches the exactly error messages thrown by the queries.
--
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] senthh commented on a change in pull request #35390: [SPARK-37936][SQL] Use error classes in the parsing errors of intervals
Posted by GitBox <gi...@apache.org>.
senthh commented on a change in pull request #35390:
URL: https://github.com/apache/spark/pull/35390#discussion_r800050353
##########
File path: core/src/main/resources/error/error-classes.json
##########
@@ -138,6 +153,9 @@
"UNABLE_TO_ACQUIRE_MEMORY" : {
"message" : [ "Unable to acquire %s bytes of memory, got %s" ]
},
+ "UNSUPPORTED_FROM_TO_INTERVAL" : {
Review comment:
@MaxGekk I have Modified out /ansi/interval.sql.out and results/interval.sql.out files in order to avoid test failure of query "select interval '1' year to second".
--
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] senthh commented on a change in pull request #35390: [SPARK-37936][SQL] Use error classes in the parsing errors of intervals
Posted by GitBox <gi...@apache.org>.
senthh commented on a change in pull request #35390:
URL: https://github.com/apache/spark/pull/35390#discussion_r800140909
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryParsingErrors.scala
##########
@@ -190,29 +190,31 @@ object QueryParsingErrors {
}
def moreThanOneFromToUnitInIntervalLiteralError(ctx: ParserRuleContext): Throwable = {
- new ParseException("Can only have a single from-to unit in the interval literal syntax", ctx)
+ new ParseException(s"MORE_THAN_ONE_FROM_TO_UNIT_IN_INTERVAL_LITERAL", Array.empty, ctx)
}
def invalidIntervalLiteralError(ctx: IntervalContext): Throwable = {
- new ParseException("at least one time unit should be given for interval literal", ctx)
+ new ParseException(s"INVALID_INTERVAL_LITERAL", Array.empty, ctx)
}
def invalidIntervalFormError(value: String, ctx: MultiUnitsIntervalContext): Throwable = {
- new ParseException("Can only use numbers in the interval value part for" +
- s" multiple unit value pairs interval form, but got invalid value: $value", ctx)
+ new ParseException(s"INVALID_INTERVAL_FORM", Array(value), ctx)
}
def invalidFromToUnitValueError(ctx: IntervalValueContext): Throwable = {
- new ParseException("The value of from-to unit must be a string", ctx)
+ new ParseException(s"INVALID_FROM_TO_UNIT_VALUE", Array.empty, ctx)
}
def fromToIntervalUnsupportedError(
from: String, to: String, ctx: ParserRuleContext): Throwable = {
- new ParseException(s"Intervals FROM $from TO $to are not supported.", ctx)
+ new ParseException(
+ errorClass = s"UNSUPPORTED_FEATURE",
+ messageParameters = Array(s"Intervals FROM $from TO $to are not supported"),
+ ctx)
}
def mixedIntervalUnitsError(literal: String, ctx: ParserRuleContext): Throwable = {
- new ParseException(s"Cannot mix year-month and day-time fields: $literal", ctx)
+ new ParseException(s"MIXED_INTERVAL_UNITS", Array(literal), ctx)
Review comment:
done
--
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] senthh commented on pull request #35390: [SPARK-37936][SQL] Use error classes in the parsing errors of intervals
Posted by GitBox <gi...@apache.org>.
senthh commented on pull request #35390:
URL: https://github.com/apache/spark/pull/35390#issuecomment-1030887285
@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] senthh commented on a change in pull request #35390: [SPARK-37936][SQL] Use error classes in the parsing errors of intervals
Posted by GitBox <gi...@apache.org>.
senthh commented on a change in pull request #35390:
URL: https://github.com/apache/spark/pull/35390#discussion_r800140873
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryParsingErrors.scala
##########
@@ -190,29 +190,31 @@ object QueryParsingErrors {
}
def moreThanOneFromToUnitInIntervalLiteralError(ctx: ParserRuleContext): Throwable = {
- new ParseException("Can only have a single from-to unit in the interval literal syntax", ctx)
+ new ParseException(s"MORE_THAN_ONE_FROM_TO_UNIT_IN_INTERVAL_LITERAL", Array.empty, ctx)
}
def invalidIntervalLiteralError(ctx: IntervalContext): Throwable = {
- new ParseException("at least one time unit should be given for interval literal", ctx)
+ new ParseException(s"INVALID_INTERVAL_LITERAL", Array.empty, ctx)
Review comment:
done
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryParsingErrors.scala
##########
@@ -190,29 +190,31 @@ object QueryParsingErrors {
}
def moreThanOneFromToUnitInIntervalLiteralError(ctx: ParserRuleContext): Throwable = {
- new ParseException("Can only have a single from-to unit in the interval literal syntax", ctx)
+ new ParseException(s"MORE_THAN_ONE_FROM_TO_UNIT_IN_INTERVAL_LITERAL", Array.empty, ctx)
}
def invalidIntervalLiteralError(ctx: IntervalContext): Throwable = {
- new ParseException("at least one time unit should be given for interval literal", ctx)
+ new ParseException(s"INVALID_INTERVAL_LITERAL", Array.empty, ctx)
}
def invalidIntervalFormError(value: String, ctx: MultiUnitsIntervalContext): Throwable = {
- new ParseException("Can only use numbers in the interval value part for" +
- s" multiple unit value pairs interval form, but got invalid value: $value", ctx)
+ new ParseException(s"INVALID_INTERVAL_FORM", Array(value), ctx)
}
def invalidFromToUnitValueError(ctx: IntervalValueContext): Throwable = {
- new ParseException("The value of from-to unit must be a string", ctx)
+ new ParseException(s"INVALID_FROM_TO_UNIT_VALUE", Array.empty, ctx)
Review comment:
done
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryParsingErrors.scala
##########
@@ -190,29 +190,31 @@ object QueryParsingErrors {
}
def moreThanOneFromToUnitInIntervalLiteralError(ctx: ParserRuleContext): Throwable = {
- new ParseException("Can only have a single from-to unit in the interval literal syntax", ctx)
+ new ParseException(s"MORE_THAN_ONE_FROM_TO_UNIT_IN_INTERVAL_LITERAL", Array.empty, ctx)
}
def invalidIntervalLiteralError(ctx: IntervalContext): Throwable = {
- new ParseException("at least one time unit should be given for interval literal", ctx)
+ new ParseException(s"INVALID_INTERVAL_LITERAL", Array.empty, ctx)
}
def invalidIntervalFormError(value: String, ctx: MultiUnitsIntervalContext): Throwable = {
- new ParseException("Can only use numbers in the interval value part for" +
- s" multiple unit value pairs interval form, but got invalid value: $value", ctx)
+ new ParseException(s"INVALID_INTERVAL_FORM", Array(value), ctx)
}
def invalidFromToUnitValueError(ctx: IntervalValueContext): Throwable = {
- new ParseException("The value of from-to unit must be a string", ctx)
+ new ParseException(s"INVALID_FROM_TO_UNIT_VALUE", Array.empty, ctx)
}
def fromToIntervalUnsupportedError(
from: String, to: String, ctx: ParserRuleContext): Throwable = {
- new ParseException(s"Intervals FROM $from TO $to are not supported.", ctx)
+ new ParseException(
+ errorClass = s"UNSUPPORTED_FEATURE",
Review comment:
done
--
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 change in pull request #35390: [SPARK-37936][SQL] Use error classes in the parsing errors of intervals
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on a change in pull request #35390:
URL: https://github.com/apache/spark/pull/35390#discussion_r798811339
##########
File path: core/src/main/resources/error/error-classes.json
##########
@@ -138,6 +153,9 @@
"UNABLE_TO_ACQUIRE_MEMORY" : {
"message" : [ "Unable to acquire %s bytes of memory, got %s" ]
},
+ "UNSUPPORTED_FROM_TO_INTERVAL" : {
Review comment:
Could you revise the error classes you added, and try to reuse existing classes. For example, this one `UNSUPPORTED_FROM_TO_INTERVAL` is not needed. You could use `UNSUPPORTED_FEATURE`
--
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 change in pull request #35390: [SPARK-37936][SQL] Use error classes in the parsing errors of intervals
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on a change in pull request #35390:
URL: https://github.com/apache/spark/pull/35390#discussion_r799337578
##########
File path: core/src/main/resources/error/error-classes.json
##########
@@ -138,6 +153,9 @@
"UNABLE_TO_ACQUIRE_MEMORY" : {
"message" : [ "Unable to acquire %s bytes of memory, got %s" ]
},
+ "UNSUPPORTED_FROM_TO_INTERVAL" : {
Review comment:
> then tests are failed in other test suits like "ExpressionParserSuite"
I think we should move such tests to QueryParsingErrorsSuite, QueryCompilationErrorsSuite or QueryExecutionErrorsSuite when ever it is possible.
> So using UNSUPPORTED_FROM_TO_INTERVAL error class will be suitable and also we can avoid other test case failures.
After offline discussion with @cloud-fan, we agreed to make our best efforts and use existing **general** error classes. See this PR, for instance https://github.com/apache/spark/pull/35302
--
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] senthh closed pull request #35390: [SPARK-37936][SQL] Use error classes in the parsing errors of intervals
Posted by GitBox <gi...@apache.org>.
senthh closed pull request #35390:
URL: https://github.com/apache/spark/pull/35390
--
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] senthh commented on pull request #35390: [SPARK-37936][SQL] Use error classes in the parsing errors of intervals
Posted by GitBox <gi...@apache.org>.
senthh commented on pull request #35390:
URL: https://github.com/apache/spark/pull/35390#issuecomment-1029714745
@MaxGekk , @cloud-fan Could you please re-review this PR?
--
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] senthh commented on pull request #35390: [SPARK-37936][SQL] Use error classes in the parsing errors of intervals
Posted by GitBox <gi...@apache.org>.
senthh commented on pull request #35390:
URL: https://github.com/apache/spark/pull/35390#issuecomment-1030608238
@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 commented on a change in pull request #35390: [SPARK-37936][SQL] Use error classes in the parsing errors of intervals
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on a change in pull request #35390:
URL: https://github.com/apache/spark/pull/35390#discussion_r800062116
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryParsingErrors.scala
##########
@@ -190,29 +190,31 @@ object QueryParsingErrors {
}
def moreThanOneFromToUnitInIntervalLiteralError(ctx: ParserRuleContext): Throwable = {
- new ParseException("Can only have a single from-to unit in the interval literal syntax", ctx)
+ new ParseException(s"MORE_THAN_ONE_FROM_TO_UNIT_IN_INTERVAL_LITERAL", Array.empty, ctx)
}
def invalidIntervalLiteralError(ctx: IntervalContext): Throwable = {
- new ParseException("at least one time unit should be given for interval literal", ctx)
+ new ParseException(s"INVALID_INTERVAL_LITERAL", Array.empty, ctx)
}
def invalidIntervalFormError(value: String, ctx: MultiUnitsIntervalContext): Throwable = {
- new ParseException("Can only use numbers in the interval value part for" +
- s" multiple unit value pairs interval form, but got invalid value: $value", ctx)
+ new ParseException(s"INVALID_INTERVAL_FORM", Array(value), ctx)
}
def invalidFromToUnitValueError(ctx: IntervalValueContext): Throwable = {
- new ParseException("The value of from-to unit must be a string", ctx)
+ new ParseException(s"INVALID_FROM_TO_UNIT_VALUE", Array.empty, ctx)
Review comment:
remove s
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/errors/QueryParsingErrorsSuite.scala
##########
@@ -78,4 +78,68 @@ class QueryParsingErrorsSuite extends QueryTest with SharedSparkSession {
message = "Invalid SQL syntax: LATERAL can only be used with subquery.")
}
}
+ test("MORE_THAN_ONE_FROM_TO_UNIT_IN_INTERVAL_LITERAL: from-to unit in the interval literal") {
+ val e = intercept[ParseException] {
+ spark.sql("SELECT INTERVAL 1 to 3 year to month AS col")
+ }
+ assert(e.getErrorClass === "MORE_THAN_ONE_FROM_TO_UNIT_IN_INTERVAL_LITERAL")
+ assert(e.getMessage.contains(
+ "Can only have a single from-to unit in the interval literal syntax"))
+ }
+
+ test("INVALID_INTERVAL_LITERAL: invalid interval literal") {
+ val e = intercept[ParseException] {
+ spark.sql("SELECT INTERVAL DAY")
+ }
+ assert(e.getErrorClass === "INVALID_INTERVAL_LITERAL")
+ assert(e.getMessage.contains(
+ "at least one time unit should be given for interval literal"))
+ }
+
+ test("INVALID_FROM_TO_UNIT_VALUE: value of from-to unit must be a string") {
+ val e = intercept[ParseException] {
+ spark.sql("SELECT INTERVAL -2021 YEAR TO MONTH")
+ }
+ assert(e.getErrorClass === "INVALID_FROM_TO_UNIT_VALUE")
+ assert(e.getMessage.contains(
+ "The value of from-to unit must be a string"))
+ }
+
+ test("UNSUPPORTED_FEATURE: Unsupported from-to interval") {
+ val e = intercept[ParseException] {
+ spark.sql("SELECT extract(MONTH FROM INTERVAL '2021-11' YEAR TO DAY)")
+ }
+ assert(e.getErrorClass === "UNSUPPORTED_FEATURE")
+ assert(e.getSqlState === "0A000")
+ assert(e.getMessage.contains("The feature is not supported"))
Review comment:
Could you check whole error message, please.
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryParsingErrors.scala
##########
@@ -190,29 +190,31 @@ object QueryParsingErrors {
}
def moreThanOneFromToUnitInIntervalLiteralError(ctx: ParserRuleContext): Throwable = {
- new ParseException("Can only have a single from-to unit in the interval literal syntax", ctx)
+ new ParseException(s"MORE_THAN_ONE_FROM_TO_UNIT_IN_INTERVAL_LITERAL", Array.empty, ctx)
}
def invalidIntervalLiteralError(ctx: IntervalContext): Throwable = {
- new ParseException("at least one time unit should be given for interval literal", ctx)
+ new ParseException(s"INVALID_INTERVAL_LITERAL", Array.empty, ctx)
Review comment:
remove s
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryParsingErrors.scala
##########
@@ -190,29 +190,31 @@ object QueryParsingErrors {
}
def moreThanOneFromToUnitInIntervalLiteralError(ctx: ParserRuleContext): Throwable = {
- new ParseException("Can only have a single from-to unit in the interval literal syntax", ctx)
+ new ParseException(s"MORE_THAN_ONE_FROM_TO_UNIT_IN_INTERVAL_LITERAL", Array.empty, ctx)
}
def invalidIntervalLiteralError(ctx: IntervalContext): Throwable = {
- new ParseException("at least one time unit should be given for interval literal", ctx)
+ new ParseException(s"INVALID_INTERVAL_LITERAL", Array.empty, ctx)
}
def invalidIntervalFormError(value: String, ctx: MultiUnitsIntervalContext): Throwable = {
- new ParseException("Can only use numbers in the interval value part for" +
- s" multiple unit value pairs interval form, but got invalid value: $value", ctx)
+ new ParseException(s"INVALID_INTERVAL_FORM", Array(value), ctx)
}
def invalidFromToUnitValueError(ctx: IntervalValueContext): Throwable = {
- new ParseException("The value of from-to unit must be a string", ctx)
+ new ParseException(s"INVALID_FROM_TO_UNIT_VALUE", Array.empty, ctx)
}
def fromToIntervalUnsupportedError(
from: String, to: String, ctx: ParserRuleContext): Throwable = {
- new ParseException(s"Intervals FROM $from TO $to are not supported.", ctx)
+ new ParseException(
+ errorClass = s"UNSUPPORTED_FEATURE",
Review comment:
remove s
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryParsingErrors.scala
##########
@@ -190,29 +190,31 @@ object QueryParsingErrors {
}
def moreThanOneFromToUnitInIntervalLiteralError(ctx: ParserRuleContext): Throwable = {
- new ParseException("Can only have a single from-to unit in the interval literal syntax", ctx)
+ new ParseException(s"MORE_THAN_ONE_FROM_TO_UNIT_IN_INTERVAL_LITERAL", Array.empty, ctx)
}
def invalidIntervalLiteralError(ctx: IntervalContext): Throwable = {
- new ParseException("at least one time unit should be given for interval literal", ctx)
+ new ParseException(s"INVALID_INTERVAL_LITERAL", Array.empty, ctx)
}
def invalidIntervalFormError(value: String, ctx: MultiUnitsIntervalContext): Throwable = {
- new ParseException("Can only use numbers in the interval value part for" +
- s" multiple unit value pairs interval form, but got invalid value: $value", ctx)
+ new ParseException(s"INVALID_INTERVAL_FORM", Array(value), ctx)
}
def invalidFromToUnitValueError(ctx: IntervalValueContext): Throwable = {
- new ParseException("The value of from-to unit must be a string", ctx)
+ new ParseException(s"INVALID_FROM_TO_UNIT_VALUE", Array.empty, ctx)
}
def fromToIntervalUnsupportedError(
from: String, to: String, ctx: ParserRuleContext): Throwable = {
- new ParseException(s"Intervals FROM $from TO $to are not supported.", ctx)
+ new ParseException(
+ errorClass = s"UNSUPPORTED_FEATURE",
+ messageParameters = Array(s"Intervals FROM $from TO $to are not supported"),
+ ctx)
}
def mixedIntervalUnitsError(literal: String, ctx: ParserRuleContext): Throwable = {
- new ParseException(s"Cannot mix year-month and day-time fields: $literal", ctx)
+ new ParseException(s"MIXED_INTERVAL_UNITS", Array(literal), ctx)
Review comment:
remove s
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/errors/QueryParsingErrorsSuite.scala
##########
@@ -78,4 +78,68 @@ class QueryParsingErrorsSuite extends QueryTest with SharedSparkSession {
message = "Invalid SQL syntax: LATERAL can only be used with subquery.")
}
}
+ test("MORE_THAN_ONE_FROM_TO_UNIT_IN_INTERVAL_LITERAL: from-to unit in the interval literal") {
+ val e = intercept[ParseException] {
+ spark.sql("SELECT INTERVAL 1 to 3 year to month AS col")
+ }
+ assert(e.getErrorClass === "MORE_THAN_ONE_FROM_TO_UNIT_IN_INTERVAL_LITERAL")
+ assert(e.getMessage.contains(
+ "Can only have a single from-to unit in the interval literal syntax"))
+ }
+
+ test("INVALID_INTERVAL_LITERAL: invalid interval literal") {
+ val e = intercept[ParseException] {
+ spark.sql("SELECT INTERVAL DAY")
+ }
+ assert(e.getErrorClass === "INVALID_INTERVAL_LITERAL")
+ assert(e.getMessage.contains(
+ "at least one time unit should be given for interval literal"))
+ }
+
+ test("INVALID_FROM_TO_UNIT_VALUE: value of from-to unit must be a string") {
+ val e = intercept[ParseException] {
+ spark.sql("SELECT INTERVAL -2021 YEAR TO MONTH")
+ }
+ assert(e.getErrorClass === "INVALID_FROM_TO_UNIT_VALUE")
+ assert(e.getMessage.contains(
+ "The value of from-to unit must be a string"))
+ }
+
+ test("UNSUPPORTED_FEATURE: Unsupported from-to interval") {
+ val e = intercept[ParseException] {
+ spark.sql("SELECT extract(MONTH FROM INTERVAL '2021-11' YEAR TO DAY)")
+ }
+ assert(e.getErrorClass === "UNSUPPORTED_FEATURE")
+ assert(e.getSqlState === "0A000")
+ assert(e.getMessage.contains("The feature is not supported"))
+ }
+
+ // Moved from ExpressionParserSuite
+ test("UNSUPPORTED_FEATURE: Unsupported month to second interval") {
+ val e = intercept[ParseException] {
+ spark.sql("SELECT interval '10' month to second")
+ }
+ assert(e.getErrorClass === "UNSUPPORTED_FEATURE")
+ assert(e.getSqlState === "0A000")
+ assert(e.getMessage.contains("The feature is not supported"))
+ }
+
+ test("MIXED_INTERVAL_UNITS: Cannot mix year-month and day-time fields") {
+ val e = intercept[ParseException] {
+ spark.sql("SELECT INTERVAL 1 MONTH 2 HOUR")
+ }
+ assert(e.getErrorClass === "MIXED_INTERVAL_UNITS")
+
Review comment:
remove the empty line
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryParsingErrors.scala
##########
@@ -190,29 +190,31 @@ object QueryParsingErrors {
}
def moreThanOneFromToUnitInIntervalLiteralError(ctx: ParserRuleContext): Throwable = {
- new ParseException("Can only have a single from-to unit in the interval literal syntax", ctx)
+ new ParseException(s"MORE_THAN_ONE_FROM_TO_UNIT_IN_INTERVAL_LITERAL", Array.empty, ctx)
Review comment:
s is not needed before "
##########
File path: sql/core/src/test/resources/sql-tests/results/ansi/interval.sql.out
##########
@@ -1213,7 +1213,7 @@ struct<>
-- !query output
org.apache.spark.sql.catalyst.parser.ParseException
-Intervals FROM year TO second are not supported.(line 1, pos 16)
+The feature is not supported: Intervals FROM year TO second are not supported(line 1, pos 16)
Review comment:
It would be better to put a point at the end:
not supported(line -> not supported.(line
--
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] senthh commented on a change in pull request #35390: [SPARK-37936][SQL] Use error classes in the parsing errors of intervals
Posted by GitBox <gi...@apache.org>.
senthh commented on a change in pull request #35390:
URL: https://github.com/apache/spark/pull/35390#discussion_r800140866
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/errors/QueryParsingErrors.scala
##########
@@ -190,29 +190,31 @@ object QueryParsingErrors {
}
def moreThanOneFromToUnitInIntervalLiteralError(ctx: ParserRuleContext): Throwable = {
- new ParseException("Can only have a single from-to unit in the interval literal syntax", ctx)
+ new ParseException(s"MORE_THAN_ONE_FROM_TO_UNIT_IN_INTERVAL_LITERAL", Array.empty, ctx)
Review comment:
done
--
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] AmplabJenkins commented on pull request #35390: [SPARK-37936][SQL] Use error classes in the parsing errors of intervals
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #35390:
URL: https://github.com/apache/spark/pull/35390#issuecomment-1028995764
Can one of the admins verify this patch?
--
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] senthh commented on a change in pull request #35390: [SPARK-37936][SQL] Use error classes in the parsing errors of intervals
Posted by GitBox <gi...@apache.org>.
senthh commented on a change in pull request #35390:
URL: https://github.com/apache/spark/pull/35390#discussion_r798848736
##########
File path: core/src/main/resources/error/error-classes.json
##########
@@ -138,6 +153,9 @@
"UNABLE_TO_ACQUIRE_MEMORY" : {
"message" : [ "Unable to acquire %s bytes of memory, got %s" ]
},
+ "UNSUPPORTED_FROM_TO_INTERVAL" : {
Review comment:
Sure
--
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] senthh commented on a change in pull request #35390: [SPARK-37936][SQL] Use error classes in the parsing errors of intervals
Posted by GitBox <gi...@apache.org>.
senthh commented on a change in pull request #35390:
URL: https://github.com/apache/spark/pull/35390#discussion_r800140917
##########
File path: sql/core/src/test/resources/sql-tests/results/ansi/interval.sql.out
##########
@@ -1213,7 +1213,7 @@ struct<>
-- !query output
org.apache.spark.sql.catalyst.parser.ParseException
-Intervals FROM year TO second are not supported.(line 1, pos 16)
+The feature is not supported: Intervals FROM year TO second are not supported(line 1, pos 16)
Review comment:
Yes included
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/errors/QueryParsingErrorsSuite.scala
##########
@@ -78,4 +78,68 @@ class QueryParsingErrorsSuite extends QueryTest with SharedSparkSession {
message = "Invalid SQL syntax: LATERAL can only be used with subquery.")
}
}
+ test("MORE_THAN_ONE_FROM_TO_UNIT_IN_INTERVAL_LITERAL: from-to unit in the interval literal") {
+ val e = intercept[ParseException] {
+ spark.sql("SELECT INTERVAL 1 to 3 year to month AS col")
+ }
+ assert(e.getErrorClass === "MORE_THAN_ONE_FROM_TO_UNIT_IN_INTERVAL_LITERAL")
+ assert(e.getMessage.contains(
+ "Can only have a single from-to unit in the interval literal syntax"))
+ }
+
+ test("INVALID_INTERVAL_LITERAL: invalid interval literal") {
+ val e = intercept[ParseException] {
+ spark.sql("SELECT INTERVAL DAY")
+ }
+ assert(e.getErrorClass === "INVALID_INTERVAL_LITERAL")
+ assert(e.getMessage.contains(
+ "at least one time unit should be given for interval literal"))
+ }
+
+ test("INVALID_FROM_TO_UNIT_VALUE: value of from-to unit must be a string") {
+ val e = intercept[ParseException] {
+ spark.sql("SELECT INTERVAL -2021 YEAR TO MONTH")
+ }
+ assert(e.getErrorClass === "INVALID_FROM_TO_UNIT_VALUE")
+ assert(e.getMessage.contains(
+ "The value of from-to unit must be a string"))
+ }
+
+ test("UNSUPPORTED_FEATURE: Unsupported from-to interval") {
+ val e = intercept[ParseException] {
+ spark.sql("SELECT extract(MONTH FROM INTERVAL '2021-11' YEAR TO DAY)")
+ }
+ assert(e.getErrorClass === "UNSUPPORTED_FEATURE")
+ assert(e.getSqlState === "0A000")
+ assert(e.getMessage.contains("The feature is not supported"))
+ }
+
+ // Moved from ExpressionParserSuite
+ test("UNSUPPORTED_FEATURE: Unsupported month to second interval") {
+ val e = intercept[ParseException] {
+ spark.sql("SELECT interval '10' month to second")
+ }
+ assert(e.getErrorClass === "UNSUPPORTED_FEATURE")
+ assert(e.getSqlState === "0A000")
+ assert(e.getMessage.contains("The feature is not supported"))
+ }
+
+ test("MIXED_INTERVAL_UNITS: Cannot mix year-month and day-time fields") {
+ val e = intercept[ParseException] {
+ spark.sql("SELECT INTERVAL 1 MONTH 2 HOUR")
+ }
+ assert(e.getErrorClass === "MIXED_INTERVAL_UNITS")
+
Review comment:
yes removed
--
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] senthh commented on a change in pull request #35390: [SPARK-37936][SQL] Use error classes in the parsing errors of intervals
Posted by GitBox <gi...@apache.org>.
senthh commented on a change in pull request #35390:
URL: https://github.com/apache/spark/pull/35390#discussion_r800140572
##########
File path: sql/core/src/test/scala/org/apache/spark/sql/errors/QueryParsingErrorsSuite.scala
##########
@@ -78,4 +78,68 @@ class QueryParsingErrorsSuite extends QueryTest with SharedSparkSession {
message = "Invalid SQL syntax: LATERAL can only be used with subquery.")
}
}
+ test("MORE_THAN_ONE_FROM_TO_UNIT_IN_INTERVAL_LITERAL: from-to unit in the interval literal") {
+ val e = intercept[ParseException] {
+ spark.sql("SELECT INTERVAL 1 to 3 year to month AS col")
+ }
+ assert(e.getErrorClass === "MORE_THAN_ONE_FROM_TO_UNIT_IN_INTERVAL_LITERAL")
+ assert(e.getMessage.contains(
+ "Can only have a single from-to unit in the interval literal syntax"))
+ }
+
+ test("INVALID_INTERVAL_LITERAL: invalid interval literal") {
+ val e = intercept[ParseException] {
+ spark.sql("SELECT INTERVAL DAY")
+ }
+ assert(e.getErrorClass === "INVALID_INTERVAL_LITERAL")
+ assert(e.getMessage.contains(
+ "at least one time unit should be given for interval literal"))
+ }
+
+ test("INVALID_FROM_TO_UNIT_VALUE: value of from-to unit must be a string") {
+ val e = intercept[ParseException] {
+ spark.sql("SELECT INTERVAL -2021 YEAR TO MONTH")
+ }
+ assert(e.getErrorClass === "INVALID_FROM_TO_UNIT_VALUE")
+ assert(e.getMessage.contains(
+ "The value of from-to unit must be a string"))
+ }
+
+ test("UNSUPPORTED_FEATURE: Unsupported from-to interval") {
+ val e = intercept[ParseException] {
+ spark.sql("SELECT extract(MONTH FROM INTERVAL '2021-11' YEAR TO DAY)")
+ }
+ assert(e.getErrorClass === "UNSUPPORTED_FEATURE")
+ assert(e.getSqlState === "0A000")
+ assert(e.getMessage.contains("The feature is not supported"))
Review comment:
@MaxGekk Changed as per review comments.
--
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] senthh commented on a change in pull request #35390: [SPARK-37936][SQL] Use error classes in the parsing errors of intervals
Posted by GitBox <gi...@apache.org>.
senthh commented on a change in pull request #35390:
URL: https://github.com/apache/spark/pull/35390#discussion_r799656703
##########
File path: core/src/main/resources/error/error-classes.json
##########
@@ -138,6 +153,9 @@
"UNABLE_TO_ACQUIRE_MEMORY" : {
"message" : [ "Unable to acquire %s bytes of memory, got %s" ]
},
+ "UNSUPPORTED_FROM_TO_INTERVAL" : {
Review comment:
@MaxGekk As per the above input, I have moved test query from ExpressionParserSuite.scala and Interval.sql to QueryParsingErrorSuite.
--
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] senthh commented on a change in pull request #35390: [SPARK-37936][SQL] Use error classes in the parsing errors of intervals
Posted by GitBox <gi...@apache.org>.
senthh commented on a change in pull request #35390:
URL: https://github.com/apache/spark/pull/35390#discussion_r799656703
##########
File path: core/src/main/resources/error/error-classes.json
##########
@@ -138,6 +153,9 @@
"UNABLE_TO_ACQUIRE_MEMORY" : {
"message" : [ "Unable to acquire %s bytes of memory, got %s" ]
},
+ "UNSUPPORTED_FROM_TO_INTERVAL" : {
Review comment:
@MaxGekk As per the above input, I have moved test query from ExpressionParserSuite.scala to QueryParsingErrorSuite.
--
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 change in pull request #35390: [SPARK-37936][SQL] Use error classes in the parsing errors of intervals
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on a change in pull request #35390:
URL: https://github.com/apache/spark/pull/35390#discussion_r798811339
##########
File path: core/src/main/resources/error/error-classes.json
##########
@@ -138,6 +153,9 @@
"UNABLE_TO_ACQUIRE_MEMORY" : {
"message" : [ "Unable to acquire %s bytes of memory, got %s" ]
},
+ "UNSUPPORTED_FROM_TO_INTERVAL" : {
Review comment:
Could you revisit the error classes you added, and try to reuse existing classes. For example, this one `UNSUPPORTED_FROM_TO_INTERVAL` is not needed. You could use `UNSUPPORTED_FEATURE`
--
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] senthh commented on a change in pull request #35390: [SPARK-37936][SQL] Use error classes in the parsing errors of intervals
Posted by GitBox <gi...@apache.org>.
senthh commented on a change in pull request #35390:
URL: https://github.com/apache/spark/pull/35390#discussion_r799213361
##########
File path: core/src/main/resources/error/error-classes.json
##########
@@ -138,6 +153,9 @@
"UNABLE_TO_ACQUIRE_MEMORY" : {
"message" : [ "Unable to acquire %s bytes of memory, got %s" ]
},
+ "UNSUPPORTED_FROM_TO_INTERVAL" : {
Review comment:
@MaxGekk Error message "Intervals FROM * TO * are not supported" is thrown from many places(AstBuilder, Interval_sql etc.) so if we reuse UNSUPPORTED_FEATURE as an alternate to UNSUPPORTED_FROM_TO_INTERVAL , then tests are failed in other test suits like "ExpressionParserSuite" which is not related to this jira. So using UNSUPPORTED_FROM_TO_INTERVAL error class will be suitable and also we can avoid other test case failures.
--
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