Skip to content

Inject null provider when thrift execution is not needed #15501

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.

Sign up for GitHub

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

Merged
rongrong merged 1 commit into from
Dec 8, 2020
Merged

Inject null provider when thrift execution is not needed #15501

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions ...s/src/main/java/com/facebook/presto/functionNamespace/execution/SqlFunctionExecutors.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,25 +23,29 @@
import com.facebook.presto.spi.function.ThriftScalarFunctionImplementation;
import com.google.inject.Inject;

import javax.annotation.Nullable;

import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.concurrent.CompletableFuture;

import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkState;
import static java.lang.String.format;
import static java.util.Objects.requireNonNull;

public class SqlFunctionExecutors
{
private final Map<Language, FunctionImplementationType> supportedLanguages;
private final ThriftSqlFunctionExecutor thriftSqlFunctionExecutor;
private final Optional<ThriftSqlFunctionExecutor> thriftSqlFunctionExecutor;

@Inject
public SqlFunctionExecutors(Map<Language, FunctionImplementationType> supportedLanguages, ThriftSqlFunctionExecutor thriftSqlFunctionExecutor)
public SqlFunctionExecutors(Map<Language, FunctionImplementationType> supportedLanguages, @Nullable ThriftSqlFunctionExecutor thriftSqlFunctionExecutor)
Copy link
Copy Markdown
Contributor

@wenleix wenleix Dec 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just make this Optional? :)

Copy link
Copy Markdown
Contributor

@caithagoras caithagoras Dec 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also prefer Optional here.

Copy link
Copy Markdown
Contributor

@caithagoras caithagoras Dec 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you not pushed yet since it's still @nullable?

Copy link
Copy Markdown
Contributor Author

@rongrong rongrong Dec 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decided to go with nullable for injection but change the member variable to optional. This for the most part avoids potential bugs of having a nullable variable, and doesn't require me to figure out how to inject Optional 😂

{
this.supportedLanguages = requireNonNull(supportedLanguages, "supportedLanguages is null");
this.thriftSqlFunctionExecutor = requireNonNull(thriftSqlFunctionExecutor, "thriftSqlFunctionExecutor is null");
this.thriftSqlFunctionExecutor = Optional.ofNullable(thriftSqlFunctionExecutor);
}

public Set<Language> getSupportedLanguages()
Expand All @@ -57,6 +61,7 @@ public FunctionImplementationType getFunctionImplementationType(Language languag
public CompletableFuture<Block> executeFunction(ScalarFunctionImplementation functionImplementation, Page input, List<Integer> channels, List<Type> argumentTypes, Type returnType)
{
checkArgument(functionImplementation instanceof ThriftScalarFunctionImplementation, format("Only support ThriftScalarFunctionImplementation, got %s", functionImplementation.getClass()));
return thriftSqlFunctionExecutor.executeFunction((ThriftScalarFunctionImplementation) functionImplementation, input, channels, argumentTypes, returnType);
checkState(thriftSqlFunctionExecutor.isPresent(), "Thrift SQL function executor is not setup");
return thriftSqlFunctionExecutor.get().executeFunction((ThriftScalarFunctionImplementation) functionImplementation, input, channels, argumentTypes, returnType);
}
}
5 changes: 5 additions & 0 deletions ...sto/functionNamespace/execution/thrift/SimpleAddressThriftSqlFunctionExecutionModule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import com.facebook.presto.thrift.api.udf.ThriftUdfService;
import com.google.inject.Binder;
import com.google.inject.Module;
import com.google.inject.util.Providers;

import java.util.Map;

Expand All @@ -40,6 +41,10 @@ public SimpleAddressThriftSqlFunctionExecutionModule(Map<Language, SimpleAddress
@Override
public void configure(Binder binder)
{
if (supportedLanguages.isEmpty()) {
binder.bind(ThriftSqlFunctionExecutor.class).toProvider(Providers.of(null));
return;
}
binder.bind(ThriftSqlFunctionExecutor.class).in(SINGLETON);

driftClientBinder(binder)
Expand Down
6 changes: 2 additions & 4 deletions ...ava/com/facebook/presto/functionNamespace/execution/thrift/ThriftSqlFunctionExecutor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import static com.facebook.presto.thrift.api.udf.ThriftUdfPage.thriftPage;
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.collect.Iterables.getOnlyElement;
import static java.util.Objects.requireNonNull;

public class ThriftSqlFunctionExecutor
{
Expand All @@ -47,14 +48,11 @@ public class ThriftSqlFunctionExecutor
@Inject
public ThriftSqlFunctionExecutor(DriftClient<ThriftUdfService> thriftUdfClient)
{
this.thriftUdfClient = thriftUdfClient;
this.thriftUdfClient = requireNonNull(thriftUdfClient, "thriftUdfClient is null");
}

public CompletableFuture<Block> executeFunction(ThriftScalarFunctionImplementation functionImplementation, Page input, List<Integer> channels, List<Type> argumentTypes, Type returnType)
{
if (thriftUdfClient == null) {
throw new UnsupportedOperationException("Thrift function execution is not supported");
}
ImmutableList.Builder<PrestoThriftBlock> blocks = ImmutableList.builder();
for (int i = 0; i < channels.size(); i++) {
Block block = input.getBlock(channels.get(i));
Expand Down
5 changes: 2 additions & 3 deletions ...st/java/com/facebook/presto/functionNamespace/TestSqlInvokedFunctionNamespaceManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
package com.facebook.presto.functionNamespace;

import com.facebook.presto.functionNamespace.execution.SqlFunctionExecutors;
import com.facebook.presto.functionNamespace.execution.thrift.ThriftSqlFunctionExecutor;
import com.facebook.presto.functionNamespace.testing.InMemoryFunctionNamespaceManager;
import com.facebook.presto.spi.ErrorCodeSupplier;
import com.facebook.presto.spi.PrestoException;
Expand Down Expand Up @@ -84,7 +83,7 @@ public void testTransactionalGetFunction()
TEST_CATALOG,
new SqlFunctionExecutors(
ImmutableMap.of(SQL, FunctionImplementationType.SQL),
new ThriftSqlFunctionExecutor(null)),
null),
new SqlInvokedFunctionNamespaceManagerConfig()
.setFunctionCacheExpiration(new Duration(0, MILLISECONDS))
.setFunctionInstanceCacheExpiration(new Duration(0, MILLISECONDS)));
Expand Down Expand Up @@ -159,7 +158,7 @@ private static InMemoryFunctionNamespaceManager createFunctionNamespaceManager()
TEST_CATALOG,
new SqlFunctionExecutors(
ImmutableMap.of(SQL, FunctionImplementationType.SQL),
new ThriftSqlFunctionExecutor(null)),
null),
new SqlInvokedFunctionNamespaceManagerConfig());
}

Expand Down
4 changes: 4 additions & 0 deletions presto-main/etc/function-namespace/example.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
function-namespace-manager.name=mysql
database-url=jdbc:mysql://localhost:3306
function-namespaces-table-name=example_function_namespaces
functions-table-name=example_sql_functions
3 changes: 1 addition & 2 deletions presto-main/src/test/java/com/facebook/presto/sql/analyzer/AbstractAnalyzerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
import com.facebook.presto.execution.warnings.WarningCollectorConfig;
import com.facebook.presto.functionNamespace.SqlInvokedFunctionNamespaceManagerConfig;
import com.facebook.presto.functionNamespace.execution.SqlFunctionExecutors;
import com.facebook.presto.functionNamespace.execution.thrift.ThriftSqlFunctionExecutor;
import com.facebook.presto.functionNamespace.testing.InMemoryFunctionNamespaceManager;
import com.facebook.presto.metadata.Catalog;
import com.facebook.presto.metadata.CatalogManager;
Expand Down Expand Up @@ -138,7 +137,7 @@ public void setup()
"unittest",
new SqlFunctionExecutors(
ImmutableMap.of(SQL, FunctionImplementationType.SQL),
new ThriftSqlFunctionExecutor(null)),
null),
new SqlInvokedFunctionNamespaceManagerConfig().setSupportedFunctionLanguages("sql")));

metadata.getFunctionAndTypeManager().createFunction(SQL_FUNCTION_SQUARE, true);
Expand Down
3 changes: 1 addition & 2 deletions .../src/test/java/com/facebook/presto/sql/planner/iterative/rule/TestInlineSqlFunctions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
import com.facebook.presto.common.type.Type;
import com.facebook.presto.functionNamespace.SqlInvokedFunctionNamespaceManagerConfig;
import com.facebook.presto.functionNamespace.execution.SqlFunctionExecutors;
import com.facebook.presto.functionNamespace.execution.thrift.ThriftSqlFunctionExecutor;
import com.facebook.presto.functionNamespace.testing.InMemoryFunctionNamespaceManager;
import com.facebook.presto.metadata.FunctionAndTypeManager;
import com.facebook.presto.metadata.Metadata;
Expand Down Expand Up @@ -128,7 +127,7 @@ public void setup()
ImmutableMap.of(
SQL, FunctionImplementationType.SQL,
JAVA, THRIFT),
new ThriftSqlFunctionExecutor(null)),
null),
new SqlInvokedFunctionNamespaceManagerConfig().setSupportedFunctionLanguages("sql,java")));
functionAndTypeManager.createFunction(SQL_FUNCTION_SQUARE, true);
functionAndTypeManager.createFunction(THRIFT_FUNCTION_FOO, true);
Expand Down
5 changes: 2 additions & 3 deletions ...c/test/java/com/facebook/presto/sql/planner/iterative/rule/TestPlanRemoteProjections.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
import com.facebook.presto.common.type.StandardTypes;
import com.facebook.presto.functionNamespace.SqlInvokedFunctionNamespaceManagerConfig;
import com.facebook.presto.functionNamespace.execution.SqlFunctionExecutors;
import com.facebook.presto.functionNamespace.execution.thrift.ThriftSqlFunctionExecutor;
import com.facebook.presto.functionNamespace.testing.InMemoryFunctionNamespaceManager;
import com.facebook.presto.metadata.FunctionAndTypeManager;
import com.facebook.presto.metadata.Metadata;
Expand Down Expand Up @@ -113,7 +112,7 @@ public void setup()
ImmutableMap.of(
SQL, FunctionImplementationType.SQL,
JAVA, THRIFT),
new ThriftSqlFunctionExecutor(null)),
null),
new SqlInvokedFunctionNamespaceManagerConfig().setSupportedFunctionLanguages("sql,java")));
functionAndTypeManager.createFunction(FUNCTION_REMOTE_FOO_0, true);
functionAndTypeManager.createFunction(FUNCTION_REMOTE_FOO_1, true);
Expand Down Expand Up @@ -280,7 +279,7 @@ public void testRemoteFunctionDisabled()
ImmutableMap.of(
SQL, FunctionImplementationType.SQL,
JAVA, THRIFT),
new ThriftSqlFunctionExecutor(null)),
null),
new SqlInvokedFunctionNamespaceManagerConfig().setSupportedFunctionLanguages("sql,java")));
functionAndTypeManager.createFunction(FUNCTION_REMOTE_FOO_1, true);
tester.assertThat(new PlanRemotePojections(functionAndTypeManager))
Expand Down
3 changes: 1 addition & 2 deletions ...ook/presto/sql/planner/iterative/rule/TestRewriteFilterWithExternalFunctionToProject.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
import com.facebook.presto.common.type.StandardTypes;
import com.facebook.presto.functionNamespace.SqlInvokedFunctionNamespaceManagerConfig;
import com.facebook.presto.functionNamespace.execution.SqlFunctionExecutors;
import com.facebook.presto.functionNamespace.execution.thrift.ThriftSqlFunctionExecutor;
import com.facebook.presto.functionNamespace.testing.InMemoryFunctionNamespaceManager;
import com.facebook.presto.metadata.FunctionAndTypeManager;
import com.facebook.presto.spi.function.FunctionImplementationType;
Expand Down Expand Up @@ -73,7 +72,7 @@ public void setup()
ImmutableMap.of(
SQL, FunctionImplementationType.SQL,
JAVA, THRIFT),
new ThriftSqlFunctionExecutor(null)),
null),
new SqlInvokedFunctionNamespaceManagerConfig().setSupportedFunctionLanguages("sql,java")));
functionAndTypeManager.createFunction(FUNCTION_TANGENT, true);
functionAndTypeManager.createFunction(FUNCTION_REMOTE_FOO, true);
Expand Down