Repository navigation
[core] Use Locale.ROOT for every case conversion in main sources - #9771
Conversation
toLowerCaseIfNeed, toLowerCase, and toUpperCase converted with the JVM default locale. Under a Turkish or Azeri default locale, 'I' lowercases to a dotless glyph and 'i' uppercases to a dotted capital, so the case-sensitive=false identifier matching used by CDC table mapping, computed columns, and the Arrow readers silently broke for columns containing 'I'/'i'. Convert with Locale.ROOT, and align the record side of the same CDC flow: CdcRecord.fieldNameLowerCase also lowercased with the default locale, so fixing only the schema side would newly diverge the two halves of the record-schema join under tr/az (previously both sides mangled identically and data still flowed). Assisted-by: GLM-5.3
…common/core
Fixing only StringUtils left the same bug in sites that actually throw. Under a
Turkish default locale 'i' uppercases to a dotted capital, so
PartitionMarkDoneAction.valueOf("SUCCESS_FILE") gets "SUCCESS_FİLE" and
IllegalArgumentException, RowKind.fromShortString("+i") stops matching "+I",
JdbcProtocol.valueOf loses SQLITE and MARIADB, and Solaris detection in
OperatingSystem stops recognising its own name. The DLF request signers, option
key lookups, format identifiers and system table names have the same exposure.
Every converted site here is machine-facing: enum names, protocol tokens,
option keys, header names, OS names, format identifiers, hex digits. None
should follow the JVM default locale. BinaryString.toLowerCase/toUpperCase are
left alone: their ASCII path uses Character.toLowerCase and their fallback
already pins Locale.ROOT, so the SQL upper()/lower() transforms over user data
were never locale-dependent.
Co-Authored-By: Claude Code <noreply@anthropic.com>
getIdentifierPrefixOptions lowercased the key to test the prefix and then
sliced the original by the prefix length, which assumes lowercasing preserves
length. It does not: ROOT maps 'İ' to 'i' plus a combining dot. Match
case-insensitively on the original key instead.
RowKind.fromShortString("-d") passes whichever conversion the code uses, since
Turkish differs from ROOT on 'i' and 'I' alone. Only the "+i" case can catch
the bug, so keep that one and say why.
Co-Authored-By: Claude Code <noreply@anthropic.com>
…olowercase-locale
Pins Locale.ROOT on the remaining paimon-flink-cdc conversions, and matches the format prefix against the identifier as written so the option key is sliced at an offset it has.
Covers paimon-filesystems, paimon-hive, paimon-flink-common, paimon-spark, paimon-lumina, the vendored OrcFile copy, and the docs, benchmark and CI tooling. Also makes the Hive clone copy of getIdentifierPrefixOptions length-safe, like the FileFormat original.
…d names The bulk edit matched only paren-bearing calls, so it left every Scala call and, with it, the pre-lowered format-name list that isFormatTable compares against. Also pins Locale.ROOT on the String.format calls that build file names.
…olowercase-locale
JingsongLi
left a comment
There was a problem hiding this comment.
Reviewed 12fef4b. Requirement fit: SUPPORTED. Implementation: FINDINGS.
Locale-independent machine tokens and identifier matching have clear operational value. However, changing the shared field conversion without changing CDC's key-list conversion introduces a fresh-job schema regression on Turkish JVMs; this is separate from the existing-table migration caveat disclosed in the PR body. Details and the head/base reproduction are inline.
All 122 focused locale/string/options tests passed with the exact changed classes on JDK 8, and current CI is successful. The additional real CDC schema-construction probe fails on this head and succeeds with the exact-base helper; no live database was used. After fixing the mismatch, the disclosed migration also needs care where upper/lower is part of a persisted primary or partition key: changed computed values can make subsequent DELETEs address a different key. Those jobs need a controlled rewrite/rebuild or preserved legacy expression semantics before resuming, rather than a binary restart alone.
| 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); |
There was a problem hiding this comment.
[P1] Normalize CDC key lists with the same locale as fields
buildPaimonSchema uses this helper for field names, but CdcActionCommonUtils.listCaseConvert still maps String::toLowerCase for source/configured primary keys and partition keys. With Locale tr-TR and a case-insensitive catalog, a fresh source column ID with primary key ID now becomes field id and key ıd, and Schema rejects the table. In the non-strict database-sync path, fields id/CITY with configured partition CITY instead become fields [id, city] and partitionKeys=[], silently dropping the requested partitioning.
I compiled the exact helper and CDC schema builder and exercised both paths: this head fails/drops the partition as above; the exact-base helper consistently produces [ıd]/[ıd] and [id, cıty]/[cıty]. JDBC metadata supplies source column/key names without an earlier normalization, so these inputs are reachable. Please convert listCaseConvert with the same explicit locale and add Turkish schema tests for inferred/configured keys and non-strict partition handling. The audit must include method references such as String::toLowerCase, which a search for .toLowerCase() misses.
There was a problem hiding this comment.
Fixed in d741c20.
You are right about the audit gap: I matched .toLowerCase() textually, so every String::toLowerCase method reference stayed on the default locale. listCaseConvert is the one with a correctness consequence, and I reproduced both paths you describe before changing anything.
The same search over main sources turned up six more references in that form, all machine tokens, so they are converted in the same commit: TypeMapping.parse (an upper-case --type-mapping value stops matching a mode whose name contains an i, e.g. TINYINT1-NOT-BOOL), the Kafka offset-reset hint in KafkaActionUtils, Hive partition key names in PaimonMetaHook, predicate-pushdown column names in SearchArgumentToPredicateConverter, and option keys in FileIO. grep -rn '::toLowerCase\|::toUpperCase' over src/main is now empty.
TurkishLocaleSchemaKeyTest covers buildPaimonSchema under tr-TR: a primary key inferred from the source schema, a specified primary key under both strict and non-strict checking, and a specified partition key under non-strict checking, plus the type-mapping case. On the parent commit the four schema cases fail (3 assertion failures, 1 error); on this head all five pass. Also ran TurkishLocaleTypeNameTest, CdcRecordTest, FileIOTest, StringUtilsTest and SearchArgumentToPredicateConverterTest: green.
On the persisted-value half of your review: this patch does not touch UpperTransform or LowerTransform, so no computed column changes value here. I have added your point to the PR body, since it applies to the disclosed migration rather than to a code path this patch changes: where upper/lower output is part of a primary or partition key, resuming a job after this change can address a different key, so those tables need a controlled rewrite or preserved legacy semantics rather than a restart.
There was a problem hiding this 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 (
IllegalStateExceptionfromSchema's ownallFields.containsAll(primaryKeys)check,IllegalArgumentExceptionfromsetPrimaryKeys,UnsupportedOperationExceptionfromTypeMappingMode.mode); - only
listCaseConvertreverted: the four schema cases fail, the type-mapping case passes; - only
TypeMapping.parsereverted: only the type-mapping case fails; - head: 5/5 pass.
620f15f 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 they 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.
The sweep matched `.toLowerCase()` textually and missed `String::toLowerCase` method references, so seven sites kept the JVM default locale. One of them is a correctness bug rather than an inconsistency: CdcActionCommonUtils.buildPaimonSchema folds field names through toLowerCaseIfNeed (Locale.ROOT) and key lists through listCaseConvert (default locale), so on a Turkish JVM with a case-insensitive catalog a source column ID with primary key ID yields field id and key ıd. The strict path rejects the schema; the non-strict database-sync path drops the requested partitioning instead. The other six are the same class: type-mapping option parsing, the Kafka offset-reset hint, Hive partition key names, Hive predicate-pushdown column names, and FileIO option keys. TurkishLocaleSchemaKeyTest covers inferred keys, specified keys under both strict and non-strict checking, non-strict partition handling, and an upper-case type-mapping option. All four schema cases fail on the parent commit.
assertThatCode(...).doesNotThrowAnyException() could not tell "the strict path accepted the key" from "it accepted the key and stored a different one". Assert the primary keys instead. Renamed the class: it also covers a --type-mapping option value, which is not a schema key.
JingsongLi
left a comment
There was a problem hiding this comment.
Reviewed the CDC key-list normalization. Using Locale.ROOT removes locale-dependent casing, including the Turkish-locale failure mode, while preserving the expected key matching behavior. The regression coverage looks good to me.
|
Please resolve conflicts. |
…olowercase-locale # Conflicts: # paimon-spark/paimon-spark-common/src/main/scala/org/apache/paimon/spark/catalyst/analysis/ReplacePaimonFunctions.scala
done |
JingsongLi
left a comment
There was a problem hiding this comment.
Requirement fit: SUPPORTED. The earlier CDC key-list finding is fixed by using the same explicit locale as the field-name conversion, and the targeted Turkish-locale coverage remains in the current diff. I rechecked the latest head after the upstream merge: GitHub now reports it mergeable and clean, and I found no new issue in the locale-conversion changes. The documented migration caveat for existing locale-dependent computed keys still needs attention in rollout notes. I did not rerun the full multi-module suite locally.
|
Thank you @JingsongLi |
Purpose
close #9770
String.toLowerCase()andString.toUpperCase()follow the JVM default locale. Under a Turkish or Azeri default,iuppercases to the dottedİandIlowercases to the dotlessı, so any token that is case-folded before being matched or parsed stops matching what it is compared against. This is a real failure, not a theoretical one:CoreOptions.partitionMarkDoneActions()didPartitionMarkDoneAction.valueOf(x.replace('-','_').toUpperCase()), and bothsuccess-fileanddone-partitioncontain ani, so the default configuration threwIllegalArgumentException: No enum constant ...PartitionMarkDoneAction.SUCCESS_FİLE.RowKind.fromShortString("+i")uppercases to+İ, matches no case arm, and threwUnsupportedOperationException.OrcFileresolves the codec withCompressionKind.valueOf(...toUpperCase()), andZLIBcontains anI, soorc.compress = zlibthrew.CachedClientPoolparses the Hive client cache keys withKeyElementType.valueOf(trimmed.toUpperCase()), andUGIcontains anI, so augicache key threwNo enum constant ...KeyElementType.UGİ. The line directly above it already pinnedLocale.ROOTfor theconf:check, so the two halves of one method disagreed.MySqlTypeUtils.getTypeInfouppercases a source type name before switching on it, andINT,BIGINT,DECIMAL,TIMESTAMPand every geometry name contain ani, so CDC type conversion threwDon't support MySQL type 'İNT' yet.andisGeoTypestopped recognizing geometry columns.MultiTablesSinkMode.fromString("DIVIDED")threwUnsupported mode: dıvıded, andLuminaVectorMetric.fromString("cosine")threwNo enum constant ...COSİNE.DistributedLockDialectFactorylostSQLITEandMARIADBthe same way, so JDBC catalog locking failed to resolve its dialect.OperatingSystemlowercasesos.nameand then looks forsolaris, which becomessolarıs, so the OS came backUNKNOWN. The class is duplicated inpaimon-benchmark, which had the same bug.HttpClientlooks for therequest-idheader by lowercased name, so error messages silently lost the request id.StartupModeandFormatenum parsing, mongodb startup modes, the CDC data format identifier, the OSS/OBS/COSN/Jindo credential key maps,ActionFactory, the global index procedures and vector index type names are all exposed the same way.The issue started from identifier matching:
StringUtils.toLowerCaseIfNeeddrives case-insensitive column and table matching, andCdcRecord.fieldNameLowerCaseis the record side of that same join, so the two had to agree or a column silently nulled out.The rule
No case conversion without an explicit locale anywhere under
src/main, Java or Scala. Two greps verify it:git grep -nE '\.to(Lower|Upper)Case\(\)' -- '*/src/main/java/*'returns five lines, all of themBinaryString's own two methods or a call on aBinaryStringreceiver, andgit grep -nE '\.to(Lower|Upper)Case([^(]|$)' -- '*/src/main/scala/*'returns nothing.BinaryStringis already locale-independent: the ASCII path usesCharacter.toLowerCaseand the non-ASCII fallback pinsLocale.ROOT.The Scala half matters more than a count suggests. A first pass matched only calls written with parentheses, which is every Java call and no paren-less Scala one, and that left
SparkSource.FORMAT_NAMESfolding with the default locale whileFormatTableCatalog.isFormatTablehad been moved to ROOT.MOSAICcontains anI, so on atrJVM the list heldmosaıcand the lookup asked formosaic: creating a MOSAIC format table went from working in its uppercase spelling to failing in both.isFormatTablenow compares againstFormatTable.Format.values()withequalsIgnoreCaseinstead of consulting a pre-lowered list, so the answer no longer depends on the locale in effect when a Scala object initialized, which is also what makes it testable.That rule covers the vendored
org.apache.orc.OrcFilecopy underpaimon-format. It is third-party source, but it is source we ship and run, and the failure is reachable from a documented option, so exempting it would have made this "fixed where convenient" rather than a rule. It also covers the docs generator, the cluster benchmark and the CI license checker, which are not shipped but are equally locale-dependent.Four
String.formatcalls are in scope for the same reason and are not case conversions:%drenders through the default locale's digits, and these four build names rather than messages.IcebergPathFactorywrotev%d.metadata.json, which on anar-EG,fa-IR,my-MMorbn-INJVM producedv٥.metadata.json— a file no Iceberg reader resolves and which Paimon's ownv-prefix version scan cannot parse back, whilenewManifestListFiletwo methods up builds its name by concatenation and is unaffected. The other three name local scratch files (LocalKvDb,FileIOChannel,LocalKvStateFactory).String.formatinside exception messages is deliberately left alone: a number rendered in the reader's locale is correct there.The two that are not one word
FileFormat.getIdentifierPrefixOptionsmatched an option key against the lower-cased format identifier and then sliced the key at that prefix's length. Lower-casing can lengthen a string, so the slice offset does not have to exist in the key: with identifierİthe prefix is three characters and the keyİ.is two, which threwStringIndexOutOfBoundsException: begin 3, end 2, length 2. It now matches case-insensitively against the identifier as written, which keeps the two lengths in step, and lower-cases only the key it puts in the result. For an ASCII identifier the two forms are equivalent, which is why the ORC, Parquet and Avro paths are unaffected.HiveTableCloneExtractor.getIdentifierPrefixOptionsis a copy of that method and got the same treatment. Scope worth stating plainly: every identifier Paimon itself passes is lowercase ASCII (avro,orc,parquet,json,csv, and on the Hive clone path onlyavroreaches the method at all, since the others return earlier), so no shipped format can trigger the overrun. What the guard buys is thatregionMatchesreturns false when the region runs past the key, which makes thesubstringprovably in range for any identifier a customFileFormatimplementation might pass. The three tests that use U+0130 document that contract rather than a scenario a user reaches today.One change that is not a machine token
StringUtils.toUpperCase/toLowerCaseare what the CDCupper()andlower()computed columns run on, so this changes data written into the table under atr,azorltdefault locale:lower("ISTANBUL")now persistsistanbulwhere it previously persistedıstanbul. Locale-independence is the behaviour you want there, since otherwise the value depends on which TaskManager ran the job, and it matches whatBinaryStringalready does. It is still a data change rather than a token fix.There is also an upgrade caveat for a table whose schema was inferred under such a locale. The persisted column name is
ıd; after this change both sides of the join produceid, so the old column stops matching and schema evolution can append a second column beside it.TableNameConverterlikewise resolves a different physical name. Being bug-compatible with a locale-dependent schema is not possible while also being correct, so this is a disclosure rather than something the patch works around. Whereupper/loweroutput is part of a persisted primary or partition key, the same disclosure has a sharper edge: the computed value changes, so a resumed job can address a different key than the rows it wrote earlier. Those tables need a controlled rewrite, or the old expression semantics preserved, rather than a restart.Tests
Each test sets a Turkish default locale and restores it.
TurkishLocaleParsingTestcoverspartitionMarkDoneActions()andRowKind.fromShortString("+i"); only+ican discriminate there, because Turkish differs from ROOT oniandIalone.TurkishLocaleTypeNameTestcovers the CDC type path withgetTypeInfo("int").f0andisGeoType("point").MultiTablesSinkModeTestandLuminaVectorMetricTestcover the two remaining enum lookups.TestCachedClientPoolgains theugicache key.FileFormatPrefixOptionsTestandHiveTableCloneExtractorTestcover the prefix slicing on both copies.StringUtilsTestandCdcRecordTestcover the identifier path.FormatTableCatalogTestcovers everyFormatTable.Formatin both spellings under a Turkish default, which is the assertion that fails on the pre-fix code with[MOSAIC].Not covered, deliberately: the four
FileIOcredential key maps build their lookup table in a static initializer, so a test would depend on class-load order rather than on the fix, and theHiveSchema,PaimonMetaHookandPaimonRecordReaderpaths need a live metastore. Those seven files are one-word changes verified by compilation and by the module's existing tests.Verified on JDK 11. The whole reactor builds with checkstyle, spotless, rat and enforcer enabled. Fail-on-base was run for every new test, not reasoned about: reverting the corresponding site produces
No enum constant ...KeyElementType.UGİ,No enum constant ...LuminaVectorMetric.COSİNE,Unsupported mode: dıvıded,expected: "INT" but was: "İNT",isGeoType("point")false,No enum constant ...PartitionMarkDoneAction.SUCCESS_FİLE,Unsupported short string '+i' for row kind., andStringIndexOutOfBoundsException: begin 3, end 2, length 2for the two prefix copies. The ORC, Parquet and Avro format tests that consume the prefix options pass (19 tests).