chore: Make ANSI fallback more granular #509

andygrove · 2024-06-03T22:13:17Z

Which issue does this PR close?

Part of #313

Rationale for this change

Rather than fall back to Spark for all plans when ANSI mode is enabled, it would be better to only fall back to Spark if the plan involves any expressions that have ANSI-specific behavior that we do not yet support.

What changes are included in this PR?

Introduce CometEvalMode enum to use instead of string literals
Add shim code for getting eval mode for Round and Cast
Add code to fallback to Spark for Round in ANSI mode
Remove code that falls back to Spark for all plans if ANSI mode is enabled

How are these changes tested?

TBD

andygrove · 2024-06-03T22:15:37Z

@kazuyukitanimura I have more work to do in this PR but wanted to make you aware of it since it is related to Spark 4 support.

andygrove · 2024-06-04T21:33:02Z

I am looking into the test failures:

2024-06-04T17:46:08.5118601Z [info] - SPARK-39175: Query context of Cast should be serialized to executors when WSCG is off *** FAILED *** (168 milliseconds)
2024-06-04T17:47:32.8159859Z [info] - postgreSQL/float8.sql *** FAILED *** (2 seconds, 108 milliseconds)
2024-06-04T17:47:37.1749187Z [info] - postgreSQL/groupingsets.sql *** FAILED *** (4 seconds, 331 milliseconds)
2024-06-04T18:04:31.0221492Z [info] *** 3 TESTS FAILED ***

parthchandra

LGTM. Some minor comments.

parthchandra · 2024-06-05T00:53:08Z

spark/src/main/scala/org/apache/comet/expressions/CometEvalMode.scala

+ * Expression evaluation modes.
+ *   - LEGACY: the default evaluation mode, which is compliant to Hive SQL.
+ *   - ANSI: a evaluation mode which is compliant to ANSI SQL standard.
+ *   - TRY: a evaluation mode for `try_*` functions. It is identical to ANSI evaluation mode


Is conf.ansiEnabled ignored for try_* ? Maybe the comment can clarify.

parthchandra · 2024-06-05T00:57:34Z

spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala

+    evalMode match {
+      case CometEvalMode.LEGACY => ExprOuterClass.EvalMode.LEGACY
+      case CometEvalMode.TRY => ExprOuterClass.EvalMode.TRY
+      case CometEvalMode.ANSI => ExprOuterClass.EvalMode.ANSI


Add a 'catch all' case for when someone tries to change CometEvalMode and things don't work as planned?

Good point. I have added this.

parthchandra · 2024-06-05T01:03:49Z

spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala

+        case c @ Cast(child, dt, timeZoneId, _) =>
+          handleCast(child, inputs, dt, timeZoneId, evalMode(c))
+
+        case expr: Add if evalMode(expr) == CometEvalMode.ANSI && !cometAnsiEnabled =>


Should we move these to be next to the corresponding supported implementations so new developers can discover this case easily?

The code has been refactored significantly since this comment. There is now a single match arm for ANSI fallback and there is an isUnsupportedAnsiExpr method which determines if the expression is one where we do not yet have ANSI support.

vaibhawvipul

Looks neat!

viirya · 2024-06-05T13:29:04Z

spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala

+          withInfo(expr, ansiNotSupported)
+          None
+
+        case expr: Multiply if evalMode(expr) == CometEvalMode.ANSI && !cometAnsiEnabled =>


Maybe we can add a simple method containing all these checks together, instead of scattering them here.

kazuyukitanimura · 2024-06-05T18:03:35Z

spark/src/main/scala/org/apache/comet/CometSparkSessionExtensions.scala

-      if (isANSIEnabled(conf)) {
-        if (COMET_ANSI_MODE_ENABLED.get()) {
-          logWarning("Using Comet's experimental support for ANSI mode.")
-        } else {
-          logInfo("Comet extension disabled for ANSI mode")
-          return plan
-        }
-      }


For Spark 3.x, I feel we still need this protection in case users enable ANSI mode as well as native execution.
If I understand correctly we have not set up CI yet with ANSI enabled.

This current check will make all plans fall back to Spark if ANSI is enabled, so no native plans will run, unless COMET_ANSI_MODE_ENABLED is enabled.

The changes in this PR mean that we still have the same check but only if the plan actually contains any expressions that would be affected by enabling ANSI support and we still fall back to Spark by default unless COMET_ANSI_MODE_ENABLED is enabled.

The main risk with this PR is if we don't have the complete list of expressions that support ANSI mode.

If I understand correctly we have not set up CI yet with ANSI enabled.

Well, we have the Spark 4 tests where ANSI is enabled and that is catching issues for sure.

I noticed that we were explicitly disabling ANSI mode in CometTestBase and I have removed that so we use the default ANSI mode for whatever Spark version we are running against. This should mean that all of our unit tests will now run with ANSI enabled when running against Spark 4+. Let's see how many issues this finds 😰

If I understand correctly we have not set up CI yet with ANSI enabled.

It would be good to add a CI pipeline for Spark 3.4 with ANSI enabled.

I am close to finish my PR to enable the spark tests with Spark 4.0 I.e. ANSI enabled, and I am disabling expressions that failed those tests with ANSI enabled.

I would propose to hold this part of change for now. Perhaps after we fix all Spark 4.0 issues, we can backport the learning to Spark 3.4

Sure. I have moved this to draft for now.

andygrove · 2024-06-05T19:53:26Z

This is a pretty interesting test failure:

- postgreSQL/groupingsets.sql *** FAILED *** (4 seconds, 331 milliseconds)
 postgreSQL/groupingsets.sql
 Expected "... 1       128
 NULL  1       1       0       1       [1
 NULL  1       1       0       1       2
 NULL  2       1       0       1       4
 NULL  2       1       0       1       8
 NULL  2       1       0       1       16
 NULL  2       1       0       1       32
 NULL  1       1       0       1       64
 NULL  1       1       0       1       128]", but got "...     1       128
 NULL  1       1       0       1       [-1673396112
NULL  2       1       0       1       -1673198320
 NULL  2       1       0       1       -596264016
 NULL  1       1       0       1       -596231120
 NULL  1       1       0       2       65102
 NULL  2       1       0       2       65102]"

I am assuming that this was due to us not supporting the ANSI overflow checks in SUM or AVG. We now fall back for those if ANSI is enabled.

andygrove · 2024-06-06T12:42:32Z

spark/src/test/scala/org/apache/spark/sql/CometTestBase.scala

@@ -69,7 +69,6 @@ abstract class CometTestBase
    conf.set("spark.hadoop.fs.file.impl", classOf[DebugFilesystem].getName)
    conf.set("spark.ui.enabled", "false")
    conf.set(SQLConf.SHUFFLE_PARTITIONS, 10) // reduce parallelism in tests
-    conf.set(SQLConf.ANSI_ENABLED.key, "false")


We no longer explicitly disable ANSI in our test suites.

kazuyukitanimura · 2024-06-07T07:56:41Z

spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala

+        case c @ Cast(child, dt, timeZoneId, _) =>
+          handleCast(child, inputs, dt, timeZoneId, evalMode(c))
+
+        case expr if isUnsupportedAnsiExpr(expr) && !CometConf.COMET_ANSI_MODE_ENABLED.get() =>


I feel !CometConf.COMET_ANSI_MODE_ENABLED.get() condition needs to be removed.

When COMET_ANSI_MODE_ENABLED=true, we still need to fallback to Spark if the expression is not ANSI compatible

Sure, let's remove this. It was intended as as short term workaround but I agree that we don't really need it now that we have this fallback logic.

kazuyukitanimura · 2024-06-07T07:57:39Z

spark/src/test/scala/org/apache/spark/sql/CometTestBase.scala

@@ -69,7 +69,7 @@ abstract class CometTestBase
    conf.set("spark.hadoop.fs.file.impl", classOf[DebugFilesystem].getName)
    conf.set("spark.ui.enabled", "false")
    conf.set(SQLConf.SHUFFLE_PARTITIONS, 10) // reduce parallelism in tests
-    conf.set(SQLConf.ANSI_ENABLED.key, "false")
+    conf.set(CometConf.COMET_ANSI_MODE_ENABLED.key, "true")


What about leaving COMET_ANSI_MODE_ENABLED as default value as well? I.e. false for 3.x, true for 4.0

Yes, good point

make ANSI fallback more granular

84f6f70

andygrove added 3 commits June 3, 2024 16:31

Add CometEvalMode

5ab3ee0

Add CometEvalMode enum

e377e44

remove old code

cccee7d

andygrove mentioned this pull request Jun 3, 2024

build: Enable comet tests with spark-4.0 profile #493

Merged

andygrove added 6 commits June 3, 2024 16:55

shim

e0bddd6

more shim work

338ba6d

more shim work

54032df

lint and spark 4 shim

97959a9

formatting

bf8ed7f

Merge remote-tracking branch 'apache/main' into ansi-fallback

bc72ba2

andygrove marked this pull request as ready for review June 4, 2024 14:09

andygrove marked this pull request as draft June 4, 2024 14:10

andygrove added 3 commits June 4, 2024 10:57

more ansi fallback

e7d648d

format

60de09c

refine

4d43485

simplify PR

992ad20

andygrove marked this pull request as ready for review June 4, 2024 21:44

parthchandra approved these changes Jun 5, 2024

View reviewed changes

vaibhawvipul approved these changes Jun 5, 2024

View reviewed changes

viirya reviewed Jun 5, 2024

View reviewed changes

viirya approved these changes Jun 5, 2024

View reviewed changes

address feedback

9688cc6

parthchandra approved these changes Jun 5, 2024

View reviewed changes

kazuyukitanimura reviewed Jun 5, 2024

View reviewed changes

andygrove added 2 commits June 6, 2024 06:26

fall back for more expressions in ANSI mode

901a622

stop disabling ANSI mode in tests

23f5996

andygrove commented Jun 6, 2024

View reviewed changes

andygrove added 2 commits June 6, 2024 06:47

fix 3.3 shim

9f30b4c

Enable Comet ANSI mode by default

ba21817

kazuyukitanimura reviewed Jun 7, 2024

View reviewed changes

andygrove added 2 commits June 7, 2024 10:14

use default Comet ANSI mode

fda7b30

remove COMET_ANSI_MODE_ENABLED

1bb6d04

andygrove marked this pull request as draft June 7, 2024 16:36

andygrove mentioned this pull request Jun 7, 2024

chore: Add CometEvalMode enum to replace string literals #539

Merged

andygrove closed this Jul 18, 2024

andygrove deleted the ansi-fallback branch December 3, 2024 04:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Make ANSI fallback more granular #509

chore: Make ANSI fallback more granular #509

andygrove commented Jun 3, 2024 •

edited

Loading

andygrove commented Jun 3, 2024

andygrove commented Jun 4, 2024

parthchandra left a comment

parthchandra Jun 5, 2024

parthchandra Jun 5, 2024

andygrove Jun 5, 2024

parthchandra Jun 5, 2024

andygrove Jun 6, 2024

vaibhawvipul left a comment

viirya Jun 5, 2024

kazuyukitanimura Jun 5, 2024

andygrove Jun 5, 2024

andygrove Jun 6, 2024 •

edited

Loading

andygrove Jun 6, 2024

kazuyukitanimura Jun 7, 2024

andygrove Jun 7, 2024

andygrove commented Jun 5, 2024 •

edited

Loading

andygrove Jun 6, 2024

kazuyukitanimura Jun 7, 2024

andygrove Jun 7, 2024

kazuyukitanimura Jun 7, 2024

andygrove Jun 7, 2024

chore: Make ANSI fallback more granular #509

chore: Make ANSI fallback more granular #509

Conversation

andygrove commented Jun 3, 2024 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

How are these changes tested?

andygrove commented Jun 3, 2024

andygrove commented Jun 4, 2024

parthchandra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vaibhawvipul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andygrove Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andygrove commented Jun 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andygrove commented Jun 3, 2024 •

edited

Loading

andygrove Jun 6, 2024 •

edited

Loading

andygrove commented Jun 5, 2024 •

edited

Loading