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.

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

More string format removals #15316

Merged
merged 3 commits into from
Mar 27, 2023
Merged

More string format removals #15316

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
4cdae49
Use checkArgument formatting in StatementUtils
pettyjamesm Dec 5, 2022
941e300
Avoid String.format in ExpressionFormatter
pettyjamesm Dec 5, 2022
6f269f4
Replace String.format with String concat
pettyjamesm Dec 5, 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
15 changes: 6 additions & 9 deletions client/trino-client/src/main/java/io/trino/client/ClientTypeSignature.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down
3 changes: 1 addition & 2 deletions client/trino-client/src/main/java/io/trino/client/NamedClientTypeSignature.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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();
}
Expand Down
3 changes: 1 addition & 2 deletions client/trino-client/src/main/java/io/trino/client/Row.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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());
})
Expand Down
3 changes: 1 addition & 2 deletions client/trino-client/src/main/java/io/trino/client/Warning.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@

import java.util.Objects;

import static java.lang.String.format;
import static java.util.Objects.requireNonNull;

public final class Warning
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion client/trino-client/src/main/java/io/trino/client/auth/kerberos/SpnegoHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
4 changes: 3 additions & 1 deletion 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 @@ -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);
}
Expand Down
3 changes: 1 addition & 2 deletions core/trino-main/src/main/java/io/trino/sql/gen/AndCodeGenerator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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...")
Copy link
Member

Choose a reason for hiding this comment

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

These make the code less readable, IMO. Also, the code generation is not such a frequent operation that performance of concatenation vs String.format is a concern.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure I agree about concatenation being less readable, but that's a matter of taste so for the sake of argument, let's assume that format is slightly more readable. Even so, I don't agree with the idea that we should prefer the less efficient implementation (in non-exceptional, non-debugging code paths) by default. I'm arguing the opposite: even when the codepath is not especially "hot", we should prefer the more efficient of the two approaches when the readability difference is small to try to avoid accumulating thousands of small hits to performance overall.

If you still disagree, then I'm happy to remove either/both of the two places you left PR feedback comments about (and any others you want to flag)- I just wanted to make the case that we should reconsider the practice of adding these avoidable performance costs and prefer concatenation over formatting in the general.

Copy link
Member

Choose a reason for hiding this comment

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

I'm sympathetic to the "accumulating thousands of small hits to performance" argument, but the question is if this has any measurable impact. Our general philosophy is to prefer readability, unless the performance impact matters. This is a slippery slope.

In this specific case, I'm not sure it's less readable. But this one seems worse:

- .setDescription(format("cursor.get%s(%d)", type, field))
+ .setDescription("cursor.get" + type + "(" + field + ")")

Hopefully String Templates will allow both readability and performance.

Copy link
Member Author

Choose a reason for hiding this comment

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

@electrum - Fair point on the other change, I've pushed a new revision that removes that particular change in CursorProcessorCompiler.

.condition(wasNull);

ifWasNull.ifTrue()
Expand Down
2 changes: 1 addition & 1 deletion core/trino-main/src/main/java/io/trino/sql/gen/BytecodeUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
3 changes: 1 addition & 2 deletions core/trino-main/src/main/java/io/trino/sql/gen/OrCodeGenerator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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()
Expand Down
8 changes: 4 additions & 4 deletions core/trino-main/src/main/java/io/trino/sql/planner/LogicalPlanner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}
Comment on lines +246 to +248
Copy link
Member

Choose a reason for hiding this comment

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

I doubt this is significant enough to warrant making the code more verbose and harder to read.

Copy link
Member Author

Choose a reason for hiding this comment

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

While I'm sure that the performance cost isn't massive, this is called inside of the loop for each optimizer. On option would be to switch from requireNonNull(root, format("%s ...")) to requireNonNull(root, () -> format("%s ...")) but that would still be a capturing lambda creation per loop iteration even if it avoids the String.format overhead, which is why I changed it over to an explicit null check.

Copy link
Member

Choose a reason for hiding this comment

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

True, but there are only a handful of optimizers. Also, they do way more complicated things, so this is going to be a drop in the bucket.

Copy link
Member

Choose a reason for hiding this comment

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

Does lambda capture matter here? I suspect this would be optimized away by inlining.

Copy link
Member Author

Choose a reason for hiding this comment

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

It won't matter as long as the JIT does actually inline the requireNonNull and the lambda, but if either one of those doesn't happen then you'll get an extra allocation inside of the loop (which would still be better than calling String.format inside of each loop iteration, but is avoidable by using an explicit null check).


if (LOG.isDebugEnabled()) {
LOG.debug("%s:\n%s", optimizer.getClass().getName(), PlanPrinter.textLogicalPlan(
Expand Down Expand Up @@ -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<Expression, Expression> failIfPredicateIsNotMet(Metadata metadata, Session session, ErrorCodeSupplier errorCode, String errorMessage)
Expand Down
12 changes: 3 additions & 9 deletions core/trino-main/src/main/java/io/trino/type/TypeUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
{
Expand Down Expand Up @@ -107,16 +103,14 @@ private static String getDisplayLabelForLegacyClients(Type type)

private static String getRowDisplayLabelForLegacyClients(RowType type)
{
List<String> fields = type.getFields().stream()
return type.getFields().stream()
.map(field -> {
String typeDisplayName = getDisplayLabelForLegacyClients(field.getType());
if (field.getName().isPresent()) {
return field.getName().get() + ' ' + typeDisplayName;
}
return typeDisplayName;
})
.collect(toImmutableList());

return format("%s(%s)", ROW, Joiner.on(", ").join(fields));
.collect(joining(", ", ROW + "(", ")"));
}
}
2 changes: 1 addition & 1 deletion core/trino-main/src/main/java/io/trino/util/StatementUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ private static <T extends Statement> void verifyTaskInterfaceType(Class<T> 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;
}
}
Expand Down
47 changes: 21 additions & 26 deletions core/trino-parser/src/main/java/io/trino/sql/ExpressionFormatter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -298,11 +298,9 @@ protected String visitAllRows(AllRows node, Void context)
@Override
protected String visitArray(Array node, Void context)
{
ImmutableList.Builder<String> 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
Expand Down Expand Up @@ -993,9 +991,9 @@ private String formatBinaryExpression(String operator, Expression left, Expressi

private String joinExpressions(List<Expression> expressions)
{
return Joiner.on(", ").join(expressions.stream()
.map((e) -> process(e, null))
.iterator());
return expressions.stream()
.map(e -> process(e, null))
.collect(joining(", "));
}

/**
Expand Down Expand Up @@ -1095,9 +1093,9 @@ public static String formatOrderBy(OrderBy orderBy)

public static String formatSortItems(List<SortItem> sortItems)
{
return Joiner.on(", ").join(sortItems.stream()
return sortItems.stream()
.map(sortItemFormatterFunction())
.iterator());
.collect(joining(", "));
}

private static String formatWindow(Window window)
Expand Down Expand Up @@ -1220,9 +1218,7 @@ public static String formatSkipTo(SkipTo skipTo)

static String formatGroupBy(List<GroupingElement> groupingElements)
{
ImmutableList.Builder<String> resultStrings = ImmutableList.builder();

for (GroupingElement groupingElement : groupingElements) {
return groupingElements.stream().map(groupingElement -> {
String result = "";
if (groupingElement instanceof SimpleGroupBy) {
List<Expression> columns = groupingElement.getExpressions();
Expand All @@ -1234,20 +1230,19 @@ static String formatGroupBy(List<GroupingElement> 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)
Expand All @@ -1257,9 +1252,9 @@ private static boolean isAsciiPrintable(int codePoint)

private static String formatGroupingSet(List<Expression> groupingSet)
{
return format("(%s)", Joiner.on(", ").join(groupingSet.stream()
return groupingSet.stream()
.map(ExpressionFormatter::formatExpression)
.iterator()));
.collect(joining(", ", "(", ")"));
}

private static Function<SortItem, String> sortItemFormatterFunction()
Expand Down
8 changes: 3 additions & 5 deletions core/trino-parser/src/main/java/io/trino/sql/SqlFormatter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);
}
Expand Down
4 changes: 3 additions & 1 deletion core/trino-parser/src/main/java/io/trino/sql/parser/AstBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
3 changes: 1 addition & 2 deletions core/trino-parser/src/main/java/io/trino/sql/tree/PathElement.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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();
}
Expand Down
Loading