[SPARK-23711][SPARK-25140][SQL] Catch correct exceptions when expr codegen fails #22154

maropu · 2018-08-20T15:31:42Z

What changes were proposed in this pull request?

This pr is to fix bugs when expr codegen fails; we need to catch java.util.concurrent.ExecutionException instead of InternalCompilerException and CompileException . This handling is the same with the WholeStageCodegenExec one:

spark/sql/core/src/main/scala/org/apache/spark/sql/execution/WholeStageCodegenExec.scala

Line 585 in 60af250

case _: Exception if !Utils.isTesting && sqlContext.conf.codegenFallback =>

How was this patch tested?

Added tests in CodeGeneratorWithInterpretedFallbackSuite

SparkQA · 2018-08-20T19:22:12Z

Test build #94961 has finished for PR 22154 at commit b31dd28.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
abstract class CodeGeneratorWithInterpretedFallback[IN, OUT] extends Logging

maropu · 2018-08-20T22:09:57Z

IIUC CacheLoader throws ExecutionException when CodeGenerator.doCompile throws InternalCompilerException or CompileException;

https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala#L1305

maropu · 2018-08-20T22:11:43Z

cc: @gatorsmile @cloud-fan @viirya @rednaxelafx

viirya · 2018-08-20T22:57:17Z

...n/scala/org/apache/spark/sql/catalyst/expressions/CodeGeneratorWithInterpretedFallback.scala

-          case CodegenError(_) => createInterpretedObject(in)
+          case _: Exception =>
+            // We should have already see error message in `CodeGenerator`
+            logError("Expr codegen disabled and falls back to the interpreter mode")


Expr codegen error and falls back to interpreter mode?

viirya · 2018-08-20T23:57:14Z

...n/scala/org/apache/spark/sql/catalyst/expressions/CodeGeneratorWithInterpretedFallback.scala

-          case CodegenError(_) => createInterpretedObject(in)
+          case _: Exception =>
+            // We should have already see error message in `CodeGenerator`
+            logError("Expr codegen disabled and falls back to the interpreter mode")


logWarning instead of logError? It uses logWarning in WholeStageCodegenExec too.

viirya · 2018-08-20T23:57:29Z

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

-      case CodegenError(_) => InterpretedUnsafeProjection.createProjection(unsafeExprs)
+      case _: Exception =>
+        // We should have already see error message in `CodeGenerator`
+        logError("Expr codegen disabled and falls back to the interpreter mode")


ditto. logWarning.

viirya · 2018-08-21T00:20:54Z

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

-      case CodegenError(_) => InterpretedUnsafeProjection.createProjection(unsafeExprs)
+      case _: Exception =>
+        // We should have already see error message in `CodeGenerator`
+        logError("Expr codegen error and falls back to interpreter mode")


logWarning too?

oh, I missed...

viirya · 2018-08-21T01:05:36Z

LGTM

SparkQA · 2018-08-21T04:06:32Z

Test build #94983 has finished for PR 22154 at commit e3e86c6.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-08-21T04:12:31Z

Test build #94985 has finished for PR 22154 at commit 5f0ff13.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2018-08-21T04:15:02Z

retest this please.

rednaxelafx

Thank you very much for adding the logging and also catching that we weren't catching the correct exception type! (Pun intended ;-)

Mostly LGTM. Can I ask for an additional test case of the CODEGEN_ONLY mode with codegen failure, where we should assert an exception was thrown and the cause of the exception is the intended Janino exception type? Thanks!

rednaxelafx · 2018-08-21T05:47:31Z

...la/org/apache/spark/sql/catalyst/expressions/CodeGeneratorWithInterpretedFallbackSuite.scala

+    override protected def createCodeGeneratedObject(in: Seq[Expression]): UnsafeProjection = {
+      val invalidCode = new CodeAndComment("invalid code", Map.empty)
+      CodeGenerator.compile(invalidCode)
+      null.asInstanceOf[UnsafeProjection]


Nit: do we need the cast here?

rednaxelafx · 2018-08-21T05:48:37Z

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

    } catch {
-      case CodegenError(_) => InterpretedUnsafeProjection.createProjection(unsafeExprs)
+      case _: Exception =>
+        // We should have already see error message in `CodeGenerator`


Nit: Perhaps // the error message is already logged in CodeGenerator? Or // We should have already seen the error message in CodeGenerator?

rednaxelafx · 2018-08-21T05:49:30Z

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

-      case CodegenError(_) => InterpretedUnsafeProjection.createProjection(unsafeExprs)
+      case _: Exception =>
+        // We should have already see error message in `CodeGenerator`
+        logWarning("Expr codegen error and falls back to interpreter mode")


Nit: I prefer "falling back" instead of "falls back" here. e.g. "Expr codegen error, falling back to interpreter mode"

rednaxelafx · 2018-08-21T05:54:52Z

...la/org/apache/spark/sql/catalyst/expressions/CodeGeneratorWithInterpretedFallbackSuite.scala

  }
+
+  test("fallback to the interpreter mode") {
+    val input = Seq(IntegerType).zipWithIndex.map(x => BoundReference(x._2, x._1, true))


Nit: if we only wanted a one-element-sequence, would it be cleaner to just write
Seq(BoundReference(0, IntegerType, true))?
Or for those that prefer the cons list syntax, BoundReference(0, IntegerType, true) :: Nil. I prefer the former one but I don't have a strong opinion here.

I'd also like to suggest changing the other test cases in this suite to do something like Seq(IntegerType).zipWithIndex.map { case (tpe, ordinal) => BoundReference(ordinal, tpe, true) } to get rid of the _2, _1s. This is also optional. WDYT @maropu @viirya ?

rednaxelafx · 2018-08-21T05:58:09Z

...la/org/apache/spark/sql/catalyst/expressions/CodeGeneratorWithInterpretedFallbackSuite.scala

+    val fallback = CodegenObjectFactoryMode.FALLBACK.toString
+    withSQLConf(SQLConf.CODEGEN_FACTORY_MODE.key -> fallback) {
+      val obj = FailedCodegenProjection.createObject(input)
+      assert(obj.isInstanceOf[InterpretedUnsafeProjection])


Should/could we also assert on the log message being printed out? Perhaps something similar to https://github.com/apache/spark/pull/22103/files#diff-cf187b40d98ff322d4bde4185701baa2R508 ?

This is optional; the fact that we've gotten back an InterpretedUnsafeProjection is a good enough indicator to me.

yea, we could. If other reviewers say +1, I'll update (either's fine to me).

rednaxelafx · 2018-08-21T05:59:08Z

...n/scala/org/apache/spark/sql/catalyst/expressions/CodeGeneratorWithInterpretedFallback.scala

        } catch {
-          case CodegenError(_) => createInterpretedObject(in)
+          case _: Exception =>
+            // We should have already see error message in `CodeGenerator`


Same as the comment below.

SparkQA · 2018-08-21T07:05:01Z

Test build #94992 has finished for PR 22154 at commit 5f0ff13.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

viirya · 2018-08-21T07:40:20Z

retest this please.

maropu · 2018-08-21T11:34:48Z

@rednaxelafx Thanks for your checks ;) addressed.

SparkQA · 2018-08-21T11:49:29Z

Test build #95004 has finished for PR 22154 at commit 5f0ff13.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

rednaxelafx

LGTM, thanks a lot!

SparkQA · 2018-08-21T14:23:34Z

Test build #95014 has finished for PR 22154 at commit 5b2c777.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

maropu · 2018-08-21T14:25:31Z

retest this please

SparkQA · 2018-08-21T17:54:36Z

Test build #95025 has finished for PR 22154 at commit 5481255.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-08-21T18:18:27Z

Test build #95015 has finished for PR 22154 at commit 5481255.

This patch fails from timeout after a configured wait of `400m`.
This patch merges cleanly.
This patch adds no public classes.

gatorsmile · 2018-08-21T18:43:14Z

retest this please

gatorsmile · 2018-08-21T19:24:59Z

...n/scala/org/apache/spark/sql/catalyst/expressions/CodeGeneratorWithInterpretedFallback.scala

          createCodeGeneratedObject(in)
        } catch {
-          case CodegenError(_) => createInterpretedObject(in)
+          case _: Exception =>


Oh yeah that's a good point. +1 on using the NonFatal extractor, and we probably wanna do the same change for whole-stage codegen's fallback logic as well.

gatorsmile · 2018-08-21T19:26:21Z

...n/scala/org/apache/spark/sql/catalyst/expressions/CodeGeneratorWithInterpretedFallback.scala

-          case CodegenError(_) => createInterpretedObject(in)
+          case _: Exception =>
+            // We should have already seen the error message in `CodeGenerator`
+            logWarning("Expr codegen error and falling back to interpreter mode")


Do we need to include the exception type we captured in the message?

We could; although the detail exception type/message/stack trace info would already be printed in the preceding logging from the CodeGenerator so I wouldn't push strongly for adding the exception type here.
(It certainly is easier to do data science on just single lines of logs instead of having to search across a few lines for context, so I do see the benefits of adding the type information albeit redundant. I'll leave it up to you guys @maropu @viirya @cloud-fan @gatorsmile )

+1 to keep the current message. CodeGenerator already has printed many infos for errors.

gatorsmile · 2018-08-21T19:29:24Z

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

      GenerateUnsafeProjection.generate(unsafeExprs, subexpressionEliminationEnabled)
    } catch {
-      case CodegenError(_) => InterpretedUnsafeProjection.createProjection(unsafeExprs)
+      case _: Exception =>


The same here.

gatorsmile · 2018-08-21T19:30:32Z

...n/scala/org/apache/spark/sql/catalyst/expressions/CodeGeneratorWithInterpretedFallback.scala

-object CodegenError {
-  def unapply(throwable: Throwable): Option[Exception] = throwable match {
-    case e: InternalCompilerException => Some(e)
-    case e: CompileException => Some(e)


We do not have a test case for checking the fallback before?

We didn't in the original PR. @maropu -san added one in this PR which is great, plugs the hole.

SparkQA · 2018-08-21T22:49:17Z

Test build #95047 has finished for PR 22154 at commit 5481255.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-08-21T23:54:15Z

Test build #95060 has finished for PR 22154 at commit f657a7f.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

bersprockets · 2018-08-22T00:04:50Z

Re: Your build failure ('statefulOperators.scala:95: value asJava is not a member of scala.collection.immutable.Map[String,Long]).

I am also seeing this in my fork on my laptop (I just updated my fork about 10-15 minutes ago).

..and it was fixed: #22175

maropu · 2018-08-22T02:18:08Z

(it it jus a note) btw, currently, if expr codegen fails, the many error messages could happen in both a driver side and executor sides. I feel this is a little noisy for users. I think it'd be super nice if we could validate if all the expr codegen can works well in a driver side in a similar way of WholeStageCodegen. I don't have a nice idea now though...

SparkQA · 2018-08-22T05:10:48Z

Test build #95069 has finished for PR 22154 at commit 129c25d.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

gatorsmile · 2018-08-22T05:17:02Z

LGTM

Thanks! Merged to master.

jaceklaskowski · 2018-11-16T11:42:17Z

...la/org/apache/spark/sql/catalyst/expressions/CodeGeneratorWithInterpretedFallbackSuite.scala

+
+    override protected def createCodeGeneratedObject(in: Seq[Expression]): UnsafeProjection = {
+      val invalidCode = new CodeAndComment("invalid code", Map.empty)
+      // We assume this compilation throws an exception


I'd use this comment as part of an exception (say IllegalStateException or similar) that should be thrown rather than returning null. I think that would make the comment part of the code itself and can be checked in tests (by catching the exception).

The suggested change is only for making this test suite cleaner, right? In that case I'd +1 with the suggestion of being able to clearly check we're catching the exception we know we're throwing.
Would you like to submit a PR for it?

rather than returning null
The intent is never to actually reach the null-return, but always cause an exception to be thrown at CodeGenerator.compile() and abruptly return to the caller with the exception. To make the compiler happy you'll have to have some definite-returning statement to end the function, so a useless null-return would probably have to be there anyway (since the compiler can't tell you'll always be throwing an exception unless you do a throw inline)

Fix

b31dd28

viirya reviewed Aug 20, 2018

View reviewed changes

Fix

e3e86c6

viirya reviewed Aug 21, 2018

View reviewed changes

Fix

5f0ff13

rednaxelafx reviewed Aug 21, 2018

View reviewed changes

maropu added 2 commits August 21, 2018 20:13

Fix

5b2c777

Add tests

5481255

rednaxelafx reviewed Aug 21, 2018

View reviewed changes

gatorsmile reviewed Aug 21, 2018

View reviewed changes

Fix

129c25d

maropu force-pushed the SPARK-25140 branch from f657a7f to 129c25d Compare August 22, 2018 01:07

asfgit closed this in 07737c8 Aug 22, 2018

jaceklaskowski reviewed Nov 16, 2018

View reviewed changes

[SPARK-23711][SPARK-25140][SQL] Catch correct exceptions when expr codegen fails #22154

[SPARK-23711][SPARK-25140][SQL] Catch correct exceptions when expr codegen fails #22154

Uh oh!

Conversation

maropu commented Aug 20, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Aug 20, 2018

Uh oh!

maropu commented Aug 20, 2018

Uh oh!

maropu commented Aug 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viirya Aug 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viirya commented Aug 21, 2018

Uh oh!

SparkQA commented Aug 21, 2018

Uh oh!

SparkQA commented Aug 21, 2018

Uh oh!

viirya commented Aug 21, 2018

Uh oh!

rednaxelafx left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 21, 2018

Uh oh!

viirya commented Aug 21, 2018

Uh oh!

maropu commented Aug 21, 2018

Uh oh!

SparkQA commented Aug 21, 2018

Uh oh!

rednaxelafx left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 21, 2018

Uh oh!

maropu commented Aug 21, 2018

Uh oh!

SparkQA commented Aug 21, 2018

Uh oh!

SparkQA commented Aug 21, 2018

Uh oh!

gatorsmile commented Aug 21, 2018

viirya Aug 21, 2018 •

edited

Loading

rednaxelafx left a comment •

edited

Loading

maropu Aug 21, 2018 •

edited

Loading

maropu Aug 21, 2018 •

edited

Loading

bersprockets commented Aug 22, 2018 •

edited

Loading