-
Notifications
You must be signed in to change notification settings - Fork 29.3k
[SPARK-22856][SQL] Add wrappers for codegen output and nullability #20043
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
5ace8b8
d120750
81c9b6e
53926cc
69422d4
4384c84
d864aff
8715d32
f59bb19
37ae9b0
0841c4a
e530f01
c8c70a9
ac2e595
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 |
|---|---|---|
|
|
@@ -23,7 +23,7 @@ import java.util.{Map => JavaMap} | |
| import scala.collection.JavaConverters._ | ||
| import scala.collection.mutable | ||
| import scala.collection.mutable.ArrayBuffer | ||
| import scala.language.existentials | ||
| import scala.language.{existentials, implicitConversions} | ||
| import scala.util.control.NonFatal | ||
|
|
||
| import com.google.common.cache.{CacheBuilder, CacheLoader} | ||
|
|
@@ -56,7 +56,36 @@ import org.apache.spark.util.{ParentClassLoader, Utils} | |
| * @param value A term for a (possibly primitive) value of the result of the evaluation. Not | ||
| * valid if `isNull` is set to `true`. | ||
| */ | ||
| case class ExprCode(var code: String, var isNull: String, var value: String) | ||
| case class ExprCode(var code: String, var isNull: ExprValue, var value: ExprValue) | ||
|
|
||
|
|
||
| // An abstraction that represents the evaluation result of [[ExprCode]]. | ||
| abstract class ExprValue | ||
|
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. I think we should classify So basically we can combine
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. IMHO I prefer this approach because in the future we might need to distinguish these two cases, thus I think is a good thing to let them be distinct.
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. For now I don't have strong preference here.
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. @kiszk WDYT?
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. In summary, I have no strong preference. In the future, we will want to distinguish If this PR just focuses on classifying types between arguments and non-arguments, it is fine to combine
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. If no strong preference for combining them, I'd keep it as two concepts for now, if we foresee the need to distinguish them.
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. @cloud-fan What do you think?
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. OK let's keep it. |
||
|
|
||
| object ExprValue { | ||
| implicit def exprValueToString(exprValue: ExprValue): String = exprValue.toString | ||
| } | ||
|
|
||
| // A literal evaluation of [[ExprCode]]. | ||
| case class LiteralValue(val value: String) extends ExprValue { | ||
| override def toString: String = value | ||
| } | ||
|
|
||
| // A variable evaluation of [[ExprCode]]. | ||
| case class VariableValue(val variableName: String) extends ExprValue { | ||
| override def toString: String = variableName | ||
| } | ||
|
|
||
| // A statement evaluation of [[ExprCode]]. | ||
| case class StatementValue(val statement: String) extends ExprValue { | ||
| override def toString: String = statement | ||
| } | ||
|
|
||
| // A global variable evaluation of [[ExprCode]]. | ||
| case class GlobalValue(val value: String) extends ExprValue { | ||
|
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. for compacted global variables, we may get something like
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. It is considered as global variable now, as it can be accessed globally and don't/can't/shouldn't be a parameter. Actually we don't want to take global variables as parameters. |
||
| override def toString: String = value | ||
| } | ||
|
|
||
|
|
||
| /** | ||
| * State used for subexpression elimination. | ||
|
|
@@ -66,7 +95,7 @@ case class ExprCode(var code: String, var isNull: String, var value: String) | |
| * @param value A term for a value of a common sub-expression. Not valid if `isNull` | ||
| * is set to `true`. | ||
| */ | ||
| case class SubExprEliminationState(isNull: String, value: String) | ||
| case class SubExprEliminationState(isNull: ExprValue, value: ExprValue) | ||
|
|
||
| /** | ||
| * Codes and common subexpressions mapping used for subexpression elimination. | ||
|
|
@@ -264,7 +293,7 @@ class CodegenContext { | |
| case _: StructType | _: ArrayType | _: MapType => s"$value = $initCode.copy();" | ||
| case _ => s"$value = $initCode;" | ||
| } | ||
| ExprCode(code, "false", value) | ||
| ExprCode(code, LiteralValue("false"), GlobalValue(value)) | ||
| } | ||
|
|
||
| def declareMutableStates(): String = { | ||
|
|
@@ -1144,7 +1173,7 @@ class CodegenContext { | |
| // at least two nodes) as the cost of doing it is expected to be low. | ||
|
|
||
| subexprFunctions += s"${addNewFunction(fnName, fn)}($INPUT_ROW);" | ||
| val state = SubExprEliminationState(isNull, value) | ||
| val state = SubExprEliminationState(GlobalValue(isNull), GlobalValue(value)) | ||
| e.foreach(subExprEliminationExprs.put(_, state)) | ||
| } | ||
| } | ||
|
|
||
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.
nit: shall we introduce a
TrueLiteralandFalseLiteral?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.
I think it should be useful to the
isNullfield.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 a good idea.