Replace String.format calls with string concatenation where possible
#3660
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This goes through our codebase and audits our `String.format` usage. It makes a few changes:
1. If the `String.format` call can be replaced with `String` concatenation, it does so. This mainly means taking cases where strings or numbers are naively placed together (e.g., `String.format("%s -> %d", myStr, myInt)`) and replacing it with the equivalent concatentation (e.g., `myStr + " -> " + myInt`). The reason this is done is efficiency: it is more efficient to construct the strings and then concatenate them than to use the Java `String` formatter most of the time, mainly because the format string parsing is done at runtime with every invocation rather than compiled into some kind of formatting object.
1. In places where that is more difficult, this ensures that the `String.format` call has set `Locale.ROOT` to avoid localization problems. This is what caused FoundationDB#3645, as the `MetricsDiffAnalyzer` was formatting numbers with decimals, and then test asserts were firing if the test ran with a different locale, which is a thing we do in our nightly tests. In theory, that's actually sort of appropriate for a tool like this (for example, it would make sense to localize `3.5` to `3,5` if run on a system where that is the convention), but the rest of the product doesn't support it, and it's not like the actual text is localized.
When running on a system with `Locale.ROOT`, this should be a transparent change.
This fixes FoundationDB#3645.
alecgrieser
added
performance
normen662
approved these changes
Oct 3, 2025
| .hasSize(1); | ||
| final String predicateDetail = root.getDetails().get(0); | ||
| assertThat(predicateDetail) | ||
| .as("large where clauses should be converted into a hex representation of their hash") |
There was a problem hiding this comment.
I think I am going to revert that particular "feature".
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This goes through our codebase and audits our
String.formatusage. It makes a few changes:String.formatcall can be replaced withStringconcatenation, it does so. This mainly means taking cases where strings or numbers are naively placed together (e.g.,String.format("%s -> %d", myStr, myInt)) and replacing it with the equivalent concatentation (e.g.,myStr + " -> " + myInt). The reason this is done is efficiency: it is more efficient to construct the strings and then concatenate them than to use the JavaStringformatter most of the time, mainly because the format string parsing is done at runtime with every invocation rather than compiled into some kind of formatting object.String.formatcall has setLocale.ROOTto avoid localization problems. This is what caused Metrics diff test uses locality for report formatting #3645, as theMetricsDiffAnalyzerwas formatting numbers with decimals, and then test asserts were firing if the test ran with a different locale, which is a thing we do in our nightly tests. In theory, that's actually sort of appropriate for a tool like this (for example, it would make sense to localize3.5to3,5if run on a system where that is the convention), but the rest of the product doesn't support it, and it's not like the actual text is localized.When running on a system with
Locale.ROOT, this should be a transparent change.This fixes #3645.