Skip to content

Commit

Permalink
Ensure Parameters are in proper order for queries having WITH clause
Browse files Browse the repository at this point in the history 
Cherry-pick of trinodb/trino#1529

Fixes #17012

Co-authored-by: praveenkrishna <praveenkrishna@tutanota.com>
  • Loading branch information
@v-jizhang @Praveen2112 @zhenxiao
2 people authored and zhenxiao committed Mar 1, 2022
1 parent dd673a9 commit 25b2f25
Show file tree
Hide file tree
Showing 58 changed files with 268 additions and 115 deletions.
2 changes: 1 addition & 1 deletion presto-druid/src/test/java/com/facebook/presto/druid/TestDruidQueryBase.java
View file
Expand Up @@ -197,7 +197,7 @@ protected RowExpression toRowExpression(Expression expression, Session session)
new SqlParser(),
typeProvider,
expression,
ImmutableList.of(),
ImmutableMap.of(),
WarningCollector.NOOP);
return SqlToRowExpressionTranslator.translate(expression, expressionTypes, ImmutableMap.of(), functionAndTypeManager, session);
}
Expand Down
7 changes: 4 additions & 3 deletions presto-main/src/main/java/com/facebook/presto/cost/FilterStatsCalculator.java
View file
Expand Up @@ -55,6 +55,7 @@
import com.facebook.presto.sql.tree.SymbolReference;
import com.google.common.base.VerifyException;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;

import javax.annotation.Nullable;
import javax.inject.Inject;
Expand Down Expand Up @@ -96,7 +97,7 @@
import static java.lang.Double.isNaN;
import static java.lang.Double.min;
import static java.lang.String.format;
import static java.util.Collections.emptyList;
import static java.util.Collections.emptyMap;
import static java.util.Objects.requireNonNull;

public class FilterStatsCalculator
Expand Down Expand Up @@ -181,7 +182,7 @@ private Map<NodeRef<Expression>, Type> getExpressionTypes(Session session, Expre
metadata.getFunctionAndTypeManager(),
session,
types,
emptyList(),
emptyMap(),
node -> new IllegalStateException("Unexpected node: %s" + node),
WarningCollector.NOOP,
false);
Expand Down Expand Up @@ -463,7 +464,7 @@ private Type getType(Expression expression)
metadata.getFunctionAndTypeManager(),
session,
types,
ImmutableList.of(),
ImmutableMap.of(),
// At this stage, there should be no subqueries in the plan.
node -> new VerifyException("Unexpected subquery"),
WarningCollector.NOOP,
Expand Down
8 changes: 4 additions & 4 deletions presto-main/src/main/java/com/facebook/presto/cost/ScalarStatsCalculator.java
View file
Expand Up @@ -49,7 +49,7 @@
import com.facebook.presto.sql.tree.NullLiteral;
import com.facebook.presto.sql.tree.SymbolReference;
import com.facebook.presto.type.TypeUtils;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;

import javax.inject.Inject;

Expand All @@ -72,7 +72,7 @@
import static java.lang.Double.isNaN;
import static java.lang.Math.abs;
import static java.lang.String.format;
import static java.util.Collections.emptyList;
import static java.util.Collections.emptyMap;
import static java.util.Objects.requireNonNull;

public class ScalarStatsCalculator
Expand Down Expand Up @@ -356,7 +356,7 @@ protected VariableStatsEstimate visitNullLiteral(NullLiteral node, Void context)
protected VariableStatsEstimate visitLiteral(Literal node, Void context)
{
Object value = evaluate(metadata, session.toConnectorSession(), node);
Type type = ExpressionAnalyzer.createConstantAnalyzer(metadata, session, ImmutableList.of(), WarningCollector.NOOP).analyze(node, Scope.create());
Type type = ExpressionAnalyzer.createConstantAnalyzer(metadata, session, ImmutableMap.of(), WarningCollector.NOOP).analyze(node, Scope.create());
OptionalDouble doubleValue = toStatsRepresentation(metadata, session, type, value);
VariableStatsEstimate.Builder estimate = VariableStatsEstimate.builder()
.setNullsFraction(0)
Expand Down Expand Up @@ -398,7 +398,7 @@ private Map<NodeRef<Expression>, Type> getExpressionTypes(Session session, Expre
metadata.getFunctionAndTypeManager(),
session,
types,
emptyList(),
emptyMap(),
node -> new IllegalStateException("Unexpected node: %s" + node),
WarningCollector.NOOP,
false);
Expand Down
4 changes: 2 additions & 2 deletions presto-main/src/main/java/com/facebook/presto/cost/ValuesStatsRule.java
View file
Expand Up @@ -22,7 +22,7 @@
import com.facebook.presto.spi.relation.VariableReferenceExpression;
import com.facebook.presto.sql.planner.TypeProvider;
import com.facebook.presto.sql.planner.iterative.Lookup;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;

import java.util.List;
import java.util.Objects;
Expand Down Expand Up @@ -87,7 +87,7 @@ private List<Object> getVariableValues(ValuesNode valuesNode, int symbolId, Sess
.map(row -> row.get(symbolId))
.map(rowExpression -> {
if (isExpression(rowExpression)) {
return evaluateConstantExpression(castToExpression(rowExpression), type, metadata, session, ImmutableList.of());
return evaluateConstantExpression(castToExpression(rowExpression), type, metadata, session, ImmutableMap.of());
}
return evaluateConstantRowExpression(rowExpression, metadata, session.toConnectorSession());
})
Expand Down
3 changes: 2 additions & 1 deletion presto-main/src/main/java/com/facebook/presto/execution/AddColumnTask.java
View file
Expand Up @@ -42,6 +42,7 @@
import static com.facebook.presto.spi.StandardErrorCode.NOT_FOUND;
import static com.facebook.presto.spi.connector.ConnectorCapabilities.NOT_NULL_COLUMN_CONSTRAINT;
import static com.facebook.presto.sql.NodeUtils.mapFromProperties;
import static com.facebook.presto.sql.ParameterUtils.parameterExtractor;
import static com.facebook.presto.sql.analyzer.SemanticErrorCode.COLUMN_ALREADY_EXISTS;
import static com.facebook.presto.sql.analyzer.SemanticErrorCode.MISSING_TABLE;
import static com.facebook.presto.sql.analyzer.SemanticErrorCode.NOT_SUPPORTED;
Expand Down Expand Up @@ -113,7 +114,7 @@ public ListenableFuture<?> execute(AddColumn statement, TransactionManager trans
sqlProperties,
session,
metadata,
parameters);
parameterExtractor(statement, parameters));

ColumnMetadata column = new ColumnMetadata(
element.getName().getValue(),
Expand Down
7 changes: 6 additions & 1 deletion presto-main/src/main/java/com/facebook/presto/execution/AlterFunctionTask.java
View file
Expand Up @@ -25,15 +25,19 @@
import com.facebook.presto.sql.parser.SqlParser;
import com.facebook.presto.sql.tree.AlterFunction;
import com.facebook.presto.sql.tree.Expression;
import com.facebook.presto.sql.tree.NodeRef;
import com.facebook.presto.sql.tree.Parameter;
import com.facebook.presto.transaction.TransactionManager;
import com.google.common.util.concurrent.ListenableFuture;

import javax.inject.Inject;

import java.util.List;
import java.util.Map;
import java.util.Optional;

import static com.facebook.presto.metadata.FunctionAndTypeManager.qualifyObjectName;
import static com.facebook.presto.sql.ParameterUtils.parameterExtractor;
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.util.concurrent.Futures.immediateFuture;
import static java.util.Objects.requireNonNull;
Expand Down Expand Up @@ -64,7 +68,8 @@ public String explain(AlterFunction statement, List<Expression> parameters)
@Override
public ListenableFuture<?> execute(AlterFunction statement, TransactionManager transactionManager, Metadata metadata, AccessControl accessControl, Session session, List<Expression> parameters, WarningCollector warningCollector)
{
Analyzer analyzer = new Analyzer(session, metadata, sqlParser, accessControl, Optional.empty(), parameters, warningCollector);
Map<NodeRef<Parameter>, Expression> parameterLookup = parameterExtractor(statement, parameters);
Analyzer analyzer = new Analyzer(session, metadata, sqlParser, accessControl, Optional.empty(), parameters, parameterLookup, warningCollector);
analyzer.analyze(statement);

QualifiedObjectName functionName = qualifyObjectName(statement.getFunctionName());
Expand Down
8 changes: 6 additions & 2 deletions presto-main/src/main/java/com/facebook/presto/execution/CallTask.java
View file
Expand Up @@ -31,6 +31,8 @@
import com.facebook.presto.sql.tree.CallArgument;
import com.facebook.presto.sql.tree.Expression;
import com.facebook.presto.sql.tree.ExpressionTreeRewriter;
import com.facebook.presto.sql.tree.NodeRef;
import com.facebook.presto.sql.tree.Parameter;
import com.facebook.presto.transaction.TransactionManager;
import com.google.common.util.concurrent.ListenableFuture;

Expand All @@ -51,6 +53,7 @@
import static com.facebook.presto.spi.StandardErrorCode.INVALID_PROCEDURE_DEFINITION;
import static com.facebook.presto.spi.StandardErrorCode.NOT_SUPPORTED;
import static com.facebook.presto.spi.StandardErrorCode.PROCEDURE_CALL_FAILED;
import static com.facebook.presto.sql.ParameterUtils.parameterExtractor;
import static com.facebook.presto.sql.analyzer.SemanticErrorCode.INVALID_PROCEDURE_ARGUMENTS;
import static com.facebook.presto.sql.analyzer.SemanticErrorCode.MISSING_CATALOG;
import static com.facebook.presto.sql.planner.ExpressionInterpreter.evaluateConstantExpression;
Expand Down Expand Up @@ -128,16 +131,17 @@ else if (i < procedure.getArguments().size()) {

// get argument values
Object[] values = new Object[procedure.getArguments().size()];
Map<NodeRef<Parameter>, Expression> parameterLookup = parameterExtractor(call, parameters);
for (Entry<String, CallArgument> entry : names.entrySet()) {
CallArgument callArgument = entry.getValue();
int index = positions.get(entry.getKey());
Argument argument = procedure.getArguments().get(index);

Expression expression = ExpressionTreeRewriter.rewriteWith(new ParameterRewriter(parameters), callArgument.getValue());
Expression expression = ExpressionTreeRewriter.rewriteWith(new ParameterRewriter(parameterLookup), callArgument.getValue());
Type type = metadata.getType(argument.getType());
checkCondition(type != null, INVALID_PROCEDURE_DEFINITION, "Unknown procedure argument type: %s", argument.getType());

Object value = evaluateConstantExpression(expression, type, metadata, session, parameters);
Object value = evaluateConstantExpression(expression, type, metadata, session, parameterLookup);

values[index] = toTypeObjectValue(session, type, value);
}
Expand Down
5 changes: 4 additions & 1 deletion presto-main/src/main/java/com/facebook/presto/execution/CreateFunctionTask.java
View file
Expand Up @@ -34,6 +34,7 @@
import com.facebook.presto.sql.tree.Expression;
import com.facebook.presto.sql.tree.ExpressionRewriter;
import com.facebook.presto.sql.tree.ExpressionTreeRewriter;
import com.facebook.presto.sql.tree.NodeRef;
import com.facebook.presto.sql.tree.Return;
import com.facebook.presto.sql.tree.RoutineBody;
import com.facebook.presto.transaction.TransactionManager;
Expand All @@ -53,6 +54,7 @@
import static com.facebook.presto.spi.StandardErrorCode.ALREADY_EXISTS;
import static com.facebook.presto.spi.StandardErrorCode.NOT_SUPPORTED;
import static com.facebook.presto.spi.function.FunctionVersion.notVersioned;
import static com.facebook.presto.sql.ParameterUtils.parameterExtractor;
import static com.facebook.presto.sql.SqlFormatter.formatSql;
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.util.concurrent.Futures.immediateFuture;
Expand Down Expand Up @@ -86,7 +88,8 @@ public String explain(CreateFunction statement, List<Expression> parameters)
@Override
public ListenableFuture<?> execute(CreateFunction statement, TransactionManager transactionManager, Metadata metadata, AccessControl accessControl, Session session, List<Expression> parameters, WarningCollector warningCollector)
{
Analyzer analyzer = new Analyzer(session, metadata, sqlParser, accessControl, Optional.empty(), parameters, warningCollector);
Map<NodeRef<com.facebook.presto.sql.tree.Parameter>, Expression> parameterLookup = parameterExtractor(statement, parameters);
Analyzer analyzer = new Analyzer(session, metadata, sqlParser, accessControl, Optional.empty(), parameters, parameterLookup, warningCollector);
Analysis analysis = analyzer.analyze(statement);
if (analysis.getFunctionHandles().values().stream()
.anyMatch(SqlFunctionHandle.class::isInstance)) {
Expand Down
8 changes: 6 additions & 2 deletions presto-main/src/main/java/com/facebook/presto/execution/CreateMaterializedViewTask.java
View file
Expand Up @@ -32,6 +32,8 @@
import com.facebook.presto.sql.parser.SqlParser;
import com.facebook.presto.sql.tree.CreateMaterializedView;
import com.facebook.presto.sql.tree.Expression;
import com.facebook.presto.sql.tree.NodeRef;
import com.facebook.presto.sql.tree.Parameter;
import com.facebook.presto.transaction.TransactionManager;
import com.google.common.util.concurrent.ListenableFuture;

Expand All @@ -46,6 +48,7 @@
import static com.facebook.presto.spi.StandardErrorCode.ALREADY_EXISTS;
import static com.facebook.presto.spi.StandardErrorCode.NOT_FOUND;
import static com.facebook.presto.sql.NodeUtils.mapFromProperties;
import static com.facebook.presto.sql.ParameterUtils.parameterExtractor;
import static com.facebook.presto.sql.SqlFormatterUtil.getFormattedSql;
import static com.facebook.presto.sql.analyzer.SemanticErrorCode.MATERIALIZED_VIEW_ALREADY_EXISTS;
import static com.facebook.presto.sql.analyzer.SemanticErrorCode.NOT_SUPPORTED;
Expand Down Expand Up @@ -86,7 +89,8 @@ public ListenableFuture<?> execute(CreateMaterializedView statement, Transaction
accessControl.checkCanCreateTable(session.getRequiredTransactionId(), session.getIdentity(), session.getAccessControlContext(), viewName);
accessControl.checkCanCreateView(session.getRequiredTransactionId(), session.getIdentity(), session.getAccessControlContext(), viewName);

Analyzer analyzer = new Analyzer(session, metadata, sqlParser, accessControl, Optional.empty(), parameters, warningCollector);
Map<NodeRef<Parameter>, Expression> parameterLookup = parameterExtractor(statement, parameters);
Analyzer analyzer = new Analyzer(session, metadata, sqlParser, accessControl, Optional.empty(), parameters, parameterLookup, warningCollector);
Analysis analysis = analyzer.analyze(statement);

ConnectorId connectorId = metadata.getCatalogHandle(session, viewName.getCatalogName())
Expand All @@ -103,7 +107,7 @@ public ListenableFuture<?> execute(CreateMaterializedView statement, Transaction
sqlProperties,
session,
metadata,
parameters);
parameterLookup);

ConnectorTableMetadata viewMetadata = new ConnectorTableMetadata(
toSchemaTableName(viewName),
Expand Down
3 changes: 2 additions & 1 deletion presto-main/src/main/java/com/facebook/presto/execution/CreateSchemaTask.java
View file
Expand Up @@ -33,6 +33,7 @@
import static com.facebook.presto.metadata.MetadataUtil.createCatalogSchemaName;
import static com.facebook.presto.spi.StandardErrorCode.NOT_FOUND;
import static com.facebook.presto.sql.NodeUtils.mapFromProperties;
import static com.facebook.presto.sql.ParameterUtils.parameterExtractor;
import static com.facebook.presto.sql.analyzer.SemanticErrorCode.SCHEMA_ALREADY_EXISTS;
import static com.google.common.util.concurrent.Futures.immediateFuture;

Expand Down Expand Up @@ -76,7 +77,7 @@ public ListenableFuture<?> execute(CreateSchema statement, TransactionManager tr
mapFromProperties(statement.getProperties()),
session,
metadata,
parameters);
parameterExtractor(statement, parameters));

metadata.createSchema(session, schema, properties);

Expand Down
8 changes: 6 additions & 2 deletions presto-main/src/main/java/com/facebook/presto/execution/CreateTableTask.java
View file
Expand Up @@ -30,6 +30,8 @@
import com.facebook.presto.sql.tree.CreateTable;
import com.facebook.presto.sql.tree.Expression;
import com.facebook.presto.sql.tree.LikeClause;
import com.facebook.presto.sql.tree.NodeRef;
import com.facebook.presto.sql.tree.Parameter;
import com.facebook.presto.sql.tree.TableElement;
import com.facebook.presto.transaction.TransactionManager;
import com.google.common.annotations.VisibleForTesting;
Expand All @@ -54,6 +56,7 @@
import static com.facebook.presto.spi.StandardErrorCode.NOT_FOUND;
import static com.facebook.presto.spi.connector.ConnectorCapabilities.NOT_NULL_COLUMN_CONSTRAINT;
import static com.facebook.presto.sql.NodeUtils.mapFromProperties;
import static com.facebook.presto.sql.ParameterUtils.parameterExtractor;
import static com.facebook.presto.sql.analyzer.SemanticErrorCode.DUPLICATE_COLUMN_NAME;
import static com.facebook.presto.sql.analyzer.SemanticErrorCode.MISSING_CATALOG;
import static com.facebook.presto.sql.analyzer.SemanticErrorCode.MISSING_TABLE;
Expand Down Expand Up @@ -89,6 +92,7 @@ public ListenableFuture<?> internalExecute(CreateTable statement, Metadata metad
{
checkArgument(!statement.getElements().isEmpty(), "no columns for table");

Map<NodeRef<Parameter>, Expression> parameterLookup = parameterExtractor(statement, parameters);
QualifiedObjectName tableName = createQualifiedObjectName(session, statement, statement.getName());
Optional<TableHandle> tableHandle = metadata.getTableHandle(session, tableName);
if (tableHandle.isPresent()) {
Expand Down Expand Up @@ -132,7 +136,7 @@ public ListenableFuture<?> internalExecute(CreateTable statement, Metadata metad
sqlProperties,
session,
metadata,
parameters);
parameterLookup);

columns.put(name, new ColumnMetadata(
name,
Expand Down Expand Up @@ -188,7 +192,7 @@ else if (element instanceof LikeClause) {
sqlProperties,
session,
metadata,
parameters);
parameterLookup);

Map<String, Object> finalProperties = combineProperties(sqlProperties.keySet(), properties, inheritedProperties);

Expand Down
3 changes: 2 additions & 1 deletion presto-main/src/main/java/com/facebook/presto/execution/CreateViewTask.java
View file
Expand Up @@ -40,6 +40,7 @@
import static com.facebook.presto.metadata.MetadataUtil.createQualifiedObjectName;
import static com.facebook.presto.metadata.MetadataUtil.toSchemaTableName;
import static com.facebook.presto.metadata.ViewDefinition.ViewColumn;
import static com.facebook.presto.sql.ParameterUtils.parameterExtractor;
import static com.facebook.presto.sql.SqlFormatterUtil.getFormattedSql;
import static com.facebook.presto.sql.tree.CreateView.Security.INVOKER;
import static com.google.common.collect.ImmutableList.toImmutableList;
Expand Down Expand Up @@ -111,7 +112,7 @@ public ListenableFuture<?> execute(CreateView statement, TransactionManager tran

private Analysis analyzeStatement(Statement statement, Session session, Metadata metadata, AccessControl accessControl, List<Expression> parameters, WarningCollector warningCollector)
{
Analyzer analyzer = new Analyzer(session, metadata, sqlParser, accessControl, Optional.empty(), parameters, warningCollector);
Analyzer analyzer = new Analyzer(session, metadata, sqlParser, accessControl, Optional.empty(), parameters, parameterExtractor(statement, parameters), warningCollector);
return analyzer.analyze(statement);
}
}
7 changes: 6 additions & 1 deletion presto-main/src/main/java/com/facebook/presto/execution/DropFunctionTask.java
View file
Expand Up @@ -25,6 +25,8 @@
import com.facebook.presto.sql.parser.SqlParser;
import com.facebook.presto.sql.tree.DropFunction;
import com.facebook.presto.sql.tree.Expression;
import com.facebook.presto.sql.tree.NodeRef;
import com.facebook.presto.sql.tree.Parameter;
import com.facebook.presto.transaction.TransactionManager;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.Sets;
Expand All @@ -33,12 +35,14 @@
import javax.inject.Inject;

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

import static com.facebook.presto.metadata.FunctionAndTypeManager.qualifyObjectName;
import static com.facebook.presto.metadata.SessionFunctionHandle.SESSION_NAMESPACE;
import static com.facebook.presto.spi.StandardErrorCode.NOT_FOUND;
import static com.facebook.presto.sql.ParameterUtils.parameterExtractor;
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.util.concurrent.Futures.immediateFuture;
import static java.lang.String.format;
Expand Down Expand Up @@ -72,7 +76,8 @@ public String explain(DropFunction statement, List<Expression> parameters)
@Override
public ListenableFuture<?> execute(DropFunction statement, TransactionManager transactionManager, Metadata metadata, AccessControl accessControl, Session session, List<Expression> parameters, WarningCollector warningCollector)
{
Analyzer analyzer = new Analyzer(session, metadata, sqlParser, accessControl, Optional.empty(), parameters, warningCollector);
Map<NodeRef<Parameter>, Expression> parameterLookup = parameterExtractor(statement, parameters);
Analyzer analyzer = new Analyzer(session, metadata, sqlParser, accessControl, Optional.empty(), parameters, parameterLookup, warningCollector);
analyzer.analyze(statement);
Optional<List<TypeSignature>> parameterTypes = statement.getParameterTypes().map(types -> types.stream().map(TypeSignature::parseTypeSignature).collect(toImmutableList()));

Expand Down

0 comments on commit 25b2f25

Please sign in to comment.