-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[core] Use Locale.ROOT for every case conversion in main sources #9771
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.
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
Open
LuciferYang
wants to merge
10
commits into
apache:master
Choose a base branch
from
LuciferYang:fix/stringutils-tolowercase-locale
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
8fa106f
[api] Use Locale.ROOT in StringUtils case conversions
LuciferYang c215217
fix: use Locale.ROOT for every machine-facing case conversion in api/…
LuciferYang aee1f5a
fix: keep prefix slicing length-safe, drop a test case that pins nothing
LuciferYang b142182
Merge remote-tracking branch 'upstream/master' into fix/stringutils-t…
LuciferYang 788491b
fix: finish the locale sweep and keep prefix slicing length-safe
LuciferYang bb81158
fix: pin Locale.ROOT on every remaining case conversion in main sources
LuciferYang 2a75774
fix: finish the sweep in Scala sources and pin the locale on generate…
LuciferYang 12fef4b
Merge remote-tracking branch 'upstream/master' into fix/stringutils-t…
LuciferYang d741c20
fix: pin Locale.ROOT in the method-reference case conversions too
LuciferYang 620f15f
test: assert the resulting keys in the strict-checking case
LuciferYang File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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 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 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 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 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 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 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 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 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
73 changes: 73 additions & 0 deletions
73
paimon-api/src/test/java/org/apache/paimon/TurkishLocaleParsingTest.java
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,73 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package org.apache.paimon; | ||
|
|
||
| import org.apache.paimon.options.Options; | ||
| import org.apache.paimon.types.RowKind; | ||
|
|
||
| import org.junit.jupiter.api.AfterEach; | ||
| import org.junit.jupiter.api.BeforeEach; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| import java.util.Locale; | ||
|
|
||
| import static org.apache.paimon.CoreOptions.PARTITION_MARK_DONE_ACTION; | ||
| import static org.assertj.core.api.Assertions.assertThat; | ||
|
|
||
| /** | ||
| * Parsing an option value, an enum name or a protocol token uppercases or lowercases it first. | ||
| * Under a Turkish default locale 'i' maps to a dotted capital and 'I' to a dotless small letter, so | ||
| * those conversions must pin {@link Locale#ROOT} or the token no longer matches what it is compared | ||
| * against. | ||
| */ | ||
| class TurkishLocaleParsingTest { | ||
|
|
||
| private Locale original; | ||
|
|
||
| @BeforeEach | ||
| void setUp() { | ||
| original = Locale.getDefault(); | ||
| Locale.setDefault(new Locale("tr", "TR")); | ||
| } | ||
|
|
||
| @AfterEach | ||
| void tearDown() { | ||
| Locale.setDefault(original); | ||
| } | ||
|
|
||
| @Test | ||
| void partitionMarkDoneActionsParse() { | ||
| // SUCCESS_FILE and DONE_PARTITION both contain an 'i': a locale-sensitive uppercase | ||
| // turns them into names no enum constant has, and valueOf throws | ||
| Options options = new Options(); | ||
| options.set(PARTITION_MARK_DONE_ACTION, "success-file,done-partition"); | ||
|
|
||
| assertThat(new CoreOptions(options).partitionMarkDoneActions()) | ||
| .containsExactlyInAnyOrder( | ||
| CoreOptions.PartitionMarkDoneAction.SUCCESS_FILE, | ||
| CoreOptions.PartitionMarkDoneAction.DONE_PARTITION); | ||
| } | ||
|
|
||
| @Test | ||
| void rowKindFromLowerCaseShortString() { | ||
| // "+i" is the only short string this can catch: Turkish differs from ROOT on 'i' and | ||
| // 'I' alone, so "-d" or "-u" would pass whichever conversion the code uses | ||
| assertThat(RowKind.fromShortString("+i")).isEqualTo(RowKind.INSERT); | ||
| } | ||
| } |
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 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 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 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 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 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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in d741c20.
You are right about the audit gap: I matched
.toLowerCase()textually, so everyString::toLowerCasemethod reference stayed on the default locale.listCaseConvertis 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-mappingvalue stops matching a mode whose name contains ani, e.g.TINYINT1-NOT-BOOL), the Kafka offset-reset hint inKafkaActionUtils, Hive partition key names inPaimonMetaHook, predicate-pushdown column names inSearchArgumentToPredicateConverter, and option keys inFileIO.grep -rn '::toLowerCase\|::toUpperCase'oversrc/mainis now empty.TurkishLocaleSchemaKeyTestcoversbuildPaimonSchemaundertr-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 ranTurkishLocaleTypeNameTest,CdcRecordTest,FileIOTest,StringUtilsTestandSearchArgumentToPredicateConverterTest: green.On the persisted-value half of your review: this patch does not touch
UpperTransformorLowerTransform, 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: whereupper/loweroutput 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrections to my numbers above, and two follow-up commits.
The failure counts I posted were from an intermediate state, where only
listCaseConvertwas reverted and the type-mapping case did not exist yet. Re-measured against the current test:IllegalStateExceptionfromSchema's ownallFields.containsAll(primaryKeys)check,IllegalArgumentExceptionfromsetPrimaryKeys,UnsupportedOperationExceptionfromTypeMappingMode.mode);listCaseConvertreverted: the four schema cases fail, the type-mapping case passes;TypeMapping.parsereverted: only the type-mapping case fails;620f15f came out of reviewing that test.
specifiedPrimaryKeyPassesStrictCheckingasserted onlydoesNotThrowAnyException(), 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 renamedTurkishLocaleCaseFoldingTest, 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 aBinaryString, not ajava.lang.String. The ASCII paths useCharacter.toUpperCase(int)/toLowerCase(int)(BinaryString.java:598,632) and the non-ASCII fallbacks are alreadytoString().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'supperunder the binary collation reachesUTF8String.toUpperCaseSlow(), which is an unpinnedtoString().toUpperCase(), but only for non-ASCII input: full-ASCII strings taketoUpperCaseAscii(), soupper('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/mainfor no-arg.toLowerCase()/.toUpperCase(), method references on any receiver (zero repo-wide now, tests included),Locale.getDefault(),%S/%Tformat conversions, Commons/Guava/ICU case helpers,java.text.Collator,Normalizer, Scala's paren-less and.capitalizeforms, orvalueOf(x.toUpperCase(...))-style enum folding. Six explicitLocale.USconversions remain, inMemorySize,TimeUtils,HadoopFileIOandFlinkFileIO, all on machine tokens; the JDK applies special casing only for the language codestr,azandlt, so those are byte-identical to ROOT for every input (checked over every defined code point: zero differences forLocale.US, differences undertr-TR).equalsIgnoreCase,CASE_INSENSITIVE_ORDER,regionMatches(true, ...)andPattern.CASE_INSENSITIVEdo not consult the default locale, so they are out of scope.One judgement call worth naming:
paimon-api'sStringUtils.toLowerCaseis already ROOT-pinned and null-safe, soStringUtils::toLowerCasewould have been a literal drop-in forString::toLowerCase. I used inline lambdas instead, because that helper mapsnulltonulland would turn today's NPE on a null key element into anullsitting in a key list. Happy to switch if you prefer the shared helper.