-
Notifications
You must be signed in to change notification settings - Fork 4.8k
HIVE-26889 - Implement array_join udf to concatenate the elements of an array with a specified delimiter #3896
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…an array with a specified delimiter
…an array with a specified delimiter
…an array with a specified delimiter - 3
…an array with a specified delimiter - 3
| this.minArgCount = minArgCount; | ||
| this.maxArgCount = maxArgCount; | ||
| this.outputCategory = outputCategory; | ||
| this.outputPrimitiveCategory = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line seems unnecessary. Isnt this null by default since it is not initialized?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed that variable
| if (outputCategory == ObjectInspector.Category.LIST) { | ||
| return initListOI(arguments); | ||
| } else if (outputCategory == ObjectInspector.Category.PRIMITIVE && outputPrimitiveCategory == PrimitiveObjectInspector.PrimitiveCategory.STRING) { | ||
| return PrimitiveObjectInspectorFactory.writableStringObjectInspector; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel its better to push this down to GenericUDFArrayJoin by overriding initialize in GenericUDFArrayJoin. This condition seems specific to Array join and I dont see this condition used by other potential UDFs.
This will avoid the addition of another constructor as well to this generic class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pushed down to GenericUDFArrayJoin
| this.outputPrimitiveCategory = null; | ||
| } | ||
|
|
||
| public AbstractGenericUDFArrayBase(String functionName, int minArgCount, int maxArgCount, ObjectInspector.Category outputCategory, PrimitiveObjectInspector.PrimitiveCategory outputPrimitiveCategory) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably this constructor and variable might not be required in this class.
Please check this comment - https://github.com/apache/hive/pull/3896/files#r1066553884.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
made changes as recommended
…an array with a specified delimiter - 4
…an array with a specified delimiter - 5
SourabhBadhya
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM +1
saihemanth-cloudera
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM +1. Pending green Jenkins tests
|
Kudos, SonarCloud Quality Gate passed! |
…an array with a specified delimiter (apache#3896)(Taraka Rama Rao Lethavadla, reviewed by Sourabh Badhya, Sai Hemanth)
…an array with a specified delimiter (apache#3896)(Taraka Rama Rao Lethavadla, reviewed by Sourabh Badhya, Sai Hemanth)








What changes were proposed in this pull request?
Implement array_join function in Hive
Why are the changes needed?
This enhancement is already implemented in Spark
Does this PR introduce any user-facing change?
How was this patch tested?
Created Junit tests as well as qtests as part of this change