Chore: use Locale.ROOT in String.format.
#3117
Conversation
...relational-core/src/main/java/com/apple/foundationdb/relational/api/ProtobufDataBuilder.java
Show resolved
Hide resolved
| @@ -184,7 +185,7 @@ public static LogicalOperator generateAccess(@Nonnull Identifier identifier, | |||
| return logicalOperatorCatalog.lookupTableAccess(identifier, alias, requestedIndexes, semanticAnalyzer); | |||
| } else { | |||
| final var correlatedField = semanticAnalyzer.resolveCorrelatedIdentifier(identifier, currentPlanFragment.getLogicalOperatorsIncludingOuter()); | |||
| Assert.thatUnchecked(requestedIndexes.isEmpty(), ErrorCode.UNSUPPORTED_QUERY, () -> String.format("Can not hint indexes with correlated field access %s", identifier)); | |||
| Assert.thatUnchecked(requestedIndexes.isEmpty(), ErrorCode.UNSUPPORTED_QUERY, () -> String.format(Locale.ROOT, "Can not hint indexes with correlated field access %s", identifier)); | |||
There was a problem hiding this comment.
Probably quite a few of these would be better off using string concatenation, but using the simple mechanical change of adding Locale.ROOT seems good for this broad change.
There was a problem hiding this comment.
Yes, I will propose a different PR to remove usage of String.format where applicable.
Admittedly, most (if not all) of the current usages of String.format in relational are in an exceptional code path; either in an assertion failure, or to formulate an exception message. I do not think optimizing exceptional code paths for performance is crucial (they should not be in a tight loop, stack unrolling is already very expensive, ... etc).
Nevertheless, it is probably a good practice to avoid using String.format all together, and have a code analysis check that errors out when one uses it by mistake, considering how terrible its performance is.
...core/src/main/java/com/apple/foundationdb/relational/recordlayer/query/SemanticAnalyzer.java
Outdated
Show resolved
Hide resolved
| @@ -162,7 +162,8 @@ public Stream<? extends Arguments> provideArguments(final ExtensionContext conte | |||
| Arguments.of(75, "with recursive c as (select * from t, c) select * from c", "recursive CTE does not contain non-recursive term"), | |||
| Arguments.of(76, "with recursive c1 as (select * from restaurant union all select * from c1) select * From c1", null), | |||
| Arguments.of(77, "with recursive c as (with recursive c1 as (select * from restaurant union all select * from c1) select * From c1 union all select * from restaurant, c) select * from c", null), | |||
| Arguments.of(78, "with recursive c as (with c as (select * from restaurant) select * from c union all select * from restaurant) select * from c union all select * from restaurant", "ambiguous nested recursive CTE name") | |||
| Arguments.of(78, "with recursive c as (with c as (select * from restaurant) select * from c union all select * from restaurant) select * from c union all select * from restaurant", "ambiguous nested recursive CTE name"), | |||
| Arguments.of(79, "with recursive c1(name) as (select * from restaurant union all select * from c1) select * From c", "cte query has 7 column(s), however 1 aliases defined") | |||
There was a problem hiding this comment.
I always approve of additional testing, but to make sure I didn't miss a code change, why an additional test here?
There was a problem hiding this comment.
Right, the additional test was to confirm the fix done here [0] in SemanticAnalyzer's validation of the CTE columns, where the incorrect %s in the error message is replaced with %d as it is a placeholder for column count which is a number.
Use
String.formatin locale-neutral to avoid rendering message in an unexpected way when the JVM process is configured with a specific locale such aslrc_IR.This fixes #3121.