[GLUTEN-8108] fix: Update logic to throw on failure for cast #8107

acvictor · 2024-11-29T10:18:33Z

What changes were proposed in this pull request?

There are 3 failure behaviors defined for cast in Substrait

enum FailureBehavior { 
    FAILURE_BEHAVIOR_UNSPECIFIED = 0; 
    FAILURE_BEHAVIOR_RETURN_NULL = 1; 
    FAILURE_BEHAVIOR_THROW_EXCEPTION = 2; 
}

Failure behavior should be set to 2 only when evaluation mode is ANSI. The current logic in Spark 3.4 and 3.5 uses the evaluation of ansiEnabled = evalMode == EvalMode.ANSI || (evalMode == EvalMode.TRY && !canUseLegacyCastForTryCast) to set the failure behavior and is incorrectly evaluated in some cases depending on the value of canUseLegacyCastForTryCast. canUseLegacyCastForTryCast should not matter in Velox/Gluten since it is used in Spark to optimize execution by avoiding adding a try-catch block around the cast.

A minimal repro shows that
select try_cast(value as bigint) from tbl translates to Velox plan cast((trim( :VARCHAR, n0_0)) as BIGINT), implying that failure behavior is incorrectly set to 2 and so SparkCastExpr in Velox is created with nullOnFailure_ set to false instead of true https://github.com/facebookincubator/velox/blob/main/velox/expression/ExprCompiler.cpp#L411.

(Fixes: #ISSUE-ID #8108)

How was this patch tested?

Unit tests.

github-actions · 2024-11-29T10:18:50Z

Thanks for opening a pull request!

Could you open an issue for this pull request on Github Issues?

https://github.com/apache/incubator-gluten/issues

Then could you also rename commit message and pull request title in the following format?

[GLUTEN-${ISSUES_ID}][COMPONENT]feat/fix: ${detailed message}

See also:

Other pull requests

github-actions · 2024-11-29T10:19:06Z

Run Gluten Clickhouse CI on x86

github-actions · 2024-11-29T10:24:39Z

#8108

acvictor · 2024-11-29T10:35:10Z

@zhli1142015 @rui-mo can you please review?

gluten-substrait/src/main/scala/org/apache/gluten/expression/UnaryExpressionTransformer.scala

backends-velox/src/test/scala/org/apache/gluten/execution/ScalarFunctionsValidateSuite.scala

...k35/src/test/scala/org/apache/spark/sql/catalyst/expressions/GlutenCastWithAnsiOnSuite.scala

github-actions · 2024-12-02T10:06:34Z

Run Gluten Clickhouse CI on x86

github-actions · 2024-12-02T10:11:51Z

Run Gluten Clickhouse CI on x86

github-actions · 2024-12-02T11:26:02Z

Run Gluten Clickhouse CI on x86

github-actions · 2024-12-02T11:26:16Z

Run Gluten Clickhouse CI on x86

github-actions · 2024-12-02T11:30:11Z

Run Gluten Clickhouse CI on x86

github-actions · 2024-12-02T11:31:20Z

Run Gluten Clickhouse CI on x86

github-actions · 2024-12-02T12:00:32Z

Run Gluten Clickhouse CI on x86

github-actions · 2024-12-02T14:40:41Z

Run Gluten Clickhouse CI on x86

rui-mo

Thanks.

rui-mo · 2024-12-02T14:47:12Z

gluten-ut/spark35/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala

+    .exclude(
+      "Process Infinity, -Infinity, NaN in case insensitive manner" // +inf not supported in folly.
+    )
+    .exclude("ANSI mode: Throw exception on casting out-of-range value to byte type")


Would you add some comments on why these tests are excluded? I remember ANSI ON causes execution fallback, can the tests produce Spark expected result?

These tests come from https://github.com/apache/spark/blob/master/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CastWithAnsiOnSuite.scala and when the suite is run separately with SQLConf.get.setConf(SQLConf.ANSI_ENABLED, true) falls back but when run as part of GlutenTryCastSuite where the conf is not set does not give expected results. (Also, the tests fail without the code changes in this PR.)

the conf is not set does not give expected results

Thanks for providing the details. These tests are for ANSI OFF cases, right?

These tests are for ANSI enabled (Spark views ANSI is enabled for TRY mode).
@acvictor, for these tests triggered from TryCastSuite, exceptions are not thrown, but null results are returned? It should be an expected behavior for Velox cast.
I cannot figure out how these exception checks can pass in vanilla Spark, assuming exception is handled internally in TRY mode.

@PHILO-HE I don't fully understand what you mean. Are you saying Velox behaviour is incorrect?

@acvictor, no. I am just curious why these tests of TryCastSuite can pass for vanilla Spark. I am assuming the exceptions are handled internally by Spark and null result is returned by internally catching these exceptions. But seems in these tests, exception can still be caught from external side in vanilla Spark.

zhli1142015

LGTM, thanks

zhli1142015 · 2024-12-03T01:46:41Z

Run Gluten Clickhouse CI on x86

PHILO-HE · 2024-12-03T03:26:31Z

gluten-substrait/src/main/scala/org/apache/gluten/expression/UnaryExpressionTransformer.scala

+    ExpressionBuilder.makeCast(
+      typeNode,
+      child.doTransform(args),
+      SparkShimLoader.getSparkShims.withAnsiEvalMode(original))


Yes, I note Spark also sets ansiEnabled = true for EvalMode.TRY, which is for re-using the code logic of EvalMode.ANSI. The difference is, in TRY mode any exception is caught and then NULL is returned.
Velox has different implementation, which requires us to set this flag simply according to whether EvalMode is ANSI or not.

@acvictor, please leave some comments here to clarify, which should be helpful for future code maintenance.

https://github.com/apache/spark/blob/v3.5.3/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Cast.scala#L471

PHILO-HE · 2024-12-03T03:41:33Z

gluten-ut/spark35/src/test/scala/org/apache/gluten/utils/velox/VeloxTestSettings.scala

+    .exclude(
+      "Process Infinity, -Infinity, NaN in case insensitive manner" // +inf not supported in folly.
+    )
+    .exclude("ANSI mode: Throw exception on casting out-of-range value to byte type")


These tests are for ANSI enabled (Spark views ANSI is enabled for TRY mode).
@acvictor, for these tests triggered from TryCastSuite, exceptions are not thrown, but null results are returned? It should be an expected behavior for Velox cast.
I cannot figure out how these exception checks can pass in vanilla Spark, assuming exception is handled internally in TRY mode.

Update try_cast logic

2b6390d

github-actions bot added CORE works for Gluten Core VELOX labels Nov 29, 2024

acvictor changed the title ~~[VL] Update logic to throw on failure for cast~~ [GLUTEN-8108] fix: Update logic to throw on failure for cast Nov 29, 2024

acvictor marked this pull request as ready for review November 29, 2024 10:35

zhli1142015 reviewed Nov 29, 2024

View reviewed changes

gluten-substrait/src/main/scala/org/apache/gluten/expression/UnaryExpressionTransformer.scala Outdated Show resolved Hide resolved

zhli1142015 reviewed Nov 29, 2024

View reviewed changes

backends-velox/src/test/scala/org/apache/gluten/execution/ScalarFunctionsValidateSuite.scala Outdated Show resolved Hide resolved

...k35/src/test/scala/org/apache/spark/sql/catalyst/expressions/GlutenCastWithAnsiOnSuite.scala Outdated Show resolved Hide resolved

Address comments

bd318a4

Reuse func

e3755a6

acvictor added 2 commits December 2, 2024 16:55

Fix format

5144292

Merge branch 'main' into users/anvicto/updateTryCastNode

34dfc87

Fix format

3fbef08

Add to settings

a6df752

Add import

4d2de46

Fix typo

ca38e62

rui-mo reviewed Dec 2, 2024

View reviewed changes

rui-mo requested a review from PHILO-HE December 3, 2024 01:33

rui-mo approved these changes Dec 3, 2024

View reviewed changes

zhli1142015 approved these changes Dec 3, 2024

View reviewed changes

PHILO-HE reviewed Dec 3, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GLUTEN-8108] fix: Update logic to throw on failure for cast #8107

[GLUTEN-8108] fix: Update logic to throw on failure for cast #8107

acvictor commented Nov 29, 2024 •

edited

Loading

github-actions bot commented Nov 29, 2024

github-actions bot commented Nov 29, 2024

github-actions bot commented Nov 29, 2024

acvictor commented Nov 29, 2024

github-actions bot commented Dec 2, 2024

github-actions bot commented Dec 2, 2024

github-actions bot commented Dec 2, 2024

github-actions bot commented Dec 2, 2024

github-actions bot commented Dec 2, 2024

github-actions bot commented Dec 2, 2024

github-actions bot commented Dec 2, 2024

github-actions bot commented Dec 2, 2024

rui-mo left a comment

rui-mo Dec 2, 2024

acvictor Dec 2, 2024

rui-mo Dec 3, 2024

PHILO-HE Dec 3, 2024 •

edited

Loading

acvictor Dec 12, 2024

PHILO-HE Dec 17, 2024

zhli1142015 left a comment

zhli1142015 commented Dec 3, 2024

PHILO-HE Dec 3, 2024

PHILO-HE Dec 3, 2024 •

edited

Loading

[GLUTEN-8108] fix: Update logic to throw on failure for cast #8107

Are you sure you want to change the base?

[GLUTEN-8108] fix: Update logic to throw on failure for cast #8107

Conversation

acvictor commented Nov 29, 2024 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

github-actions bot commented Nov 29, 2024

github-actions bot commented Nov 29, 2024

github-actions bot commented Nov 29, 2024

acvictor commented Nov 29, 2024

github-actions bot commented Dec 2, 2024

github-actions bot commented Dec 2, 2024

github-actions bot commented Dec 2, 2024

github-actions bot commented Dec 2, 2024

github-actions bot commented Dec 2, 2024

github-actions bot commented Dec 2, 2024

github-actions bot commented Dec 2, 2024

github-actions bot commented Dec 2, 2024

rui-mo left a comment

Choose a reason for hiding this comment

rui-mo Dec 2, 2024

Choose a reason for hiding this comment

acvictor Dec 2, 2024

Choose a reason for hiding this comment

rui-mo Dec 3, 2024

Choose a reason for hiding this comment

PHILO-HE Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

acvictor Dec 12, 2024

Choose a reason for hiding this comment

PHILO-HE Dec 17, 2024

Choose a reason for hiding this comment

zhli1142015 left a comment

Choose a reason for hiding this comment

zhli1142015 commented Dec 3, 2024

PHILO-HE Dec 3, 2024

Choose a reason for hiding this comment

PHILO-HE Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

acvictor commented Nov 29, 2024 •

edited

Loading

PHILO-HE Dec 3, 2024 •

edited

Loading

PHILO-HE Dec 3, 2024 •

edited

Loading