LuciferYang commented on code in PR #9771:
URL: https://github.com/apache/paimon/pull/9771#discussion_r4002023411
##########
paimon-api/src/main/java/org/apache/paimon/utils/StringUtils.java:
##########
@@ -668,7 +669,9 @@ public static String quote(String str) {
}
public static String toLowerCaseIfNeed(String str, boolean caseSensitive) {
- return caseSensitive ? str : str.toLowerCase();
+ // Locale.ROOT: identifier matching must not depend on the JVM default
locale
+ // (e.g. Turkish lowercases 'I' to a dotless glyph and breaks column
mapping)
+ return caseSensitive ? str : str.toLowerCase(Locale.ROOT);
Review Comment:
Corrections to my numbers above, and two follow-up commits.
The failure counts I posted were from an intermediate state, where only
`listCaseConvert` was reverted and the type-mapping case did not exist yet.
Re-measured against the current test:
- parent-equivalent tree (the conversions reachable from these tests
reverted): all five cases fail, 2 assertion failures and 3 exceptions
(`IllegalStateException` from `Schema`'s own
`allFields.containsAll(primaryKeys)` check, `IllegalArgumentException` from
`setPrimaryKeys`, `UnsupportedOperationException` from `TypeMappingMode.mode`);
- only `listCaseConvert` reverted: the four schema cases fail, the
type-mapping case passes;
- only `TypeMapping.parse` reverted: only the type-mapping case fails;
- head: 5/5 pass.
620f15f29 came out of reviewing that test.
`specifiedPrimaryKeyPassesStrictChecking` asserted only
`doesNotThrowAnyException()`, which cannot distinguish "the strict path
accepted the key" from "it accepted the key and stored a different one"; it now
asserts the resulting primary keys. The class is renamed
`TurkishLocaleCaseFoldingTest`, since the fifth case is an option value rather
than a schema key.
I also owe you a correction on `UpperTransform` / `LowerTransform`. I
justified leaving them alone as SQL semantics, which is weaker than the actual
reason: they fold a `BinaryString`, not a `java.lang.String`. The ASCII paths
use `Character.toUpperCase(int)` / `toLowerCase(int)`
(`BinaryString.java:598,632`) and the non-ASCII fallbacks are already
`toString().toUpperCase(Locale.ROOT)` / `toLowerCase(Locale.ROOT)` (`:609-611`,
`:643-645`). This patch touches neither file, so there is no default-locale
dependence there to remove and no computed-column value changes with it. For
contrast, Spark's `upper` under the binary collation reaches
`UTF8String.toUpperCaseSlow()`, which is an unpinned
`toString().toUpperCase()`, but only for non-ASCII input: full-ASCII strings
take `toUpperCaseAscii()`, so `upper('istanbul')` looks the same either way.
On the completeness question your last paragraph raises: I enumerated the
spellings rather than searching for one. Nothing is left in any `src/main` for
no-arg `.toLowerCase()`/`.toUpperCase()`, method references on any receiver
(zero repo-wide now, tests included), `Locale.getDefault()`, `%S`/`%T` format
conversions, Commons/Guava/ICU case helpers, `java.text.Collator`,
`Normalizer`, Scala's paren-less and `.capitalize` forms, or
`valueOf(x.toUpperCase(...))`-style enum folding. Six explicit `Locale.US`
conversions remain, in `MemorySize`, `TimeUtils`, `HadoopFileIO` and
`FlinkFileIO`, all on machine tokens; the JDK applies special casing only for
the language codes `tr`, `az` and `lt`, so those are byte-identical to ROOT for
every input (checked over every defined code point: zero differences for
`Locale.US`, differences under `tr-TR`). `equalsIgnoreCase`,
`CASE_INSENSITIVE_ORDER`, `regionMatches(true, ...)` and
`Pattern.CASE_INSENSITIVE` do not consult the default locale, so th
ey are out of scope.
One judgement call worth naming: `paimon-api`'s `StringUtils.toLowerCase` is
already ROOT-pinned and null-safe, so `StringUtils::toLowerCase` would have
been a literal drop-in for `String::toLowerCase`. I used inline lambdas
instead, because that helper maps `null` to `null` and would turn today's NPE
on a null key element into a `null` sitting in a key list. Happy to switch if
you prefer the shared helper.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
