From 4cdae494164d82312ce0f51bb66830575a675552 Mon Sep 17 00:00:00 2001 From: James Petty Date: Mon, 5 Dec 2022 16:12:39 -0500 Subject: [PATCH 1/3] Use checkArgument formatting in StatementUtils Avoids an eager and unnecessary String.format call by letting checkArgument perform the required formatting only when the check fails. --- core/trino-main/src/main/java/io/trino/util/StatementUtils.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/trino-main/src/main/java/io/trino/util/StatementUtils.java b/core/trino-main/src/main/java/io/trino/util/StatementUtils.java index eb8b78d1116c..8f01a60fe12b 100644 --- a/core/trino-main/src/main/java/io/trino/util/StatementUtils.java +++ b/core/trino-main/src/main/java/io/trino/util/StatementUtils.java @@ -283,7 +283,7 @@ private static void verifyTaskInterfaceType(Class state if (genericInterface instanceof ParameterizedType parameterizedInterface) { if (parameterizedInterface.getRawType().equals(expectedInterfaceType)) { Type actualStatementType = parameterizedInterface.getActualTypeArguments()[0]; - checkArgument(actualStatementType.equals(statementType), format("Expected %s statement type to be %s", statementType.getSimpleName(), taskType.getSimpleName())); + checkArgument(actualStatementType.equals(statementType), "Expected %s statement type to be %s", statementType.getSimpleName(), taskType.getSimpleName()); return; } } From 941e30073547cf23dc2a048912fc3b15cb6fc4a0 Mon Sep 17 00:00:00 2001 From: James Petty Date: Mon, 5 Dec 2022 16:29:14 -0500 Subject: [PATCH 2/3] Avoid String.format in ExpressionFormatter Also replaces unnecessary usages of Guava's Joiner in favor of Collectors.joining where appropriate. --- .../io/trino/sql/ExpressionFormatter.java | 47 +++++++++---------- 1 file changed, 21 insertions(+), 26 deletions(-) diff --git a/core/trino-parser/src/main/java/io/trino/sql/ExpressionFormatter.java b/core/trino-parser/src/main/java/io/trino/sql/ExpressionFormatter.java index 0e31e4c0f0b3..aa10586b50bf 100644 --- a/core/trino-parser/src/main/java/io/trino/sql/ExpressionFormatter.java +++ b/core/trino-parser/src/main/java/io/trino/sql/ExpressionFormatter.java @@ -169,9 +169,9 @@ protected String visitNode(Node node, Void context) @Override protected String visitRow(Row node, Void context) { - return "ROW (" + Joiner.on(", ").join(node.getItems().stream() + return node.getItems().stream() .map(child -> process(child, context)) - .collect(toList())) + ")"; + .collect(joining(", ", "ROW (", ")")); } @Override @@ -298,11 +298,9 @@ protected String visitAllRows(AllRows node, Void context) @Override protected String visitArray(Array node, Void context) { - ImmutableList.Builder valueStrings = ImmutableList.builder(); - for (Expression value : node.getValues()) { - valueStrings.add(formatSql(value)); - } - return "ARRAY[" + Joiner.on(",").join(valueStrings.build()) + "]"; + return node.getValues().stream() + .map(SqlFormatter::formatSql) + .collect(joining(",", "ARRAY[", "]")); } @Override @@ -993,9 +991,9 @@ private String formatBinaryExpression(String operator, Expression left, Expressi private String joinExpressions(List expressions) { - return Joiner.on(", ").join(expressions.stream() - .map((e) -> process(e, null)) - .iterator()); + return expressions.stream() + .map(e -> process(e, null)) + .collect(joining(", ")); } /** @@ -1095,9 +1093,9 @@ public static String formatOrderBy(OrderBy orderBy) public static String formatSortItems(List sortItems) { - return Joiner.on(", ").join(sortItems.stream() + return sortItems.stream() .map(sortItemFormatterFunction()) - .iterator()); + .collect(joining(", ")); } private static String formatWindow(Window window) @@ -1220,9 +1218,7 @@ public static String formatSkipTo(SkipTo skipTo) static String formatGroupBy(List groupingElements) { - ImmutableList.Builder resultStrings = ImmutableList.builder(); - - for (GroupingElement groupingElement : groupingElements) { + return groupingElements.stream().map(groupingElement -> { String result = ""; if (groupingElement instanceof SimpleGroupBy) { List columns = groupingElement.getExpressions(); @@ -1234,20 +1230,19 @@ static String formatGroupBy(List groupingElements) } } else if (groupingElement instanceof GroupingSets) { - result = format("GROUPING SETS (%s)", Joiner.on(", ").join( - ((GroupingSets) groupingElement).getSets().stream() - .map(ExpressionFormatter::formatGroupingSet) - .iterator())); + result = ((GroupingSets) groupingElement).getSets().stream() + .map(ExpressionFormatter::formatGroupingSet) + .collect(joining(", ", "GROUPING SETS (", ")")); } else if (groupingElement instanceof Cube) { - result = format("CUBE %s", formatGroupingSet(groupingElement.getExpressions())); + result = "CUBE " + formatGroupingSet(groupingElement.getExpressions()); } else if (groupingElement instanceof Rollup) { - result = format("ROLLUP %s", formatGroupingSet(groupingElement.getExpressions())); + result = "ROLLUP " + formatGroupingSet(groupingElement.getExpressions()); } - resultStrings.add(result); - } - return Joiner.on(", ").join(resultStrings.build()); + return result; + }) + .collect(joining(", ")); } private static boolean isAsciiPrintable(int codePoint) @@ -1257,9 +1252,9 @@ private static boolean isAsciiPrintable(int codePoint) private static String formatGroupingSet(List groupingSet) { - return format("(%s)", Joiner.on(", ").join(groupingSet.stream() + return groupingSet.stream() .map(ExpressionFormatter::formatExpression) - .iterator())); + .collect(joining(", ", "(", ")")); } private static Function sortItemFormatterFunction() From 6f269f49202b18ad7054de430f0935f46464d105 Mon Sep 17 00:00:00 2001 From: James Petty Date: Mon, 5 Dec 2022 15:18:04 -0500 Subject: [PATCH 3/3] Replace String.format with String concat Replaces simple String.format usages in non-exceptional code paths with simple string concatenations where applicable. --- .../java/io/trino/client/ClientTypeSignature.java | 15 ++++++--------- .../io/trino/client/NamedClientTypeSignature.java | 3 +-- .../src/main/java/io/trino/client/Row.java | 3 +-- .../src/main/java/io/trino/client/Warning.java | 3 +-- .../trino/client/auth/kerberos/SpnegoHandler.java | 2 +- .../io/trino/sql/analyzer/ExpressionAnalyzer.java | 4 +++- .../java/io/trino/sql/gen/AndCodeGenerator.java | 3 +-- .../main/java/io/trino/sql/gen/BytecodeUtils.java | 2 +- .../java/io/trino/sql/gen/OrCodeGenerator.java | 3 +-- .../java/io/trino/sql/planner/LogicalPlanner.java | 8 ++++---- .../src/main/java/io/trino/type/TypeUtils.java | 12 +++--------- .../src/main/java/io/trino/sql/SqlFormatter.java | 8 +++----- .../main/java/io/trino/sql/parser/AstBuilder.java | 4 +++- .../main/java/io/trino/sql/tree/PathElement.java | 3 +-- .../java/io/trino/sql/tree/UpdateAssignment.java | 3 +-- 15 files changed, 31 insertions(+), 45 deletions(-) diff --git a/client/trino-client/src/main/java/io/trino/client/ClientTypeSignature.java b/client/trino-client/src/main/java/io/trino/client/ClientTypeSignature.java index 8f3b336a2da1..36e7c992e0b2 100644 --- a/client/trino-client/src/main/java/io/trino/client/ClientTypeSignature.java +++ b/client/trino-client/src/main/java/io/trino/client/ClientTypeSignature.java @@ -31,7 +31,6 @@ import static com.google.common.collect.ImmutableList.toImmutableList; import static io.trino.client.ClientStandardTypes.ROW; import static io.trino.client.ClientStandardTypes.VARCHAR; -import static java.lang.String.format; import static java.util.Collections.unmodifiableList; import static java.util.Objects.requireNonNull; import static java.util.stream.Collectors.joining; @@ -104,20 +103,18 @@ public String toString() @Deprecated private String rowToString() { - String fields = arguments.stream() + if (arguments.isEmpty()) { + return "row"; + } + return arguments.stream() .map(ClientTypeSignatureParameter::getNamedTypeSignature) .map(parameter -> { if (parameter.getName().isPresent()) { - return format("%s %s", parameter.getName().get(), parameter.getTypeSignature()); + return parameter.getName().get() + " " + parameter.getTypeSignature(); } return parameter.getTypeSignature().toString(); }) - .collect(joining(",")); - - if (fields.isEmpty()) { - return "row"; - } - return format("row(%s)", fields); + .collect(joining(",", "row(", ")")); } @Override diff --git a/client/trino-client/src/main/java/io/trino/client/NamedClientTypeSignature.java b/client/trino-client/src/main/java/io/trino/client/NamedClientTypeSignature.java index be13163b2e47..a4c6f344242e 100644 --- a/client/trino-client/src/main/java/io/trino/client/NamedClientTypeSignature.java +++ b/client/trino-client/src/main/java/io/trino/client/NamedClientTypeSignature.java @@ -19,7 +19,6 @@ import java.util.Objects; import java.util.Optional; -import static java.lang.String.format; import static java.util.Objects.requireNonNull; public class NamedClientTypeSignature @@ -73,7 +72,7 @@ public boolean equals(Object o) public String toString() { if (fieldName.isPresent()) { - return format("%s %s", fieldName.get(), typeSignature); + return fieldName.get() + " " + typeSignature; } return typeSignature.toString(); } diff --git a/client/trino-client/src/main/java/io/trino/client/Row.java b/client/trino-client/src/main/java/io/trino/client/Row.java index 39039bece2d8..df6ffbfe5392 100644 --- a/client/trino-client/src/main/java/io/trino/client/Row.java +++ b/client/trino-client/src/main/java/io/trino/client/Row.java @@ -21,7 +21,6 @@ import java.util.Objects; import java.util.Optional; -import static java.lang.String.format; import static java.util.Objects.requireNonNull; import static java.util.stream.Collectors.joining; @@ -64,7 +63,7 @@ public String toString() return fields.stream() .map(field -> { if (field.getName().isPresent()) { - return format("%s=%s", field.getName().get(), field.getValue()); + return field.getName().get() + "=" + field.getValue(); } return String.valueOf(field.getValue()); }) diff --git a/client/trino-client/src/main/java/io/trino/client/Warning.java b/client/trino-client/src/main/java/io/trino/client/Warning.java index d6b885c16cf2..8a0045c6fd57 100644 --- a/client/trino-client/src/main/java/io/trino/client/Warning.java +++ b/client/trino-client/src/main/java/io/trino/client/Warning.java @@ -18,7 +18,6 @@ import java.util.Objects; -import static java.lang.String.format; import static java.util.Objects.requireNonNull; public final class Warning @@ -70,7 +69,7 @@ public int hashCode() @Override public String toString() { - return format("%s, %s", warningCode, message); + return warningCode + ", " + message; } public static final class Code diff --git a/client/trino-client/src/main/java/io/trino/client/auth/kerberos/SpnegoHandler.java b/client/trino-client/src/main/java/io/trino/client/auth/kerberos/SpnegoHandler.java index e22f487ba266..f603d905464f 100644 --- a/client/trino-client/src/main/java/io/trino/client/auth/kerberos/SpnegoHandler.java +++ b/client/trino-client/src/main/java/io/trino/client/auth/kerberos/SpnegoHandler.java @@ -122,7 +122,7 @@ private Request authenticate(Request request) String principal = makeServicePrincipal(servicePrincipalPattern, remoteServiceName, hostName, useCanonicalHostname); byte[] token = generateToken(principal); - String credential = format("%s %s", NEGOTIATE, Base64.getEncoder().encodeToString(token)); + String credential = NEGOTIATE + " " + Base64.getEncoder().encodeToString(token); return request.newBuilder() .header(AUTHORIZATION, credential) .build(); diff --git a/core/trino-main/src/main/java/io/trino/sql/analyzer/ExpressionAnalyzer.java b/core/trino-main/src/main/java/io/trino/sql/analyzer/ExpressionAnalyzer.java index 3a21988710ea..bdc7b4bd0d91 100644 --- a/core/trino-main/src/main/java/io/trino/sql/analyzer/ExpressionAnalyzer.java +++ b/core/trino-main/src/main/java/io/trino/sql/analyzer/ExpressionAnalyzer.java @@ -1293,7 +1293,9 @@ else if (node.getArguments().size() > 127) { for (int i = 0; i < argumentTypes.size(); i++) { Expression expression = node.getArguments().get(i); Type expectedType = signature.getArgumentTypes().get(i); - requireNonNull(expectedType, format("Type '%s' not found", signature.getArgumentTypes().get(i))); + if (expectedType == null) { + throw new NullPointerException(format("Type '%s' not found", signature.getArgumentTypes().get(i))); + } if (node.isDistinct() && !expectedType.isComparable()) { throw semanticException(TYPE_MISMATCH, node, "DISTINCT can only be applied to comparable types (actual: %s)", expectedType); } diff --git a/core/trino-main/src/main/java/io/trino/sql/gen/AndCodeGenerator.java b/core/trino-main/src/main/java/io/trino/sql/gen/AndCodeGenerator.java index e7eb48202329..4f643a120c4e 100644 --- a/core/trino-main/src/main/java/io/trino/sql/gen/AndCodeGenerator.java +++ b/core/trino-main/src/main/java/io/trino/sql/gen/AndCodeGenerator.java @@ -25,7 +25,6 @@ import static com.google.common.base.Preconditions.checkArgument; import static io.airlift.bytecode.expression.BytecodeExpressions.constantFalse; -import static java.lang.String.format; import static java.util.Objects.requireNonNull; public class AndCodeGenerator @@ -58,7 +57,7 @@ public BytecodeNode generateExpression(BytecodeGeneratorContext generator) RowExpression term = terms.get(i); block.append(generator.generate(term)); - IfStatement ifWasNull = new IfStatement(format("if term %s wasNull...", i)) + IfStatement ifWasNull = new IfStatement("if term " + i + " wasNull...") .condition(wasNull); ifWasNull.ifTrue() diff --git a/core/trino-main/src/main/java/io/trino/sql/gen/BytecodeUtils.java b/core/trino-main/src/main/java/io/trino/sql/gen/BytecodeUtils.java index 672698e39634..8fe78ac8d5f9 100644 --- a/core/trino-main/src/main/java/io/trino/sql/gen/BytecodeUtils.java +++ b/core/trino-main/src/main/java/io/trino/sql/gen/BytecodeUtils.java @@ -111,7 +111,7 @@ public static BytecodeNode handleNullValue( isNull.pushJavaDefault(returnType); String loadDefaultComment; - loadDefaultComment = format("loadJavaDefault(%s)", returnType.getName()); + loadDefaultComment = "loadJavaDefault(" + returnType.getName() + ")"; isNull.gotoLabel(label); diff --git a/core/trino-main/src/main/java/io/trino/sql/gen/OrCodeGenerator.java b/core/trino-main/src/main/java/io/trino/sql/gen/OrCodeGenerator.java index fbb874053755..0a44976b9af9 100644 --- a/core/trino-main/src/main/java/io/trino/sql/gen/OrCodeGenerator.java +++ b/core/trino-main/src/main/java/io/trino/sql/gen/OrCodeGenerator.java @@ -25,7 +25,6 @@ import static com.google.common.base.Preconditions.checkArgument; import static io.airlift.bytecode.expression.BytecodeExpressions.constantFalse; -import static java.lang.String.format; import static java.util.Objects.requireNonNull; public class OrCodeGenerator @@ -57,7 +56,7 @@ public BytecodeNode generateExpression(BytecodeGeneratorContext generator) RowExpression term = terms.get(i); block.append(generator.generate(term)); - IfStatement ifWasNull = new IfStatement(format("if term %s wasNull...", i)) + IfStatement ifWasNull = new IfStatement("if term " + i + " wasNull...") .condition(wasNull); ifWasNull.ifTrue() diff --git a/core/trino-main/src/main/java/io/trino/sql/planner/LogicalPlanner.java b/core/trino-main/src/main/java/io/trino/sql/planner/LogicalPlanner.java index 2ff6b7c303e8..dd308d53231c 100644 --- a/core/trino-main/src/main/java/io/trino/sql/planner/LogicalPlanner.java +++ b/core/trino-main/src/main/java/io/trino/sql/planner/LogicalPlanner.java @@ -243,7 +243,9 @@ public Plan plan(Analysis analysis, Stage stage, boolean collectPlanStatistics) if (stage.ordinal() >= OPTIMIZED.ordinal()) { for (PlanOptimizer optimizer : planOptimizers) { root = optimizer.optimize(root, session, symbolAllocator.getTypes(), symbolAllocator, idAllocator, warningCollector, tableStatsProvider); - requireNonNull(root, format("%s returned a null plan", optimizer.getClass().getName())); + if (root == null) { + throw new NullPointerException(optimizer.getClass().getName() + " returned a null plan"); + } if (LOG.isDebugEnabled()) { LOG.debug("%s:\n%s", optimizer.getClass().getName(), PlanPrinter.textLogicalPlan( @@ -542,9 +544,7 @@ private RelationPlan getInsertPlan( private Expression createNullNotAllowedFailExpression(String columnName, Type type) { - return new Cast(failFunction(metadata, session, CONSTRAINT_VIOLATION, format( - "NULL value not allowed for NOT NULL column: %s", - columnName)), toSqlType(type)); + return new Cast(failFunction(metadata, session, CONSTRAINT_VIOLATION, "NULL value not allowed for NOT NULL column: " + columnName), toSqlType(type)); } private static Function failIfPredicateIsNotMet(Metadata metadata, Session session, ErrorCodeSupplier errorCode, String errorMessage) diff --git a/core/trino-main/src/main/java/io/trino/type/TypeUtils.java b/core/trino-main/src/main/java/io/trino/type/TypeUtils.java index 3f2f330c68a1..1c7737f759e3 100644 --- a/core/trino-main/src/main/java/io/trino/type/TypeUtils.java +++ b/core/trino-main/src/main/java/io/trino/type/TypeUtils.java @@ -13,7 +13,6 @@ */ package io.trino.type; -import com.google.common.base.Joiner; import io.trino.spi.TrinoException; import io.trino.spi.type.ArrayType; import io.trino.spi.type.CharType; @@ -28,14 +27,11 @@ import io.trino.spi.type.Type; import io.trino.spi.type.VarcharType; -import java.util.List; - -import static com.google.common.collect.ImmutableList.toImmutableList; import static io.trino.spi.StandardErrorCode.NOT_SUPPORTED; import static io.trino.spi.type.StandardTypes.ARRAY; import static io.trino.spi.type.StandardTypes.MAP; import static io.trino.spi.type.StandardTypes.ROW; -import static java.lang.String.format; +import static java.util.stream.Collectors.joining; public final class TypeUtils { @@ -107,7 +103,7 @@ private static String getDisplayLabelForLegacyClients(Type type) private static String getRowDisplayLabelForLegacyClients(RowType type) { - List fields = type.getFields().stream() + return type.getFields().stream() .map(field -> { String typeDisplayName = getDisplayLabelForLegacyClients(field.getType()); if (field.getName().isPresent()) { @@ -115,8 +111,6 @@ private static String getRowDisplayLabelForLegacyClients(RowType type) } return typeDisplayName; }) - .collect(toImmutableList()); - - return format("%s(%s)", ROW, Joiner.on(", ").join(fields)); + .collect(joining(", ", ROW + "(", ")")); } } diff --git a/core/trino-parser/src/main/java/io/trino/sql/SqlFormatter.java b/core/trino-parser/src/main/java/io/trino/sql/SqlFormatter.java index e7a3de6093e4..9692ae9129a5 100644 --- a/core/trino-parser/src/main/java/io/trino/sql/SqlFormatter.java +++ b/core/trino-parser/src/main/java/io/trino/sql/SqlFormatter.java @@ -154,7 +154,6 @@ import static io.trino.sql.ExpressionFormatter.formatStringLiteral; import static io.trino.sql.ExpressionFormatter.formatWindowSpecification; import static io.trino.sql.RowPatternFormatter.formatPattern; -import static java.lang.String.format; import static java.util.Objects.requireNonNull; import static java.util.stream.Collectors.joining; @@ -1343,10 +1342,9 @@ protected Void visitCreateTableAsSelect(CreateTableAsSelect node, Integer indent builder.append(formatName(node.getName())); node.getColumnAliases().ifPresent(columnAliases -> { - String columnList = columnAliases.stream() + builder.append(columnAliases.stream() .map(SqlFormatter::formatName) - .collect(joining(", ")); - builder.append(format("( %s )", columnList)); + .collect(joining(", ", "( ", " )"))); }); node.getComment().ifPresent(comment -> builder @@ -1469,7 +1467,7 @@ private static String formatPrincipal(PrincipalSpecification principal) return principal.getName().toString(); case USER: case ROLE: - return format("%s %s", type.name(), principal.getName()); + return type.name() + " " + principal.getName(); } throw new IllegalArgumentException("Unsupported principal type: " + type); } diff --git a/core/trino-parser/src/main/java/io/trino/sql/parser/AstBuilder.java b/core/trino-parser/src/main/java/io/trino/sql/parser/AstBuilder.java index 0b7ce2cfd3c1..8cbdcce8b3b6 100644 --- a/core/trino-parser/src/main/java/io/trino/sql/parser/AstBuilder.java +++ b/core/trino-parser/src/main/java/io/trino/sql/parser/AstBuilder.java @@ -3468,7 +3468,9 @@ else if (isHexDigit(ch)) { } else { char currentCodePoint = (char) codePoint; - check(!Character.isSurrogate(currentCodePoint), format("Invalid escaped character: %s. Escaped character is a surrogate. Use '\\+123456' instead.", currentEscapedCode), context); + if (Character.isSurrogate(currentCodePoint)) { + throw parseError(format("Invalid escaped character: %s. Escaped character is a surrogate. Use '\\+123456' instead.", currentEscapedCode), context); + } unicodeStringBuilder.append(currentCodePoint); } state = UnicodeDecodeState.EMPTY; diff --git a/core/trino-parser/src/main/java/io/trino/sql/tree/PathElement.java b/core/trino-parser/src/main/java/io/trino/sql/tree/PathElement.java index 102aa7532859..648703d1d023 100644 --- a/core/trino-parser/src/main/java/io/trino/sql/tree/PathElement.java +++ b/core/trino-parser/src/main/java/io/trino/sql/tree/PathElement.java @@ -20,7 +20,6 @@ import java.util.Objects; import java.util.Optional; -import static java.lang.String.format; import static java.util.Objects.requireNonNull; public final class PathElement @@ -98,7 +97,7 @@ public int hashCode() public String toString() { if (catalog.isPresent()) { - return format("%s.%s", catalog.get(), schema); + return catalog.get() + "." + schema; } return schema.toString(); } diff --git a/core/trino-parser/src/main/java/io/trino/sql/tree/UpdateAssignment.java b/core/trino-parser/src/main/java/io/trino/sql/tree/UpdateAssignment.java index 49c10261553e..c8455193a160 100644 --- a/core/trino-parser/src/main/java/io/trino/sql/tree/UpdateAssignment.java +++ b/core/trino-parser/src/main/java/io/trino/sql/tree/UpdateAssignment.java @@ -19,7 +19,6 @@ import java.util.Objects; import java.util.Optional; -import static java.lang.String.format; import static java.util.Objects.requireNonNull; public class UpdateAssignment @@ -90,6 +89,6 @@ public int hashCode() @Override public String toString() { - return format("%s = %s", name, value); + return name + " = " + value; } }