feat(udfs): generic support for lists/maps in UDFs #3054

agavra · 2019-07-03T21:27:53Z

Description

NOTE: This PR looks large, but the entire non-test, non-description functionality is under 500 lines and contained to a few files (see description below).

This change adds support for a single generic in UDFs so that container types are more accessible in our UDF framework. For example, the following UDF can now be used to create a map from two lists, the second of which has a generic type (see the QTT test for this UDF for an example):

  @Udf
  public final <T> Map<String, T> asMap(
      @UdfParameter final List<String> keys,
      @UdfParameter final List<T> values) {
    final Map<String, T> map = new HashMap<>();
    for (int i = 0; i < keys.size(); i++) {
      map.putIfAbsent(keys.get(i), values.get(i));
    }
    return map;
  }

To do this, the following changes were made:

when resolving the return type from a method, KsqlFunction will apply some logic to resolve any generics in the return type based on the actual argument types being passed in
UdfIndex will index generic types, and will traverse the trie for any input
Generics are represented as BYTES with schema name GENERIC:<name>. If we ever move to our own type system (i.e. not connect) this can have first class support
GenericsUtil and JavaToConnect have all of the magic, take a look there
some UDFs to showcase this feature were added: SUBLIST, AS_LIST and AS_MAP
Some SchemaUtil methods were moved to JavaToConnect (and corresponding tests were moved as well)

Missing

DESCRIBE functionality

Testing done

unit tets
QTTs with some sample UDFs

Reviewer checklist

Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
Ensure relevant issues are linked (description should include text like "Fixes #")

big-andy-coates · 2019-07-11T12:28:05Z

Generics are represented as BYTES with schema name GENERIC:. If we ever move to our own type system (i.e. not connect) this can have first class support

We should do the move to our own schema type first IMHO.

big-andy-coates

If its not ready for review... don't raise a review ;)

agavra · 2019-07-15T15:42:03Z

We should do the move to our own schema type first IMHO.

While I agree with you in principle I think it depends on how invasive this change is, and looking at the code it's really not that bad. The hack is very limited to a small part of the code and the investment of moving to our own schema type system is very large.

If its not ready for review... don't raise a review ;)

I did this to share code in a way that was easy to reference :)

big-andy-coates

Thanks @agavra main thing is the naming and zero-indexing of arrays, and edge cases on the generics that may or may not be an issue. Otherwise looking good. Wouldn't mind another review myself, but if this is holding you up, don't let me stop you.

big-andy-coates · 2019-07-19T18:21:02Z

ksql-common/src/main/java/io/confluent/ksql/function/KsqlFunction.java

@@ -129,8 +135,25 @@ private KsqlFunction(
  }


-  public Schema getReturnType() {
-    return returnType;
+  public Schema getReturnType(final List<Schema> arguments) {


nit: having a param called arguments and a field with the same name doesn't make the code the easiest to read.

What's the difference between the two? Can we express this in the naming somehow to make it easier to grok?

I have renamed the field to parameters and what is passed in as arguments. At some point I might go through the codebase and make this change everywhere because it's a distinction that is confused often.

I'm not sure of the different between arguments and parameters - both are the same to me. So I'm not sure changing the name makes the code in getReturnType any more readable. But if we're changing the name, lets change the name of the getter, constructor param and toString too!

A parameter is a variable in a method definition. When a method is called, the arguments are the data you pass into the method's parameters.

https://stackoverflow.com/a/156787/2258040

EDIT: I will make the corresponding changes in another PR since it touches a surprising amount of files.

ksql-common/src/main/java/io/confluent/ksql/function/UdfIndex.java

big-andy-coates · 2019-07-19T18:29:00Z

ksql-common/src/main/java/io/confluent/ksql/schema/connect/SqlSchemaFormatter.java

-      return "DECIMAL("
-          + DecimalUtil.precision(schema) + ", "
-          + DecimalUtil.scale(schema) + ")";
+      SchemaUtil.requireValidBytesType(schema);


This feels wrong - why are we extending the general use formatter to support this? The use of this special schema type should be very self contained within the UDF code...

ksql-engine/src/main/java/io/confluent/ksql/function/udf/list/Sublist.java

ksql-engine/src/main/java/io/confluent/ksql/function/udf/list/AsList.java

ksql-functional-tests/src/test/resources/query-validation-tests/asmap.json

ksql-functional-tests/src/test/resources/query-validation-tests/aslist.json

docs/developer-guide/syntax-reference.rst

docs/developer-guide/udf.rst

JimGalasyn

LGTM, with a couple of suggestions.

rodesai

Took the first pass through, still reviewing tests. Left some comments/questions inline

rodesai · 2019-07-23T00:23:33Z

ksql-common/src/main/java/io/confluent/ksql/function/GenericsUtil.java

+        return Sets.union(
+            constituentGenerics(type.keySchema()),
+            constituentGenerics(type.valueSchema()));
+      case STRUCT:


This case is currently not supported, right? maybe just throw an exception? It made me confused going between this and identifyGenerics.

This is implemented because it's called to determine whether or not something has generics, which is used to determine whether or not to call identifyGenerics. Throwing an exception here would break the normal flow (this method is called for every KsqlFunction)

ah yeah that makes perfect sense. thanks!

rodesai · 2019-07-23T00:33:49Z

ksql-common/src/main/java/io/confluent/ksql/function/GenericsUtil.java

+      }
+    }
+
+    return Sets.difference(constituentGenerics(schema), asMap.keySet()).isEmpty();


what is the case when this would be true?

asMap represents the "resolved" generics. if every one of the constituent generics exists in the mapping of resolved generics, then this would be true (e.g. constituents(Map<K,V>) ~ {K:String, V:Integer} would be true because K and V are both represented)

if you meant to ask when this would ever be false, then you're right - I'm not actually sure it'll ever be false

ksql-common/src/main/java/io/confluent/ksql/function/UdfIndex.java

rodesai · 2019-07-23T00:42:23Z

ksql-common/src/main/java/io/confluent/ksql/function/UdfIndex.java

+          && GenericsUtil.instanceOf(schema, argument)) {
+        // check if this specific generic has already been resolved to some other
+        // type - if not, reserve it for future checks to accept
+        final Schema old = reservedGenerics.putIfAbsent(schema, argument);


shouldn't we be tracking the resolved generic schemas in reservedGenerics, rather than the container schemas?

e.g. if I call accepts with MAP<A, B>, MAP<STRING, INT>, reservedGenerics should have A->STRING, B->INT not MAP<A, B> -> MAP<STRING, INT> ?

This way, if I have a signature like FOO(MAP<A, B>, LIST<A>), I make sure that A is always resolved to the same type.

yes that was the intention - it seems that I only have tests covering the case with generic top levels. Good catch!

ksql-common/src/main/java/io/confluent/ksql/function/UdfIndex.java

ksql-common/src/test/java/io/confluent/ksql/function/UdfIndexTest.java

rodesai · 2019-07-23T01:25:47Z

docs/developer-guide/udf.rst

+
+- Any generic in the return value of a method must appear in at least one of the method parameters
+- The generic must not adhere to any interface. For example, ``<T extends Number>`` is not valid).
+


We don't support type coercion for generics, but do for specific types, right? e.g. if I have a function like add(T a, T b) and I try to call it with something that resolves to INT, BIGINT, that won't work. But if I did add(Long a, Long b) it would. Might want to call that out in the docs.

ksql-common/src/main/java/io/confluent/ksql/function/GenericsUtil.java

big-andy-coates

Thanks @agavra couple more minors.

Also be aware I've added: #3114.

big-andy-coates · 2019-07-23T15:32:34Z

ksql-common/src/main/java/io/confluent/ksql/function/KsqlFunction.java

@@ -129,8 +135,25 @@ private KsqlFunction(
  }


-  public Schema getReturnType() {
-    return returnType;
+  public Schema getReturnType(final List<Schema> arguments) {


I'm not sure of the different between arguments and parameters - both are the same to me. So I'm not sure changing the name makes the code in getReturnType any more readable. But if we're changing the name, lets change the name of the getter, constructor param and toString too!

ksql-common/src/main/java/io/confluent/ksql/schema/connect/SqlSchemaFormatter.java

ksql-engine/src/main/java/io/confluent/ksql/function/JavaToConnect.java

ksql-engine/src/main/java/io/confluent/ksql/function/udf/list/Slice.java

ksql-engine/src/main/java/io/confluent/ksql/function/udf/map/AsMap.java

ksql-engine/src/main/java/io/confluent/ksql/function/udf/list/Slice.java

ksql-functional-tests/src/test/resources/query-validation-tests/asarray.json

rodesai

one nit inline, otherwise LGTM

rodesai · 2019-07-24T17:55:58Z

ksql-common/src/main/java/io/confluent/ksql/function/UdfIndex.java

+      }
+
+      final Map<Schema, Schema> genericMapping = GenericsUtil.resolveGenerics(schema, argument);
+      for (final Entry<Schema, Schema> entry : genericMapping.entrySet()) {


nit: consider pulling this pattern out into a common function in GenericsUtil, e.g:

public boolean checkGenericsConsistent(final Collection Entry<Schema, Schema> mappings, Optional<Map<Schema, Schema>> oReserved) { final Map<Schema, Schema> reserved = oReserved.orElseGet(new HashMap<>()); for (final Entry<Schema, Schema> entry : mappings) { final Schema old = reserved.putIfAbsent(entry.getKey(), entry.getValue()); if (old != null && !old.equals(entry.getValue())) { return false; } } return true; }

Then you can use it from instanceOf, resolveGenerics, and here

I wanted to have as few methods with side-effects as possible, but the problem is that we need to both return a boolean and update a map... 🤔 I'll see if I can make that clean

I remember now why I didn't do this originally, for resolveGenerics I wanted to be able to have a detailed exception message that tell me exactly which one was the problem. If I throw an exception after returning I lose that information. I think the amount of boiler plate to get this to work for all three of the methods is more confusing than just doing it in place.

agavra requested a review from blueedgenick July 3, 2019 21:27

agavra requested a review from a team as a code owner July 3, 2019 21:27

big-andy-coates reviewed Jul 12, 2019

View reviewed changes

agavra force-pushed the generics_udf branch from b4cf296 to 0757f8e Compare July 18, 2019 00:44

agavra changed the title ~~feat(udfs): generic support for lists/maps in UDFs (PROTOTYPE)~~ feat(udfs): generic support for lists/maps in UDFs Jul 18, 2019

agavra requested a review from a team July 18, 2019 00:49

big-andy-coates approved these changes Jul 19, 2019

View reviewed changes

agavra requested a review from a team July 19, 2019 20:03

agavra requested a review from JimGalasyn as a code owner July 22, 2019 16:19

agavra force-pushed the generics_udf branch from 13ee5d0 to e9d2e42 Compare July 22, 2019 16:21

JimGalasyn reviewed Jul 22, 2019

View reviewed changes

docs/developer-guide/syntax-reference.rst Outdated Show resolved Hide resolved

JimGalasyn reviewed Jul 22, 2019

View reviewed changes

docs/developer-guide/syntax-reference.rst Outdated Show resolved Hide resolved

JimGalasyn reviewed Jul 22, 2019

View reviewed changes

docs/developer-guide/udf.rst Outdated Show resolved Hide resolved

JimGalasyn approved these changes Jul 22, 2019

View reviewed changes

rodesai reviewed Jul 23, 2019

View reviewed changes

ksql-common/src/main/java/io/confluent/ksql/function/GenericsUtil.java Outdated Show resolved Hide resolved

rodesai reviewed Jul 23, 2019

View reviewed changes

ksql-common/src/main/java/io/confluent/ksql/function/GenericsUtil.java Show resolved Hide resolved

big-andy-coates approved these changes Jul 23, 2019

View reviewed changes

agavra added 5 commits July 23, 2019 15:10

feat(udf): support generics in UDFs

6276359

feat: addressed andys comments, added tests and improved functionality

b203016

docs: minor changes to docs

d8a816f

fix: rohan and andys comments

d66373b

refactor: remove GenericUtil from SqlSchemaFormatter

ce93721

agavra force-pushed the generics_udf branch from 2cb157e to ce93721 Compare July 23, 2019 22:39

rodesai approved these changes Jul 24, 2019

View reviewed changes

agavra merged commit a381c48 into confluentinc:master Jul 24, 2019

agavra deleted the generics_udf branch July 24, 2019 20:00

agavra mentioned this pull request Aug 6, 2019

UDF does not support List as a return type #1791

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(udfs): generic support for lists/maps in UDFs #3054

feat(udfs): generic support for lists/maps in UDFs #3054

agavra commented Jul 3, 2019 •

edited

Loading

big-andy-coates commented Jul 11, 2019

big-andy-coates left a comment

agavra commented Jul 15, 2019 •

edited

Loading

big-andy-coates left a comment

big-andy-coates Jul 19, 2019

agavra Jul 19, 2019

big-andy-coates Jul 23, 2019

agavra Jul 23, 2019 •

edited

Loading

big-andy-coates Jul 19, 2019

JimGalasyn left a comment

rodesai left a comment

rodesai Jul 23, 2019

agavra Jul 23, 2019

rodesai Jul 23, 2019

rodesai Jul 23, 2019

agavra Jul 23, 2019

rodesai Jul 23, 2019

agavra Jul 23, 2019

rodesai Jul 23, 2019

big-andy-coates left a comment •

edited

Loading

big-andy-coates Jul 23, 2019

rodesai left a comment

rodesai Jul 24, 2019

agavra Jul 24, 2019

agavra Jul 24, 2019


		- Any generic in the return value of a method must appear in at least one of the method parameters
		- The generic must not adhere to any interface. For example, ``<T extends Number>`` is not valid).

feat(udfs): generic support for lists/maps in UDFs #3054

feat(udfs): generic support for lists/maps in UDFs #3054

Conversation

agavra commented Jul 3, 2019 • edited Loading

Description

Missing

Testing done

Reviewer checklist

big-andy-coates commented Jul 11, 2019

big-andy-coates left a comment

Choose a reason for hiding this comment

agavra commented Jul 15, 2019 • edited Loading

big-andy-coates left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agavra Jul 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JimGalasyn left a comment

Choose a reason for hiding this comment

rodesai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

big-andy-coates left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodesai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agavra commented Jul 3, 2019 •

edited

Loading

agavra commented Jul 15, 2019 •

edited

Loading

agavra Jul 23, 2019 •

edited

Loading

big-andy-coates left a comment •

edited

Loading