GH-51225: [C++] utf8_normalize: compose for NFC and NFKC - #51237
GH-51225: [C++] utf8_normalize: compose for NFC and NFKC#51237singhpratech wants to merge 1 commit into
Conversation
utf8proc_decompose() only decomposes; call utf8proc_normalize_utf32() on the scratch buffer when the form asks for composition. Fix the json_composed test fixture, whose bytes were the decomposed form, and add composed/decomposed pairs to the pyarrow test.
|
|
|
|
github-actions
Bot
added
Component: C++
Component: Python
awaiting review
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WQYE6hSNYZur5fRZC3mqGT
raulcd
left a comment
raulcd
left a comment
There was a problem hiding this comment.
This is not a critical fix, can you update the description.
| // decomposed: U+0061(LATIN SMALL LETTER A) + U+0301(COMBINING ACUTE ACCENT) | ||
| // composed: U+00E1(LATIN SMALL LETTER A WITH ACUTE) | ||
| const char* json_composed = "[\"foo\", \"á\"]"; | ||
| const char* json_composed = "[\"foo\", \"\xc3\xa1\"]"; |
There was a problem hiding this comment.
this does not seem correct and does not match the comment above
There was a problem hiding this comment.
The literal on main renders as á, but its bytes are 61 cc 81,
a followed by U+0301, the decomposed form, so it was the same string as json_decomposed on the
next line and the compose assertions below compared a value with itself. That is why the test passed
with the bug (Santoshkumarpuppala spotted it on the issue). To check:
git show main:cpp/src/arrow/compute/kernels/scalar_string_test.cc | sed -n 1248p | xxd
... 5c22 61cc 815c 22 ...
\xc3\xa1 is U+00E1 in UTF-8, which is what the comment describes. I used the escape so the two
fixtures are visibly different in the source, the way json_decomposed already is; happy to write the
literal á (composed, c3 a1) instead, or add the byte values to the comment, whichever you prefer.
|
|
2 participants
Rationale for this change
utf8_normalizewithform=NFCorNFKCreturned the decomposed forms (NFD, NFKD).Utf8NormalizeBasebuilds the right
utf8procoptions for each form but only callsutf8proc_decompose(), whichdecomposes regardless of
UTF8PROC_COMPOSE; the composition step lives inutf8proc_normalize_utf32(), which the kernel never called. See #51225.What changes are included in this PR?
utf8proc_decompose(), when the options includeUTF8PROC_COMPOSE, callutf8proc_normalize_utf32()on the scratch buffer. It composes in place and returns the new codepoint count; the existing UTF-8 encode loop is unchanged. NFD and NFKD take the same path as before.
json_composedfixture inscalar_string_test.cc: its bytes were the decomposed form(
61 CC 81), the same string asjson_decomposed, so the compose assertions were comparing a valuewith itself and passed with the bug. Thanks to @Santoshkumarpuppala for spotting that on the issue.
test_utf8_normalizein pyarrow; the existing input, U+00B2, is its own NFC.Are these changes tested?
Yes. With the corrected fixture,
TestStringKernels.Utf8Normalizefails on the unpatched kernel andpasses with this change; the pyarrow test covers the composed forms from Python.
Are there any user-facing changes?
Yes:
utf8_normalizewithNFCandNFKCnow returns composed output. Callers that depended on theprevious (decomposed) result for those forms will see different bytes.
formoption: NFC and NFKC output is decomposed #51225