-
Notifications
You must be signed in to change notification settings - Fork 29k
[SPARK-29968][SQL] Remove the Predicate code from SparkPlan #26604
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 all 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 |
|---|---|---|
|
|
@@ -21,20 +21,28 @@ import scala.collection.immutable.TreeSet | |
|
|
||
| import org.apache.spark.sql.catalyst.InternalRow | ||
| import org.apache.spark.sql.catalyst.analysis.TypeCheckResult | ||
| import org.apache.spark.sql.catalyst.expressions.BindReferences.bindReference | ||
| import org.apache.spark.sql.catalyst.expressions.aggregate.AggregateExpression | ||
| import org.apache.spark.sql.catalyst.expressions.codegen.{CodegenContext, CodeGenerator, ExprCode, FalseLiteral, GenerateSafeProjection, GenerateUnsafeProjection, Predicate => BasePredicate} | ||
| import org.apache.spark.sql.catalyst.expressions.codegen._ | ||
| import org.apache.spark.sql.catalyst.expressions.codegen.Block._ | ||
| import org.apache.spark.sql.catalyst.plans.logical.{Aggregate, LeafNode, LogicalPlan, Project} | ||
| import org.apache.spark.sql.catalyst.util.TypeUtils | ||
| import org.apache.spark.sql.internal.SQLConf | ||
| import org.apache.spark.sql.types._ | ||
|
|
||
|
|
||
| object InterpretedPredicate { | ||
| def create(expression: Expression, inputSchema: Seq[Attribute]): InterpretedPredicate = | ||
| create(BindReferences.bindReference(expression, inputSchema)) | ||
| /** | ||
| * A base class for generated/interpreted predicate | ||
| */ | ||
| abstract class BasePredicate { | ||
|
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. It seems reasonable because we renamed |
||
| def eval(r: InternalRow): Boolean | ||
|
|
||
| def create(expression: Expression): InterpretedPredicate = new InterpretedPredicate(expression) | ||
| /** | ||
| * Initializes internal states given the current partition index. | ||
| * This is used by nondeterministic expressions to set initial states. | ||
| * The default implementation does nothing. | ||
| */ | ||
| def initialize(partitionIndex: Int): Unit = {} | ||
| } | ||
|
|
||
| case class InterpretedPredicate(expression: Expression) extends BasePredicate { | ||
|
|
@@ -56,6 +64,35 @@ trait Predicate extends Expression { | |
| override def dataType: DataType = BooleanType | ||
| } | ||
|
|
||
| /** | ||
| * The factory object for `BasePredicate`. | ||
| */ | ||
| object Predicate extends CodeGeneratorWithInterpretedFallback[Expression, BasePredicate] { | ||
cloud-fan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| override protected def createCodeGeneratedObject(in: Expression): BasePredicate = { | ||
| GeneratePredicate.generate(in) | ||
| } | ||
|
|
||
| override protected def createInterpretedObject(in: Expression): BasePredicate = { | ||
| InterpretedPredicate(in) | ||
| } | ||
|
|
||
| def createInterpreted(e: Expression): InterpretedPredicate = InterpretedPredicate(e) | ||
|
|
||
| /** | ||
| * Returns a BasePredicate for an Expression, which will be bound to `inputSchema`. | ||
| */ | ||
| def create(e: Expression, inputSchema: Seq[Attribute]): BasePredicate = { | ||
| createObject(bindReference(e, inputSchema)) | ||
| } | ||
|
|
||
|
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. We can add a method
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. ok. |
||
| /** | ||
| * Returns a BasePredicate for a given bound Expression. | ||
| */ | ||
| def create(e: Expression): BasePredicate = { | ||
| createObject(e) | ||
| } | ||
| } | ||
|
|
||
| trait PredicateHelper { | ||
| protected def splitConjunctivePredicates(condition: Expression): Seq[Expression] = { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1507,7 +1507,7 @@ object ConvertToLocalRelation extends Rule[LogicalPlan] { | |
|
|
||
| case Filter(condition, LocalRelation(output, data, isStreaming)) | ||
| if !hasUnevaluableExpr(condition) => | ||
| val predicate = InterpretedPredicate.create(condition, output) | ||
| val predicate = Predicate.create(condition, output) | ||
|
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. This is to optimize local relation so perf doesn't matter too much. The change should be fine.
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 |
||
| predicate.initialize(0) | ||
| LocalRelation(output, data.filter(row => predicate.eval(row)), isStreaming) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -21,7 +21,6 @@ import java.io.{ByteArrayInputStream, ByteArrayOutputStream, DataInputStream, Da | |||
| import java.util.concurrent.atomic.AtomicInteger | ||||
|
|
||||
| import scala.collection.mutable.ArrayBuffer | ||||
| import scala.concurrent.ExecutionContext | ||||
|
|
||||
| import org.codehaus.commons.compiler.CompileException | ||||
| import org.codehaus.janino.InternalCompilerException | ||||
|
|
@@ -33,7 +32,7 @@ import org.apache.spark.rdd.{RDD, RDDOperationScope} | |||
| import org.apache.spark.sql.{Row, SparkSession} | ||||
| import org.apache.spark.sql.catalyst.{CatalystTypeConverters, InternalRow} | ||||
| import org.apache.spark.sql.catalyst.expressions._ | ||||
| import org.apache.spark.sql.catalyst.expressions.codegen.{Predicate => GenPredicate, _} | ||||
| import org.apache.spark.sql.catalyst.expressions.codegen._ | ||||
| import org.apache.spark.sql.catalyst.plans.QueryPlan | ||||
| import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan | ||||
| import org.apache.spark.sql.catalyst.plans.physical._ | ||||
|
|
@@ -471,28 +470,6 @@ abstract class SparkPlan extends QueryPlan[SparkPlan] with Logging with Serializ | |||
| MutableProjection.create(expressions, inputSchema) | ||||
| } | ||||
|
|
||||
| private def genInterpretedPredicate( | ||||
| expression: Expression, inputSchema: Seq[Attribute]): InterpretedPredicate = { | ||||
| val str = expression.toString | ||||
| 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) | ||||
|
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. I didn't follow the context in the previous PR. Is this
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, Line 63 in 5a70af7
|
||||
| } | ||||
|
|
||||
| protected def newPredicate( | ||||
| expression: Expression, inputSchema: Seq[Attribute]): GenPredicate = { | ||||
| try { | ||||
| GeneratePredicate.generate(expression, inputSchema) | ||||
| } catch { | ||||
| case _ @ (_: InternalCompilerException | _: CompileException) if codeGenFallBack => | ||||
| genInterpretedPredicate(expression, inputSchema) | ||||
| } | ||||
| } | ||||
|
|
||||
| protected def newOrdering( | ||||
| order: Seq[SortOrder], inputSchema: Seq[Attribute]): Ordering[InternalRow] = { | ||||
| GenerateOrdering.generate(order, inputSchema) | ||||
|
|
||||
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.
Let's not touch this file in this PR~
Uh oh!
There was an error while loading. Please reload this page.
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.
You meant a separate PR for this typo?