[SPARK-25444][SQL] Refactor GenArrayData.genCodeToCreateArrayData method#22439
[SPARK-25444][SQL] Refactor GenArrayData.genCodeToCreateArrayData method#22439kiszk wants to merge 3 commits intoapache:masterfrom
Conversation
|
Test build #96120 has finished for PR 22439 at commit
|
| val isNullAssignment = if (!isMapKey) { | ||
| s"$arrayDataName.setNullAt($i);" | ||
|
|
||
| if (eval.isNull == TrueLiteral) { |
There was a problem hiding this comment.
Since I saw the following case, I added this condition to reduce the generated Java byte code size.
if (true) {
...
} else {
...
}
I am neutral on keeping or removing this.
There was a problem hiding this comment.
I don't think we need the case because there are very few expressions that set isNull to TrueLiteral.
| arrayDataName, elementType, i.toString, eval.value) | ||
|
|
||
| val assignment = if (eval.isNull == FalseLiteral) { | ||
| s"\n$setArrayElement\n" |
There was a problem hiding this comment.
nit: do we need to surround setArrayElement with \ns? Only setArrayElement should work?
There was a problem hiding this comment.
It works functionally without \n. This is only for readability of generation code to avoid this output:
arrayData.setInt(0, 0);arrayData.setInt(1, 1);...
WDYT?
| val setArrayElement = CodeGenerator.setArrayElement( | ||
| arrayDataName, elementType, i.toString, eval.value) | ||
|
|
||
| val assignment = if (eval.isNull == FalseLiteral) { |
There was a problem hiding this comment.
How about taking an argument elements: Seq[Expression] instead of elementCodes: Seq[ExprCode] and use element.nullable instead of eval.isNull if we want to skip null-check when the nullable is false?
There was a problem hiding this comment.
I see. We will do if (!element.nullable || eval.isNull == FalseLiteral) {
There was a problem hiding this comment.
Maybe we don't need || eval.isNull == FalseLiteral?
| val setArrayElement = CodeGenerator.setArrayElement( | ||
| arrayDataName, elementType, i.toString, eval.value) | ||
|
|
||
| val assignment = if (!expr.nullable || eval.isNull == FalseLiteral) { |
There was a problem hiding this comment.
Maybe we don't need || eval.isNull == FalseLiteral?
|
Test build #96133 has finished for PR 22439 at commit
|
|
LGTM. |
|
Test build #96144 has finished for PR 22439 at commit
|
|
Thanks! merging to master. |
3 participants
What changes were proposed in this pull request?
This PR makes
GenArrayData.genCodeToCreateArrayDatamethod simple by usingArrayData.createArrayDatamethod.Before this PR,
genCodeToCreateArrayDatamethod was complicatedArrayDataGenericArrayDataandUnsafeArrayDataAfter this PR, the method
GenericArrayDataorUnsafeArrayDatawithout a temporary arrayHow was this patch tested?
Existing UTs