[GitHub] spark pull request #22154: [SPARK-23711][SPARK-25140][SQL] Catch correct exc...
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/22154#discussion_r234385204
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CodeGeneratorWithInterpretedFallbackSuite.scala
---
@@ -17,17 +17,33 @@
package org.apache.spark.sql.catalyst.expressions
+import java.util.concurrent.ExecutionException
+
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}
+import org.apache.spark.sql.types.IntegerType
class CodeGeneratorWithInterpretedFallbackSuite extends SparkFunSuite with
PlanTestBase {
- test("UnsafeProjection with codegen factory mode") {
-val input = Seq(LongType, IntegerType)
- .zipWithIndex.map(x => BoundReference(x._2, x._1, true))
+ object FailedCodegenProjection
+ extends CodeGeneratorWithInterpretedFallback[Seq[Expression],
UnsafeProjection] {
+
+override protected def createCodeGeneratedObject(in: Seq[Expression]):
UnsafeProjection = {
+ val invalidCode = new CodeAndComment("invalid code", Map.empty)
+ // We assume this compilation throws an exception
--- End diff --
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)
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22154: [SPARK-23711][SPARK-25140][SQL] Catch correct exc...
Github user jaceklaskowski commented on a diff in the pull request:
https://github.com/apache/spark/pull/22154#discussion_r234177079
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CodeGeneratorWithInterpretedFallbackSuite.scala
---
@@ -17,17 +17,33 @@
package org.apache.spark.sql.catalyst.expressions
+import java.util.concurrent.ExecutionException
+
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}
+import org.apache.spark.sql.types.IntegerType
class CodeGeneratorWithInterpretedFallbackSuite extends SparkFunSuite with
PlanTestBase {
- test("UnsafeProjection with codegen factory mode") {
-val input = Seq(LongType, IntegerType)
- .zipWithIndex.map(x => BoundReference(x._2, x._1, true))
+ object FailedCodegenProjection
+ extends CodeGeneratorWithInterpretedFallback[Seq[Expression],
UnsafeProjection] {
+
+override protected def createCodeGeneratedObject(in: Seq[Expression]):
UnsafeProjection = {
+ val invalidCode = new CodeAndComment("invalid code", Map.empty)
+ // We assume this compilation throws an exception
--- End diff --
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).
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22154: [SPARK-23711][SPARK-25140][SQL] Catch correct exc...
Github user asfgit closed the pull request at: https://github.com/apache/spark/pull/22154 --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22154: [SPARK-23711][SPARK-25140][SQL] Catch correct exc...
Github user maropu commented on a diff in the pull request:
https://github.com/apache/spark/pull/22154#discussion_r211792988
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/CodeGeneratorWithInterpretedFallback.scala
---
@@ -63,7 +49,10 @@ abstract class CodeGeneratorWithInterpretedFallback[IN,
OUT] {
try {
createCodeGeneratedObject(in)
} catch {
- 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")
--- End diff --
+ 1 to keep the current message. `CodeGenerator` already has printed many
infos for errors.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22154: [SPARK-23711][SPARK-25140][SQL] Catch correct exc...
Github user maropu commented on a diff in the pull request:
https://github.com/apache/spark/pull/22154#discussion_r211787035
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/CodeGeneratorWithInterpretedFallback.scala
---
@@ -63,7 +49,10 @@ abstract class CodeGeneratorWithInterpretedFallback[IN,
OUT] {
try {
createCodeGeneratedObject(in)
} catch {
- case CodegenError(_) => createInterpretedObject(in)
+ case _: Exception =>
--- End diff --
ok. better to fix the WholeStageCodegenExec, too?
https://github.com/apache/spark/blob/60af2501e1afc00192c779f2736a4e3de12428fa/sql/core/src/main/scala/org/apache/spark/sql/execution/WholeStageCodegenExec.scala#L585
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22154: [SPARK-23711][SPARK-25140][SQL] Catch correct exc...
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/22154#discussion_r211731921
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/CodeGeneratorWithInterpretedFallback.scala
---
@@ -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)
--- End diff --
We didn't in the original PR. @maropu -san added one in this PR which is
great, plugs the hole.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22154: [SPARK-23711][SPARK-25140][SQL] Catch correct exc...
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/22154#discussion_r211731624
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/CodeGeneratorWithInterpretedFallback.scala
---
@@ -63,7 +49,10 @@ abstract class CodeGeneratorWithInterpretedFallback[IN,
OUT] {
try {
createCodeGeneratedObject(in)
} catch {
- case CodegenError(_) => createInterpretedObject(in)
+ case _: Exception =>
--- End diff --
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.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22154: [SPARK-23711][SPARK-25140][SQL] Catch correct exc...
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/22154#discussion_r211731392
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/CodeGeneratorWithInterpretedFallback.scala
---
@@ -63,7 +49,10 @@ abstract class CodeGeneratorWithInterpretedFallback[IN,
OUT] {
try {
createCodeGeneratedObject(in)
} catch {
- 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")
--- End diff --
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 )
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22154: [SPARK-23711][SPARK-25140][SQL] Catch correct exc...
Github user gatorsmile commented on a diff in the pull request:
https://github.com/apache/spark/pull/22154#discussion_r211730772
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/CodeGeneratorWithInterpretedFallback.scala
---
@@ -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)
--- End diff --
We do not have a test case for checking the fallback before?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22154: [SPARK-23711][SPARK-25140][SQL] Catch correct exc...
Github user gatorsmile commented on a diff in the pull request:
https://github.com/apache/spark/pull/22154#discussion_r211730445
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Projection.scala
---
@@ -180,7 +180,10 @@ object UnsafeProjection
try {
GenerateUnsafeProjection.generate(unsafeExprs,
subexpressionEliminationEnabled)
} catch {
- case CodegenError(_) =>
InterpretedUnsafeProjection.createProjection(unsafeExprs)
+ case _: Exception =>
--- End diff --
The same here.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22154: [SPARK-23711][SPARK-25140][SQL] Catch correct exc...
Github user gatorsmile commented on a diff in the pull request:
https://github.com/apache/spark/pull/22154#discussion_r211729573
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/CodeGeneratorWithInterpretedFallback.scala
---
@@ -63,7 +49,10 @@ abstract class CodeGeneratorWithInterpretedFallback[IN,
OUT] {
try {
createCodeGeneratedObject(in)
} catch {
- 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")
--- End diff --
Do we need to include the exception type we captured in the message?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22154: [SPARK-23711][SPARK-25140][SQL] Catch correct exc...
Github user gatorsmile commented on a diff in the pull request:
https://github.com/apache/spark/pull/22154#discussion_r211729173
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/CodeGeneratorWithInterpretedFallback.scala
---
@@ -63,7 +49,10 @@ abstract class CodeGeneratorWithInterpretedFallback[IN,
OUT] {
try {
createCodeGeneratedObject(in)
} catch {
- case CodegenError(_) => createInterpretedObject(in)
+ case _: Exception =>
--- End diff --
NonFatal ?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22154: [SPARK-23711][SPARK-25140][SQL] Catch correct exc...
Github user maropu commented on a diff in the pull request:
https://github.com/apache/spark/pull/22154#discussion_r211565872
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CodeGeneratorWithInterpretedFallbackSuite.scala
---
@@ -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))
+val fallback = CodegenObjectFactoryMode.FALLBACK.toString
+withSQLConf(SQLConf.CODEGEN_FACTORY_MODE.key -> fallback) {
+ val obj = FailedCodegenProjection.createObject(input)
+ assert(obj.isInstanceOf[InterpretedUnsafeProjection])
--- End diff --
yea, we could. If other reviewers say +1, I'll update (either's fine to me).
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22154: [SPARK-23711][SPARK-25140][SQL] Catch correct exc...
Github user maropu commented on a diff in the pull request:
https://github.com/apache/spark/pull/22154#discussion_r211565120
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CodeGeneratorWithInterpretedFallbackSuite.scala
---
@@ -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))
--- End diff --
ok
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22154: [SPARK-23711][SPARK-25140][SQL] Catch correct exc...
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/22154#discussion_r211506399
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CodeGeneratorWithInterpretedFallbackSuite.scala
---
@@ -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))
--- End diff --
SGTM.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22154: [SPARK-23711][SPARK-25140][SQL] Catch correct exc...
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/22154#discussion_r211487231
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CodeGeneratorWithInterpretedFallbackSuite.scala
---
@@ -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))
--- End diff --
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`, `_1`s. This is
also optional. WDYT @maropu @viirya ?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22154: [SPARK-23711][SPARK-25140][SQL] Catch correct exc...
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/22154#discussion_r211484515
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Projection.scala
---
@@ -180,7 +180,10 @@ object UnsafeProjection
try {
GenerateUnsafeProjection.generate(unsafeExprs,
subexpressionEliminationEnabled)
} catch {
- case CodegenError(_) =>
InterpretedUnsafeProjection.createProjection(unsafeExprs)
+ case _: Exception =>
+// We should have already see error message in `CodeGenerator`
--- End diff --
Nit: Perhaps `// the error message is already logged in CodeGenerator`? Or
`// We should have already seen the error message in CodeGenerator`?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22154: [SPARK-23711][SPARK-25140][SQL] Catch correct exc...
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/22154#discussion_r211485732
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CodeGeneratorWithInterpretedFallbackSuite.scala
---
@@ -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))
+val fallback = CodegenObjectFactoryMode.FALLBACK.toString
+withSQLConf(SQLConf.CODEGEN_FACTORY_MODE.key -> fallback) {
+ val obj = FailedCodegenProjection.createObject(input)
+ assert(obj.isInstanceOf[InterpretedUnsafeProjection])
--- End diff --
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.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22154: [SPARK-23711][SPARK-25140][SQL] Catch correct exc...
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/22154#discussion_r211485314
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CodeGeneratorWithInterpretedFallbackSuite.scala
---
@@ -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))
--- End diff --
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.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22154: [SPARK-23711][SPARK-25140][SQL] Catch correct exc...
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/22154#discussion_r211485848
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/CodeGeneratorWithInterpretedFallback.scala
---
@@ -63,7 +49,10 @@ abstract class CodeGeneratorWithInterpretedFallback[IN,
OUT] {
try {
createCodeGeneratedObject(in)
} catch {
- case CodegenError(_) => createInterpretedObject(in)
+ case _: Exception =>
+// We should have already see error message in `CodeGenerator`
--- End diff --
Same as the comment below.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22154: [SPARK-23711][SPARK-25140][SQL] Catch correct exc...
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/22154#discussion_r211484616
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Projection.scala
---
@@ -180,7 +180,10 @@ object UnsafeProjection
try {
GenerateUnsafeProjection.generate(unsafeExprs,
subexpressionEliminationEnabled)
} catch {
- 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")
--- End diff --
Nit: I prefer "falling back" instead of "falls back" here. e.g. "Expr
codegen error, falling back to interpreter mode"
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22154: [SPARK-23711][SPARK-25140][SQL] Catch correct exc...
Github user rednaxelafx commented on a diff in the pull request:
https://github.com/apache/spark/pull/22154#discussion_r211484374
--- Diff:
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/CodeGeneratorWithInterpretedFallbackSuite.scala
---
@@ -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]
--- End diff --
Nit: do we need the cast here?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22154: [SPARK-23711][SPARK-25140][SQL] Catch correct exc...
Github user maropu commented on a diff in the pull request:
https://github.com/apache/spark/pull/22154#discussion_r211451705
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Projection.scala
---
@@ -180,7 +180,10 @@ object UnsafeProjection
try {
GenerateUnsafeProjection.generate(unsafeExprs,
subexpressionEliminationEnabled)
} catch {
- 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")
--- End diff --
oh, I missed...
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22154: [SPARK-23711][SPARK-25140][SQL] Catch correct exc...
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/22154#discussion_r211446570
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Projection.scala
---
@@ -180,7 +180,10 @@ object UnsafeProjection
try {
GenerateUnsafeProjection.generate(unsafeExprs,
subexpressionEliminationEnabled)
} catch {
- 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")
--- End diff --
`logWarning`?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22154: [SPARK-23711][SPARK-25140][SQL] Catch correct exc...
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/22154#discussion_r211443168
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/Projection.scala
---
@@ -180,7 +180,10 @@ object UnsafeProjection
try {
GenerateUnsafeProjection.generate(unsafeExprs,
subexpressionEliminationEnabled)
} catch {
- 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")
--- End diff --
ditto. `logWarning`.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22154: [SPARK-23711][SPARK-25140][SQL] Catch correct exc...
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/22154#discussion_r211443069
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/CodeGeneratorWithInterpretedFallback.scala
---
@@ -63,7 +49,10 @@ abstract class CodeGeneratorWithInterpretedFallback[IN,
OUT] {
try {
createCodeGeneratedObject(in)
} catch {
- 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")
--- End diff --
`logWarning` instead of `logError`? It uses `logWarning` in
`WholeStageCodegenExec` too.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22154: [SPARK-23711][SPARK-25140][SQL] Catch correct exc...
Github user maropu commented on a diff in the pull request:
https://github.com/apache/spark/pull/22154#discussion_r211435227
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/CodeGeneratorWithInterpretedFallback.scala
---
@@ -63,7 +49,10 @@ abstract class CodeGeneratorWithInterpretedFallback[IN,
OUT] {
try {
createCodeGeneratedObject(in)
} catch {
- 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")
--- End diff --
ok
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22154: [SPARK-23711][SPARK-25140][SQL] Catch correct exc...
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/22154#discussion_r211432763
--- Diff:
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/CodeGeneratorWithInterpretedFallback.scala
---
@@ -63,7 +49,10 @@ abstract class CodeGeneratorWithInterpretedFallback[IN,
OUT] {
try {
createCodeGeneratedObject(in)
} catch {
- 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")
--- End diff --
`Expr codegen error and falls back to interpreter mode`?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #22154: [SPARK-23711][SPARK-25140][SQL] Catch correct exc...
GitHub user maropu opened a pull request: https://github.com/apache/spark/pull/22154 [SPARK-23711][SPARK-25140][SQL] Catch correct exceptions when expr codegen fails ## 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: https://github.com/apache/spark/blob/60af2501e1afc00192c779f2736a4e3de12428fa/sql/core/src/main/scala/org/apache/spark/sql/execution/WholeStageCodegenExec.scala#L585 ## How was this patch tested? Added tests in `CodeGeneratorWithInterpretedFallbackSuite` You can merge this pull request into a Git repository by running: $ git pull https://github.com/maropu/spark SPARK-25140 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/spark/pull/22154.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #22154 commit b31dd28afbdd8d1078d9acf7785c18dc55afd9c2 Author: Takeshi Yamamuro Date: 2018-08-20T15:11:45Z Fix --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
