sunchao commented on code in PR #6841:
URL: https://github.com/apache/datafusion-comet/pull/6841#discussion_r4236643046
##########
native/spark-expr/src/conversion_funcs/string.rs:
##########
@@ -461,38 +461,83 @@ fn cast_string_to_decimal256_impl(
))
}
-/// Normalize fullwidth Unicode digits (U+FF10–U+FF19) to their ASCII
equivalents.
+/// Code point of the zero of each run of ten non-ASCII decimal digits
(Unicode general
+/// category `Nd`) in the Basic Multilingual Plane, in ascending order. Every
run is contiguous,
+/// so the digit at `zero + n` has value `n`.
///
-/// Spark's UTF8String parser treats fullwidth digits as numerically
equivalent to
-/// ASCII digits, e.g. "１２３.４５" parses as 123.45. Each fullwidth digit encodes
-/// to exactly three UTF-8 bytes: [0xEF, 0xBC, 0x90+n] for digit n. The ASCII
-/// equivalent is 0x30+n, so the conversion is: third_byte - 0x60.
+/// This is the set of `char`s for which Java's `Character.isDigit(char)` is
true, minus ASCII.
+/// It is identical under JDK 17 (Unicode 13) and JDK 21 (Unicode 15); all
`Nd` digits added
+/// since lie outside the BMP.
+const BMP_NON_ASCII_DIGIT_ZEROS: [u16; 36] = [
+ 0x0660, // Arabic-Indic
+ 0x06F0, // Extended Arabic-Indic
+ 0x07C0, // NKo
+ 0x0966, // Devanagari
+ 0x09E6, // Bengali
+ 0x0A66, // Gurmukhi
+ 0x0AE6, // Gujarati
+ 0x0B66, // Oriya
+ 0x0BE6, // Tamil
+ 0x0C66, // Telugu
+ 0x0CE6, // Kannada
+ 0x0D66, // Malayalam
+ 0x0DE6, // Sinhala Lith
+ 0x0E50, // Thai
+ 0x0ED0, // Lao
+ 0x0F20, // Tibetan
+ 0x1040, // Myanmar
+ 0x1090, // Myanmar Shan
+ 0x17E0, // Khmer
+ 0x1810, // Mongolian
+ 0x1946, // Limbu
+ 0x19D0, // New Tai Lue
+ 0x1A80, // Tai Tham Hora
+ 0x1A90, // Tai Tham Tham
+ 0x1B50, // Balinese
+ 0x1BB0, // Sundanese
+ 0x1C40, // Lepcha
+ 0x1C50, // Ol Chiki
+ 0xA620, // Vai
+ 0xA8D0, // Saurashtra
+ 0xA900, // Kayah Li
+ 0xA9D0, // Javanese
+ 0xA9F0, // Myanmar Tai Laing
+ 0xAA50, // Cham
+ 0xABF0, // Meetei Mayek
+ 0xFF10, // Fullwidth
+];
+
+/// The value of `c` if it is a non-ASCII decimal digit that Java's
`BigDecimal` accepts.
///
-/// All other bytes (ASCII or other multi-byte sequences) are passed through
-/// unchanged, so the output is valid UTF-8 whenever the input is.
-fn normalize_fullwidth_digits(s: &str) -> String {
- let bytes = s.as_bytes();
- let mut out = Vec::with_capacity(s.len());
- let mut i = 0;
- while i < bytes.len() {
- if i + 2 < bytes.len()
- && bytes[i] == 0xEF
- && bytes[i + 1] == 0xBC
- && bytes[i + 2] >= 0x90
- && bytes[i + 2] <= 0x99
- {
- // e.g. 0x91 - 0x60 = 0x31 = b'1'
- out.push(bytes[i + 2] - 0x60);
- i += 3;
- } else {
- out.push(bytes[i]);
- i += 1;
- }
+/// `BigDecimal` scans UTF-16 code units and tests each with
`Character.isDigit(char)`, so only
+/// BMP digits count: a supplementary digit such as U+1D7CE MATHEMATICAL BOLD
DIGIT ZERO is a
+/// surrogate pair, neither half of which is a digit, and the parse fails.
+#[inline]
+fn non_ascii_digit_value(c: char) -> Option<u8> {
+ let cp = u32::from(c);
+ if cp > 0xFFFF {
+ return None;
}
- // SAFETY: we only replace valid 3-byte UTF-8 sequences [EF BC 9X] with a
- // single ASCII byte; all other bytes are copied unchanged, preserving the
- // UTF-8 invariant of the input.
- unsafe { String::from_utf8_unchecked(out) }
+ let idx = BMP_NON_ASCII_DIGIT_ZEROS.partition_point(|&zero|
u32::from(zero) <= cp);
+ let zero = u32::from(BMP_NON_ASCII_DIGIT_ZEROS[idx.checked_sub(1)?]);
+ let value = cp - zero;
+ (value < 10).then_some(value as u8)
+}
+
+/// Replace every non-ASCII decimal digit that Java's `BigDecimal` accepts
with its ASCII
+/// equivalent, e.g. "١٢٣.٤٥" and "１２３.４５" both become "123.45".
+///
+/// Spark parses decimals with `new java.math.BigDecimal(str)`, which reads
each digit through
+/// `Character.digit(c, 10)` in both the mantissa and the exponent, so digits
from any script
+/// can be mixed freely ("1٢e३" is 12000). All other characters are kept
unchanged and are
+/// rejected later by [`parse_decimal_str`].
+fn normalize_unicode_digits(s: &str) -> String {
+ s.chars()
+ .map(|c| match non_ascii_digit_value(c) {
Review Comment:
[P2] Could we preserve direct ASCII/fullwidth handling before invoking the
general digit lookup? For an existing column containing `１２３.４５` or
`-１２３４５６７８９.１２３４５６７８９`, casting to `DECIMAL(38,10)` still returns the correct
value, but this normalizer now binary-searches the table for every character.
An optimized A/B benchmark of the actual array-kernel functions measured median
costs of 50.6→90.9 ns/row and 92.0→231.2 ns/row respectively. Mixed
ASCII/fullwidth input also regressed, while pure ASCII remained unchanged. This
materially slows already-supported input. Keeping ASCII/fullwidth fast paths
and a preallocated output buffer, with table lookup for other scripts, restored
near-baseline timing in a disposable prototype.
Evidence: Reproduction artifacts: `/tmp/review-6841/batch.rs` and
`/tmp/review-6841/batch.tsv`. Extracted the unchanged decimal cast/parser
functions from base bfacc43a50aba13da4fa8a828db52309541191b4 and head
fb5f351aac38f7ccb7708ac86d7fbc9743a0a2a7 into one binary, compiled with `rustc
--edition 2021 -O`, and linked the repository-built Arrow and Comet common
dependencies. Each trial repeatedly cast an 8,192-row StringArray to
Decimal128(38,10) in Legacy mode, using black_box and 256 batches. Seven trials
alternated base/head order and asserted identical output arrays. Median ns/row:
ASCII `123.45` 30.67→30.50; fullwidth `１２３.４５` 50.59→90.91; longer fullwidth
`-１２３４５６７８９.１２３４５６７８９` 92.01→231.15; mixed `12345678901234567890.12345678９`
126.00→268.06. An independent parser-only benchmark also reproduced the
regression. A disposable fast-path/preallocation variant measured 53.08 ns/row
for the short fullwidth case versus 51.65
at base and 85.21 at head in the same run.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
