Skip to content

Replace String.format usage with simple string concatenation #18885

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
wendigo wants to merge 2 commits into from
Closed

Replace String.format usage with simple string concatenation #18885

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
49d6121
Replace String.format usage with simple string concatenation
wendigo Aug 31, 2023
eb40ee2
Use enhanced switch
wendigo Aug 31, 2023
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
59 changes: 27 additions & 32 deletions plugin/trino-base-jdbc/src/main/java/io/trino/plugin/jdbc/DefaultQueryBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ public class DefaultQueryBuilder
private static final String ALWAYS_TRUE = "1=1";
private static final String ALWAYS_FALSE = "1=0";

private static final Joiner SPACE_JOINER = Joiner.on(' ');
Copy link
Copy Markdown
Member

@hashhar hashhar Aug 31, 2023

Choose a reason for hiding this comment

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

this seems more like convenience rather than required? You can surely append a trailing space to each line.

I'll look into how Joiner is implemented - maybe I'm overthinking.

Copy link
Copy Markdown
Contributor

@ksobolew ksobolew Sep 1, 2023

Choose a reason for hiding this comment

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

Is this for performance, perhaps? I don't find it more readable or more convenient...

Copy link
Copy Markdown
Contributor Author

@wendigo wendigo Sep 1, 2023

Choose a reason for hiding this comment

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

Actually it is more readable as with the format you need to understand which argument goes where

Copy link
Copy Markdown
Member

@hashhar hashhar Sep 1, 2023

Choose a reason for hiding this comment

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

Joiner is actually more performant and less garbage generating compared to even concat since it uses good old StringBuilder underneath and some intelligent logic.

Yes, this is for perf.

Copy link
Copy Markdown
Contributor

@ksobolew ksobolew Sep 1, 2023

Choose a reason for hiding this comment

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

Don't underestimate plain concatenation, in Java 9+ it no longer desugars to StringBuilder and uses some INVOKEDYNAMIC magic instead :)

Copy link
Copy Markdown
Contributor

@ksobolew ksobolew Sep 1, 2023

Choose a reason for hiding this comment

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

(I'm not blocking this PR, BTW, do what you have to do)

Copy link
Copy Markdown
Member

@hashhar hashhar Sep 1, 2023

Choose a reason for hiding this comment

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

Yes, I tested with a JMH harness, Joiner is still faster.

Copy link
Copy Markdown
Member

@hashhar hashhar Sep 1, 2023

Choose a reason for hiding this comment

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

wait a sec, my measurements were wrong.

Plain concat is the fastest indeed.
JOINER is MUCH faster than format.
format must be avoided.

So if plain concat preserves readability (it seems to do) then let's use it.

For me joiner won because my JMH was generating random inputs for each method being benchmarked instead of each run having same input.

Copy link
Copy Markdown
Member

@hashhar hashhar Sep 1, 2023

Choose a reason for hiding this comment

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

Benchmark                     Mode  Cnt    Score    Error  Units
DifferentConcats.concat       avgt   15  143.218 ±  4.177  ns/op
DifferentConcats.guavaJoiner  avgt   15  398.257 ± 12.523  ns/op
DifferentConcats.formatStr    avgt   15  750.233 ± 16.783  ns/op

There's a steady 2x improvement between each approach.

Copy link
Copy Markdown
Member

@martint martint Sep 1, 2023

Choose a reason for hiding this comment

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

For this case, readability matters more than performance. How many of these operations are being performed per Trino query?


private final RemoteQueryModifier queryModifier;

@Inject
Expand Down Expand Up @@ -131,18 +133,18 @@ public PreparedQuery prepareJoinQuery(
String leftRelationAlias = "l";
String rightRelationAlias = "r";

String query = format(
"SELECT %s, %s FROM (%s) %s %s (%s) %s ON %s",
formatAssignments(client, leftRelationAlias, leftAssignments),
String query = SPACE_JOINER.join(
"SELECT",
formatAssignments(client, leftRelationAlias, leftAssignments) + ",",
formatAssignments(client, rightRelationAlias, rightAssignments),
leftSource.getQuery(),
leftRelationAlias,
"FROM",
"(" + leftSource.getQuery() + ")", leftRelationAlias,
formatJoinType(joinType),
rightSource.getQuery(),
rightRelationAlias,
joinConditions.stream()
.map(condition -> formatJoinCondition(client, leftRelationAlias, rightRelationAlias, condition))
.collect(joining(" AND ")));
"(" + rightSource.getQuery() + ")", rightRelationAlias,
"ON", joinConditions.stream()
.map(condition -> formatJoinCondition(client, leftRelationAlias, rightRelationAlias, condition))
.collect(joining(" AND ")));

List<QueryParameter> parameters = ImmutableList.<QueryParameter>builder()
.addAll(leftSource.getParameters())
.addAll(rightSource.getParameters())
Expand Down Expand Up @@ -231,13 +233,10 @@ public CallableStatement callProcedure(JdbcClient client, ConnectorSession sessi

protected String formatJoinCondition(JdbcClient client, String leftRelationAlias, String rightRelationAlias, JdbcJoinCondition condition)
{
return format(
"%s.%s %s %s.%s",
leftRelationAlias,
buildJoinColumn(client, condition.getLeftColumn()),
return SPACE_JOINER.join(
leftRelationAlias + "." + buildJoinColumn(client, condition.getLeftColumn()),
condition.getOperator().getValue(),
rightRelationAlias,
buildJoinColumn(client, condition.getRightColumn()));
rightRelationAlias + "." + buildJoinColumn(client, condition.getRightColumn()));
}

protected String buildJoinColumn(JdbcClient client, JdbcColumnHandle columnHandle)
Expand All @@ -248,23 +247,18 @@ protected String buildJoinColumn(JdbcClient client, JdbcColumnHandle columnHandl
protected String formatAssignments(JdbcClient client, String relationAlias, Map<JdbcColumnHandle, String> assignments)
{
return assignments.entrySet().stream()
.map(entry -> format("%s.%s AS %s", relationAlias, client.quoted(entry.getKey().getColumnName()), client.quoted(entry.getValue())))
.map(entry -> SPACE_JOINER.join(relationAlias + "." + client.quoted(entry.getKey().getColumnName()), "AS", client.quoted(entry.getValue())))
.collect(joining(", "));
}

protected String formatJoinType(JoinType joinType)
{
switch (joinType) {
case INNER:
return "INNER JOIN";
case LEFT_OUTER:
return "LEFT JOIN";
case RIGHT_OUTER:
return "RIGHT JOIN";
case FULL_OUTER:
return "FULL JOIN";
}
throw new IllegalStateException("Unsupported join type: " + joinType);
return switch (joinType) {
case INNER -> "INNER JOIN";
case LEFT_OUTER -> "LEFT JOIN";
case RIGHT_OUTER -> "RIGHT JOIN";
case FULL_OUTER -> "FULL JOIN";
};
}

protected String getRelation(JdbcClient client, RemoteTableName remoteTableName)
Expand All @@ -285,7 +279,7 @@ protected String getProjection(JdbcClient client, List<JdbcColumnHandle> columns
projections.add(columnAlias);
}
else {
projections.add(format("%s AS %s", expression.expression(), columnAlias));
projections.add(SPACE_JOINER.join(expression.expression(), "AS", columnAlias));
expression.parameters().forEach(accumulator);
}
}
Expand Down Expand Up @@ -345,7 +339,8 @@ protected String toPredicate(JdbcClient client, ConnectorSession session, Connec
if (!domain.isNullAllowed()) {
return predicate;
}
return format("(%s OR %s IS NULL)", predicate, client.quoted(column.getColumnName()));

return SPACE_JOINER.join("(" + predicate, "OR", client.quoted(column.getColumnName()), "IS NULL)");
}

protected String toPredicate(JdbcClient client, ConnectorSession session, Connection connection, JdbcColumnHandle column, ValueSet valueSet, Consumer<QueryParameter> accumulator)
Expand All @@ -355,7 +350,7 @@ protected String toPredicate(JdbcClient client, ConnectorSession session, Connec
if (!valueSet.isDiscreteSet()) {
ValueSet complement = valueSet.complement();
if (complement.isDiscreteSet()) {
return format("NOT (%s)", toPredicate(client, session, connection, column, complement, accumulator));
return "NOT (" + toPredicate(client, session, connection, column, complement, accumulator) + ")";
}
}

Expand Down Expand Up @@ -411,7 +406,7 @@ else if (singleValues.size() > 1) {
protected String toPredicate(JdbcClient client, ConnectorSession session, JdbcColumnHandle column, JdbcTypeHandle jdbcType, Type type, WriteFunction writeFunction, String operator, Object value, Consumer<QueryParameter> accumulator)
{
accumulator.accept(new QueryParameter(jdbcType, type, Optional.of(value)));
return format("%s %s %s", client.quoted(column.getColumnName()), operator, writeFunction.getBindExpression());
return SPACE_JOINER.join(client.quoted(column.getColumnName()), operator, writeFunction.getBindExpression());
}

protected String getGroupBy(JdbcClient client, Optional<List<List<JdbcColumnHandle>>> groupingSets)
Expand Down