-
Notifications
You must be signed in to change notification settings - Fork 28.5k
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
[SPARK-19372][SQL] Fix throwing a Java exception at df.fliter() due to 64KB bytecode size limit #17087
[SPARK-19372][SQL] Fix throwing a Java exception at df.fliter() due to 64KB bytecode size limit #17087
Changes from all commits
45c02b5
6a94c2f
1bb2211
96c1033
e870e3d
cb291cd
e653d50
8548b0e
51a71a6
ea67c8a
c2e6b8c
8b6ba75
1f19c80
3868bf5
a5fd465
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 |
---|---|---|
|
@@ -22,6 +22,9 @@ import java.io.{ByteArrayInputStream, ByteArrayOutputStream, DataInputStream, Da | |
import scala.collection.mutable.ArrayBuffer | ||
import scala.concurrent.ExecutionContext | ||
|
||
import org.codehaus.commons.compiler.CompileException | ||
import org.codehaus.janino.JaninoRuntimeException | ||
|
||
import org.apache.spark.{broadcast, SparkEnv} | ||
import org.apache.spark.internal.Logging | ||
import org.apache.spark.io.CompressionCodec | ||
|
@@ -353,9 +356,27 @@ abstract class SparkPlan extends QueryPlan[SparkPlan] with Logging with Serializ | |
GenerateMutableProjection.generate(expressions, inputSchema, useSubexprElimination) | ||
} | ||
|
||
private def genInterpretedPredicate( | ||
expression: Expression, inputSchema: Seq[Attribute]): InterpretedPredicate = { | ||
val str = expression.toString | ||
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. I think Expression.toString will truncate too big expression. Right? 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. Yeah, |
||
val logMessage = if (str.length > 256) { | ||
str.substring(0, 256 - 3) + "..." | ||
} else { | ||
str | ||
} | ||
logWarning(s"Codegen disabled for this expression:\n $logMessage") | ||
InterpretedPredicate.create(expression, inputSchema) | ||
} | ||
|
||
protected def newPredicate( | ||
expression: Expression, inputSchema: Seq[Attribute]): GenPredicate = { | ||
GeneratePredicate.generate(expression, inputSchema) | ||
try { | ||
GeneratePredicate.generate(expression, inputSchema) | ||
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. Shall we only do this fallback if 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. It is good to control it using an option. This is not a part of the whole-stage codegen. 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. I am wondering if it makes sense that 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. Sure, now look at |
||
} catch { | ||
case e @ (_: JaninoRuntimeException | _: CompileException) | ||
if sqlContext == null || sqlContext.conf.wholeStageFallback => | ||
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.
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. Pretty risky if we always fallback. This might hide bugs. 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. Let me fix it by another PR. |
||
genInterpretedPredicate(expression, inputSchema) | ||
} | ||
} | ||
|
||
protected def newOrdering( | ||
|
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.
Looks like
CompileException
can be thrown from janino?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.
Thank you for pointing out . Now,
CompileException
can be caught.