[SPARK-24878][SQL] Fix reverse function for array type of primitive type containing null. #21830
Conversation
|
Thanks @ueshin for this PR! Good to know about the re-assignments. |
|
Test build #93353 has finished for PR 21830 at commit
|
| ) | ||
| } | ||
|
|
||
| // Test with local relation, the Project will be evaluated without codegen |
There was a problem hiding this comment.
There are many tests here, so I think it'd be nice to split the tests into two or three parts.
| | ${ev.value}.$setFunc(k, ${getCall("l")}); | ||
| | ${ev.value}.setNullAt(l); | ||
| |}""".stripMargin | ||
| val arrayDataClass = classOf[GenericArrayData].getName() |
|
|
||
| val initialization = if (isPrimitiveType) { | ||
| s"$childName.copy()" | ||
| ctx.createUnsafeArray(arrayData, numElements, elementType, s" $prettyName failed.") |
There was a problem hiding this comment.
The code in the master doesn't create UnsafeArrayData per input row though, it seems this change does so. Can we avoid it?
There was a problem hiding this comment.
IIUC, the code in the master also create UnsafeArrayData per input row in UnsafeArrayData.copy().
There was a problem hiding this comment.
You can check this? https://gist.github.com/maropu/e9e8afd64ce30cdf824bb4e18d0c9b4f
|
Test build #93425 has finished for PR 21830 at commit
|
| |final int $numElements = $childName.numElements(); | ||
| |$initialization | ||
| |for (int $i = 0; $i < $numElements; $i++) { | ||
| | int $j = $numElements - $i - 1; |
There was a problem hiding this comment.
we don't need j if we do
for (int i = numElements - 1; i >=0; i--)
There was a problem hiding this comment.
We still need to calculate the index of the opposite side?
| "update" | ||
| } | ||
|
|
||
| val assignment = if (isPrimitiveType && dataType.asInstanceOf[ArrayType].containsNull) { |
There was a problem hiding this comment.
nit: we can simplify the code if we do override def dataType: ArrayType = child.dataType.asInstanceOf[ArrayType]
There was a problem hiding this comment.
We can't override dataType only for ArrayType because Reverse is also used for StringType.
|
LGTM |
|
thanks, merging to master! |
6 participants
What changes were proposed in this pull request?
If we use
reversefunction for array type of primitive type containingnulland the child array isUnsafeArrayData, the function returns a wrong result becauseUnsafeArrayDatadoesn't define the behavior of re-assignment, especially we can't set a valid value after we setnull.How was this patch tested?
Added some tests.