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

Redesign function management abstraction #196

Open
wants to merge 28 commits into
base: master
from

Conversation

3 participants
@dain
Copy link
Member

dain commented Feb 8, 2019

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

This comment has been minimized.

Copy link

cla-bot bot commented Feb 8, 2019

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.

@dain dain requested a review from martint Feb 8, 2019

rongrong and others added some commits Nov 9, 2018

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

@dain dain force-pushed the dain:function-handle branch from fef9e65 to a14d758 Feb 9, 2019

@cla-bot

This comment has been minimized.

Copy link

cla-bot bot commented Feb 9, 2019

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
Copy link
Contributor

rongrong left a comment

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

Signature exactOperator = registry.resolveOperator(operatorType, resolveTypes(function.getSignature().getArgumentTypes(), typeManager));
assertEquals(exactOperator, function.getSignature());
FunctionHandle exactOperator = registry.resolveOperator(operatorType, resolveTypes(function.getSignature().getArgumentTypes(), typeManager));
assertEquals(exactOperator, new FunctionHandle(function.getSignature()));

This comment has been minimized.

@rongrong

rongrong Feb 12, 2019

Contributor

It makes more sense to me to have

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

at this moment of the refactoring.

@@ -1381,7 +1382,8 @@ private void analyzeWindowFunctions(QuerySpecification node, List<Expression> ou

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));

This comment has been minimized.

@rongrong

rongrong Feb 12, 2019

Contributor

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. 😛

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

This comment has been minimized.

@rongrong

rongrong Feb 12, 2019

Contributor

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

}

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

This comment has been minimized.

@rongrong

rongrong Feb 12, 2019

Contributor

What's the rational behind the name change?

import static io.prestosql.util.Reflection.methodHandle;
import static java.lang.String.format;

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);

This comment has been minimized.

@rongrong

rongrong Feb 12, 2019

Contributor

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

return invoke(implementation, session, arguments);
}

private Object invoke(ScalarFunctionImplementation function, ConnectorSession session, List<Object> arguments)

This comment has been minimized.

@rongrong

rongrong Feb 12, 2019

Contributor

Seems that implementation is the right name.

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);

This comment has been minimized.

@rongrong

rongrong Feb 13, 2019

Contributor

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. 🤣

This comment has been minimized.

@dain

dain Feb 21, 2019

Author Member

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.

@@ -36,16 +34,6 @@ public static ConstantExpression constantNull(Type type)
return new ConstantExpression(null, type);
}

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

This comment has been minimized.

@rongrong

rongrong Feb 13, 2019

Contributor

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?

this.functionManager = requireNonNull(functionManager, "functionManager is null");
}

public FunctionHandle notFunction()

This comment has been minimized.

@rongrong

rongrong Feb 13, 2019

Contributor

This name is super confusing 🤣

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

public FunctionHandle likeVarcharSignature()

This comment has been minimized.

@rongrong

rongrong Feb 13, 2019

Contributor

Why is this one called signature?

return functionManager.resolveFunction(session, QualifiedName.of("LIKE_PATTERN"), fromTypes(VARCHAR, VARCHAR));
}

// public FunctionHandle tryCastFunction(Type returnType, Type valueType)

This comment has been minimized.

@rongrong

rongrong Feb 13, 2019

Contributor

Is this supposed to be here or not?

@@ -104,4 +104,9 @@ public FunctionHandle lookupSaturatedFloorCast(TypeSignature fromType, TypeSigna
{
return functionRegistry.lookupSaturatedFloorCast(fromType, toType);
}

public FunctionHandle lookupInternalCastFunction(String name, TypeSignature fromType, TypeSignature toType)

This comment has been minimized.

@rongrong

rongrong Feb 13, 2019

Contributor

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


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

public class CastCodeGenerator

This comment has been minimized.

@rongrong

rongrong Feb 13, 2019

Contributor

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

@@ -60,6 +60,11 @@ public void addFunctions(List<? extends SqlFunction> functions)
return functionRegistry.list();
}

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

This comment has been minimized.

@rongrong

rongrong Feb 13, 2019

Contributor

Why is this one not called resolveFunction?

@@ -131,7 +131,7 @@ public static ImplementationDependency createDependency(Annotation annotation, S
parseTypeSignature(operatorDependency.returnType(), literalParameters),
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");

This comment has been minimized.

@rongrong

rongrong Feb 14, 2019

Contributor

Seems like this should be in the operator dependencies diff.

@@ -109,4 +109,9 @@ public FunctionHandle lookupInternalCastFunction(String name, TypeSignature from
{
return functionRegistry.lookupInternalCastFunction(name, fromType, toType);
}

public FunctionMetadata getFunctionMetadata(FunctionHandle functionHandle)

This comment has been minimized.

@rongrong

rongrong Feb 14, 2019

Contributor

Why this is getFunctionMatadata(FunctionHandle) rather than FunctionHandle.getFunctionMetadata? Also, conceptually, it seems to me that you'd only need FunctionMetadata at plan time, so in some ways FunctionMetadata can be FunctionHandle (or the other way around). Why do we need both?

@@ -264,24 +263,17 @@ protected RowExpression visitBinaryLiteral(BinaryLiteral node, Void context)
@Override
protected RowExpression visitGenericLiteral(GenericLiteral node, Void context)
{
Type type;

This comment has been minimized.

@rongrong

rongrong Feb 14, 2019

Contributor

Why is this no longer needed?

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

This comment has been minimized.

@rongrong

rongrong Feb 20, 2019

Contributor

expectedFunction

@martint martint referenced this pull request Feb 20, 2019

Open

Function and type namespaces #8

0 of 6 tasks complete
assertAggregation(
unknownKey,
null,
createBooleansBlock(null, null),
createDoublesBlock(1.0, 2.0));
InternalAggregationFunction unknownValue = METADATA.getFunctionManager().getAggregateFunctionImplementation(
new Signature("min_by", AGGREGATE, parseTypeSignature(StandardTypes.DOUBLE), parseTypeSignature(StandardTypes.DOUBLE), parseTypeSignature(UnknownType.NAME)));
InternalAggregationFunction unknownValue = getInternalAggregationFunction("min_by", DOUBLE, UNKNOWN);
assertAggregation(
unknownKey,

This comment has been minimized.

@rongrong

rongrong Feb 21, 2019

Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment