Skip to content
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

feat: new UDFs for working with Maps #5536

Merged

Conversation

blueedgenick
Copy link
Contributor

Three new UDFs:

  • map_keys(some_map) - get array of all keys from a map
  • map_values(some_map) - get array of all values from a map
  • map_union(map1, map2) - add all entries from 2 maps into a new map

This is an updated version of functions and their tests originally proposed waaaay back in PR #1611 and blocked at that time by limited scope for handling generics in the UDF invocation framework.

Ideally I'd like to also make map_union variadic but the invocation framework will choke on that until #5511 is resolved.

@blueedgenick blueedgenick requested review from JimGalasyn and a team as code owners June 3, 2020 08:18

@UdfDescription(
name = "map_union",
description = "Returns a new map containing the union of all entries from both input maps. "
Copy link
Member

@vpapavas vpapavas Jun 3, 2020

Choose a reason for hiding this comment

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

What if the maps have the same key with different values? Currently, the latter will be in the result. Maybe point this out in the description

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah - this is described in the docs file but not here, will add something to call it out here also

public class MapUnion {

@Udf
public <T> Map<String, T> union(final Map<String, T> input1, final Map<String, T> input2) {
Copy link
Member

Choose a reason for hiding this comment

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

Add @UdfParameter just so that it can compile without the -parameters in the pom file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense

Copy link
Member

@vpapavas vpapavas left a comment

Choose a reason for hiding this comment

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

Thank you @blueedgenick ! LGTM! Maybe just add the @UdfParameter to the udfs so that they can compile without problems if the -parameter is missing from the pom file

@blueedgenick
Copy link
Contributor Author

Thanks @vpapavas ! ok to merge with these changes ?

Copy link
Member

@JimGalasyn JimGalasyn left a comment

Choose a reason for hiding this comment

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

LGTM, with a few suggestions.

blueedgenick and others added 3 commits June 3, 2020 15:01
Jim's doc feedback

Co-authored-by: Jim Galasyn <jim.galasyn@confluent.io>
Jim's doc feedback

Co-authored-by: Jim Galasyn <jim.galasyn@confluent.io>
Jim's doc feedback

Co-authored-by: Jim Galasyn <jim.galasyn@confluent.io>
@vpapavas
Copy link
Member

vpapavas commented Jun 3, 2020

Can you add the @UdfParameter also in the other two udfs please? Other than that, you are good to go :)

@blueedgenick blueedgenick merged commit bc9ad2e into confluentinc:master Jun 3, 2020
@blueedgenick blueedgenick deleted the map_keys_values_union branch June 11, 2020 22:48
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