Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
maropu wants to merge 6 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b31dd28
Fix
maropu Aug 20, 2018
e3e86c6
Fix
maropu Aug 21, 2018
5f0ff13
Fix
maropu Aug 21, 2018
5b2c777
Fix
maropu Aug 21, 2018
5481255
Add tests
maropu Aug 21, 2018
129c25d
Fix
maropu Aug 21, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 6 additions & 17 deletions ...cala/org/apache/spark/sql/catalyst/expressions/CodeGeneratorWithInterpretedFallback.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,24 +17,10 @@

package org.apache.spark.sql.catalyst.expressions

import org.codehaus.commons.compiler.CompileException
import org.codehaus.janino.InternalCompilerException

import org.apache.spark.TaskContext
import org.apache.spark.internal.Logging
import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.util.Utils

/**
* Catches compile error during code generation.
*/
object CodegenError {
def unapply(throwable: Throwable): Option[Exception] = throwable match {
case e: InternalCompilerException => Some(e)
case e: CompileException => Some(e)
Copy link
Member

@gatorsmile gatorsmile Aug 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@rednaxelafx rednaxelafx Aug 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

case _ => None
}
}

/**
* Defines values for `SQLConf` config of fallback mode. Use for test only.
*/
Expand All @@ -47,7 +33,7 @@ object CodegenObjectFactoryMode extends Enumeration {
* error happens, it can fallback to interpreted implementation. In tests, we can use a SQL config
* `SQLConf.CODEGEN_FACTORY_MODE` to control fallback behavior.
*/
abstract class CodeGeneratorWithInterpretedFallback[IN, OUT] {
abstract class CodeGeneratorWithInterpretedFallback[IN, OUT] extends Logging {

def createObject(in: IN): OUT = {
// We are allowed to choose codegen-only or no-codegen modes if under tests.
Expand All @@ -63,7 +49,10 @@ abstract class CodeGeneratorWithInterpretedFallback[IN, OUT] {
try {
createCodeGeneratedObject(in)
} catch {
case CodegenError(_) => createInterpretedObject(in)
case _: Exception =>
Copy link
Member

@gatorsmile gatorsmile Aug 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NonFatal ?

Copy link
Contributor

@rednaxelafx rednaxelafx Aug 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@maropu maropu Aug 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok.

// We should have already see error message in `CodeGenerator`
Copy link
Contributor

@rednaxelafx rednaxelafx Aug 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as the comment below.

logWarning("Expr codegen error and falls back to interpreter mode")
createInterpretedObject(in)
}
}
}
Expand Down
5 changes: 4 additions & 1 deletion sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Projection.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,10 @@ object UnsafeProjection
try {
GenerateUnsafeProjection.generate(unsafeExprs, subexpressionEliminationEnabled)
} catch {
case CodegenError(_) => InterpretedUnsafeProjection.createProjection(unsafeExprs)
case _: Exception =>
Copy link
Member

@gatorsmile gatorsmile Aug 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same here.

// We should have already see error message in `CodeGenerator`
Copy link
Contributor

@rednaxelafx rednaxelafx Aug 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

logWarning("Expr codegen error and falls back to interpreter mode")
Copy link
Contributor

@rednaxelafx rednaxelafx Aug 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

InterpretedUnsafeProjection.createProjection(unsafeExprs)
}
}
}
Expand Down
24 changes: 24 additions & 0 deletions ...org/apache/spark/sql/catalyst/expressions/CodeGeneratorWithInterpretedFallbackSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,27 @@
package org.apache.spark.sql.catalyst.expressions

import org.apache.spark.SparkFunSuite
import org.apache.spark.sql.catalyst.expressions.codegen.{CodeAndComment, CodeGenerator}
import org.apache.spark.sql.catalyst.plans.PlanTestBase
import org.apache.spark.sql.internal.SQLConf
import org.apache.spark.sql.types.{IntegerType, LongType}

class CodeGeneratorWithInterpretedFallbackSuite extends SparkFunSuite with PlanTestBase {

object FailedCodegenProjection
extends CodeGeneratorWithInterpretedFallback[Seq[Expression], UnsafeProjection] {

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

@rednaxelafx rednaxelafx Aug 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: do we need the cast here?

}

override protected def createInterpretedObject(in: Seq[Expression]): UnsafeProjection = {
InterpretedUnsafeProjection.createProjection(in)
}
}

test("UnsafeProjection with codegen factory mode") {
val input = Seq(LongType, IntegerType)
.zipWithIndex.map(x => BoundReference(x._2, x._1, true))
Expand All @@ -40,4 +55,13 @@ class CodeGeneratorWithInterpretedFallbackSuite extends SparkFunSuite with PlanT
assert(obj.isInstanceOf[InterpretedUnsafeProjection])
}
}

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

@rednaxelafx rednaxelafx Aug 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@rednaxelafx rednaxelafx Aug 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member

@viirya viirya Aug 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM.

Copy link
Member Author

@maropu maropu Aug 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

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

@rednaxelafx rednaxelafx Aug 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@maropu maropu Aug 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}
}