[SPARK-22693][SQL] CreateNamedStruct and InSet should not use global variables#19896
[SPARK-22693][SQL] CreateNamedStruct and InSet should not use global variables#19896mgaido91 wants to merge 5 commits intoapache:masterfrom
Conversation
|
Test build #84484 has finished for PR 19896 at commit
|
|
@cloud-fan @kiszk @viirya may you please review this? Thanks |
| }) | ||
| valCodes, | ||
| "createNamedStruct", | ||
| "Object[]" -> values :: Nil) |
There was a problem hiding this comment.
nit: please keep the parameter name
ctx.splitExpressionsWithCurrentInputs(
expressions = valCodes,
funcName = ...
| ev.copy(code = | ||
| s""" | ||
| |$values = new Object[${valExprs.size}]; | ||
| |Object[] $values = new Object[${valExprs.size}]; |
There was a problem hiding this comment.
actually I'm not very sure about this. The previous code can avoid creating this array every time.
There was a problem hiding this comment.
oh it didn't, but it can, you know my point...
There was a problem hiding this comment.
why? It is creating it every time (new Object[${valExprs.size}]), the only thing which is reused is the pointer to the array.
There was a problem hiding this comment.
I mean it can reuse the array...
There was a problem hiding this comment.
I see what you mean now. Since currently this is not done, what should we do? Should I create a new PR to reuse the array and remove this from here?
|
Test build #84546 has finished for PR 19896 at commit
|
mgaido91
changed the title
|
LGTM |
|
LGTM |
1 similar comment
|
LGTM |
|
Test build #84552 has finished for PR 19896 at commit
|
This reverts commit b5bd951.
mgaido91
changed the title
|
I added back the changes for CreateNamedStruct. May you review this part again too? Thanks. |
|
LGTM |
|
Test build #84564 has finished for PR 19896 at commit
|
|
Thanks! Merged to master. |
6 participants
What changes were proposed in this pull request?
CreateNamedStruct and InSet are using a global variable which is not needed. This can generate some unneeded entries in the constant pool.
The PR removes the unnecessary mutable states and makes them local variables.
How was this patch tested?
added UT