-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
HIVE-28347: Make a UDAF 'collect_set' work with complex types, even when map-side aggregation is disabled. #5323
base: master
Are you sure you want to change the base?
Conversation
//no map aggregation. | ||
inputOI = parameters[0]; | ||
return ObjectInspectorFactory.getStandardListObjectInspector( | ||
ObjectInspectorUtils.getStandardObjectInspector(inputOI)); |
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.
Can we merge this case with PARTIAL1?
if (m == Mode.PARTIAL1 || m == Mode.COMPLETE) {
...
@JeongDaeKim Taking a glance, your finding and fix seems to be very reasonable. Could you please regenerate the result file of |
@okumin Thank you for a quick review! 👍 |
Quality Gate passedIssues Measures |
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 have done three things.
First, I checked that the evaluator is likely correctly implemented now.
Second, I checked that we have new regrettion tests. Nice work.
Third, I ran the original udaf_collect_set_2.q
with set hive.map.aggr = true or false
. Both generated the same result set.
Then, the changes here look to me. Note that you also need +1 from a committer.
What changes were proposed in this pull request?
UDAFs, collect_set and collect_list, won't work with complex types, if map-side aggregation is disabled. This PR will fix this bug.
Why are the changes needed?
When users disable the map-side aggregation feature(
"hive.map.aggr
), collect_set doesn't work with complex types, such as struct.Does this PR introduce any user-facing change?
No.
Is the change a dependency upgrade?
No.
How was this patch tested?
Tested with qtest (TestMiniTezCliDriver)