Skip to content

Commit 0363c8a

Browse files
rschlusselrschlussel
committed
Fix CREATE/DROP TEMPORARY FUNCTION
This was broken by #16699 and has not worked since release 0.268. The CreateFunctionTask and DropFunctionTask should be adding functions directly to the QueryStateMachine rather than keeping a special map in the task. This commit basically reverts the changes that #16699 introduced for create and drop function.
1 parent 911eaa2 commit 0363c8a

File tree

7 files changed

+19
-85
lines changed

7 files changed

+19
-85
lines changed

‎presto-analyzer/src/main/java/com/facebook/presto/sql/analyzer/utils/StatementUtils.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -129,9 +129,9 @@ private StatementUtils() {}
129129
builder.put(DropView.class, QueryType.DATA_DEFINITION);
130130
builder.put(CreateMaterializedView.class, QueryType.DATA_DEFINITION);
131131
builder.put(DropMaterializedView.class, QueryType.DATA_DEFINITION);
132-
builder.put(CreateFunction.class, QueryType.DATA_DEFINITION);
132+
builder.put(CreateFunction.class, QueryType.CONTROL);
133133
builder.put(AlterFunction.class, QueryType.DATA_DEFINITION);
134-
builder.put(DropFunction.class, QueryType.DATA_DEFINITION);
134+
builder.put(DropFunction.class, QueryType.CONTROL);
135135
builder.put(Use.class, QueryType.CONTROL);
136136
builder.put(SetSession.class, QueryType.CONTROL);
137137
builder.put(ResetSession.class, QueryType.CONTROL);

‎presto-main/src/main/java/com/facebook/presto/execution/CreateFunctionTask.java‎

Lines changed: 5 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@
1919
import com.facebook.presto.common.type.TypeSignature;
2020
import com.facebook.presto.metadata.Metadata;
2121
import com.facebook.presto.spi.PrestoException;
22-
import com.facebook.presto.spi.WarningCollector;
2322
import com.facebook.presto.spi.function.Parameter;
2423
import com.facebook.presto.spi.function.RoutineCharacteristics;
2524
import com.facebook.presto.spi.function.SqlFunctionHandle;
@@ -38,20 +37,17 @@
3837
import com.facebook.presto.sql.tree.Return;
3938
import com.facebook.presto.sql.tree.RoutineBody;
4039
import com.facebook.presto.transaction.TransactionManager;
41-
import com.google.common.annotations.VisibleForTesting;
4240
import com.google.common.util.concurrent.ListenableFuture;
4341

4442
import javax.inject.Inject;
4543

4644
import java.util.List;
4745
import java.util.Map;
4846
import java.util.Optional;
49-
import java.util.concurrent.ConcurrentHashMap;
5047

5148
import static com.facebook.presto.common.type.TypeSignature.parseTypeSignature;
5249
import static com.facebook.presto.metadata.FunctionAndTypeManager.qualifyObjectName;
5350
import static com.facebook.presto.metadata.SessionFunctionHandle.SESSION_NAMESPACE;
54-
import static com.facebook.presto.spi.StandardErrorCode.ALREADY_EXISTS;
5551
import static com.facebook.presto.spi.StandardErrorCode.NOT_SUPPORTED;
5652
import static com.facebook.presto.spi.function.FunctionVersion.notVersioned;
5753
import static com.facebook.presto.sql.SqlFormatter.formatSql;
@@ -62,10 +58,9 @@
6258
import static java.util.Objects.requireNonNull;
6359

6460
public class CreateFunctionTask
65-
implements DDLDefinitionTask<CreateFunction>
61+
implements SessionTransactionControlTask<CreateFunction>
6662
{
6763
private final SqlParser sqlParser;
68-
private final Map<SqlFunctionId, SqlInvokedFunction> addedSessionFunctions = new ConcurrentHashMap<>();
6964

7065
@Inject
7166
public CreateFunctionTask(SqlParser sqlParser)
@@ -86,10 +81,11 @@ public String explain(CreateFunction statement, List<Expression> parameters)
8681
}
8782

8883
@Override
89-
public ListenableFuture<?> execute(CreateFunction statement, TransactionManager transactionManager, Metadata metadata, AccessControl accessControl, Session session, List<Expression> parameters, WarningCollector warningCollector)
84+
public ListenableFuture<?> execute(CreateFunction statement, TransactionManager transactionManager, Metadata metadata, AccessControl accessControl, QueryStateMachine stateMachine, List<Expression> parameters)
9085
{
9186
Map<NodeRef<com.facebook.presto.sql.tree.Parameter>, Expression> parameterLookup = parameterExtractor(statement, parameters);
92-
Analyzer analyzer = new Analyzer(session, metadata, sqlParser, accessControl, Optional.empty(), parameters, parameterLookup, warningCollector);
87+
Session session = stateMachine.getSession();
88+
Analyzer analyzer = new Analyzer(session, metadata, sqlParser, accessControl, Optional.empty(), parameters, parameterLookup, stateMachine.getWarningCollector());
9389
Analysis analysis = analyzer.analyze(statement);
9490
if (analysis.getFunctionHandles().values().stream()
9591
.anyMatch(SqlFunctionHandle.class::isInstance)) {
@@ -98,7 +94,7 @@ public ListenableFuture<?> execute(CreateFunction statement, TransactionManager
9894

9995
SqlInvokedFunction function = createSqlInvokedFunction(statement, metadata, analysis);
10096
if (statement.isTemporary()) {
101-
addSessionFunction(session, new SqlFunctionId(function.getSignature().getName(), function.getSignature().getArgumentTypes()), function);
97+
stateMachine.addSessionFunction(new SqlFunctionId(function.getSignature().getName(), function.getSignature().getArgumentTypes()), function);
10298
}
10399
else {
104100
metadata.getFunctionAndTypeManager().createFunction(function, statement.isReplace());
@@ -165,20 +161,4 @@ public Expression rewriteExpression(Expression expression, Void context, Express
165161
formatSql(body, Optional.empty()),
166162
notVersioned());
167163
}
168-
169-
private void addSessionFunction(Session session, SqlFunctionId signature, SqlInvokedFunction function)
170-
{
171-
requireNonNull(signature, "signature is null");
172-
requireNonNull(function, "function is null");
173-
174-
if (session.getSessionFunctions().containsKey(signature) || addedSessionFunctions.putIfAbsent(signature, function) != null) {
175-
throw new PrestoException(ALREADY_EXISTS, format("Session function %s has already been defined", signature));
176-
}
177-
}
178-
179-
@VisibleForTesting
180-
public Map<SqlFunctionId, SqlInvokedFunction> getAddedSessionFunctions()
181-
{
182-
return addedSessionFunctions;
183-
}
184164
}

‎presto-main/src/main/java/com/facebook/presto/execution/DropFunctionTask.java‎

Lines changed: 4 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -13,12 +13,9 @@
1313
*/
1414
package com.facebook.presto.execution;
1515

16-
import com.facebook.presto.Session;
1716
import com.facebook.presto.common.QualifiedObjectName;
1817
import com.facebook.presto.common.type.TypeSignature;
1918
import com.facebook.presto.metadata.Metadata;
20-
import com.facebook.presto.spi.PrestoException;
21-
import com.facebook.presto.spi.WarningCollector;
2219
import com.facebook.presto.spi.function.SqlFunctionId;
2320
import com.facebook.presto.spi.security.AccessControl;
2421
import com.facebook.presto.sql.analyzer.Analyzer;
@@ -28,20 +25,16 @@
2825
import com.facebook.presto.sql.tree.NodeRef;
2926
import com.facebook.presto.sql.tree.Parameter;
3027
import com.facebook.presto.transaction.TransactionManager;
31-
import com.google.common.annotations.VisibleForTesting;
32-
import com.google.common.collect.Sets;
3328
import com.google.common.util.concurrent.ListenableFuture;
3429

3530
import javax.inject.Inject;
3631

3732
import java.util.List;
3833
import java.util.Map;
3934
import java.util.Optional;
40-
import java.util.Set;
4135

4236
import static com.facebook.presto.metadata.FunctionAndTypeManager.qualifyObjectName;
4337
import static com.facebook.presto.metadata.SessionFunctionHandle.SESSION_NAMESPACE;
44-
import static com.facebook.presto.spi.StandardErrorCode.NOT_FOUND;
4538
import static com.facebook.presto.sql.analyzer.utils.ParameterUtils.parameterExtractor;
4639
import static com.google.common.collect.ImmutableList.toImmutableList;
4740
import static com.google.common.util.concurrent.Futures.immediateFuture;
@@ -50,10 +43,9 @@
5043
import static java.util.Objects.requireNonNull;
5144

5245
public class DropFunctionTask
53-
implements DDLDefinitionTask<DropFunction>
46+
implements SessionTransactionControlTask<DropFunction>
5447
{
5548
private final SqlParser sqlParser;
56-
private final Set<SqlFunctionId> removedSessionFunctions = Sets.newConcurrentHashSet();
5749

5850
@Inject
5951
public DropFunctionTask(SqlParser sqlParser)
@@ -74,15 +66,15 @@ public String explain(DropFunction statement, List<Expression> parameters)
7466
}
7567

7668
@Override
77-
public ListenableFuture<?> execute(DropFunction statement, TransactionManager transactionManager, Metadata metadata, AccessControl accessControl, Session session, List<Expression> parameters, WarningCollector warningCollector)
69+
public ListenableFuture<?> execute(DropFunction statement, TransactionManager transactionManager, Metadata metadata, AccessControl accessControl, QueryStateMachine stateMachine, List<Expression> parameters)
7870
{
7971
Map<NodeRef<Parameter>, Expression> parameterLookup = parameterExtractor(statement, parameters);
80-
Analyzer analyzer = new Analyzer(session, metadata, sqlParser, accessControl, Optional.empty(), parameters, parameterLookup, warningCollector);
72+
Analyzer analyzer = new Analyzer(stateMachine.getSession(), metadata, sqlParser, accessControl, Optional.empty(), parameters, parameterLookup, stateMachine.getWarningCollector());
8173
analyzer.analyze(statement);
8274
Optional<List<TypeSignature>> parameterTypes = statement.getParameterTypes().map(types -> types.stream().map(TypeSignature::parseTypeSignature).collect(toImmutableList()));
8375

8476
if (statement.isTemporary()) {
85-
removeSessionFunction(session,
77+
stateMachine.removeSessionFunction(
8678
new SqlFunctionId(
8779
QualifiedObjectName.valueOf(SESSION_NAMESPACE, statement.getFunctionName().getSuffix()),
8880
parameterTypes.orElse(emptyList())),
@@ -97,24 +89,4 @@ public ListenableFuture<?> execute(DropFunction statement, TransactionManager tr
9789

9890
return immediateFuture(null);
9991
}
100-
101-
private void removeSessionFunction(Session session, SqlFunctionId signature, boolean suppressNotFoundException)
102-
{
103-
requireNonNull(signature, "signature is null");
104-
105-
if (!session.getSessionFunctions().containsKey(signature)) {
106-
if (!suppressNotFoundException) {
107-
throw new PrestoException(NOT_FOUND, format("Session function %s not found", signature.getFunctionName()));
108-
}
109-
}
110-
else {
111-
removedSessionFunctions.add(signature);
112-
}
113-
}
114-
115-
@VisibleForTesting
116-
public Set<SqlFunctionId> getRemovedSessionFunctions()
117-
{
118-
return removedSessionFunctions;
119-
}
12092
}

‎presto-main/src/main/java/com/facebook/presto/util/PrestoDataDefBindingHelper.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -121,9 +121,7 @@ private PrestoDataDefBindingHelper() {}
121121
dataDefBuilder.put(DropView.class, DropViewTask.class);
122122
dataDefBuilder.put(CreateMaterializedView.class, CreateMaterializedViewTask.class);
123123
dataDefBuilder.put(DropMaterializedView.class, DropMaterializedViewTask.class);
124-
dataDefBuilder.put(CreateFunction.class, CreateFunctionTask.class);
125124
dataDefBuilder.put(AlterFunction.class, AlterFunctionTask.class);
126-
dataDefBuilder.put(DropFunction.class, DropFunctionTask.class);
127125
dataDefBuilder.put(Call.class, CallTask.class);
128126
dataDefBuilder.put(CreateRole.class, CreateRoleTask.class);
129127
dataDefBuilder.put(DropRole.class, DropRoleTask.class);
@@ -141,6 +139,8 @@ private PrestoDataDefBindingHelper() {}
141139
transactionDefBuilder.put(SetRole.class, SetRoleTask.class);
142140
transactionDefBuilder.put(Prepare.class, PrepareTask.class);
143141
transactionDefBuilder.put(Deallocate.class, DeallocateTask.class);
142+
transactionDefBuilder.put(CreateFunction.class, CreateFunctionTask.class);
143+
transactionDefBuilder.put(DropFunction.class, DropFunctionTask.class);
144144

145145
STATEMENT_TASK_TYPES = dataDefBuilder.build();
146146
TRANSACTION_CONTROL_TYPES = transactionDefBuilder.build();

‎presto-main/src/test/java/com/facebook/presto/execution/TestCreateFunctionTask.java‎

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

1616
import com.facebook.presto.metadata.MetadataManager;
1717
import com.facebook.presto.spi.PrestoException;
18-
import com.facebook.presto.spi.WarningCollector;
1918
import com.facebook.presto.spi.security.AllowAllAccessControl;
2019
import com.facebook.presto.sql.parser.ParsingOptions;
2120
import com.facebook.presto.sql.parser.SqlParser;
@@ -54,10 +53,8 @@ public void testCreateTemporaryFunction()
5453
CreateFunction statement = (CreateFunction) parser.createStatement(sqlString, ParsingOptions.builder().build());
5554
TransactionManager transactionManager = createTestTransactionManager();
5655
QueryStateMachine stateMachine = createQueryStateMachine(sqlString, TEST_SESSION, false, transactionManager, executorService, metadataManager);
57-
WarningCollector warningCollector = stateMachine.getWarningCollector();
58-
CreateFunctionTask createFunctionTask = new CreateFunctionTask(parser);
59-
createFunctionTask.execute(statement, transactionManager, metadataManager, new AllowAllAccessControl(), TEST_SESSION, emptyList(), warningCollector);
60-
assertEquals(createFunctionTask.getAddedSessionFunctions().size(), 1);
56+
new CreateFunctionTask(parser).execute(statement, transactionManager, metadataManager, new AllowAllAccessControl(), stateMachine, emptyList());
57+
assertEquals(stateMachine.getAddedSessionFunctions().size(), 1);
6158
}
6259

6360
@Test(expectedExceptions = PrestoException.class, expectedExceptionsMessageRegExp = "Session function .* has already been defined")
@@ -68,9 +65,7 @@ public void testCreateTemporaryFunctionWithSameNameFails()
6865
CreateFunction statement = (CreateFunction) parser.createStatement(sqlString, ParsingOptions.builder().build());
6966
TransactionManager transactionManager = createTestTransactionManager();
7067
QueryStateMachine stateMachine = createQueryStateMachine(sqlString, TEST_SESSION, false, transactionManager, executorService, metadataManager);
71-
WarningCollector warningCollector = stateMachine.getWarningCollector();
72-
CreateFunctionTask createFunctionTask = new CreateFunctionTask(parser);
73-
createFunctionTask.execute(statement, transactionManager, metadataManager, new AllowAllAccessControl(), TEST_SESSION, emptyList(), warningCollector);
74-
createFunctionTask.execute(statement, transactionManager, metadataManager, new AllowAllAccessControl(), TEST_SESSION, emptyList(), warningCollector);
68+
new CreateFunctionTask(parser).execute(statement, transactionManager, metadataManager, new AllowAllAccessControl(), stateMachine, emptyList());
69+
new CreateFunctionTask(parser).execute(statement, transactionManager, metadataManager, new AllowAllAccessControl(), stateMachine, emptyList());
7570
}
7671
}

‎presto-main/src/test/java/com/facebook/presto/execution/TestDropFunctionTask.java‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,6 @@
1717
import com.facebook.presto.common.QualifiedObjectName;
1818
import com.facebook.presto.metadata.MetadataManager;
1919
import com.facebook.presto.spi.PrestoException;
20-
import com.facebook.presto.spi.WarningCollector;
2120
import com.facebook.presto.spi.function.SqlFunctionId;
2221
import com.facebook.presto.spi.security.AllowAllAccessControl;
2322
import com.facebook.presto.sql.parser.ParsingOptions;
@@ -86,9 +85,7 @@ private Set<SqlFunctionId> executeAndGetRemovedSessionFunctions(String sqlString
8685
DropFunction statement = (DropFunction) parser.createStatement(sqlString, ParsingOptions.builder().build());
8786
TransactionManager tm = createTestTransactionManager();
8887
QueryStateMachine stateMachine = createQueryStateMachine(sqlString, session, false, tm, executorService, metadataManager);
89-
WarningCollector warningCollector = stateMachine.getWarningCollector();
90-
DropFunctionTask dropFunctionTask = new DropFunctionTask(parser);
91-
dropFunctionTask.execute(statement, tm, metadataManager, new AllowAllAccessControl(), session, emptyList(), warningCollector);
92-
return dropFunctionTask.getRemovedSessionFunctions();
88+
new DropFunctionTask(parser).execute(statement, tm, metadataManager, new AllowAllAccessControl(), stateMachine, emptyList());
89+
return stateMachine.getRemovedSessionFunctions();
9390
}
9491
}

‎presto-spark-base/src/test/java/com/facebook/presto/spark/TestPrestoSparkQueryRunner.java‎

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1302,16 +1302,6 @@ public void testDropColumn()
13021302
assertQuerySucceeds("DROP TABLE test_drop_column");
13031303
}
13041304

1305-
@Test
1306-
public void testCreateFunction()
1307-
{
1308-
assertQuerySucceeds("CREATE FUNCTION unittest.memory.tan (x int) RETURNS double COMMENT 'tangent trigonometric function' LANGUAGE SQL DETERMINISTIC CALLED ON NULL INPUT RETURN sin(x) / cos(x)");
1309-
MaterializedResult actual = computeActual("select unittest.memory.tan(5)");
1310-
assertEquals("-3.380515006246586", actual.getOnlyValue().toString());
1311-
//PoS currently supports one query per session. So temporary function created is not discoverable by another query.
1312-
assertQuerySucceeds("CREATE TEMPORARY FUNCTION foo() RETURNS int RETURN 1");
1313-
}
1314-
13151305
@Test
13161306
public void testCreateType()
13171307
{

0 commit comments

Comments
 (0)