UDF tidy ups #1429

dguy · 2018-06-13T19:11:20Z

Description

Follow up from #1366

apurvam

Thanks @dguy, these are some nice improvements. My main comment is around the handling of unmatched types.

apurvam · 2018-06-14T04:36:24Z

ksql-common/src/main/java/io/confluent/ksql/function/udf/Udf.java

@@ -21,6 +21,10 @@
 import java.lang.annotation.RetentionPolicy;
 import java.lang.annotation.Target;

+/**
+ * Used to signal that a method in a class that has the @UdfDescription annotation
+ * as a function that can be invoked.


do you mean "is a function that can be invoked"?

Maybe: "The {@code Udf} annotation on a method tells KSQL that this method should be exposed as a user-defined function in KSQL. The enclosing class must also be annotated with {@code UdfDescription}."

apurvam · 2018-06-14T04:37:54Z

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

@@ -51,6 +53,22 @@
      .put(List.class, index -> typeConversionCode("List", index))
      .build();

+  // Templates used to generate the UDF code
+  private static final String genericTemplate =


Much nicer!

apurvam · 2018-06-14T04:40:12Z

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

+          + "if(args[#INDEX] == null) arg#INDEX = null;\n"
+          + "else if (args[#INDEX] instanceof #TYPE) arg#INDEX = (#TYPE)args[#INDEX];\n"
+          + "else if (args[#INDEX] instanceof String) arg#INDEX = "
+          + "#TYPE.valueOf((String)args[#INDEX]);\n";


Is this line basically coercing an unmatched type into a string? Wouldn't it be better to just throw an exception?

Yay - grokable code!

I'm with @apurvam though - implicit type coercion scares me. Why do we need it? Why not just throw?

If we need this, then at the very least let's wrap this in a try/catch so we can throw a more meaningful error. Also, what if #Type.valueOf(String) does not exist?

No - there are no unmatched types as the types need to be in typeConverters map.
For types that are not Map or List (as they are handled differently), it is converting it from a String to the type by using the valueOf() method, i.e., Boolean.valueOf(String), Long.valueOf(String)

i added a test to UdfCompilerTest to show that an exception will be raised if the type isn't one that we accept

apurvam · 2018-06-14T04:42:49Z

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

-      this.blackList = builder.toString().equals(EMPTY_BLACKLIST)
-          ? ""
-          : builder.toString();
+      this.blackList = Files.readLines(inputFile, Charset.forName(StandardCharsets.UTF_8.name()))


It would be nice to have a comment describing how this block is parsing the blacklist file. Specifically, how are strings being transformed with replace and join? The regexes are not straightforward to interpret.

hjafarpour

LGTM.

big-andy-coates

Hey @dguy - looking much better mate. Thanks for making the changes.

Some more comments, suggests and a few concerns below...

big-andy-coates · 2018-06-14T09:31:44Z

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

+          .filter(line -> !line.isEmpty())
+          .filter(line -> !line.startsWith("#"))
+          .map(line -> line.replaceAll("\\.", "\\\\."))
+          .collect(Collectors.joining("|", "^(?:",")\\.?.*$"));


nit: formatting - needs space after comma, to make it easier to grok.

big-andy-coates · 2018-06-14T09:34:28Z

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


-  private String blackList;
+  private String blackList = ".*";


nit: moving ".*" into constant BLACKLIST_ALL would help impart meaning more.

big-andy-coates · 2018-06-14T09:37:06Z

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


 /**
 * Used to restrict the classes that can be loaded by user supplied UDFs
 */
 public class Blacklist implements Predicate<String> {
  private static final Logger logger = LoggerFactory.getLogger(Blacklist.class);
-  private static final String EMPTY_BLACKLIST = "^(?)\\.?.*$";
+  private static final String EMPTY_BLACKLIST = "^(?:)\\.?.*$";


Split into two constants:

private static final String BLACKLIST_PREFIX = "^(?:"; private static final String BLACKLIST_SUFFIX = ")\\.?.*$";

Then your collect line can be:

.collect(Collectors.joining("|", BLACKLIST_PREFIX, BLACKLIST_SUFFIX));

And your empty check can be:

if (this.blackList.equals(BLACKLIST_PREFIX + BLACKLIST_SUFFIX))

Which I think is easier to understand.

big-andy-coates · 2018-06-14T09:38:22Z

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

@@ -60,6 +59,6 @@

  @Override
  public boolean test(final String resourceName) {
-    return blackList == null || resourceName.matches(blackList);
+    return resourceName.matches(blackList);


out of interest, why are we using a regex for such simple matching? Can't we just dump each line into a Set and use contains(resourceName)???

It is intended to do partial matches, i.e., the regex produced would be something like:
^(?:java\.lang\.Process|java\.lang\.Runtime|javax)\.?.*$
which will match anything starting with java.lang.Process, java.lang.Runtime, javax

Ah, gotcha.

big-andy-coates · 2018-06-14T09:41:32Z

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

+          + "if(args[#INDEX] == null) arg#INDEX = null;\n"
+          + "else if (args[#INDEX] instanceof #TYPE) arg#INDEX = (#TYPE)args[#INDEX];\n"
+          + "else if (args[#INDEX] instanceof String) arg#INDEX = "
+          + "#TYPE.valueOf((String)args[#INDEX]);\n";


Yay - grokable code!

I'm with @apurvam though - implicit type coercion scares me. Why do we need it? Why not just throw?

If we need this, then at the very least let's wrap this in a try/catch so we can throw a more meaningful error. Also, what if #Type.valueOf(String) does not exist?

big-andy-coates · 2018-06-14T09:51:51Z

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

  private static String typeConversionCode(final String type, final int index) {
    if (type.equals("Map") || type.equals("List")) {
      return type + " arg" + index + " = (" + type + ")args[" + index + "];\n";
    }
-    final String argArrayVal = "args[" + index + "]";
-    final String argVarAssignment = "arg" + index + " = ";
+


numericValue can just be in the code template, and then you can simplify this code:

private static final String INTEGER_NUMBER_TEMPLATE = "else if (args[#INDEX] instanceof Number) arg#INDEX = " + "((Number)args[#INDEX]).intValue();\n"; private static final String NUMBER_TEMPLATE = "else if (args[#INDEX] instanceof Number) arg#INDEX = " + "((Number)args[#INDEX]).#LC_TYPEValue();\n"; private static String typeConversionCode(final String type, final int index) { if (type.equals("Map") || type.equals("List")) { return type + " arg" + index + " = (" + type + ")args[" + index + "];\n"; } final StringBuilder builder = new StringBuilder(); builder.append(genericTemplate); if (type.equals("Integer")) { builder.append(INTEGER_NUMBER_TEMPLATE); } else if (!type.equals("String") && !type.equals("Boolean")) { builder.append(GENERIC_NUMBER_TEMPLATE); } builder.append(THROWS_TEMPLATE); return builder.toString() .replaceAll("#TYPE", type) .replaceAll("#LC_TYPE", type.toLowerCase()) .replaceAll("#INDEX", String.valueOf(index)); }

However, I'm still wondering if there is scope for this function to generate invalid code. Even if we know today that its only going to be called with a known set of type parameter values, I think it would still be sensible to validate type against a supported list and throw if not supported. This is better then generating code that won't compile.

See line 113

i don't really see your code as simplification!

Well, it switching the code to only have one set of replaceAlls, i.e. it generates the whole set of code before doing the replacements, and moves the code generation in:

final String numericValue = type.equals("Integer") ? "intValue()" : type.toLowerCase() .append(argVarAssignment) + + "Value()";

Into the easy to grok constants.

big-andy-coates · 2018-06-14T11:06:01Z

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

        }));
  }

+  private Object instantiateUdfClass(final Method method,


nit: Static

big-andy-coates

LGTM mate

apurvam

LGTM, thanks!

dguy added 3 commits June 13, 2018 20:09

UDF tidy ups

80772f5

Merge branch '5.0.x' into udf-load-follow-up

4a12643

remove blank line

d270c6b

dguy requested review from big-andy-coates, apurvam and hjafarpour June 13, 2018 23:08

apurvam reviewed Jun 14, 2018

View reviewed changes

hjafarpour approved these changes Jun 14, 2018

View reviewed changes

big-andy-coates reviewed Jun 14, 2018

View reviewed changes

comments

4497c53

big-andy-coates reviewed Jun 14, 2018

View reviewed changes

big-andy-coates approved these changes Jun 14, 2018

View reviewed changes

comments

8fb77a0

dguy merged commit f291a99 into confluentinc:5.0.x Jun 14, 2018

dguy deleted the udf-load-follow-up branch June 14, 2018 12:44

apurvam reviewed Jun 14, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UDF tidy ups #1429

UDF tidy ups #1429

dguy commented Jun 13, 2018

apurvam left a comment •

edited

apurvam Jun 14, 2018

big-andy-coates Jun 14, 2018 •

edited

apurvam Jun 14, 2018

apurvam Jun 14, 2018

big-andy-coates Jun 14, 2018

dguy Jun 14, 2018

dguy Jun 14, 2018

apurvam Jun 14, 2018

hjafarpour left a comment

big-andy-coates left a comment •

edited

big-andy-coates Jun 14, 2018

big-andy-coates Jun 14, 2018

big-andy-coates Jun 14, 2018

big-andy-coates Jun 14, 2018

dguy Jun 14, 2018

big-andy-coates Jun 14, 2018

big-andy-coates Jun 14, 2018

big-andy-coates Jun 14, 2018

big-andy-coates Jun 14, 2018

dguy Jun 14, 2018

dguy Jun 14, 2018

big-andy-coates Jun 14, 2018 •

edited

big-andy-coates Jun 14, 2018

big-andy-coates left a comment

apurvam left a comment

UDF tidy ups #1429

UDF tidy ups #1429

Conversation

dguy commented Jun 13, 2018

Description

apurvam left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

big-andy-coates Jun 14, 2018 • edited

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

hjafarpour left a comment

Choose a reason for hiding this comment

big-andy-coates left a comment • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

big-andy-coates Jun 14, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

big-andy-coates left a comment

Choose a reason for hiding this comment

apurvam left a comment

Choose a reason for hiding this comment

apurvam left a comment •

edited

big-andy-coates Jun 14, 2018 •

edited

big-andy-coates left a comment •

edited

big-andy-coates Jun 14, 2018 •

edited