neilconway opened a new pull request, #10850: URL: https://github.com/apache/arrow-rs/pull/10850
# Which issue does this PR close? - Closes #10787, closes #10788, closes #10789, closes #10790, closes #10791, closes #10792, closes #10793, closes #10794 # Rationale for this change arrow-cast had two different string-to-decimal parsers: `parse_string_to_decimal_native`, used by `cast`, and `parse_decimal`, used by the CSV and JSON readers. Aside from redundancy, these code paths behaved differently (e.g., truncating vs rounding for digits beyond the target type's scale, whitespace trimming, support for e-notation, etc.), so decimal conversion behaved differently depending on how the decimal value arrived into arrow-rs. This PR replaces these parsers with a single unified parser; `parse_decimal(s, precision_scale)` is now the public entry point and `parse_string_to_decimal_native` is a thin, deprecated wrapper over it. The new parser is based on the one-pass, u64-chunked parser in #10668, extended with support for e-notation and negative scales. This fixes a lot of bugs and ensures consistent behavior, but it does result in some behavior changes and a small performance regression for the CSV/JSON path; more details below. Bugs fixed: (all in the JSON/CSV path) - divide-by-zero panic or wrong values for some inputs in exponent notation (#10788, #5762) - overflow on inputs with >= 256 digits or long exponents (#10787) - negative scales were ignored (#10791) - `0e0`/`-0e0` were incorrectly rejected (#10789), while `e5` and `-.` were incorrectly accepted (#10790) Behaviour changes: - CSV and JSON readers now round half away from zero instead of truncating digits beyond the scale (#9410, #9422, #7355) - `cast` from strings accepts exponent notation (#5068) and negative scales, which `can_cast_types` already advertised (#10792), and validates the target precision and scale before parsing any values - CSV and JSON readers now trim whitespace (#10793). Only ASCII whitespace characters are trimmed, which matches the behavior of the CSV float/int parsers; previously, the `cast` path trimmed Unicode whitespace as well, but it will no longer do so. - parse errors use `ArrowError::ParseError` with unified messages - `variant_get` validates the target precision for string inputs (#10794) Performance: - `cast` string-to-decimal: ~unchanged. The cast path already used the fast single-pass parser from #10668; the unified parser benchmarks within Criterion noise (~4%) of it. - CSV/JSON reader path: short inputs cost 1-2 ns more per value, while long Decimal256 inputs are ~35% faster. End-to-end, CSV reads of decimal columns are ~8% slower. I suspect there is room for further optimization here (which will now benefit both code paths!) to reach or exceed the previous performance, but I'd like to land the unified parser first before we tackle further optimizations. # What changes are included in this PR? See above. # Are these changes tested? New tests added to cover rounding, exponents, negative scale, whitespace, long inputs and the four widths, a seeded differential test checks 20k random inputs against a BigInt reference (num-bigint was added as a dev-dependency), and the CSV/JSON readers gain end-to-end tests for the new behavior and bugfixes listed above. # Are there any user-facing changes? Yes; a deprecated public API, and user-visible behavioral changes in decimal parsing. # AI usage Iterated primarily with CC Fable 5; code reviewed by Codex GPT 5.6. I read, understand, and revised the resulting code. -- 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]
