[SPARK-40054][SQL] Restore the error handling syntax of try_cast() #37486

gengliangwang · 2022-08-12T05:05:33Z

What changes were proposed in this pull request?

For the following query

SET spark.sql.ansi.enabled=true;
SELECT try_cast(1/0 AS string);

Spark 3.3 will throw an exception for the division by zero error. In the current master branch, it returns null after the refactoring PR #36703

This PR is to restore the previous error handling syntax.

Why are the changes needed?

Restore the behavior of try_cast()
The previous syntax is more reasonable. Users can cleanly catch the exception from the child of try_cast.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Unit tests

gengliangwang · 2022-08-12T05:10:27Z

Some of the code are from the removed part of #36703, which is still reliable.

gengliangwang · 2022-08-12T05:16:22Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/TryCastSuite.scala

 import org.apache.spark.SparkFunSuite
 import org.apache.spark.sql.catalyst.InternalRow
 import org.apache.spark.sql.catalyst.util.DateTimeTestUtils.UTC_OPT
 import org.apache.spark.sql.types._
 import org.apache.spark.unsafe.types.UTF8String

 // A test suite to check analysis behaviors of `TryCast`.
-class TryCastSuite extends SparkFunSuite {
+class TryCastSuite extends CastWithAnsiOnSuite {


This recovers the test coverage before #36703

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/TryCastSuite.scala

cloud-fan · 2022-08-12T07:02:04Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala

+      // TODO: Implement a more accurate method for checking whether a decimal value can be cast
+      //       as integral types without overflow. Currently, the cast can overflow even if
+      //       "Cast.canUpCast" method returns true.
+      case (_: DecimalType, _: IntegralType) => true


is this a bug in spark 3.3?

Decimal(10, 0) is considered as "canUpCast" as Int. However, Int.MaxValue + 1 can fit in Decimal(10, 0) too..
It has been a long time and the strict table insertion policy relies on this.
I would consider fixing it in another PR. For this one I will leave it as it is now.

Note that this is not a bug in Spark 3.3. I am avoiding making the nullability of try_cast as always null here.

cloud-fan · 2022-08-12T07:04:47Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/TryCastSuite.scala

    }
  }

+  override def checkExceptionInExpression[T <: Throwable : ClassTag](


this is a bit weird, I see code like this

if (!isTryCast) { checkExceptionInExpression[ArithmeticException](ret, "overflow") } else { checkEvaluation(ret, Row(null)) }

if we override checkExceptionInExpression, why do we still need if-else?

That one was "Row(null)" with schema struct<a:int>. Different from the default answer null

It's a bit weird to have two different testing approaches for try_cast, but given that it was already the case before, I don't have a strong opinion.

gengliangwang · 2022-08-12T16:57:23Z

@cloud-fan Thanks for the review. Merging to master.

### What changes were proposed in this pull request? Similar to #37486 and #37663, this PR refactors the implementation of try_sum() so that the errors from its child should be shown instead of ignored. ### Why are the changes needed? Fix the semantics of try_sum() function ### Does this PR introduce _any_ user-facing change? Yes, after changes, the error from the child of try_sum function will be shown instead of ignored. ### How was this patch tested? New UT Closes #37764 from gengliangwang/trySum. Authored-by: Gengliang Wang <gengliang@apache.org> Signed-off-by: Gengliang Wang <gengliang@apache.org>

### What changes were proposed in this pull request? Similar to #37486, #37663 and #37764, this PR refactors the implementation of try_avg() so that the errors from its child should be shown instead of ignored. ### Why are the changes needed? Fix the semantics of try_avg() function ### Does this PR introduce _any_ user-facing change? Yes, after changes, the error from the child of the try_avg function will be shown instead of ignored. ### How was this patch tested? New UT Closes #37775 from gengliangwang/tryAvg. Authored-by: Gengliang Wang <gengliang@apache.org> Signed-off-by: Gengliang Wang <gengliang@apache.org>

### What changes were proposed in this pull request? Similar to apache/spark#37486, apache/spark#37663 and apache/spark#37764, this PR refactors the implementation of try_avg() so that the errors from its child should be shown instead of ignored. ### Why are the changes needed? Fix the semantics of try_avg() function ### Does this PR introduce _any_ user-facing change? Yes, after changes, the error from the child of the try_avg function will be shown instead of ignored. ### How was this patch tested? New UT Closes #37775 from gengliangwang/tryAvg. Authored-by: Gengliang Wang <gengliang@apache.org> Signed-off-by: Gengliang Wang <gengliang@apache.org>

gengliangwang added 7 commits August 11, 2022 00:12

refactor parameter

33c704f

support try mode in cast

3b69b59

fix TryCast Suite

9b833ac

fix base try_cast tests

38193aa

fix a few tests

4d2ab5b

fix

d8f8352

add comments

d904eb2

gengliangwang requested review from cloud-fan and JoshRosen August 12, 2022 05:05

github-actions bot added the SQL label Aug 12, 2022

gengliangwang commented Aug 12, 2022

View reviewed changes