HIVE-27482 - Minor issues in array_intersect udf - #4469
Conversation
okumin
left a comment
okumin
left a comment
There was a problem hiding this comment.
Looks good! I confirmed all test cases generate the same result as Spark SQL. Thanks for the follow-up! It's really helpful.
We need an approval of a committer because I'm not a committer.
| ObjectInspector defaultOI = super.initialize(arguments); | ||
| checkArgCategory(arguments, ARRAY2_IDX, ObjectInspector.Category.LIST, FUNC_NAME, | ||
| org.apache.hadoop.hive.serde.serdeConstants.LIST_TYPE_NAME); //Array1 is already getting validated in Parent class | ||
| if (!ObjectInspectorUtils.compareTypes(arrayOI.getListElementObjectInspector(), ((ListObjectInspector) arguments[ARRAY2_IDX]).getListElementObjectInspector())) { // check if elements of arrays are comparable |
There was a problem hiding this comment.
NIT: We should add a line break if one line has 120 characters.
|
It'll be good to rerun CI. I guess the failure is unrelated to this change. |
kgyrtkirk
added
tests pending
tests passed
and removed
tests failed
tests pending
labels
|
|
||
| Object array = arguments[ARRAY_IDX].get(); | ||
| Object array2 = arguments[ARRAY2_IDX].get(); | ||
| if (array == null) { |
There was a problem hiding this comment.
I would change it for if (array == null || array2 == null) { } I don't see the value to have separate IF statement
There was a problem hiding this comment.
Changed as recommended
| return null; | ||
| } | ||
|
|
||
| List<?> retArray3 = ((ListObjectInspector) argumentOIs[ARRAY_IDX]).getList(array); |
There was a problem hiding this comment.
If you already touch this code I would suggest to change the retArray3 to resultArray or just simple result I think it would be more expressive
There was a problem hiding this comment.
changed variable names to match their usage
kgyrtkirk
added
tests pending
tests unstable
and removed
tests passed
tests pending
labels
saihemanth-cloudera
left a comment
saihemanth-cloudera
left a comment
There was a problem hiding this comment.
LGTM +1. Pending green Ptests. Will approve this PR once it is green.
|
There is a test failure, but as I see it should be unrelated. |
|
Kudos, SonarCloud Quality Gate passed!
|
…a Rama Rao Lethavadla, reviewed by Okumin, Attila Turoczy, Sai Hemanth Gantasala)
…a Rama Rao Lethavadla, reviewed by Okumin, Attila Turoczy, Sai Hemanth Gantasala)
5 participants
What changes were proposed in this pull request?
[1,3] [1,3]
instead of
[1,2,3][1,3]
2. SELECT array_intersect(array(),array()); should give empty array instead of NULL
3. Error handling when elements of array are not comparable
These fixes were already present on array_except udf whose implementation is identical to array_intersect, Ref: #4090
Why are the changes needed?
Does this PR introduce any user-facing change?
No
Is the change a dependency upgrade?
No
How was this patch tested?
Added qtest files as well as junit test cases