Skip to content

Commit da0f19e

Browse files
pdabre12Pratik Joseph Dabre
pdabre12
authored and
Pratik Joseph Dabre
committed
Address review comments
1 parent 021fb1c commit da0f19e

File tree

7 files changed

+68
-26
lines changed

7 files changed

+68
-26
lines changed

‎presto-main-base/src/main/java/com/facebook/presto/metadata/BuiltInFunctionHandle.java‎

Lines changed: 17 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515

1616
import com.facebook.presto.common.CatalogSchemaName;
1717
import com.facebook.presto.common.type.TypeSignature;
18+
import com.facebook.presto.spi.function.BuiltInFunctionKind;
1819
import com.facebook.presto.spi.function.FunctionHandle;
1920
import com.facebook.presto.spi.function.FunctionKind;
2021
import com.facebook.presto.spi.function.Signature;
@@ -24,22 +25,26 @@
2425
import java.util.List;
2526
import java.util.Objects;
2627

28+
import static com.facebook.presto.spi.function.BuiltInFunctionKind.ENGINE;
2729
import static java.util.Objects.requireNonNull;
2830

2931
public class BuiltInFunctionHandle
3032
implements FunctionHandle
3133
{
3234
private final Signature signature;
33-
private final boolean isBuiltInPluginFunction;
35+
private final BuiltInFunctionKind builtInFunctionKind;
3436

3537
@JsonCreator
36-
public BuiltInFunctionHandle(
37-
@JsonProperty("signature") Signature signature,
38-
@JsonProperty("isBuiltInPluginFunction") boolean isBuiltInPluginFunction)
38+
public BuiltInFunctionHandle(@JsonProperty("signature") Signature signature)
39+
{
40+
this(signature, ENGINE);
41+
}
42+
43+
public BuiltInFunctionHandle(Signature signature, BuiltInFunctionKind builtInFunctionKind)
3944
{
4045
this.signature = requireNonNull(signature, "signature is null");
4146
checkArgument(signature.getTypeVariableConstraints().isEmpty(), "%s has unbound type parameters", signature);
42-
this.isBuiltInPluginFunction = isBuiltInPluginFunction;
47+
this.builtInFunctionKind = requireNonNull(builtInFunctionKind, "builtInFunctionKind is null");
4348
}
4449

4550
@JsonProperty
@@ -66,17 +71,16 @@ public List<TypeSignature> getArgumentTypes()
6671
return signature.getArgumentTypes();
6772
}
6873

69-
@JsonProperty
7074
@Override
71-
public boolean isBuiltInPluginFunction()
75+
public CatalogSchemaName getCatalogSchemaName()
7276
{
73-
return isBuiltInPluginFunction;
77+
return signature.getName().getCatalogSchemaName();
7478
}
7579

76-
@Override
77-
public CatalogSchemaName getCatalogSchemaName()
80+
@JsonProperty
81+
public BuiltInFunctionKind getBuiltInFunctionKind()
7882
{
79-
return signature.getName().getCatalogSchemaName();
83+
return builtInFunctionKind;
8084
}
8185

8286
@Override
@@ -90,13 +94,13 @@ public boolean equals(Object o)
9094
}
9195
BuiltInFunctionHandle that = (BuiltInFunctionHandle) o;
9296
return Objects.equals(signature, that.signature)
93-
&& Objects.equals(isBuiltInPluginFunction, that.isBuiltInPluginFunction);
97+
&& Objects.equals(builtInFunctionKind, that.builtInFunctionKind);
9498
}
9599

96100
@Override
97101
public int hashCode()
98102
{
99-
return Objects.hash(signature, isBuiltInPluginFunction);
103+
return Objects.hash(signature, builtInFunctionKind);
100104
}
101105

102106
@Override

‎presto-main-base/src/main/java/com/facebook/presto/metadata/BuiltInPluginFunctionNamespaceManager.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@
4343
import java.util.Optional;
4444
import java.util.concurrent.CompletableFuture;
4545

46+
import static com.facebook.presto.spi.function.BuiltInFunctionKind.PLUGIN;
4647
import static com.facebook.presto.spi.function.FunctionImplementationType.SQL;
4748
import static com.facebook.presto.spi.function.FunctionKind.SCALAR;
4849
import static com.google.common.base.Preconditions.checkArgument;
@@ -90,7 +91,7 @@ public synchronized void registerPluginFunctions(List<? extends SqlFunction> fun
9091
@Override
9192
public FunctionHandle getFunctionHandle(Optional<? extends FunctionNamespaceTransactionHandle> transactionHandle, Signature signature)
9293
{
93-
return new BuiltInFunctionHandle(signature, true);
94+
return new BuiltInFunctionHandle(signature, PLUGIN);
9495
}
9596

9697
@Override

‎presto-main-base/src/main/java/com/facebook/presto/metadata/BuiltInTypeAndFunctionNamespaceManager.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1130,7 +1130,7 @@ public Collection<SqlFunction> getFunctions(Optional<? extends FunctionNamespace
11301130
@Override
11311131
public FunctionHandle getFunctionHandle(Optional<? extends FunctionNamespaceTransactionHandle> transactionHandle, Signature signature)
11321132
{
1133-
return new BuiltInFunctionHandle(signature, false);
1133+
return new BuiltInFunctionHandle(signature);
11341134
}
11351135

11361136
@Override

‎presto-main-base/src/main/java/com/facebook/presto/metadata/FunctionAndTypeManager.java‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,7 @@
103103
import static com.facebook.presto.spi.StandardErrorCode.FUNCTION_IMPLEMENTATION_MISSING;
104104
import static com.facebook.presto.spi.StandardErrorCode.FUNCTION_NOT_FOUND;
105105
import static com.facebook.presto.spi.StandardErrorCode.GENERIC_USER_ERROR;
106+
import static com.facebook.presto.spi.function.BuiltInFunctionKind.PLUGIN;
106107
import static com.facebook.presto.spi.function.FunctionKind.SCALAR;
107108
import static com.facebook.presto.spi.function.SqlFunctionVisibility.EXPERIMENTAL;
108109
import static com.facebook.presto.spi.function.SqlFunctionVisibility.PUBLIC;
@@ -350,7 +351,7 @@ public FunctionMetadata getFunctionMetadata(FunctionHandle functionHandle)
350351
if (functionHandle.getCatalogSchemaName().equals(SESSION_NAMESPACE)) {
351352
return ((SessionFunctionHandle) functionHandle).getFunctionMetadata();
352353
}
353-
if (functionHandle.isBuiltInPluginFunction()) {
354+
if (isBuiltInPluginFunctionHandle(functionHandle)) {
354355
return builtInPluginFunctionNamespaceManager.getFunctionMetadata(functionHandle);
355356
}
356357
Optional<FunctionNamespaceManager<?>> functionNamespaceManager = getServingFunctionNamespaceManager(functionHandle.getCatalogSchemaName());
@@ -616,7 +617,7 @@ public ScalarFunctionImplementation getScalarFunctionImplementation(FunctionHand
616617
if (functionHandle.getCatalogSchemaName().equals(SESSION_NAMESPACE)) {
617618
return ((SessionFunctionHandle) functionHandle).getScalarFunctionImplementation();
618619
}
619-
if (functionHandle.isBuiltInPluginFunction()) {
620+
if (isBuiltInPluginFunctionHandle(functionHandle)) {
620621
return builtInPluginFunctionNamespaceManager.getScalarFunctionImplementation(functionHandle);
621622
}
622623
Optional<FunctionNamespaceManager<?>> functionNamespaceManager = getServingFunctionNamespaceManager(functionHandle.getCatalogSchemaName());
@@ -817,7 +818,7 @@ private FunctionHandle resolveFunctionInternal(Optional<TransactionId> transacti
817818
// verify we have one parameter of the proper type
818819
checkArgument(parameterTypes.size() == 1, "Expected one argument to literal function, but got %s", parameterTypes);
819820

820-
return new BuiltInFunctionHandle(getMagicLiteralFunctionSignature(type), false);
821+
return new BuiltInFunctionHandle(getMagicLiteralFunctionSignature(type));
821822
}
822823

823824
throw new PrestoException(FUNCTION_NOT_FOUND, constructFunctionNotFoundErrorMessage(
@@ -989,6 +990,11 @@ private Optional<Signature> getMatchingFunction(
989990
return functionSignatureMatcher.match(candidates, parameterTypes, coercionAllowed);
990991
}
991992

993+
private boolean isBuiltInPluginFunctionHandle(FunctionHandle functionHandle)
994+
{
995+
return (functionHandle instanceof BuiltInFunctionHandle) && ((BuiltInFunctionHandle) functionHandle).getBuiltInFunctionKind().equals(PLUGIN);
996+
}
997+
992998
private static class FunctionResolutionCacheKey
993999
{
9941000
private final QualifiedObjectName functionName;

‎presto-main-base/src/test/java/com/facebook/presto/metadata/TestFunctionAndTypeManager.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ public void testIdentityCast()
8383
{
8484
FunctionAndTypeManager functionAndTypeManager = createTestFunctionAndTypeManager();
8585
FunctionHandle exactOperator = functionAndTypeManager.lookupCast(CastType.CAST, HYPER_LOG_LOG, HYPER_LOG_LOG);
86-
assertEquals(exactOperator, new BuiltInFunctionHandle(new Signature(CAST.getFunctionName(), SCALAR, HYPER_LOG_LOG.getTypeSignature(), HYPER_LOG_LOG.getTypeSignature()), false));
86+
assertEquals(exactOperator, new BuiltInFunctionHandle(new Signature(CAST.getFunctionName(), SCALAR, HYPER_LOG_LOG.getTypeSignature(), HYPER_LOG_LOG.getTypeSignature())));
8787
}
8888

8989
@Test
@@ -444,7 +444,7 @@ public ResolveFunctionAssertion forParameters(String... parameters)
444444

445445
public ResolveFunctionAssertion returns(SignatureBuilder functionSignature)
446446
{
447-
FunctionHandle expectedFunction = new BuiltInFunctionHandle(functionSignature.name(TEST_FUNCTION_NAME).build(), false);
447+
FunctionHandle expectedFunction = new BuiltInFunctionHandle(functionSignature.name(TEST_FUNCTION_NAME).build());
448448
FunctionHandle actualFunction = resolveFunctionHandle();
449449
assertEquals(expectedFunction, actualFunction);
450450
return this;

‎presto-spi/src/main/java/com/facebook/presto/spi/function/BuiltInFunctionKind.java‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
/*
2+
* Licensed under the Apache License, Version 2.0 (the "License");
3+
* you may not use this file except in compliance with the License.
4+
* You may obtain a copy of the License at
5+
*
6+
* http://www.apache.org/licenses/LICENSE-2.0
7+
*
8+
* Unless required by applicable law or agreed to in writing, software
9+
* distributed under the License is distributed on an "AS IS" BASIS,
10+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
11+
* See the License for the specific language governing permissions and
12+
* limitations under the License.
13+
*/
14+
package com.facebook.presto.spi.function;
15+
16+
import com.facebook.drift.annotations.ThriftEnum;
17+
import com.facebook.drift.annotations.ThriftEnumValue;
18+
19+
@ThriftEnum
20+
public enum BuiltInFunctionKind
21+
{
22+
ENGINE(0),
23+
PLUGIN(1);
24+
25+
private final int value;
26+
27+
BuiltInFunctionKind(int value)
28+
{
29+
this.value = value;
30+
}
31+
32+
@ThriftEnumValue
33+
public int getValue()
34+
{
35+
return value;
36+
}
37+
}

‎presto-spi/src/main/java/com/facebook/presto/spi/function/FunctionHandle.java‎

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -31,10 +31,4 @@ public interface FunctionHandle
3131
FunctionKind getKind();
3232

3333
List<TypeSignature> getArgumentTypes();
34-
35-
// todo: fix this hack
36-
default boolean isBuiltInPluginFunction()
37-
{
38-
return false;
39-
}
4034
}

0 commit comments

Comments
 (0)