Redesign function management abstraction by dain · Pull Request #196 · trinodb/trino

dain · 2019-02-08T22:53:11Z

The redesigns the function APIs in preparation for extending runtime function resolution to connectors.

Add FuctionHandle abstraction which is similar to TableHandle.
Add FunctionMetadata abstraction which describes a function represented by a FunctionHandle.

cla-bot · 2019-02-08T22:53:12Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@prestosql.io. If you are contributing on behalf of someone else (e.g., your employer), the individual CLA may not be sufficient and your employer may need the Corporate CLA signed.

…nd LiteralEncoder

Update all users of FunctionRegistry to use new FunctionManager wrapper instead. The new abstraction will simplify the changes necessary to introduce FunctionHandle API.

Remove unused return tye from

cla-bot · 2019-02-09T18:10:20Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@prestosql.io. If you are contributing on behalf of someone else (e.g., your employer), the individual CLA may not be sufficient and your employer may need the Corporate CLA signed.

rongrong

Thanks for all the plumbing. 😛 Skimmed through commits up to Switch operators to FunctionHandle.

rongrong · 2019-02-09T02:42:30Z

presto-main/src/test/java/io/prestosql/metadata/TestFunctionRegistry.java

It makes more sense to me to have

assertEquals(exactOperator.getSignature(), function.getSignature());

at this moment of the refactoring.

rongrong · 2019-02-12T00:37:49Z

presto-main/src/main/java/io/prestosql/sql/analyzer/StatementAnalyzer.java

                List<TypeSignature> argumentTypes = Lists.transform(windowFunction.getArguments(), expression -> analysis.getType(expression).getTypeSignature());

-                FunctionKind kind = metadata.getFunctionManager().resolveFunction(windowFunction.getName(), fromTypeSignatures(argumentTypes)).getKind();
+                FunctionHandle functionHandle = metadata.getFunctionManager().resolveFunction(session, windowFunction.getName(), fromTypeSignatures(argumentTypes));


I thought the guideline is "if a variable is only used once it should be inlined"? I agree in some cases break it up is easier to read, but "easy to read" is very subjective. And I find it more often than not that the reviewer disagrees with me on that. 😛

rongrong · 2019-02-12T00:50:44Z

presto-main/src/main/java/io/prestosql/metadata/FunctionManager.java

    public ScalarFunctionImplementation getScalarFunctionImplementation(Signature signature)
    {
-        return functionRegistry.getScalarFunctionImplementation(signature);
+        return functionRegistry.getScalarFunctionImplementation(new FunctionHandle(signature));


FunctionRegistry. getScalarFunctionImplementation(Signature) is not removed so this change is not necessary.

rongrong · 2019-02-12T00:51:26Z

presto-main/src/main/java/io/prestosql/metadata/FunctionManager.java

-    }
-
-    public Signature getCoercion(Type fromType, Type toType)
+    public FunctionHandle lookupCast(TypeSignature fromType, TypeSignature toType)


What's the rational behind the name change?

rongrong · 2019-02-12T01:02:35Z

presto-main/src/main/java/io/prestosql/operator/scalar/MapSubscriptOperator.java

 public class MapSubscriptOperator
        extends SqlOperator
 {
-    private static final MethodHandle METHOD_HANDLE_BOOLEAN = methodHandle(MapSubscriptOperator.class, "subscript", boolean.class, InterpretedFunctionInvoker.class, Type.class, Type.class, ConnectorSession.class, Block.class, boolean.class);


Change in this class should probably be a separate commit in itself.

rongrong · 2019-02-12T01:07:23Z

presto-main/src/main/java/io/prestosql/sql/InterpretedFunctionInvoker.java

+        return invoke(implementation, session, arguments);
+    }
+
+    private Object invoke(ScalarFunctionImplementation function, ConnectorSession session, List<Object> arguments)


Seems that implementation is the right name.

rongrong · 2019-02-13T02:16:38Z

presto-main/src/main/java/io/prestosql/sql/planner/ExpressionInterpreter.java

-            Signature functionSignature = metadata.getFunctionManager().resolveFunction(node.getName(), fromTypes(argumentTypes));
-            ScalarFunctionImplementation function = metadata.getFunctionManager().getScalarFunctionImplementation(functionSignature);
+            FunctionHandle functionHandle = metadata.getFunctionManager().resolveFunction(session, node.getName(), fromTypes(argumentTypes));
+            ScalarFunctionImplementation function = metadata.getFunctionManager().getScalarFunctionImplementation(functionHandle);


Most of this commit seems to be just rename session to connectorSession other than this two lines. Failed to understand the association between the code change and the commit title here. 🤣

Resolving a function requires a full session, where as everything else in this code only needs a connector session. The renames are because connector session was called session, and I wanted to use that name for the full session.

rongrong · 2019-02-13T19:37:24Z

presto-main/src/main/java/io/prestosql/sql/relational/Expressions.java

        return new ConstantExpression(null, type);
    }

-    public static CallExpression call(Signature signature, Type returnType, RowExpression... arguments)


What's the reasoning behind removing these helpers and prefer using new ... directly? Are other helpers in the class going to be removed as well?

rongrong · 2019-02-13T19:38:53Z

presto-main/src/main/java/io/prestosql/sql/relational/StandardFunctionResolution.java

+        this.functionManager = requireNonNull(functionManager, "functionManager is null");
+    }
+
+    public FunctionHandle notFunction()


This name is super confusing 🤣

rongrong · 2019-02-13T19:39:36Z

presto-main/src/main/java/io/prestosql/sql/relational/StandardFunctionResolution.java

+        return functionManager.resolveFunction(session, QualifiedName.of("not"), fromTypes(BOOLEAN));
+    }
+
+    public FunctionHandle likeVarcharSignature()


Why is this one called signature?

rongrong · 2019-02-13T19:40:17Z

presto-main/src/main/java/io/prestosql/sql/relational/StandardFunctionResolution.java

+        return functionManager.resolveFunction(session, QualifiedName.of("LIKE_PATTERN"), fromTypes(VARCHAR, VARCHAR));
+    }
+
+//    public FunctionHandle tryCastFunction(Type returnType, Type valueType)


Is this supposed to be here or not?

rongrong · 2019-02-13T19:53:04Z

presto-main/src/main/java/io/prestosql/metadata/FunctionManager.java

        return functionRegistry.lookupSaturatedFloorCast(fromType, toType);
    }
+
+    public FunctionHandle lookupInternalCastFunction(String name, TypeSignature fromType, TypeSignature toType)


This doesn't seem to need to be a separate commit? Which commit calls this function?

rongrong · 2019-02-13T20:05:14Z

presto-main/src/main/java/io/prestosql/sql/gen/CastCodeGenerator.java

-
-import static io.prestosql.spi.function.OperatorType.CAST;
-
-public class CastCodeGenerator


I'd probably just call this commit Remove CastCodeGenerator to be more specific.

rongrong · 2019-02-13T21:02:43Z

presto-main/src/main/java/io/prestosql/metadata/FunctionManager.java

        return functionRegistry.list();
    }

+    public FunctionHandle lookupFunction(QualifiedName name, List<TypeSignatureProvider> parameterTypes)


Why is this one not called resolveFunction?

rongrong · 2019-02-13T23:42:08Z

presto-main/src/main/java/io/prestosql/operator/annotations/ImplementationDependency.java

                            toInvocationConvention(operatorDependency.convention()));
                }
-                checkArgument(!operatorDependency.returnType().equals(""), operatorDependency.operator() + " operator dependency must not have a return type");
+                checkArgument(operatorDependency.returnType().equals(""), operatorDependency.operator() + " operator dependency must not have a return type");


Seems like this should be in the operator dependencies diff.

rongrong · 2019-02-14T02:13:24Z

presto-main/src/main/java/io/prestosql/sql/relational/SqlToRowExpressionTranslator.java

        @Override
        protected RowExpression visitGenericLiteral(GenericLiteral node, Void context)
        {
-            Type type;


Why is this no longer needed?

rongrong · 2019-02-20T00:23:41Z

presto-main/src/test/java/io/prestosql/metadata/TestFunctionRegistry.java

-            Signature expectedSignature = functionSignature.name(TEST_FUNCTION_NAME).build();
-            Signature actualSignature = resolveSignature();
-            assertEquals(actualSignature, expectedSignature);
+            FunctionHandle expectedSignature = new FunctionHandle(functionSignature.name(TEST_FUNCTION_NAME).build());


expectedFunction

rongrong · 2019-02-21T06:02:31Z

...o-main/src/test/java/io/prestosql/operator/aggregation/minmaxby/TestMinMaxByAggregation.java

-                new Signature("min_by", AGGREGATE, parseTypeSignature(StandardTypes.DOUBLE), parseTypeSignature(StandardTypes.DOUBLE), parseTypeSignature(UnknownType.NAME)));
+        InternalAggregationFunction unknownValue = getInternalAggregationFunction("min_by", DOUBLE, UNKNOWN);
        assertAggregation(
                unknownKey,


I'm guessing this is meant to be unknownValue, otherwise unknownValue is an unused variable.

rongrong · 2019-03-03T07:33:06Z

presto-main/src/main/java/io/prestosql/sql/relational/RowExpressionVisitor.java

 {
    R visitCall(CallExpression call, C context);

+    R visitSpecialForm(SpecialForm specialForm, C context);


Seems like it would be more consistent to call this new class SpecialFormExpression.

dain assigned martint Feb 8, 2019

dain requested a review from martint February 8, 2019 22:53

rongrong and others added 27 commits February 9, 2019 10:10

Move static methods from FunctionRegistry to OperatorSignatureUtils a…

f40fa3f

…nd LiteralEncoder

Add FunctionManager wrapper around FunctionRegistry

ffa8a16

Update all users of FunctionRegistry to use new FunctionManager wrapper instead. The new abstraction will simplify the changes necessary to introduce FunctionHandle API.

Simplify SqlToRowExpressionTranslator visitGenericLiteral

c4d6b0b

Add FunctionHandle

cf69cfb

Resolve functions to a handle during analysis

7915b62

Switch window function to use FunctionHandle

f6f8be3

Switch aggergation function to use FunctionHandle

1066e1f

Switch cast to FunctionHandle

b5958bd

Add FunctionManager lookupSaturatedFloorCast

5128b07

Remove FunctionManager canResolveOperator

de7eb08

Switch operators to FunctionHandle

ee03112

Convert FunctionManager resolveOperator to TypeSignatureProvider

e635940

Add system to deserialize a function handle from a SQL expression

dd45579

Split special form of row expressions to separate node

f6f70dc

Remove FunctionManager isRegistered

f652ef4

Add FunctionManager lookupInternalCastFunction

37f09ea

Convert CallExpression to use FunctionHandle

0a12602

Add name hint to CallExpression

4c921ae

Removed unused SqlToRowExpressionTranslator functionKind field

246d126

Simplify CallExpression bytecode generation

f733c4c

Remove unused Signature argument from SpecialFormBytecodeGenerator

2c1dbaa

Convert function and operator dependencies to FunctionHandle

7efdb47

Remove unused return tye from

Convert LiteralInterpreter to FunctionHandle

727ea68

Remove getScalarFunctionImplementation by Signature

d5d4dda

Add FunctionManager.getFunctionMetadata

5164aca

Reduce use of resolveFunction after analysis

6693fbe

Reduce use of resolveOperator after analysis

5bb184c

Add some FunctionManager documentation

a14d758

dain force-pushed the function-handle branch from fef9e65 to a14d758 Compare February 9, 2019 18:10

rongrong reviewed Feb 12, 2019

View reviewed changes

rongrong reviewed Feb 13, 2019

View reviewed changes

rongrong reviewed Feb 14, 2019

View reviewed changes

This was referenced Feb 15, 2019

Map PostgreSQL JSON, JSONB to Presto JSON #81

Merged

[WIP] Add explicit support for type constructors #245

Closed

rongrong reviewed Feb 20, 2019

View reviewed changes

rongrong mentioned this pull request Feb 20, 2019

[WIP] Function handle prestodb/presto#12360

Closed

martint mentioned this pull request Feb 20, 2019

Function and type namespaces #8

Closed

6 tasks

rongrong reviewed Feb 21, 2019

View reviewed changes

rongrong reviewed Mar 3, 2019

View reviewed changes

dain closed this Aug 7, 2019

dain deleted the function-handle branch December 7, 2019 21:42


		import static io.prestosql.spi.function.OperatorType.CAST;

		public class CastCodeGenerator

Conversation

dain commented Feb 8, 2019

Uh oh!

cla-bot bot commented Feb 8, 2019

Uh oh!

cla-bot bot commented Feb 9, 2019

Uh oh!

rongrong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants