-
Notifications
You must be signed in to change notification settings - Fork 29.3k
[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.
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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) | ||
| case _ => None | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Defines values for `SQLConf` config of fallback mode. Use for test only. | ||
| */ | ||
|
|
@@ -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. | ||
|
|
@@ -63,7 +49,10 @@ abstract class CodeGeneratorWithInterpretedFallback[IN, OUT] { | |
| try { | ||
| createCodeGeneratedObject(in) | ||
| } catch { | ||
| case CodegenError(_) => createInterpretedObject(in) | ||
| case _: Exception => | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NonFatal ?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh yeah that's a good point. +1 on using the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok. |
||
| // We should have already see error message in `CodeGenerator` | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -180,7 +180,10 @@ object UnsafeProjection | |
| try { | ||
| GenerateUnsafeProjection.generate(unsafeExprs, subexpressionEliminationEnabled) | ||
| } catch { | ||
| case CodegenError(_) => InterpretedUnsafeProjection.createProjection(unsafeExprs) | ||
| case _: Exception => | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The same here. |
||
| // We should have already see error message in `CodeGenerator` | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: Perhaps |
||
| logError("Expr codegen error and falls back to interpreter mode") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh, I missed... |
||
| InterpretedUnsafeProjection.createProjection(unsafeExprs) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)) | ||
|
|
@@ -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)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SGTM.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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]) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). |
||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.