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/03/09 11:59:17 UTC
[GitHub] [spark] MaxGekk opened a new pull request #35787: [SPARK-38481][SQL] Substitute Java overflow exception from `TIMESTAMPADD` by Spark exception
MaxGekk opened a new pull request #35787:
URL: https://github.com/apache/spark/pull/35787
### What changes were proposed in this pull request?
In the PR, I propose to throw `SparkArithmeticException` from the datetime function: `timestampadd()` and from its aliases `date_add()`/`dateadd()` with the error class `DATETIME_OVERFLOW` in the case when internal arithmetic or datetime overflow occurs. The new error classes are added to `error-classes.json`.
### Why are the changes needed?
Porting the functions to new error framework should improve user experience with Spark SQL.
Before the changes:
```sql
spark-sql> select timestampadd(YEAR, 1000000, timestamp'2022-03-09 01:02:03');
java.lang.ArithmeticException: long overflow
at java.lang.Math.multiplyExact(Math.java:892) ~[?:1.8.0_292]
```
After:
```sql
spark-sql> select timestampadd(YEAR, 1000000, timestamp'2022-03-09 01:02:03');
org.apache.spark.SparkArithmeticException: The 'timestampadd' function overflows the input '2022-03-08T22:02:03Z' timestamp by 1000000 YEAR.
```
### Does this PR introduce _any_ user-facing change?
Yes, but the datetime functions `timestampadd()` and its aliases `dateadd()`/`date_add()` haven't released yet.
### How was this patch tested?
By running the affected test suites:
```
$ build/sbt "test:testOnly *SparkThrowableSuite"
```
and new test:
```
$ build/sbt "test:testOnly *QueryExecutionErrorsSuite"
```
--
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 #35787: [SPARK-38481][SQL] Substitute Java overflow exception from `TIMESTAMPADD` by Spark exception
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on a change in pull request #35787:
URL: https://github.com/apache/spark/pull/35787#discussion_r823379716
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala
##########
@@ -1174,29 +1174,36 @@ object DateTimeUtils {
* @return A timestamp value, expressed in microseconds since 1970-01-01 00:00:00Z.
*/
def timestampAdd(unit: String, quantity: Int, micros: Long, zoneId: ZoneId): Long = {
- unit.toUpperCase(Locale.ROOT) match {
- case "MICROSECOND" =>
- timestampAddDayTime(micros, quantity, zoneId)
- case "MILLISECOND" =>
- timestampAddDayTime(micros, quantity * MICROS_PER_MILLIS, zoneId)
- case "SECOND" =>
- timestampAddDayTime(micros, quantity * MICROS_PER_SECOND, zoneId)
- case "MINUTE" =>
- timestampAddDayTime(micros, quantity * MICROS_PER_MINUTE, zoneId)
- case "HOUR" =>
- timestampAddDayTime(micros, quantity * MICROS_PER_HOUR, zoneId)
- case "DAY" | "DAYOFYEAR" =>
- timestampAddDayTime(micros, quantity * MICROS_PER_DAY, zoneId)
- case "WEEK" =>
- timestampAddDayTime(micros, quantity * MICROS_PER_DAY * DAYS_PER_WEEK, zoneId)
- case "MONTH" =>
- timestampAddMonths(micros, quantity, zoneId)
- case "QUARTER" =>
- timestampAddMonths(micros, quantity * 3, zoneId)
- case "YEAR" =>
- timestampAddMonths(micros, quantity * MONTHS_PER_YEAR, zoneId)
- case _ =>
+ try {
+ unit.toUpperCase(Locale.ROOT) match {
+ case "MICROSECOND" =>
+ timestampAddDayTime(micros, quantity, zoneId)
+ case "MILLISECOND" =>
+ timestampAddDayTime(micros, quantity * MICROS_PER_MILLIS, zoneId)
+ case "SECOND" =>
+ timestampAddDayTime(micros, quantity * MICROS_PER_SECOND, zoneId)
+ case "MINUTE" =>
+ timestampAddDayTime(micros, quantity * MICROS_PER_MINUTE, zoneId)
+ case "HOUR" =>
+ timestampAddDayTime(micros, quantity * MICROS_PER_HOUR, zoneId)
+ case "DAY" | "DAYOFYEAR" =>
+ timestampAddDayTime(micros, quantity * MICROS_PER_DAY, zoneId)
+ case "WEEK" =>
+ timestampAddDayTime(micros, quantity * MICROS_PER_DAY * DAYS_PER_WEEK, zoneId)
+ case "MONTH" =>
+ timestampAddMonths(micros, quantity, zoneId)
+ case "QUARTER" =>
+ timestampAddMonths(micros, quantity * 3, zoneId)
+ case "YEAR" =>
+ timestampAddMonths(micros, quantity * MONTHS_PER_YEAR, zoneId)
+ }
+ } catch {
+ case _: scala.MatchError =>
throw QueryExecutionErrors.invalidUnitInTimestampAdd(unit)
+ case _: ArithmeticException | _: DateTimeException =>
+ throw QueryExecutionErrors.datetimeOverflowError("timestampadd", micros, quantity, unit)
Review comment:
or if we are going to re-use the error class in other ops:
```
"DATETIME_OVERFLOW" : {
"message" : [ "Datetime operation overflow: %s." ],
"sqlState" : "22008"
},
```
which users see as:
```
Datetime operation overflow: add 100000 YEAR to '2022-03-10T11:12:13Z'.
```
--
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 change in pull request #35787: [SPARK-38481][SQL] Substitute Java overflow exception from `TIMESTAMPADD` by Spark exception
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35787:
URL: https://github.com/apache/spark/pull/35787#discussion_r823338127
##########
File path: core/src/main/resources/error/error-classes.json
##########
@@ -25,6 +25,10 @@
"CONCURRENT_QUERY" : {
"message" : [ "Another instance of this query was just started by a concurrent session." ]
},
+ "DATETIME_OVERFLOW" : {
+ "message" : [ "The '%s' function overflows the input '%s' timestamp by %s %s." ],
Review comment:
can we make it more general? `Datetime operation overflows.` There are so many datetime operations, I think we can omit the input details.
--
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 #35787: [SPARK-38481][SQL] Substitute Java overflow exception from `TIMESTAMPADD` by Spark exception
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on a change in pull request #35787:
URL: https://github.com/apache/spark/pull/35787#discussion_r823379716
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala
##########
@@ -1174,29 +1174,36 @@ object DateTimeUtils {
* @return A timestamp value, expressed in microseconds since 1970-01-01 00:00:00Z.
*/
def timestampAdd(unit: String, quantity: Int, micros: Long, zoneId: ZoneId): Long = {
- unit.toUpperCase(Locale.ROOT) match {
- case "MICROSECOND" =>
- timestampAddDayTime(micros, quantity, zoneId)
- case "MILLISECOND" =>
- timestampAddDayTime(micros, quantity * MICROS_PER_MILLIS, zoneId)
- case "SECOND" =>
- timestampAddDayTime(micros, quantity * MICROS_PER_SECOND, zoneId)
- case "MINUTE" =>
- timestampAddDayTime(micros, quantity * MICROS_PER_MINUTE, zoneId)
- case "HOUR" =>
- timestampAddDayTime(micros, quantity * MICROS_PER_HOUR, zoneId)
- case "DAY" | "DAYOFYEAR" =>
- timestampAddDayTime(micros, quantity * MICROS_PER_DAY, zoneId)
- case "WEEK" =>
- timestampAddDayTime(micros, quantity * MICROS_PER_DAY * DAYS_PER_WEEK, zoneId)
- case "MONTH" =>
- timestampAddMonths(micros, quantity, zoneId)
- case "QUARTER" =>
- timestampAddMonths(micros, quantity * 3, zoneId)
- case "YEAR" =>
- timestampAddMonths(micros, quantity * MONTHS_PER_YEAR, zoneId)
- case _ =>
+ try {
+ unit.toUpperCase(Locale.ROOT) match {
+ case "MICROSECOND" =>
+ timestampAddDayTime(micros, quantity, zoneId)
+ case "MILLISECOND" =>
+ timestampAddDayTime(micros, quantity * MICROS_PER_MILLIS, zoneId)
+ case "SECOND" =>
+ timestampAddDayTime(micros, quantity * MICROS_PER_SECOND, zoneId)
+ case "MINUTE" =>
+ timestampAddDayTime(micros, quantity * MICROS_PER_MINUTE, zoneId)
+ case "HOUR" =>
+ timestampAddDayTime(micros, quantity * MICROS_PER_HOUR, zoneId)
+ case "DAY" | "DAYOFYEAR" =>
+ timestampAddDayTime(micros, quantity * MICROS_PER_DAY, zoneId)
+ case "WEEK" =>
+ timestampAddDayTime(micros, quantity * MICROS_PER_DAY * DAYS_PER_WEEK, zoneId)
+ case "MONTH" =>
+ timestampAddMonths(micros, quantity, zoneId)
+ case "QUARTER" =>
+ timestampAddMonths(micros, quantity * 3, zoneId)
+ case "YEAR" =>
+ timestampAddMonths(micros, quantity * MONTHS_PER_YEAR, zoneId)
+ }
+ } catch {
+ case _: scala.MatchError =>
throw QueryExecutionErrors.invalidUnitInTimestampAdd(unit)
+ case _: ArithmeticException | _: DateTimeException =>
+ throw QueryExecutionErrors.datetimeOverflowError("timestampadd", micros, quantity, unit)
Review comment:
or if we are going to re-use the error class in other ops:
```
"DATETIME_OVERFLOW" : {
"message" : [ "Datetime operation overflow: %s." ],
"sqlState" : "22008"
},
```
which users see as:
```
Datetime operation overflow: add 10000000 YEAR to '2022-03-10T11:12:13Z'.
```
--
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] HyukjinKwon commented on a change in pull request #35787: [SPARK-38481][SQL] Substitute Java overflow exception from `TIMESTAMPADD` by Spark exception
Posted by GitBox <gi...@apache.org>.
HyukjinKwon commented on a change in pull request #35787:
URL: https://github.com/apache/spark/pull/35787#discussion_r823259174
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala
##########
@@ -1174,29 +1174,36 @@ object DateTimeUtils {
* @return A timestamp value, expressed in microseconds since 1970-01-01 00:00:00Z.
*/
def timestampAdd(unit: String, quantity: Int, micros: Long, zoneId: ZoneId): Long = {
- unit.toUpperCase(Locale.ROOT) match {
- case "MICROSECOND" =>
- timestampAddDayTime(micros, quantity, zoneId)
- case "MILLISECOND" =>
- timestampAddDayTime(micros, quantity * MICROS_PER_MILLIS, zoneId)
- case "SECOND" =>
- timestampAddDayTime(micros, quantity * MICROS_PER_SECOND, zoneId)
- case "MINUTE" =>
- timestampAddDayTime(micros, quantity * MICROS_PER_MINUTE, zoneId)
- case "HOUR" =>
- timestampAddDayTime(micros, quantity * MICROS_PER_HOUR, zoneId)
- case "DAY" | "DAYOFYEAR" =>
- timestampAddDayTime(micros, quantity * MICROS_PER_DAY, zoneId)
- case "WEEK" =>
- timestampAddDayTime(micros, quantity * MICROS_PER_DAY * DAYS_PER_WEEK, zoneId)
- case "MONTH" =>
- timestampAddMonths(micros, quantity, zoneId)
- case "QUARTER" =>
- timestampAddMonths(micros, quantity * 3, zoneId)
- case "YEAR" =>
- timestampAddMonths(micros, quantity * MONTHS_PER_YEAR, zoneId)
- case _ =>
+ try {
+ unit.toUpperCase(Locale.ROOT) match {
+ case "MICROSECOND" =>
+ timestampAddDayTime(micros, quantity, zoneId)
+ case "MILLISECOND" =>
+ timestampAddDayTime(micros, quantity * MICROS_PER_MILLIS, zoneId)
+ case "SECOND" =>
+ timestampAddDayTime(micros, quantity * MICROS_PER_SECOND, zoneId)
+ case "MINUTE" =>
+ timestampAddDayTime(micros, quantity * MICROS_PER_MINUTE, zoneId)
+ case "HOUR" =>
+ timestampAddDayTime(micros, quantity * MICROS_PER_HOUR, zoneId)
+ case "DAY" | "DAYOFYEAR" =>
+ timestampAddDayTime(micros, quantity * MICROS_PER_DAY, zoneId)
+ case "WEEK" =>
+ timestampAddDayTime(micros, quantity * MICROS_PER_DAY * DAYS_PER_WEEK, zoneId)
+ case "MONTH" =>
+ timestampAddMonths(micros, quantity, zoneId)
+ case "QUARTER" =>
+ timestampAddMonths(micros, quantity * 3, zoneId)
+ case "YEAR" =>
+ timestampAddMonths(micros, quantity * MONTHS_PER_YEAR, zoneId)
+ }
+ } catch {
+ case _: scala.MatchError =>
throw QueryExecutionErrors.invalidUnitInTimestampAdd(unit)
+ case _: ArithmeticException | _: DateTimeException =>
+ throw QueryExecutionErrors.datetimeOverflowError("timestampadd", micros, quantity, unit)
Review comment:
This is a good point. Probably we should make it as a prose without showing the internal method name.
--
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 change in pull request #35787: [SPARK-38481][SQL] Substitute Java overflow exception from `TIMESTAMPADD` by Spark exception
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35787:
URL: https://github.com/apache/spark/pull/35787#discussion_r823425901
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala
##########
@@ -1174,29 +1174,36 @@ object DateTimeUtils {
* @return A timestamp value, expressed in microseconds since 1970-01-01 00:00:00Z.
*/
def timestampAdd(unit: String, quantity: Int, micros: Long, zoneId: ZoneId): Long = {
- unit.toUpperCase(Locale.ROOT) match {
- case "MICROSECOND" =>
- timestampAddDayTime(micros, quantity, zoneId)
- case "MILLISECOND" =>
- timestampAddDayTime(micros, quantity * MICROS_PER_MILLIS, zoneId)
- case "SECOND" =>
- timestampAddDayTime(micros, quantity * MICROS_PER_SECOND, zoneId)
- case "MINUTE" =>
- timestampAddDayTime(micros, quantity * MICROS_PER_MINUTE, zoneId)
- case "HOUR" =>
- timestampAddDayTime(micros, quantity * MICROS_PER_HOUR, zoneId)
- case "DAY" | "DAYOFYEAR" =>
- timestampAddDayTime(micros, quantity * MICROS_PER_DAY, zoneId)
- case "WEEK" =>
- timestampAddDayTime(micros, quantity * MICROS_PER_DAY * DAYS_PER_WEEK, zoneId)
- case "MONTH" =>
- timestampAddMonths(micros, quantity, zoneId)
- case "QUARTER" =>
- timestampAddMonths(micros, quantity * 3, zoneId)
- case "YEAR" =>
- timestampAddMonths(micros, quantity * MONTHS_PER_YEAR, zoneId)
- case _ =>
+ try {
+ unit.toUpperCase(Locale.ROOT) match {
+ case "MICROSECOND" =>
+ timestampAddDayTime(micros, quantity, zoneId)
+ case "MILLISECOND" =>
+ timestampAddDayTime(micros, quantity * MICROS_PER_MILLIS, zoneId)
+ case "SECOND" =>
+ timestampAddDayTime(micros, quantity * MICROS_PER_SECOND, zoneId)
+ case "MINUTE" =>
+ timestampAddDayTime(micros, quantity * MICROS_PER_MINUTE, zoneId)
+ case "HOUR" =>
+ timestampAddDayTime(micros, quantity * MICROS_PER_HOUR, zoneId)
+ case "DAY" | "DAYOFYEAR" =>
+ timestampAddDayTime(micros, quantity * MICROS_PER_DAY, zoneId)
+ case "WEEK" =>
+ timestampAddDayTime(micros, quantity * MICROS_PER_DAY * DAYS_PER_WEEK, zoneId)
+ case "MONTH" =>
+ timestampAddMonths(micros, quantity, zoneId)
+ case "QUARTER" =>
+ timestampAddMonths(micros, quantity * 3, zoneId)
+ case "YEAR" =>
+ timestampAddMonths(micros, quantity * MONTHS_PER_YEAR, zoneId)
+ }
+ } catch {
+ case _: scala.MatchError =>
throw QueryExecutionErrors.invalidUnitInTimestampAdd(unit)
+ case _: ArithmeticException | _: DateTimeException =>
+ throw QueryExecutionErrors.datetimeOverflowError("timestampadd", micros, quantity, unit)
Review comment:
The above LGTM.
--
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 #35787: [SPARK-38481][SQL] Substitute Java overflow exception from `TIMESTAMPADD` by Spark exception
Posted by GitBox <gi...@apache.org>.
MaxGekk closed pull request #35787:
URL: https://github.com/apache/spark/pull/35787
--
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] entong commented on a change in pull request #35787: [SPARK-38481][SQL] Substitute Java overflow exception from `TIMESTAMPADD` by Spark exception
Posted by GitBox <gi...@apache.org>.
entong commented on a change in pull request #35787:
URL: https://github.com/apache/spark/pull/35787#discussion_r823241977
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala
##########
@@ -1174,29 +1174,36 @@ object DateTimeUtils {
* @return A timestamp value, expressed in microseconds since 1970-01-01 00:00:00Z.
*/
def timestampAdd(unit: String, quantity: Int, micros: Long, zoneId: ZoneId): Long = {
- unit.toUpperCase(Locale.ROOT) match {
- case "MICROSECOND" =>
- timestampAddDayTime(micros, quantity, zoneId)
- case "MILLISECOND" =>
- timestampAddDayTime(micros, quantity * MICROS_PER_MILLIS, zoneId)
- case "SECOND" =>
- timestampAddDayTime(micros, quantity * MICROS_PER_SECOND, zoneId)
- case "MINUTE" =>
- timestampAddDayTime(micros, quantity * MICROS_PER_MINUTE, zoneId)
- case "HOUR" =>
- timestampAddDayTime(micros, quantity * MICROS_PER_HOUR, zoneId)
- case "DAY" | "DAYOFYEAR" =>
- timestampAddDayTime(micros, quantity * MICROS_PER_DAY, zoneId)
- case "WEEK" =>
- timestampAddDayTime(micros, quantity * MICROS_PER_DAY * DAYS_PER_WEEK, zoneId)
- case "MONTH" =>
- timestampAddMonths(micros, quantity, zoneId)
- case "QUARTER" =>
- timestampAddMonths(micros, quantity * 3, zoneId)
- case "YEAR" =>
- timestampAddMonths(micros, quantity * MONTHS_PER_YEAR, zoneId)
- case _ =>
+ try {
+ unit.toUpperCase(Locale.ROOT) match {
+ case "MICROSECOND" =>
+ timestampAddDayTime(micros, quantity, zoneId)
+ case "MILLISECOND" =>
+ timestampAddDayTime(micros, quantity * MICROS_PER_MILLIS, zoneId)
+ case "SECOND" =>
+ timestampAddDayTime(micros, quantity * MICROS_PER_SECOND, zoneId)
+ case "MINUTE" =>
+ timestampAddDayTime(micros, quantity * MICROS_PER_MINUTE, zoneId)
+ case "HOUR" =>
+ timestampAddDayTime(micros, quantity * MICROS_PER_HOUR, zoneId)
+ case "DAY" | "DAYOFYEAR" =>
+ timestampAddDayTime(micros, quantity * MICROS_PER_DAY, zoneId)
+ case "WEEK" =>
+ timestampAddDayTime(micros, quantity * MICROS_PER_DAY * DAYS_PER_WEEK, zoneId)
+ case "MONTH" =>
+ timestampAddMonths(micros, quantity, zoneId)
+ case "QUARTER" =>
+ timestampAddMonths(micros, quantity * 3, zoneId)
+ case "YEAR" =>
+ timestampAddMonths(micros, quantity * MONTHS_PER_YEAR, zoneId)
+ }
+ } catch {
+ case _: scala.MatchError =>
throw QueryExecutionErrors.invalidUnitInTimestampAdd(unit)
+ case _: ArithmeticException | _: DateTimeException =>
+ throw QueryExecutionErrors.datetimeOverflowError("timestampadd", micros, quantity, unit)
Review comment:
What if the original user query uses dateadd()?
--
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 change in pull request #35787: [SPARK-38481][SQL] Substitute Java overflow exception from `TIMESTAMPADD` by Spark exception
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35787:
URL: https://github.com/apache/spark/pull/35787#discussion_r823331420
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala
##########
@@ -1174,29 +1174,36 @@ object DateTimeUtils {
* @return A timestamp value, expressed in microseconds since 1970-01-01 00:00:00Z.
*/
def timestampAdd(unit: String, quantity: Int, micros: Long, zoneId: ZoneId): Long = {
- unit.toUpperCase(Locale.ROOT) match {
- case "MICROSECOND" =>
- timestampAddDayTime(micros, quantity, zoneId)
- case "MILLISECOND" =>
- timestampAddDayTime(micros, quantity * MICROS_PER_MILLIS, zoneId)
- case "SECOND" =>
- timestampAddDayTime(micros, quantity * MICROS_PER_SECOND, zoneId)
- case "MINUTE" =>
- timestampAddDayTime(micros, quantity * MICROS_PER_MINUTE, zoneId)
- case "HOUR" =>
- timestampAddDayTime(micros, quantity * MICROS_PER_HOUR, zoneId)
- case "DAY" | "DAYOFYEAR" =>
- timestampAddDayTime(micros, quantity * MICROS_PER_DAY, zoneId)
- case "WEEK" =>
- timestampAddDayTime(micros, quantity * MICROS_PER_DAY * DAYS_PER_WEEK, zoneId)
- case "MONTH" =>
- timestampAddMonths(micros, quantity, zoneId)
- case "QUARTER" =>
- timestampAddMonths(micros, quantity * 3, zoneId)
- case "YEAR" =>
- timestampAddMonths(micros, quantity * MONTHS_PER_YEAR, zoneId)
- case _ =>
+ try {
+ unit.toUpperCase(Locale.ROOT) match {
+ case "MICROSECOND" =>
+ timestampAddDayTime(micros, quantity, zoneId)
+ case "MILLISECOND" =>
+ timestampAddDayTime(micros, quantity * MICROS_PER_MILLIS, zoneId)
+ case "SECOND" =>
+ timestampAddDayTime(micros, quantity * MICROS_PER_SECOND, zoneId)
+ case "MINUTE" =>
+ timestampAddDayTime(micros, quantity * MICROS_PER_MINUTE, zoneId)
+ case "HOUR" =>
+ timestampAddDayTime(micros, quantity * MICROS_PER_HOUR, zoneId)
+ case "DAY" | "DAYOFYEAR" =>
+ timestampAddDayTime(micros, quantity * MICROS_PER_DAY, zoneId)
+ case "WEEK" =>
+ timestampAddDayTime(micros, quantity * MICROS_PER_DAY * DAYS_PER_WEEK, zoneId)
+ case "MONTH" =>
+ timestampAddMonths(micros, quantity, zoneId)
+ case "QUARTER" =>
+ timestampAddMonths(micros, quantity * 3, zoneId)
+ case "YEAR" =>
+ timestampAddMonths(micros, quantity * MONTHS_PER_YEAR, zoneId)
+ }
+ } catch {
+ case _: scala.MatchError =>
throw QueryExecutionErrors.invalidUnitInTimestampAdd(unit)
+ case _: ArithmeticException | _: DateTimeException =>
+ throw QueryExecutionErrors.datetimeOverflowError("timestampadd", micros, quantity, unit)
Review comment:
Yea we don't need to mention the function name. We should have another mechanism to provide the context of the error. cc @gengliangwang
--
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 #35787: [SPARK-38481][SQL] Substitute Java overflow exception from `TIMESTAMPADD` by Spark exception
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on a change in pull request #35787:
URL: https://github.com/apache/spark/pull/35787#discussion_r823374788
##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala
##########
@@ -1174,29 +1174,36 @@ object DateTimeUtils {
* @return A timestamp value, expressed in microseconds since 1970-01-01 00:00:00Z.
*/
def timestampAdd(unit: String, quantity: Int, micros: Long, zoneId: ZoneId): Long = {
- unit.toUpperCase(Locale.ROOT) match {
- case "MICROSECOND" =>
- timestampAddDayTime(micros, quantity, zoneId)
- case "MILLISECOND" =>
- timestampAddDayTime(micros, quantity * MICROS_PER_MILLIS, zoneId)
- case "SECOND" =>
- timestampAddDayTime(micros, quantity * MICROS_PER_SECOND, zoneId)
- case "MINUTE" =>
- timestampAddDayTime(micros, quantity * MICROS_PER_MINUTE, zoneId)
- case "HOUR" =>
- timestampAddDayTime(micros, quantity * MICROS_PER_HOUR, zoneId)
- case "DAY" | "DAYOFYEAR" =>
- timestampAddDayTime(micros, quantity * MICROS_PER_DAY, zoneId)
- case "WEEK" =>
- timestampAddDayTime(micros, quantity * MICROS_PER_DAY * DAYS_PER_WEEK, zoneId)
- case "MONTH" =>
- timestampAddMonths(micros, quantity, zoneId)
- case "QUARTER" =>
- timestampAddMonths(micros, quantity * 3, zoneId)
- case "YEAR" =>
- timestampAddMonths(micros, quantity * MONTHS_PER_YEAR, zoneId)
- case _ =>
+ try {
+ unit.toUpperCase(Locale.ROOT) match {
+ case "MICROSECOND" =>
+ timestampAddDayTime(micros, quantity, zoneId)
+ case "MILLISECOND" =>
+ timestampAddDayTime(micros, quantity * MICROS_PER_MILLIS, zoneId)
+ case "SECOND" =>
+ timestampAddDayTime(micros, quantity * MICROS_PER_SECOND, zoneId)
+ case "MINUTE" =>
+ timestampAddDayTime(micros, quantity * MICROS_PER_MINUTE, zoneId)
+ case "HOUR" =>
+ timestampAddDayTime(micros, quantity * MICROS_PER_HOUR, zoneId)
+ case "DAY" | "DAYOFYEAR" =>
+ timestampAddDayTime(micros, quantity * MICROS_PER_DAY, zoneId)
+ case "WEEK" =>
+ timestampAddDayTime(micros, quantity * MICROS_PER_DAY * DAYS_PER_WEEK, zoneId)
+ case "MONTH" =>
+ timestampAddMonths(micros, quantity, zoneId)
+ case "QUARTER" =>
+ timestampAddMonths(micros, quantity * 3, zoneId)
+ case "YEAR" =>
+ timestampAddMonths(micros, quantity * MONTHS_PER_YEAR, zoneId)
+ }
+ } catch {
+ case _: scala.MatchError =>
throw QueryExecutionErrors.invalidUnitInTimestampAdd(unit)
+ case _: ArithmeticException | _: DateTimeException =>
+ throw QueryExecutionErrors.datetimeOverflowError("timestampadd", micros, quantity, unit)
Review comment:
> What if the original user query uses dateadd()?
@entong From my point of view it is not issue because we document that `dateadd()` is an alias for `timestampadd()`.
> without showing the internal method name.
@HyukjinKwon It is not internal name - it is external function name.
> Yea we don't need to mention the function name.
I do believe we should point out to the **add** operation over datatime. How about:
```
"DATETIME_OVERFLOW" : {
"message" : [ "A datetime function overflows the input '%s' timestamp by adding of %s %s." ],
"sqlState" : "22008"
},
```
--
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 #35787: [SPARK-38481][SQL] Substitute Java overflow exception from `TIMESTAMPADD` by Spark exception
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on a change in pull request #35787:
URL: https://github.com/apache/spark/pull/35787#discussion_r823371845
##########
File path: core/src/main/resources/error/error-classes.json
##########
@@ -25,6 +25,10 @@
"CONCURRENT_QUERY" : {
"message" : [ "Another instance of this query was just started by a concurrent session." ]
},
+ "DATETIME_OVERFLOW" : {
+ "message" : [ "The '%s' function overflows the input '%s' timestamp by %s %s." ],
Review comment:
I don't think making errors too general is right approach. Error messages should help users to identify and fix issues in their queries.
> I think we can omit the input details.
Could you explain how users should find the problematic row if we don't provide any context as you propose?
--
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 #35787: [SPARK-38481][SQL] Substitute Java overflow exception from `TIMESTAMPADD` by Spark exception
Posted by GitBox <gi...@apache.org>.
MaxGekk commented on pull request #35787:
URL: https://github.com/apache/spark/pull/35787#issuecomment-1063999213
Merging to master. Thank you, @entong @HyukjinKwon and @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