Uh oh!
There was an error while loading. Please reload this page.
[SPARK-22682][SQL] HashExpression does not need to create global variables - #19878
[SPARK-22682][SQL] HashExpression does not need to create global variables#19878cloud-fan wants to merge 1 commit into
Conversation
cloud-fan
commented
Dec 4, 2017
| } | ||
| val hashResultType = ctx.javaType(dataType) | ||
| val codes = if (ctx.INPUT_ROW == null || ctx.currentVars != null) { |
There was a problem hiding this comment.
This pattern appears many times in the code base, we may need to create a ctx.splitExpressionsWithCurrentInput for it later.
There was a problem hiding this comment.
That one has been merged, but this one is still different.
| childResult, ctx)} | ||
| $localResult = (31 * $localResult) + $childResult; | ||
| """ | ||
| }.mkString( |
There was a problem hiding this comment.
We forgot to split the code for computing hive hash of struct, it's fixed now.
| val fieldsHash = fields.zipWithIndex.map { case (field, index) => | ||
| nullSafeElementHash(input, index.toString, field.nullable, field.dataType, result, ctx) | ||
| } | ||
| val hashResultType = ctx.javaType(dataType) |
There was a problem hiding this comment.
nit: this is done also in line 281. Can we do this only once? maybe with a lazy val?
There was a problem hiding this comment.
ctx is only available inside doGenCode
mgaido91
commented
Dec 4, 2017
left only one very minor comment, it LGTM |
SparkQA
commented
Dec 4, 2017
Test build #84431 has finished for PR 19878 at commit
|
| ev.copy(code = | ||
| s""" | ||
| |${ctx.JAVA_INT} ${ev.value} = $seed; | ||
| |${ctx.JAVA_INT} $childHash = 0; |
There was a problem hiding this comment.
nit: childHash is only needed to declare here when we don't split functions.
There was a problem hiding this comment.
nvm, splitExpressions could possibly not split expressions if only one block.
| """.stripMargin | ||
| } | ||
| s"${ctx.JAVA_INT} $childResult = 0;\n" + ctx.splitExpressions( |
There was a problem hiding this comment.
No need to check ctx.INPUT_ROW == null || ctx.currentVars != null here?
There was a problem hiding this comment.
Yea, the input here is a row that may be produced by row.getStruct instead of ctx.INPUT_ROW, so we don't need this check as the input won't be ctx.currentVars.
viirya
commented
Dec 5, 2017
LGTM |
cloud-fan
commented
Dec 5, 2017
thanks, merging to master! |
What changes were proposed in this pull request?
It turns out that
HashExpressioncan pass around some values via parameter when splitting codes into methods, to save some global variable slots.This can also prevent a weird case that global variable appears in parameter list, which is discovered by #19865
How was this patch tested?
existing tests