alamb opened a new pull request, #10890: URL: https://github.com/apache/arrow-rs/pull/10890
# Which issue does this PR close? - Closes #10889. # Rationale for this change The `Debug` impl for `PrimitiveArray<T>` is monomorphized for all ~32 primitive types, and each instantiation carries the entire temporal formatting `match` (date/time conversion, timezone parsing, RFC3339 formatting) plus a full copy of `print_long_array`, even for types like `Int8Type` that can never be temporal — the `match` is on the runtime `data_type()` so the compiler cannot prune it. Because generic functions are instantiated in the *consuming* crate, every downstream crate that debug-formats an array (directly or via `unwrap`/`assert_eq!`) regenerates all of it. See #10889 for `cargo llvm-lines` measurements on DataFusion crates (~85K IR lines per crate). Only the default arm (`Debug::fmt(&array.value(index), f)`) is genuinely type-specific; everything else depends only on the runtime `DataType` and the value as `i64`. # What changes are included in this PR? 1. `print_long_array` now takes `&dyn Array` and `&mut dyn FnMut(usize, ...)` instead of being generic over the array and closure types, so the head/tail truncation logic is compiled once. All `Debug` impls that use it (`GenericByteArray`, `GenericListArray`, `BooleanArray`, etc.) are updated; their per-value closures are unchanged. 2. The temporal arms of `PrimitiveArray`'s `Debug` closure move into a new non-generic `write_temporal_value(f, data_type, v)` helper, so the generic closure is just: temporal → convert value to `i64` and call the helper; otherwise → `Debug::fmt(&value)`. 3. `temporal_conversions` gains `pub(crate)` runtime-`DataType` counterparts of `as_datetime` / `as_time` / `as_datetime_with_timezone` (the match arms are identical, just driven by `&DataType` instead of `T::DATA_TYPE`). The existing generic functions delegate to them and are marked `#[inline]` so the const `T::DATA_TYPE` match still folds away for generic callers. The `Debug` output is byte-for-byte identical, including the error messages for invalid timezones and failed conversions. ## Measurements Measured on the `cast_kernels` bench target (exercises many primitive types), `cargo llvm-lines --release -p arrow --features test_utils --bench cast_kernels` (cargo-llvm-lines 0.4.41, aarch64-apple-darwin): | | before | after | delta | |---|---:|---:|---:| | total IR lines for the bench target | 323,255 | 285,694 | **−37,561 (−11.6%)** | | `<PrimitiveArray<T> as Debug>::fmt` + closures | 36,851 lines / 42 inst. | 4,737 lines / 56 inst. | −87% | | `print_long_array` | 14,730 lines / 16 inst. | 0 (compiled once in `arrow-array`) | −100% | | `temporal_conversions::*` pulled in by `Debug` | 4,299 lines / 46 inst. | 0 (compiled once in `arrow-array`) | −100% | Actual bench binary size (`cargo bench --bench cast_kernels --no-run`, default release settings): | before | after | delta | |---:|---:|---:| | 13,213,184 bytes | 12,988,032 bytes | **−225,152 bytes (−1.7%)** | `Debug` formatting is not performance-sensitive, so this should be a pure code-size / compile-time win. # Are these changes tested? Covered by the existing `Debug` tests (`test_primitive_array_debug`, the timestamp-with/without-timezone and invalid-timezone tests, etc.), which assert the exact formatted output and pass unchanged. # Are there any user-facing changes? No. `print_long_array` is a private helper and the new `*_with_data_type` conversion functions are `pub(crate)`. `Debug` output is unchanged. -- 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]
