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/14 06:15:00 UTC

[GitHub] [spark] itholic opened a new pull request, #38644: [SPARK-41130][SQL] Rename `OUT_OF_DECIMAL_TYPE_RANGE` to `NUMERIC_OUT_OF_SUPPORTED_RANGE`

itholic opened a new pull request, #38644:
URL: https://github.com/apache/spark/pull/38644

   ### What changes were proposed in this pull request?
   
   This PR proposes to rename `OUT_OF_DECIMAL_TYPE_RANGE` to `NUMERIC_OUT_OF_SUPPORTED_RANGE`,
   
   and also improve its error message.
   
   
   ### Why are the changes needed?
   
   Error class and its error message should be clear/brief.
   
   We already have `NUMERIC_OUT_OF_RANGE`, so `NUMERIC_OUT_OF_SUPPORTED_RANGE` more makes sense and the error message should be more specific.
   
   
   ### Does this PR introduce _any_ user-facing change?
   
   The error message is a bit improved.
   
   From
   ```
   Out of decimal type range: <value>.
   ```
   
   To
   ```
   The value <value> cannot be interpreted as a numeric since it has more than 38 digits.
   ```
   
   ### 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] itholic commented on a diff in pull request #38644: [SPARK-41130][SQL] Rename `OUT_OF_DECIMAL_TYPE_RANGE` to `NUMERIC_OUT_OF_SUPPORTED_RANGE`

Posted by GitBox <gi...@apache.org>.
itholic commented on code in PR #38644:
URL: https://github.com/apache/spark/pull/38644#discussion_r1027410164


##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastWithAnsiOnSuite.scala:
##########
@@ -244,7 +244,7 @@ class CastWithAnsiOnSuite extends CastSuiteBase with QueryErrorsBase {
       Decimal("12345678901234567890123456789012345678"))
     checkExceptionInExpression[ArithmeticException](
       cast("123456789012345678901234567890123456789", DecimalType(38, 0)),
-      "Out of decimal type range")
+      "NUMERIC_OUT_OF_SUPPORTED_RANGE")

Review Comment:
   Yes, just created JIRA here: https://issues.apache.org/jira/browse/SPARK-41208



-- 
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 #38644: [SPARK-41130][SQL] Rename `OUT_OF_DECIMAL_TYPE_RANGE` to `NUMERIC_OUT_OF_SUPPORTED_RANGE`

Posted by GitBox <gi...@apache.org>.
MaxGekk closed pull request #38644: [SPARK-41130][SQL] Rename `OUT_OF_DECIMAL_TYPE_RANGE` to `NUMERIC_OUT_OF_SUPPORTED_RANGE`
URL: https://github.com/apache/spark/pull/38644


-- 
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 #38644: [SPARK-41130][SQL] Rename `OUT_OF_DECIMAL_TYPE_RANGE` to `NUMERIC_OUT_OF_SUPPORTED_RANGE`

Posted by GitBox <gi...@apache.org>.
MaxGekk commented on PR #38644:
URL: https://github.com/apache/spark/pull/38644#issuecomment-1319797393

   +1, LGTM. Merging to master.
   Thank you, @itholic.


-- 
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 #38644: [SPARK-41130][SQL] Rename `OUT_OF_DECIMAL_TYPE_RANGE` to `NUMERIC_OUT_OF_SUPPORTED_RANGE`

Posted by GitBox <gi...@apache.org>.
itholic commented on code in PR #38644:
URL: https://github.com/apache/spark/pull/38644#discussion_r1025941541


##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastWithAnsiOnSuite.scala:
##########
@@ -244,7 +244,7 @@ class CastWithAnsiOnSuite extends CastSuiteBase with QueryErrorsBase {
       Decimal("12345678901234567890123456789012345678"))
     checkExceptionInExpression[ArithmeticException](
       cast("123456789012345678901234567890123456789", DecimalType(38, 0)),
-      "Out of decimal type range")
+      "NUMERIC_OUT_OF_SUPPORTED_RANGE")

Review Comment:
   Let me just test with `checkExceptionInExpression` for now, since it failed in CI when use `checkError` because `checkError` doesn't support for various test Mode such as "non-codegen mode", "codegen mode" and "unsafe mode" for now. (so, it failed in CI: https://github.com/apache/spark/pull/38644#discussion_r1021424319)
   I think we might need to similar test utils for expression test such as `checkErrorInExpression` something like that.



-- 
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 #38644: [SPARK-41130][SQL] Rename `OUT_OF_DECIMAL_TYPE_RANGE` to `NUMERIC_OUT_OF_SUPPORTED_RANGE`

Posted by GitBox <gi...@apache.org>.
itholic commented on code in PR #38644:
URL: https://github.com/apache/spark/pull/38644#discussion_r1021424319


##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastWithAnsiOnSuite.scala:
##########
@@ -242,9 +242,13 @@ class CastWithAnsiOnSuite extends CastSuiteBase with QueryErrorsBase {
   test("Fast fail for cast string type to decimal type in ansi mode") {
     checkEvaluation(cast("12345678901234567890123456789012345678", DecimalType(38, 0)),
       Decimal("12345678901234567890123456789012345678"))
-    checkExceptionInExpression[ArithmeticException](
-      cast("123456789012345678901234567890123456789", DecimalType(38, 0)),
-      "Out of decimal type range")
+    checkError(
+      exception = intercept[SparkArithmeticException] {
+        evaluateWithoutCodegen(cast("123456789012345678901234567890123456789", DecimalType(38, 0)))
+      },
+      errorClass = "NUMERIC_OUT_OF_SUPPORTED_RANGE",
+      parameters = Map("value" -> "123456789012345678901234567890123456789")
+    )

Review Comment:
   CI complains `org.scalatest.exceptions.TestFailedException: Expected exception org.apache.spark.SparkArithmeticException to be thrown, but no exception was thrown
   `, but this is passed in my local test env both with/without ANSI mode.
   
   Any suggestion for this fix? or we should just use `checkExceptionInExpression` for now ?



-- 
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 #38644: [SPARK-41130][SQL] Rename `OUT_OF_DECIMAL_TYPE_RANGE` to `NUMERIC_OUT_OF_SUPPORTED_RANGE`

Posted by GitBox <gi...@apache.org>.
itholic commented on PR #38644:
URL: https://github.com/apache/spark/pull/38644#issuecomment-1313150235

   cc @MaxGekk @srielau 


-- 
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 #38644: [SPARK-41130][SQL] Rename `OUT_OF_DECIMAL_TYPE_RANGE` to `NUMERIC_OUT_OF_SUPPORTED_RANGE`

Posted by GitBox <gi...@apache.org>.
itholic commented on PR #38644:
URL: https://github.com/apache/spark/pull/38644#issuecomment-1319485107

   cc @MaxGekk @srielau 


-- 
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 #38644: [SPARK-41130][SQL] Rename `OUT_OF_DECIMAL_TYPE_RANGE` to `NUMERIC_OUT_OF_SUPPORTED_RANGE`

Posted by GitBox <gi...@apache.org>.
itholic commented on code in PR #38644:
URL: https://github.com/apache/spark/pull/38644#discussion_r1025941541


##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastWithAnsiOnSuite.scala:
##########
@@ -244,7 +244,7 @@ class CastWithAnsiOnSuite extends CastSuiteBase with QueryErrorsBase {
       Decimal("12345678901234567890123456789012345678"))
     checkExceptionInExpression[ArithmeticException](
       cast("123456789012345678901234567890123456789", DecimalType(38, 0)),
-      "Out of decimal type range")
+      "NUMERIC_OUT_OF_SUPPORTED_RANGE")

Review Comment:
   Let me just test with `checkExceptionInExpression` for now, since it failed in CI when use `checkError` because `checkError` doesn't support for various test Mode such as "non-codegen mode", "codegen mode" and "unsafe mode" for now.
   I think we might need to similar test utils for expression test such as `checkErrorInExpression` something like that.



-- 
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 #38644: [SPARK-41130][SQL] Rename `OUT_OF_DECIMAL_TYPE_RANGE` to `NUMERIC_OUT_OF_SUPPORTED_RANGE`

Posted by GitBox <gi...@apache.org>.
MaxGekk commented on code in PR #38644:
URL: https://github.com/apache/spark/pull/38644#discussion_r1026251775


##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastWithAnsiOnSuite.scala:
##########
@@ -244,7 +244,7 @@ class CastWithAnsiOnSuite extends CastSuiteBase with QueryErrorsBase {
       Decimal("12345678901234567890123456789012345678"))
     checkExceptionInExpression[ArithmeticException](
       cast("123456789012345678901234567890123456789", DecimalType(38, 0)),
-      "Out of decimal type range")
+      "NUMERIC_OUT_OF_SUPPORTED_RANGE")

Review Comment:
   > I think we might need to similar test utils for expression test such as checkErrorInExpression something like that.
   
   Good point. Could you open an JIRA to not forget about this.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: 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 #38644: [SPARK-41130][SQL] Rename `OUT_OF_DECIMAL_TYPE_RANGE` to `NUMERIC_OUT_OF_SUPPORTED_RANGE`

Posted by GitBox <gi...@apache.org>.
itholic commented on code in PR #38644:
URL: https://github.com/apache/spark/pull/38644#discussion_r1021424319


##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastWithAnsiOnSuite.scala:
##########
@@ -242,9 +242,13 @@ class CastWithAnsiOnSuite extends CastSuiteBase with QueryErrorsBase {
   test("Fast fail for cast string type to decimal type in ansi mode") {
     checkEvaluation(cast("12345678901234567890123456789012345678", DecimalType(38, 0)),
       Decimal("12345678901234567890123456789012345678"))
-    checkExceptionInExpression[ArithmeticException](
-      cast("123456789012345678901234567890123456789", DecimalType(38, 0)),
-      "Out of decimal type range")
+    checkError(
+      exception = intercept[SparkArithmeticException] {
+        evaluateWithoutCodegen(cast("123456789012345678901234567890123456789", DecimalType(38, 0)))
+      },
+      errorClass = "NUMERIC_OUT_OF_SUPPORTED_RANGE",
+      parameters = Map("value" -> "123456789012345678901234567890123456789")
+    )

Review Comment:
   CI complains `org.scalatest.exceptions.TestFailedException: Expected exception org.apache.spark.SparkArithmeticException to be thrown, but no exception was thrown
   `, but this is passed in my local test env both with/without ANSI mode.
   
   Any suggestion for this fix? or we should just use `checkExceptionInExpression for now` ?



-- 
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