singhpratech commented on code in PR #51237:
URL: https://github.com/apache/arrow/pull/51237#discussion_r3979730277
##########
cpp/src/arrow/compute/kernels/scalar_string_test.cc:
##########
@@ -1245,7 +1245,7 @@ TYPED_TEST(TestStringKernels, Utf8Normalize) {
// 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\"]";
Review Comment:
Agreed, kept as the byte escapes; the new pairs use them too.
##########
cpp/src/arrow/compute/kernels/scalar_string_test.cc:
##########
@@ -1260,6 +1260,25 @@ TYPED_TEST(TestStringKernels, Utf8Normalize) {
&options);
}
+ // Hangul composes algorithmically in utf8proc, not through the composition
table.
Review Comment:
Removed, the code point lines stay.
##########
cpp/src/arrow/compute/kernels/scalar_string_utf8.cc:
##########
@@ -547,6 +547,16 @@ struct Utf8NormalizeBase {
if (res < 0) {
return Status::Invalid("Cannot normalize utf8 string: ",
utf8proc_errmsg(res));
}
+ if (decompose_options_ & UTF8PROC_COMPOSE) {
+ // utf8proc_decompose() only decomposes; the canonical composition step
for
+ // NFC and NFKC is done in-place by utf8proc_normalize_utf32().
Review Comment:
Yes, it is still needed. `utf8proc_normalize_utf32()` composes pairs
(through the composition
table, Hangul by arithmetic) and applies the newline and control-character
options; it does no
decomposition and no reordering of combining marks, both of which happen in
`utf8proc_decompose()`. NFC and NFKC are the full decomposition followed by
canonical
composition. Skipping `utf8proc_decompose()` would leave singletons alone
(U+212B ANGSTROM SIGN
normalizes to U+00C5, there is nothing to compose), would not reorder marks
(U+00E1 U+0323 must
become U+1EA1 U+0301), and would drop all of NFKC's compatibility mappings,
which happen in the
decompose call. utf8proc's own `utf8proc_map()` is the same sequence,
`utf8proc_decompose()` then
`utf8proc_reencode()`, which calls `utf8proc_normalize_utf32()` before
encoding; the kernel does
that into its scratch buffer instead of the malloc in `utf8proc_map()`. I
added the U+212B
singleton to the test, since it is the case that only passes with both steps.
--
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]
