Skip to content

Add aggregation function SET_UNION#14842

Merged
rongrong merged 1 commit intoprestodb:masterfrom
prithvip:set-union
Jul 24, 2020
Merged

Add aggregation function SET_UNION#14842
rongrong merged 1 commit intoprestodb:masterfrom
prithvip:set-union

Conversation

@prithvip
Copy link
Contributor

@prithvip prithvip commented Jul 15, 2020

== RELEASE NOTES ==

General Changes
* Add aggregation function SET_UNION 

@kaikalur
Copy link
Contributor

I wonder if you can just extend SET_AGG to simply override the input method. Everything else should be the same. At the very least, you can use those classes. You don't need the new state and factory classes, I don't think.

@prithvip prithvip changed the title [WIP] Add new function set_union Add new function set_union Jul 21, 2020
Copy link
Contributor

@kaikalur kaikalur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comment about reusing the State and Factory functions from SET_AGG for this as well.

@prithvip
Copy link
Contributor Author

See my comment about reusing the State and Factory functions from SET_AGG for this as well.

Thanks for the review @kaikalur. Actually, in my initial PR, I was re-using the state and factory functions from set_agg but I changed it because I thought that it might be better to separate it out ,since its rare for the other aggregation functions share state, and these classes are rather small anyways. If you feel strongly about it, I can revert to how it was before and directly use SET_AGG state and factory.

@kaikalur
Copy link
Contributor

kaikalur commented Jul 21, 2020

See my comment about reusing the State and Factory functions from SET_AGG for this as well.

Thanks for the review @kaikalur. Actually, in my initial PR, I was re-using the state and factory functions from set_agg but I changed it because I thought that it might be better to separate it out ,since its rare for the other aggregation functions share state, and these classes are rather small anyways. If you feel strongly about it, I can revert to how it was before and directly use SET_AGG state and factory.

It's code hygiene. Something doing the exact same thing should not be copied. In fact, I tried very hard to change MAP_AGG to use the set stuffs for keys but it became too complicated so I didn't.

Also note that they are not sharing state - they are just sharing code :)

@prithvip prithvip force-pushed the set-union branch 2 times, most recently from a019e3a to d8d96e2 Compare July 21, 2020 21:44
@prithvip
Copy link
Contributor Author

No problem @kaikalur, done.

Copy link
Contributor

@kaikalur kaikalur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@prithvip prithvip force-pushed the set-union branch 3 times, most recently from 419d267 to cc4e292 Compare July 23, 2020 18:59
Copy link
Contributor

@rongrong rongrong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Some nits, up to you whether you want to change them.

@rongrong
Copy link
Contributor

Maybe change the commit title and release note to Add aggregation function SET_UNION

@prithvip prithvip changed the title Add new function set_union Add aggregation function SET_UNION Jul 23, 2020
SET_UNION is a more efficient alternative to the common pattern of
ARRAY_DISTINCT(FLATTEN(ARRAY_AGG(x)))
@rongrong rongrong merged commit 4c2b8c2 into prestodb:master Jul 24, 2020
@caithagoras caithagoras mentioned this pull request Jul 28, 2020
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants