Using ExpressionTypeManager to detect the types for function look up. #1368

hjafarpour · 2018-06-01T19:01:29Z

Description

This PR will use ExpressionTypeManager to detect the argument types for functions. Argument types are used to look up for the correct function among variants of a function with different arguments.
This change will help in fixing the rebase issues in struct PRs.

Testing done

Existing tests cover this.

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 #")

rodesai · 2018-06-04T18:03:49Z

ksql-engine/src/main/java/io/confluent/ksql/codegen/CodeGenRunner.java

@@ -132,26 +128,24 @@ protected Object visitFunctionCall(FunctionCall node, Object context) {
      final int functionNumber = functionCounter++;
      functionArguments.beginFunction();


We should make functionArguments local to this method call now.

rodesai · 2018-06-04T18:19:05Z

ksql-engine/src/main/java/io/confluent/ksql/util/ExpressionTypeManager.java

@@ -198,10 +196,11 @@ protected Expression visitFunctionCall(final FunctionCall node,
      functionArguments.beginFunction();


If we're not using functionArguments to track the call stack then can we just clean it up? Otherwise all its doing is being a list.

Removed it from this part.

For my understanding, the stack is being replaced by multiple instances of ExpressionTypeManager now?

Are you going to remove the FunctionArguments? Seems it is not needed with this change, i.e., as @rodesai suggested

big-andy-coates

Hey @hjafarpour

This PR is a much nicer size! Thanks.

Few nits / comments below.

big-andy-coates · 2018-06-04T18:26:33Z

ksql-engine/src/main/java/io/confluent/ksql/codegen/CodeGenRunner.java

@@ -132,26 +128,24 @@ protected Object visitFunctionCall(FunctionCall node, Object context) {
      final int functionNumber = functionCounter++;
      functionArguments.beginFunction();
      final String functionName = node.getName().getSuffix();
+      ExpressionTypeManager expressionTypeManager =


nit: final.

big-andy-coates · 2018-06-04T18:27:19Z

ksql-engine/src/main/java/io/confluent/ksql/codegen/SqlToJavaVisitor.java

      return new Pair<>("Integer.parseInt(\"" + node.getValue() + "\")", Schema.INT32_SCHEMA);
    }

    @Override
    protected Pair<String, Schema> visitFunctionCall(FunctionCall node, Boolean unmangleNames) {
      String functionName = node.getName().getSuffix();
      UdfFactory udfFactory = functionRegistry.getUdfFactory(functionName);
+      FunctionArguments functionArguments = new FunctionArguments();


nit: final.

big-andy-coates · 2018-06-04T18:27:48Z

ksql-engine/src/main/java/io/confluent/ksql/codegen/SqlToJavaVisitor.java

      functionArguments.beginFunction();

      String instanceName = functionName + "_" + functionCounter++;

      final String arguments = node.getArguments().stream()
          .map(arg -> process(arg, unmangleNames).getLeft())
          .collect(Collectors.joining(", "));
+      node.getArguments().stream().forEach(arg -> functionArguments.addArgumentType(


nit|: stream().forEach() can be replaced with forEach

big-andy-coates · 2018-06-04T18:45:58Z

ksql-engine/src/main/java/io/confluent/ksql/codegen/CodeGenRunner.java

@@ -188,7 +182,9 @@ protected Object visitDereferenceExpression(DereferenceExpression node, Object c
        throw new RuntimeException(
            "Cannot find the select field in the available fields: " + node.toString());
      }
-      updateFunctionArgTypesAndParams(schemaField.get());
+      parameters.add(new ParameterType(


There are multiple copies of this same code - maybe it should be a method?

e.g.

private void addParameter(final Field field) { parameters.add(new ParameterType( SchemaUtil.getJavaType(field.schema()), field.name().replace(".", "_"))); }

Good point. Moved them to a method.

big-andy-coates · 2018-06-04T18:51:16Z

ksql-engine/src/main/java/io/confluent/ksql/codegen/SqlToJavaVisitor.java

      return new Pair<>("Integer.parseInt(\"" + node.getValue() + "\")", Schema.INT32_SCHEMA);
    }

    @Override
    protected Pair<String, Schema> visitFunctionCall(FunctionCall node, Boolean unmangleNames) {
      String functionName = node.getName().getSuffix();
      UdfFactory udfFactory = functionRegistry.getUdfFactory(functionName);
+      FunctionArguments functionArguments = new FunctionArguments();


There is a use of functionArguments at the bottom of this method that has no corresponding being/endFunction - so I think it can be removed.

Plus I think it could just be replace with a local List<Schema.Type>

Changed it into a list.

big-andy-coates · 2018-06-04T18:53:25Z

ksql-engine/src/main/java/io/confluent/ksql/codegen/CodeGenRunner.java

      for (Expression argExpr : node.getArguments()) {
        process(argExpr, null);
+        functionArguments.addArgumentType(expressionTypeManager.getExpressionType(argExpr).type());


consider making functionArguments just be local List<Schema.Type>...

….com/hjafarpour/ksql into Struct-Type-P2-Use-Connect-Data-Type Also temporarily removed the ExtractJsonField test from the query translation tests. Will bring it back when sycnhed with the master and merged confluentinc#1368. Applied Andy's feedback.

hjafarpour · 2018-06-05T01:02:17Z

@big-andy-coates, @rodesai applied your feedback.
@dguy this PR makes changes in UDF PR. Wanna take a look?

apurvam

Thanks for the manageably sized patch @hjafarpour . Apart from making it easier to rebase future struct PRs, does this also fix a bug with the current code when it comes to dealing with function arguments? I think there was an allusion to a bug fix in a separate context.

apurvam · 2018-06-05T05:39:59Z

ksql-engine/src/main/java/io/confluent/ksql/codegen/CodeGenRunner.java

+    private void addParameter(Optional<Field> schemaField) {
+      parameters.add(new ParameterType(
+          SchemaUtil.getJavaType(schemaField.get().schema()),
+          schemaField.get().name().replace(".", "_")));


Consider encapsulating this . -> _ translation into a semantically named --and thus self explanaing-- method inside SchemaField. This would help code readability. As it stands, why do you need to do this replace operation?

schemaField is an instance of Field which is a Connect class so I won't be able to change it from KSQL side.

apurvam · 2018-06-05T05:44:35Z

ksql-engine/src/main/java/io/confluent/ksql/codegen/SqlToJavaVisitor.java

-      final List<Schema.Type> types = functionArguments.endFunction();
-      final Schema returnType = udfFactory.getFunction(types).getReturnType();
+      node.getArguments().forEach(arg -> argumentTypes.add(
+          expressionTypeManager.getExpressionType(arg).type()


Looks like every use of getExpressionType is followed by a type() call. May be worth thinking about adding another appropriately named method to ExpresssionTypeManager which makes this type call directly and returns the value.

I'll add a method for this.

apurvam · 2018-06-05T05:46:12Z

ksql-engine/src/main/java/io/confluent/ksql/util/ExpressionTypeManager.java

@@ -198,10 +196,11 @@ protected Expression visitFunctionCall(final FunctionCall node,
      functionArguments.beginFunction();


For my understanding, the stack is being replaced by multiple instances of ExpressionTypeManager now?

dguy

Thanks for the PR @hjafarpour, just a couple of nits.

dguy · 2018-06-05T09:22:22Z

ksql-engine/src/main/java/io/confluent/ksql/codegen/CodeGenRunner.java

@@ -123,35 +121,39 @@ public ExpressionMetadata buildCodeGenFromParseTree(
      this.functionRegistry = functionRegistry;
    }

+    private void addParameter(Optional<Field> schemaField) {
+      parameters.add(new ParameterType(
+          SchemaUtil.getJavaType(schemaField.get().schema()),


We should probably move the if (schemaField.isPresent(...) into this method

I'll add ifPresent().

dguy · 2018-06-05T09:24:41Z

ksql-engine/src/main/java/io/confluent/ksql/codegen/SqlToJavaVisitor.java

-      final List<Schema.Type> types = functionArguments.endFunction();
-      final Schema returnType = udfFactory.getFunction(types).getReturnType();
+      node.getArguments().forEach(arg -> argumentTypes.add(
+          expressionTypeManager.getExpressionType(arg).type()


dguy · 2018-06-05T09:26:58Z

ksql-engine/src/main/java/io/confluent/ksql/util/ExpressionTypeManager.java

@@ -198,10 +196,11 @@ protected Expression visitFunctionCall(final FunctionCall node,
      functionArguments.beginFunction();


Are you going to remove the FunctionArguments? Seems it is not needed with this change, i.e., as @rodesai suggested

dguy · 2018-06-05T09:36:41Z

@hjafarpour can you add this test: https://github.com/confluentinc/ksql/pull/1365/files#diff-d2219c48b2a9226d6594344414507ae2R447

Then i can close that PR. Thanks

hjafarpour · 2018-06-05T17:00:48Z

@dguy removed FunctionArgument class and also added the tests you mentioned.
@apurvam yes the stack was replaced with calls to the ExpressionTypeManager. I consolidated the use of ExpressionTypeManager and now there is only one instance that is being used through out the class.

apurvam · 2018-06-05T21:43:31Z

It's curious that there are 6 failed security integration tests twice in a row. I kicked off the PR builder again right now, but we probably will have to understand what's going on.

apurvam

Thanks @hjafarpour . The latest version looks good to me. But we need to get to the bottom of the build failures before we can merge. They definitely seem to be triggered by this patch.

apurvam · 2018-06-06T01:08:30Z

Hm. It looks like the master build is also broken. Those 6 tests are failing there as well. So it isn't anything to do with your PR.

dguy

Thanks @hjafarpour, LGTM

apurvam

LGTM. Before merging, we need to:

Update the branch to be in sync with master.
Get a green Jenkins build.

Using ExpressionTypeManager to detect the types for function look up.

d0bf278

hjafarpour added the enhancement label Jun 1, 2018

hjafarpour self-assigned this Jun 1, 2018

hjafarpour requested review from dguy and a team June 1, 2018 19:01

hjafarpour added 2 commits June 1, 2018 12:42

Merge branch 'master' into Use-Expression-Type-Manager-for-UDF-arguments

f36c160

Merge branch 'master' into Use-Expression-Type-Manager-for-UDF-arguments

2e0632b

rodesai reviewed Jun 4, 2018

View reviewed changes

hjafarpour mentioned this pull request Jun 4, 2018

Struct Data Type(Part-2): Use connect data model for internal data #1323

Closed

big-andy-coates reviewed Jun 4, 2018

View reviewed changes

hjafarpour and others added 2 commits June 4, 2018 17:14

Merge branch 'master' into Use-Expression-Type-Manager-for-UDF-arguments

a90bc1a

Applied review feedback.

e968f5a

apurvam reviewed Jun 5, 2018

View reviewed changes

dguy reviewed Jun 5, 2018

View reviewed changes

dguy mentioned this pull request Jun 5, 2018

HOTFIX: handle values from maps in function calls #1365

Closed

2 tasks

hjafarpour added 2 commits June 5, 2018 09:49

Applied review feedback. Removed FunctionArgument class.

215fe1f

Minor change.

5c8db50

apurvam reviewed Jun 5, 2018

View reviewed changes

dguy approved these changes Jun 6, 2018

View reviewed changes

apurvam approved these changes Jun 6, 2018

View reviewed changes

rodesai approved these changes Jun 6, 2018

View reviewed changes

Merge branch 'master' into Use-Expression-Type-Manager-for-UDF-arguments

c016827

hjafarpour changed the base branch from master to 5.0.x June 6, 2018 18:08

hjafarpour merged commit b71c5fa into confluentinc:5.0.x Jun 6, 2018

		@@ -132,26 +128,24 @@ protected Object visitFunctionCall(FunctionCall node, Object context) {
		final int functionNumber = functionCounter++;
		functionArguments.beginFunction();

		@@ -198,10 +196,11 @@ protected Expression visitFunctionCall(final FunctionCall node,
		functionArguments.beginFunction();

Using ExpressionTypeManager to detect the types for function look up. #1368

Using ExpressionTypeManager to detect the types for function look up. #1368

Conversation

hjafarpour commented Jun 1, 2018

Description

Testing done

Reviewer checklist

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

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 4, 2018 • edited Loading

Choose a reason for hiding this comment

hjafarpour commented Jun 5, 2018

apurvam 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

dguy 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

dguy commented Jun 5, 2018

hjafarpour commented Jun 5, 2018

apurvam commented Jun 5, 2018

apurvam left a comment

Choose a reason for hiding this comment

apurvam commented Jun 6, 2018

dguy left a comment

Choose a reason for hiding this comment

apurvam left a comment

Choose a reason for hiding this comment

big-andy-coates Jun 4, 2018 •

edited

Loading