Skip to content
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

Fix incorrect order for parameters in DESCRIBE INPUT #14914

Merged
merged 5 commits into from
Nov 8, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -37,14 +37,14 @@
import java.util.Map;

import static com.google.common.util.concurrent.Futures.immediateVoidFuture;
import static io.trino.execution.ParameterExtractor.bindParameters;
import static io.trino.metadata.MetadataUtil.createQualifiedObjectName;
import static io.trino.spi.StandardErrorCode.COLUMN_ALREADY_EXISTS;
import static io.trino.spi.StandardErrorCode.COLUMN_TYPE_UNKNOWN;
import static io.trino.spi.StandardErrorCode.NOT_SUPPORTED;
import static io.trino.spi.StandardErrorCode.TABLE_NOT_FOUND;
import static io.trino.spi.StandardErrorCode.TYPE_NOT_FOUND;
import static io.trino.spi.connector.ConnectorCapabilities.NOT_NULL_COLUMN_CONSTRAINT;
import static io.trino.sql.ParameterUtils.parameterExtractor;
import static io.trino.sql.analyzer.SemanticExceptions.semanticException;
import static io.trino.sql.analyzer.TypeSignatureTranslator.toTypeSignature;
import static io.trino.type.UnknownType.UNKNOWN;
Expand Down Expand Up @@ -122,7 +122,7 @@ public ListenableFuture<Void> execute(
session,
plannerContext,
accessControl,
parameterExtractor(statement, parameters),
bindParameters(statement, parameters),
true);

ColumnMetadata column = ColumnMetadata.builder()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,14 +55,14 @@
import static com.google.common.base.Throwables.throwIfInstanceOf;
import static com.google.common.base.Verify.verify;
import static com.google.common.util.concurrent.Futures.immediateVoidFuture;
import static io.trino.execution.ParameterExtractor.bindParameters;
import static io.trino.metadata.MetadataUtil.createQualifiedObjectName;
import static io.trino.metadata.MetadataUtil.getRequiredCatalogHandle;
import static io.trino.spi.StandardErrorCode.INVALID_ARGUMENTS;
import static io.trino.spi.StandardErrorCode.INVALID_PROCEDURE_ARGUMENT;
import static io.trino.spi.StandardErrorCode.NOT_SUPPORTED;
import static io.trino.spi.StandardErrorCode.PROCEDURE_CALL_FAILED;
import static io.trino.spi.type.TypeUtils.writeNativeValue;
import static io.trino.sql.ParameterUtils.parameterExtractor;
import static io.trino.sql.analyzer.SemanticExceptions.semanticException;
import static io.trino.sql.planner.ExpressionInterpreter.evaluateConstantExpression;
import static java.util.Arrays.asList;
Expand Down Expand Up @@ -156,7 +156,7 @@ else if (i < procedure.getArguments().size()) {

// get argument values
Object[] values = new Object[procedure.getArguments().size()];
Map<NodeRef<Parameter>, Expression> parameterLookup = parameterExtractor(call, parameters);
Map<NodeRef<Parameter>, Expression> parameterLookup = bindParameters(call, parameters);
for (Entry<String, CallArgument> entry : names.entrySet()) {
CallArgument callArgument = entry.getValue();
int index = positions.get(entry.getKey());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,9 @@

import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.util.concurrent.Futures.immediateVoidFuture;
import static io.trino.execution.ParameterExtractor.bindParameters;
import static io.trino.metadata.MetadataUtil.createQualifiedObjectName;
import static io.trino.metadata.MetadataUtil.getRequiredCatalogHandle;
import static io.trino.sql.ParameterUtils.parameterExtractor;
import static io.trino.sql.SqlFormatterUtil.getFormattedSql;
import static java.util.Objects.requireNonNull;

Expand Down Expand Up @@ -84,7 +84,7 @@ public ListenableFuture<Void> execute(
{
Session session = stateMachine.getSession();
QualifiedObjectName name = createQualifiedObjectName(session, statement, statement.getName());
Map<NodeRef<Parameter>, Expression> parameterLookup = parameterExtractor(statement, parameters);
Map<NodeRef<Parameter>, Expression> parameterLookup = bindParameters(statement, parameters);

String sql = getFormattedSql(statement.getQuery(), sqlParser);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,13 @@
import java.util.Optional;

import static com.google.common.util.concurrent.Futures.immediateVoidFuture;
import static io.trino.execution.ParameterExtractor.bindParameters;
import static io.trino.metadata.MetadataUtil.checkRoleExists;
import static io.trino.metadata.MetadataUtil.createCatalogSchemaName;
import static io.trino.metadata.MetadataUtil.createPrincipal;
import static io.trino.metadata.MetadataUtil.getRequiredCatalogHandle;
import static io.trino.spi.StandardErrorCode.ALREADY_EXISTS;
import static io.trino.spi.StandardErrorCode.SCHEMA_ALREADY_EXISTS;
import static io.trino.sql.ParameterUtils.parameterExtractor;
import static io.trino.sql.analyzer.SemanticExceptions.semanticException;
import static java.util.Objects.requireNonNull;

Expand Down Expand Up @@ -109,7 +109,7 @@ static ListenableFuture<Void> internalExecute(
session,
plannerContext,
accessControl,
parameterExtractor(statement, parameters),
bindParameters(statement, parameters),
true);

TrinoPrincipal principal = getCreatePrincipal(statement, session, plannerContext.getMetadata(), catalogName);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@
import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.collect.ImmutableSet.toImmutableSet;
import static com.google.common.util.concurrent.Futures.immediateVoidFuture;
import static io.trino.execution.ParameterExtractor.bindParameters;
import static io.trino.metadata.MetadataUtil.createQualifiedObjectName;
import static io.trino.metadata.MetadataUtil.getRequiredCatalogHandle;
import static io.trino.spi.StandardErrorCode.ALREADY_EXISTS;
Expand All @@ -73,7 +74,6 @@
import static io.trino.spi.StandardErrorCode.TYPE_NOT_FOUND;
import static io.trino.spi.StandardErrorCode.UNSUPPORTED_TABLE_TYPE;
import static io.trino.spi.connector.ConnectorCapabilities.NOT_NULL_COLUMN_CONSTRAINT;
import static io.trino.sql.ParameterUtils.parameterExtractor;
import static io.trino.sql.analyzer.SemanticExceptions.semanticException;
import static io.trino.sql.analyzer.TypeSignatureTranslator.toTypeSignature;
import static io.trino.sql.tree.LikeClause.PropertiesOption.EXCLUDING;
Expand Down Expand Up @@ -124,7 +124,7 @@ ListenableFuture<Void> internalExecute(CreateTable statement, Session session, L
{
checkArgument(!statement.getElements().isEmpty(), "no columns for table");

Map<NodeRef<Parameter>, Expression> parameterLookup = parameterExtractor(statement, parameters);
Map<NodeRef<Parameter>, Expression> parameterLookup = bindParameters(statement, parameters);
QualifiedObjectName tableName = createQualifiedObjectName(session, statement, statement.getName());
Optional<TableHandle> tableHandle;
try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,9 @@

import static com.google.common.collect.ImmutableList.toImmutableList;
import static com.google.common.util.concurrent.Futures.immediateVoidFuture;
import static io.trino.execution.ParameterExtractor.bindParameters;
import static io.trino.metadata.MetadataUtil.createQualifiedObjectName;
import static io.trino.spi.StandardErrorCode.TABLE_ALREADY_EXISTS;
import static io.trino.sql.ParameterUtils.parameterExtractor;
import static io.trino.sql.SqlFormatterUtil.getFormattedSql;
import static io.trino.sql.analyzer.SemanticExceptions.semanticException;
import static io.trino.sql.tree.CreateView.Security.INVOKER;
Expand Down Expand Up @@ -92,7 +92,7 @@ else if (metadata.getTableHandle(session, name).isPresent()) {

String sql = getFormattedSql(statement.getQuery(), sqlParser);

Analysis analysis = analyzerFactory.createAnalyzer(session, parameters, parameterExtractor(statement, parameters), stateMachine.getWarningCollector())
Analysis analysis = analyzerFactory.createAnalyzer(session, parameters, bindParameters(statement, parameters), stateMachine.getWarningCollector())
.analyze(statement);

List<ViewColumn> columns = analysis.getOutputDescriptor(statement.getQuery())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,27 +13,53 @@
*/
package io.trino.execution;

import com.google.common.collect.ImmutableMap;
import io.trino.sql.tree.DefaultTraversalVisitor;
import io.trino.sql.tree.Expression;
import io.trino.sql.tree.NodeLocation;
import io.trino.sql.tree.NodeRef;
import io.trino.sql.tree.Parameter;
import io.trino.sql.tree.Statement;

import java.util.ArrayList;
import java.util.Comparator;
import java.util.Iterator;
import java.util.List;
import java.util.Map;

import static com.google.common.collect.ImmutableList.toImmutableList;

public final class ParameterExtractor
{
private ParameterExtractor() {}

public static int getParameterCount(Statement statement)
{
return getParameters(statement).size();
return extractParameters(statement).size();
}

public static List<Parameter> getParameters(Statement statement)
public static List<Parameter> extractParameters(Statement statement)
{
ParameterExtractingVisitor parameterExtractingVisitor = new ParameterExtractingVisitor();
parameterExtractingVisitor.process(statement, null);
return parameterExtractingVisitor.getParameters();
return parameterExtractingVisitor.getParameters().stream()
.sorted(Comparator.comparing(
parameter -> parameter.getLocation().get(),
Comparator.comparing(NodeLocation::getLineNumber)
.thenComparing(NodeLocation::getColumnNumber)))
.collect(toImmutableList());
}

public static Map<NodeRef<Parameter>, Expression> bindParameters(Statement statement, List<Expression> values)
{
List<Parameter> parametersList = extractParameters(statement);

ImmutableMap.Builder<NodeRef<Parameter>, Expression> builder = ImmutableMap.builder();
Iterator<Expression> iterator = values.iterator();
for (Parameter parameter : parametersList) {
builder.put(NodeRef.of(parameter), iterator.next());
}
return builder.buildOrThrow();
}

private static class ParameterExtractingVisitor
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,11 @@
import java.util.Optional;

import static com.google.common.util.concurrent.Futures.immediateVoidFuture;
import static io.trino.execution.ParameterExtractor.bindParameters;
import static io.trino.metadata.MetadataUtil.createQualifiedObjectName;
import static io.trino.metadata.MetadataUtil.getRequiredCatalogHandle;
import static io.trino.spi.StandardErrorCode.NOT_SUPPORTED;
import static io.trino.spi.StandardErrorCode.TABLE_NOT_FOUND;
import static io.trino.sql.ParameterUtils.parameterExtractor;
import static io.trino.sql.analyzer.SemanticExceptions.semanticException;
import static io.trino.sql.tree.SetProperties.Type.MATERIALIZED_VIEW;
import static io.trino.sql.tree.SetProperties.Type.TABLE;
Expand Down Expand Up @@ -85,7 +85,7 @@ public ListenableFuture<Void> execute(
session,
plannerContext,
accessControl,
parameterExtractor(statement, parameters),
bindParameters(statement, parameters),
false);
setTableProperties(statement, objectName, session, properties);
}
Expand All @@ -97,7 +97,7 @@ else if (statement.getType() == MATERIALIZED_VIEW) {
session,
plannerContext,
accessControl,
parameterExtractor(statement, parameters),
bindParameters(statement, parameters),
false);
setMaterializedViewProperties(statement, objectName, session, properties);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,11 @@
import java.util.List;

import static com.google.common.util.concurrent.Futures.immediateVoidFuture;
import static io.trino.execution.ParameterExtractor.bindParameters;
import static io.trino.metadata.MetadataUtil.getRequiredCatalogHandle;
import static io.trino.metadata.SessionPropertyManager.evaluatePropertyValue;
import static io.trino.metadata.SessionPropertyManager.serializeSessionProperty;
import static io.trino.spi.StandardErrorCode.INVALID_SESSION_PROPERTY;
import static io.trino.sql.ParameterUtils.parameterExtractor;
import static io.trino.sql.analyzer.SemanticExceptions.semanticException;
import static java.lang.String.format;
import static java.util.Objects.requireNonNull;
Expand Down Expand Up @@ -95,7 +95,7 @@ public ListenableFuture<Void> execute(
Object objectValue;

try {
objectValue = evaluatePropertyValue(statement.getValue(), type, session, plannerContext, accessControl, parameterExtractor(statement, parameters));
objectValue = evaluatePropertyValue(statement.getValue(), type, session, plannerContext, accessControl, bindParameters(statement, parameters));
}
catch (TrinoException e) {
throw new TrinoException(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,11 @@

import static com.google.common.util.concurrent.Futures.immediateVoidFuture;
import static io.trino.SystemSessionProperties.TIME_ZONE_ID;
import static io.trino.execution.ParameterExtractor.bindParameters;
import static io.trino.spi.StandardErrorCode.INVALID_LITERAL;
import static io.trino.spi.StandardErrorCode.TYPE_MISMATCH;
import static io.trino.spi.type.TimeZoneKey.getTimeZoneKey;
import static io.trino.spi.type.TimeZoneKey.getTimeZoneKeyForOffset;
import static io.trino.sql.ParameterUtils.parameterExtractor;
import static io.trino.sql.analyzer.ExpressionAnalyzer.createConstantAnalyzer;
import static io.trino.sql.planner.ExpressionInterpreter.evaluateConstantExpression;
import static io.trino.util.Failures.checkCondition;
Expand Down Expand Up @@ -91,7 +91,7 @@ private String getTimeZoneId(
List<Expression> parameters,
WarningCollector warningCollector)
{
Map<NodeRef<Parameter>, Expression> parameterLookup = parameterExtractor(statement, parameters);
Map<NodeRef<Parameter>, Expression> parameterLookup = bindParameters(statement, parameters);
ExpressionAnalyzer analyzer = createConstantAnalyzer(
plannerContext,
accessControl,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,11 +92,11 @@
import static io.trino.SystemSessionProperties.getTaskRetryAttemptsPerTask;
import static io.trino.SystemSessionProperties.isEnableDynamicFiltering;
import static io.trino.SystemSessionProperties.isFaultTolerantExecutionEventDriverSchedulerEnabled;
import static io.trino.execution.ParameterExtractor.bindParameters;
import static io.trino.execution.QueryState.FAILED;
import static io.trino.execution.QueryState.PLANNING;
import static io.trino.server.DynamicFilterService.DynamicFiltersStats;
import static io.trino.spi.StandardErrorCode.STACK_OVERFLOW;
import static io.trino.sql.ParameterUtils.parameterExtractor;
import static java.lang.Thread.currentThread;
import static java.util.Objects.requireNonNull;
import static java.util.concurrent.TimeUnit.SECONDS;
Expand Down Expand Up @@ -260,7 +260,7 @@ private static Analysis analyze(
Analyzer analyzer = analyzerFactory.createAnalyzer(
stateMachine.getSession(),
preparedQuery.getParameters(),
parameterExtractor(preparedQuery.getStatement(), preparedQuery.getParameters()),
bindParameters(preparedQuery.getStatement(), preparedQuery.getParameters()),
warningCollector);
Analysis analysis;
try {
Expand Down
51 changes: 0 additions & 51 deletions core/trino-main/src/main/java/io/trino/sql/ParameterUtils.java

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -694,7 +694,7 @@ protected Boolean visitParameter(Parameter node, Void context)
return true;
}
Map<NodeRef<Parameter>, Expression> parameters = analysis.getParameters();
checkArgument(node.getPosition() < parameters.size(), "Invalid parameter number %s, max values is %s", node.getPosition(), parameters.size() - 1);
checkArgument(node.getId() < parameters.size(), "Invalid parameter number %s, max values is %s", node.getId(), parameters.size() - 1);
return process(parameters.get(NodeRef.of(node)), context);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2070,8 +2070,8 @@ protected Type visitParameter(Parameter node, StackableAstVisitorContext<Context
if (parameters.size() == 0) {
throw semanticException(INVALID_PARAMETER_USAGE, node, "Query takes no parameters");
}
if (node.getPosition() >= parameters.size()) {
throw semanticException(INVALID_PARAMETER_USAGE, node, "Invalid parameter index %s, max value is %s", node.getPosition(), parameters.size() - 1);
if (node.getId() >= parameters.size()) {
Copy link
Member

Choose a reason for hiding this comment

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

But if it is some sort of a uniqueId (which I totally agree) then comparing getId but parameters size would make less sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's sort of true, unless we define the ids as being from 0 to the number of parameters in the query. I hesitated removing these checks, but I do agree they look wonky. Let me see if there's a better way to represent them, or I'll just remove them.

throw semanticException(INVALID_PARAMETER_USAGE, node, "Invalid parameter index %s, max value is %s", node.getId(), parameters.size() - 1);
}

Expression providedValue = parameters.get(NodeRef.of(node));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,8 @@
import java.util.List;
import java.util.Optional;

import static io.trino.execution.ParameterExtractor.bindParameters;
import static io.trino.spi.StandardErrorCode.NOT_SUPPORTED;
import static io.trino.sql.ParameterUtils.parameterExtractor;
import static io.trino.sql.analyzer.QueryType.EXPLAIN;
import static io.trino.sql.planner.LogicalPlanner.Stage.OPTIMIZED_AND_VALIDATED;
import static io.trino.sql.planner.planprinter.IoPlanPrinter.textIoPlan;
Expand Down Expand Up @@ -172,7 +172,7 @@ public Plan getLogicalPlan(Session session, Statement statement, List<Expression

private Analysis analyze(Session session, Statement statement, List<Expression> parameters, WarningCollector warningCollector)
{
Analyzer analyzer = analyzerFactory.createAnalyzer(session, parameters, parameterExtractor(statement, parameters), warningCollector);
Analyzer analyzer = analyzerFactory.createAnalyzer(session, parameters, bindParameters(statement, parameters), warningCollector);
return analyzer.analyze(statement, EXPLAIN);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ protected Expression rewriteExpression(Expression node, Void context, Expression
@Override
public Expression rewriteParameter(Parameter node, Void context, ExpressionTreeRewriter<Void> treeRewriter)
{
checkState(parameters.size() > node.getPosition(), "Too few parameter values");
checkState(parameters.size() > node.getId(), "Too few parameter values");
return coerceIfNecessary(node, parameters.get(NodeRef.of(node)));
}

Expand Down
Loading