Skip to content

Annotate semanticException factories as format methods #15963

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

Closed
ksobolew wants to merge 3 commits into from
Closed

Annotate semanticException factories as format methods #15963

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
306c41a
Annotate coerceToSingleType in ExpressionAnalyzer as format method
ksobolew Feb 3, 2023
b580e3d
Add property name to error message
ksobolew Feb 3, 2023
cc1c2ce
Annotate semanticException factories as format methods
ksobolew Nov 8, 2022
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
2 changes: 1 addition & 1 deletion core/trino-main/src/main/java/io/trino/execution/SetColumnTypeTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ else if (metadata.getView(session, qualifiedObjectName).isPresent()) {
exceptionMessage += ", but a view with that name exists.";
}
if (!statement.isTableExists()) {
throw semanticException(TABLE_NOT_FOUND, statement, exceptionMessage);
throw semanticException(TABLE_NOT_FOUND, statement, "%s", exceptionMessage);
}
return immediateVoidFuture();
}
Expand Down
2 changes: 1 addition & 1 deletion core/trino-main/src/main/java/io/trino/execution/SetSessionTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ public ListenableFuture<Void> execute(
propertyMetadata.decode(objectValue);
}
catch (RuntimeException e) {
throw semanticException(INVALID_SESSION_PROPERTY, statement, e.getMessage());
throw semanticException(INVALID_SESSION_PROPERTY, statement, "Invalid value of session property '%s': %s", propertyName.toString(), e.getMessage());
Comment thread
ksobolew marked this conversation as resolved.
Outdated
}

stateMachine.addSetSessionProperties(propertyName.toString(), value);
Expand Down
5 changes: 2 additions & 3 deletions core/trino-main/src/main/java/io/trino/sql/analyzer/AggregationAnalyzer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,8 +449,7 @@ protected Boolean visitFunctionCall(FunctionCall node, Void context)
if (node.getFilter().isPresent()) {
throw semanticException(FUNCTION_NOT_AGGREGATE,
node,
"Filter is only valid for aggregation functions",
node);
"Filter is only valid for aggregation functions");
}
if (node.getOrderBy().isPresent()) {
throw semanticException(FUNCTION_NOT_AGGREGATE, node, "ORDER BY is only valid for aggregation functions");
Expand Down Expand Up @@ -803,7 +802,7 @@ private void verifyNoOrderByReferencesToOutputColumns(Node node, StandardErrorCo
getReferencesToScope(node, analysis, orderByScope.get())
.findFirst()
.ifPresent(expression -> {
throw semanticException(errorCode, expression, errorString);
throw semanticException(errorCode, expression, "%s", errorString);
});
}
}
39 changes: 21 additions & 18 deletions core/trino-main/src/main/java/io/trino/sql/analyzer/ExpressionAnalyzer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
import com.google.common.collect.LinkedHashMultimap;
import com.google.common.collect.Multimap;
import com.google.common.collect.Streams;
import com.google.errorprone.annotations.FormatMethod;
import com.google.errorprone.annotations.FormatString;
import io.trino.Session;
import io.trino.execution.warnings.WarningCollector;
import io.trino.metadata.FunctionResolver;
Expand Down Expand Up @@ -761,7 +763,7 @@ protected Type visitDereferenceExpression(DereferenceExpression node, StackableA
for (RowType.Field rowField : rowType.getFields()) {
if (fieldName.equalsIgnoreCase(rowField.getName().orElse(null))) {
if (foundFieldName) {
throw semanticException(AMBIGUOUS_NAME, field, "Ambiguous row field reference: " + fieldName);
throw semanticException(AMBIGUOUS_NAME, field, "Ambiguous row field reference: %s", fieldName);
}
foundFieldName = true;
rowFieldType = rowField.getType();
Expand Down Expand Up @@ -1248,7 +1250,7 @@ protected Type visitFunctionCall(FunctionCall node, StackableAstVisitorContext<C
Expression expression = arguments.get(0);
Type expressionType = process(expression, context);
if (!(expressionType instanceof VarcharType)) {
throw semanticException(TYPE_MISMATCH, node, format("Expected expression of varchar, but '%s' has %s type", expression, expressionType.getDisplayName()));
throw semanticException(TYPE_MISMATCH, node, "Expected expression of varchar, but '%s' has %s type", expression, expressionType.getDisplayName());
}
}

Expand Down Expand Up @@ -1488,7 +1490,7 @@ else if (frame.getType() == GROUPS) {
}
}
else {
throw semanticException(NOT_SUPPORTED, frame, "Unsupported frame type: " + frame.getType());
throw semanticException(NOT_SUPPORTED, frame, "Unsupported frame type: %s", frame.getType());
}
}
}
Expand Down Expand Up @@ -2415,7 +2417,7 @@ protected Type visitLambdaExpression(LambdaExpression node, StackableAstVisitorC

if (types.size() != lambdaArguments.size()) {
throw semanticException(INVALID_PARAMETER_USAGE, node,
format("Expected a lambda that takes %s argument(s) but got %s", types.size(), lambdaArguments.size()));
"Expected a lambda that takes %s argument(s) but got %s", types.size(), lambdaArguments.size());
}

ImmutableList.Builder<Field> fields = ImmutableList.builder();
Expand Down Expand Up @@ -2550,7 +2552,7 @@ public Type visitJsonValue(JsonValue node, StackableAstVisitorContext<Context> c
!isDateTimeType(returnedType) ||
returnedType.equals(INTERVAL_DAY_TIME) ||
returnedType.equals(INTERVAL_YEAR_MONTH)) {
throw semanticException(TYPE_MISMATCH, node, "Invalid return type of function JSON_VALUE: " + node.getReturnedType().get());
throw semanticException(TYPE_MISMATCH, node, "Invalid return type of function JSON_VALUE: %s", node.getReturnedType().get());
}

JsonPathAnalysis pathAnalysis = jsonPathAnalyses.get(NodeRef.of(node));
Expand Down Expand Up @@ -2797,7 +2799,7 @@ private ResolvedFunction getInputFunction(Type type, JsonFormat format, Node nod
if (isStringType(type)) {
yield VARBINARY_TO_JSON;
}
throw semanticException(TYPE_MISMATCH, node, format("Cannot read input of type %s as JSON using formatting %s", type, format));
throw semanticException(TYPE_MISMATCH, node, "Cannot read input of type %s as JSON using formatting %s", type, format);
}
case UTF8 -> VARBINARY_UTF8_TO_JSON;
case UTF16 -> VARBINARY_UTF16_TO_JSON;
Expand All @@ -2822,23 +2824,23 @@ private ResolvedFunction getOutputFunction(Type type, JsonFormat format, Node no
if (isStringType(type)) {
yield JSON_TO_VARBINARY;
}
throw semanticException(TYPE_MISMATCH, node, format("Cannot output JSON value as %s using formatting %s", type, format));
throw semanticException(TYPE_MISMATCH, node, "Cannot output JSON value as %s using formatting %s", type, format);
}
case UTF8 -> {
if (!VARBINARY.equals(type)) {
throw semanticException(TYPE_MISMATCH, node, format("Cannot output JSON value as %s using formatting %s", type, format));
throw semanticException(TYPE_MISMATCH, node, "Cannot output JSON value as %s using formatting %s", type, format);
}
yield JSON_TO_VARBINARY_UTF8;
}
case UTF16 -> {
if (!VARBINARY.equals(type)) {
throw semanticException(TYPE_MISMATCH, node, format("Cannot output JSON value as %s using formatting %s", type, format));
throw semanticException(TYPE_MISMATCH, node, "Cannot output JSON value as %s using formatting %s", type, format);
}
yield JSON_TO_VARBINARY_UTF16;
}
case UTF32 -> {
if (!VARBINARY.equals(type)) {
throw semanticException(TYPE_MISMATCH, node, format("Cannot output JSON value as %s using formatting %s", type, format));
throw semanticException(TYPE_MISMATCH, node, "Cannot output JSON value as %s using formatting %s", type, format);
}
yield JSON_TO_VARBINARY_UTF32;
}
Expand Down Expand Up @@ -3129,7 +3131,8 @@ private void coerceType(StackableAstVisitorContext<Context> context, Expression
coerceType(expression, actualType, expectedType, message);
}

private Type coerceToSingleType(StackableAstVisitorContext<Context> context, Node node, String message, Expression first, Expression second)
@FormatMethod
private Type coerceToSingleType(StackableAstVisitorContext<Context> context, Node node, @FormatString String message, Expression first, Expression second)
{
Type firstType = UNKNOWN;
if (first != null) {
Expand All @@ -3155,7 +3158,7 @@ private Type coerceToSingleType(StackableAstVisitorContext<Context> context, Nod
return superType;
}

throw semanticException(TYPE_MISMATCH, node, message, firstType, secondType);
throw semanticException(TYPE_MISMATCH, node, "%s", format(message, firstType, secondType));
Copy link
Copy Markdown
Member

@findepi findepi Feb 3, 2023

Choose a reason for hiding this comment

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

is it because one @FormatMethod cannot delegate to other @FormatMethod?
if so, seems like error-prone's bug to me

Copy link
Copy Markdown
Contributor Author

@ksobolew ksobolew Feb 3, 2023

Choose a reason for hiding this comment

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

It can, but looks like it gets confused by them being different types. I wasn't sure if this is a bug or a weird corner case.

Copy link
Copy Markdown
Member

@findepi findepi Feb 3, 2023

Choose a reason for hiding this comment

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

it doesn't look like we were doing anything wrong here

Copy link
Copy Markdown
Contributor Author

@ksobolew ksobolew Feb 3, 2023

Choose a reason for hiding this comment

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

The actual message is:

java: [FormatStringAnnotation] The format argument types passed with an @FormatString must match the types of the format arguments in the @FormatMethod header where the format string was declared.
  	Format arg types passed: [io.trino.spi.type.Type, io.trino.spi.type.Type]
  	Format arg types expected: [io.trino.sql.tree.Expression, io.trino.sql.tree.Expression]

Thing is, the message is the same but the message arguments passed to semanticException are not the same as the parameters of the enclosing method. So it kinda sorta makes sense?

}

private Type coerceToSingleType(StackableAstVisitorContext<Context> context, String description, List<Expression> expressions)
Expand All @@ -3176,12 +3179,12 @@ private Type coerceToSingleType(StackableAstVisitorContext<Context> context, Str
for (Type type : types) {
Optional<Type> newSuperType = typeCoercion.getCommonSuperType(superType, type);
if (newSuperType.isEmpty()) {
throw semanticException(TYPE_MISMATCH, Iterables.get(typeExpressions.get(type), 0).getNode(), format(
throw semanticException(TYPE_MISMATCH, Iterables.get(typeExpressions.get(type), 0).getNode(),
"%s must be the same type or coercible to a common type. Cannot find common type between %s and %s, all types (without duplicates): %s",
description,
superType,
type,
typeExpressions.keySet()));
typeExpressions.keySet());
}
superType = newSuperType.get();
}
Expand All @@ -3192,12 +3195,12 @@ private Type coerceToSingleType(StackableAstVisitorContext<Context> context, Str

if (!type.equals(superType)) {
if (!typeCoercion.canCoerce(type, superType)) {
throw semanticException(TYPE_MISMATCH, Iterables.get(coercionCandidates, 0).getNode(), format(
throw semanticException(TYPE_MISMATCH, Iterables.get(coercionCandidates, 0).getNode(),
"%s must be the same type or coercible to a common type. Cannot find common type between %s and %s, all types (without duplicates): %s",
description,
superType,
type,
typeExpressions.keySet()));
typeExpressions.keySet());
}
addOrReplaceExpressionsCoercion(coercionCandidates, type, superType);
}
Expand Down Expand Up @@ -3460,7 +3463,7 @@ public static void analyzeExpressionWithoutSubqueries(
plannerContext,
accessControl,
(node, ignored) -> {
throw semanticException(errorCode, node, message);
throw semanticException(errorCode, node, "%s", message);
},
session,
TypeProvider.empty(),
Expand Down Expand Up @@ -3582,7 +3585,7 @@ public static ExpressionAnalyzer createWithoutSubqueries(
session,
TypeProvider.empty(),
parameters,
node -> semanticException(errorCode, node, message),
node -> semanticException(errorCode, node, "%s", message),
warningCollector,
isDescribe);
}
Expand Down
5 changes: 2 additions & 3 deletions core/trino-main/src/main/java/io/trino/sql/analyzer/JsonPathAnalyzer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,6 @@
import static io.trino.sql.analyzer.TypeSignatureProvider.fromTypes;
import static io.trino.sql.jsonpath.tree.ArithmeticUnary.Sign.PLUS;
import static io.trino.type.Json2016Type.JSON_2016;
import static java.lang.String.format;
import static java.util.Objects.requireNonNull;

public class JsonPathAnalyzer
Expand Down Expand Up @@ -368,9 +367,9 @@ protected Type visitNamedVariable(NamedVariable node, Void context)
.filter(name -> name.equalsIgnoreCase(node.getName()))
.findFirst();
if (similarName.isPresent()) {
throw semanticException(INVALID_PATH, pathNode, format("no value passed for parameter %s. Try quoting \"%s\" in the PASSING clause to match case", node.getName(), node.getName()));
throw semanticException(INVALID_PATH, pathNode, "no value passed for parameter %s. Try quoting \"%s\" in the PASSING clause to match case", node.getName(), node.getName());
}
throw semanticException(INVALID_PATH, pathNode, "no value passed for parameter " + node.getName());
throw semanticException(INVALID_PATH, pathNode, "no value passed for parameter %s", node.getName());
}

if (parameterType.equals(JSON_2016)) {
Expand Down
3 changes: 3 additions & 0 deletions core/trino-main/src/main/java/io/trino/sql/analyzer/SemanticExceptions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
*/
package io.trino.sql.analyzer;

import com.google.errorprone.annotations.FormatMethod;
import io.trino.spi.ErrorCodeSupplier;
import io.trino.spi.TrinoException;
import io.trino.sql.tree.Expression;
Expand All @@ -38,11 +39,13 @@ public static TrinoException ambiguousAttributeException(Expression node, Qualif
throw semanticException(AMBIGUOUS_NAME, node, "Column '%s' is ambiguous", name);
}

@FormatMethod
public static TrinoException semanticException(ErrorCodeSupplier code, Node node, String format, Object... args)
{
return semanticException(code, node, null, format, args);
}

@FormatMethod
public static TrinoException semanticException(ErrorCodeSupplier code, Node node, Throwable cause, String format, Object... args)
{
throw new TrinoException(code, extractLocation(node), format(format, args), cause);
Expand Down
4 changes: 2 additions & 2 deletions core/trino-main/src/main/java/io/trino/sql/analyzer/StatementAnalyzer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1520,7 +1520,7 @@ protected Scope visitQuery(Query node, Optional<Scope> scope)
verify(isTopLevel || node.getFunctions().isEmpty(), "Inline functions must be at the top level");
for (FunctionSpecification function : node.getFunctions()) {
if (function.getName().getPrefix().isPresent()) {
throw semanticException(SYNTAX_ERROR, function, "Inline function names cannot be qualified: " + function.getName());
throw semanticException(SYNTAX_ERROR, function, "Inline function names cannot be qualified: %s", function.getName());
}
function.getRoutineCharacteristics().stream()
.filter(SecurityCharacteristic.class::isInstance)
Expand Down Expand Up @@ -2501,7 +2501,7 @@ private Scope createScopeForView(
Query query = parseView(originalSql, name, table);

if (!query.getFunctions().isEmpty()) {
throw semanticException(NOT_SUPPORTED, table, "View contains inline function: " + name);
throw semanticException(NOT_SUPPORTED, table, "View contains inline function: %s", name);
}

analysis.registerTableForView(table);
Expand Down
6 changes: 3 additions & 3 deletions core/trino-main/src/main/java/io/trino/sql/routine/SqlRoutineAnalyzer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ private Type getType(Node node, DataType type)
return plannerContext.getTypeManager().getType(toTypeSignature(type));
}
catch (TypeNotFoundException e) {
throw semanticException(TYPE_MISMATCH, node, "Unknown type: " + type);
throw semanticException(TYPE_MISMATCH, node, "Unknown type: %s", type);
}
}

Expand All @@ -218,7 +218,7 @@ private static void validateArguments(FunctionSpecification function)
}
String name = identifierValue(parameter.getName().get());
if (!argumentNames.add(name)) {
throw semanticException(INVALID_ARGUMENTS, parameter, "Duplicate function parameter name: " + name);
throw semanticException(INVALID_ARGUMENTS, parameter, "Duplicate function parameter name: %s", name);
}
}
}
Expand Down Expand Up @@ -514,7 +514,7 @@ private void analyzeExpression(Context context, Expression expression, Type expe
return;
}
if (!typeCoercion.canCoerce(actualType, expectedType)) {
throw semanticException(TYPE_MISMATCH, expression, message + " must evaluate to %s (actual: %s)", expectedType, actualType);
throw semanticException(TYPE_MISMATCH, expression, "%s must evaluate to %s (actual: %s)", message, expectedType, actualType);
}

addCoercion(expression, actualType, expectedType);
Expand Down
4 changes: 2 additions & 2 deletions core/trino-main/src/test/java/io/trino/execution/TestSetSessionTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,15 +169,15 @@ public void testSetSessionWithValidation()

assertThatThrownBy(() -> testSetSession("positive_property", new LongLiteral("-1"), "-1"))
.isInstanceOf(TrinoException.class)
.hasMessage(MUST_BE_POSITIVE);
.hasMessage("Invalid value of session property 'my_catalog.positive_property': " + MUST_BE_POSITIVE);
}

@Test
public void testSetSessionWithInvalidEnum()
{
assertThatThrownBy(() -> testSetSession("size_property", new StringLiteral("XL"), "XL"))
.isInstanceOf(TrinoException.class)
.hasMessage("Invalid value [XL]. Valid values: [SMALL, MEDIUM, LARGE]")
.hasMessage("Invalid value of session property 'my_catalog.size_property': Invalid value [XL]. Valid values: [SMALL, MEDIUM, LARGE]")
.matches(throwable -> ((TrinoException) throwable).getErrorCode() == INVALID_SESSION_PROPERTY.toErrorCode());
}

Expand Down
4 changes: 2 additions & 2 deletions plugin/trino-hive/src/test/java/io/trino/plugin/hive/BaseHiveConnectorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -533,11 +533,11 @@ public void testInvalidValueForQueryPartitionFilterRequiredSchemas()
{
assertQueryFails(
"SET SESSION hive.query_partition_filter_required_schemas = ARRAY['tpch', null]",
"line 1:1: Invalid null or empty value in query_partition_filter_required_schemas property");
"line 1:1: Invalid value of session property 'hive.query_partition_filter_required_schemas': Invalid null or empty value in query_partition_filter_required_schemas property");

assertQueryFails(
"SET SESSION hive.query_partition_filter_required_schemas = ARRAY['tpch', '']",
"line 1:1: Invalid null or empty value in query_partition_filter_required_schemas property");
"line 1:1: Invalid value of session property 'hive.query_partition_filter_required_schemas': Invalid null or empty value in query_partition_filter_required_schemas property");
}

@Test
Expand Down