This is an automated email from the ASF dual-hosted git repository.
alamb pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow-rs.git
The following commit(s) were added to refs/heads/main by this push:
new 2c50074ca7 perf(arrow-array): compile array `Debug` formatting helpers
once, not per primitive type (~2% binary size reduction) (#10890)
2c50074ca7 is described below
commit 2c50074ca7ec21c583e7b7b6b2d6933a02059d2c
Author: Andrew Lamb <[email protected]>
AuthorDate: Fri Aug 28 07:17:18 2026 -0400
perf(arrow-array): compile array `Debug` formatting helpers once, not per
primitive type (~2% binary size reduction) (#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 (aka 32 copies) but has a big match statement that
dispatches on the type and only a small part is genuinely type-specific;
everything else depends only on the runtime `DataType` and the value as
`i64`.
This means that we have 32 copies of everything (and we get 32 copies of
that same code in each downstream tratt)
# What changes are included in this PR?
1. Update the code so there is one copy rather than a bunch
## Measurements
I measured the size of the `cast_kernels` binary with
```shell
cargo bench --bench cast_kernels --no-run
```
To see the actual number of bytes saved (and it was substantial -- 225K
/ 1.7%:
| before | after | delta |
|---:|---:|---:|
| 13,213,184 bytes | 12,988,032 bytes | **−225,152 bytes (−1.7%)** |
Measured on the `cast_kernels` bench target with
```shell
cargo llvm-lines --release -p arrow --features test_utils --bench
cast_kernels
```
| | 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% |
`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.
---
arrow-array/src/array/boolean_array.rs | 4 +-
arrow-array/src/array/byte_array.rs | 4 +-
arrow-array/src/array/byte_view_array.rs | 4 +-
arrow-array/src/array/fixed_size_binary_array.rs | 4 +-
arrow-array/src/array/fixed_size_list_array.rs | 4 +-
arrow-array/src/array/list_array.rs | 4 +-
arrow-array/src/array/list_view_array.rs | 4 +-
arrow-array/src/array/map_array.rs | 4 +-
arrow-array/src/array/mod.rs | 19 +--
arrow-array/src/array/primitive_array.rs | 142 +++++++++++++----------
arrow-array/src/array/struct_array.rs | 2 +-
arrow-array/src/temporal_conversions.rs | 41 ++++++-
12 files changed, 146 insertions(+), 90 deletions(-)
diff --git a/arrow-array/src/array/boolean_array.rs
b/arrow-array/src/array/boolean_array.rs
index 0e5df12825..be80bc553a 100644
--- a/arrow-array/src/array/boolean_array.rs
+++ b/arrow-array/src/array/boolean_array.rs
@@ -73,8 +73,8 @@ pub struct BooleanArray {
impl std::fmt::Debug for BooleanArray {
fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
write!(f, "BooleanArray\n[\n")?;
- print_long_array(self, f, |array, index, f| {
- std::fmt::Debug::fmt(&array.value(index), f)
+ print_long_array(self, f, &mut |index, f| {
+ std::fmt::Debug::fmt(&self.value(index), f)
})?;
write!(f, "]")
}
diff --git a/arrow-array/src/array/byte_array.rs
b/arrow-array/src/array/byte_array.rs
index be4ecdee50..728aeb342a 100644
--- a/arrow-array/src/array/byte_array.rs
+++ b/arrow-array/src/array/byte_array.rs
@@ -467,8 +467,8 @@ impl<T: ByteArrayType> GenericByteArray<T> {
impl<T: ByteArrayType> std::fmt::Debug for GenericByteArray<T> {
fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
write!(f, "{}{}Array\n[\n", T::Offset::PREFIX, T::PREFIX)?;
- print_long_array(self, f, |array, index, f| {
- std::fmt::Debug::fmt(&array.value(index), f)
+ print_long_array(self, f, &mut |index, f| {
+ std::fmt::Debug::fmt(&self.value(index), f)
})?;
write!(f, "]")
}
diff --git a/arrow-array/src/array/byte_view_array.rs
b/arrow-array/src/array/byte_view_array.rs
index 7a7e94d111..0ff1a77c49 100644
--- a/arrow-array/src/array/byte_view_array.rs
+++ b/arrow-array/src/array/byte_view_array.rs
@@ -878,8 +878,8 @@ impl<T: ByteViewType + ?Sized> GenericByteViewArray<T> {
impl<T: ByteViewType + ?Sized> Debug for GenericByteViewArray<T> {
fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
write!(f, "{}ViewArray\n[\n", T::PREFIX)?;
- print_long_array(self, f, |array, index, f| {
- std::fmt::Debug::fmt(&array.value(index), f)
+ print_long_array(self, f, &mut |index, f| {
+ std::fmt::Debug::fmt(&self.value(index), f)
})?;
write!(f, "]")
}
diff --git a/arrow-array/src/array/fixed_size_binary_array.rs
b/arrow-array/src/array/fixed_size_binary_array.rs
index 53c8cdb8fd..fce539c614 100644
--- a/arrow-array/src/array/fixed_size_binary_array.rs
+++ b/arrow-array/src/array/fixed_size_binary_array.rs
@@ -812,8 +812,8 @@ impl<const N: usize> TryFrom<Vec<&[u8; N]>> for
FixedSizeBinaryArray {
impl std::fmt::Debug for FixedSizeBinaryArray {
fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
write!(f, "FixedSizeBinaryArray<{}>\n[\n", self.value_length())?;
- print_long_array(self, f, |array, index, f| {
- std::fmt::Debug::fmt(&array.value(index), f)
+ print_long_array(self, f, &mut |index, f| {
+ std::fmt::Debug::fmt(&self.value(index), f)
})?;
write!(f, "]")
}
diff --git a/arrow-array/src/array/fixed_size_list_array.rs
b/arrow-array/src/array/fixed_size_list_array.rs
index 901686a4fc..de7b97c231 100644
--- a/arrow-array/src/array/fixed_size_list_array.rs
+++ b/arrow-array/src/array/fixed_size_list_array.rs
@@ -609,8 +609,8 @@ impl ArrayAccessor for FixedSizeListArray {
impl std::fmt::Debug for FixedSizeListArray {
fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
write!(f, "FixedSizeListArray<{}>\n[\n", self.value_length())?;
- print_long_array(self, f, |array, index, f| {
- std::fmt::Debug::fmt(&array.value(index), f)
+ print_long_array(self, f, &mut |index, f| {
+ std::fmt::Debug::fmt(&self.value(index), f)
})?;
write!(f, "]")
}
diff --git a/arrow-array/src/array/list_array.rs
b/arrow-array/src/array/list_array.rs
index bd08a6ccc4..ccab56db99 100644
--- a/arrow-array/src/array/list_array.rs
+++ b/arrow-array/src/array/list_array.rs
@@ -706,8 +706,8 @@ impl<OffsetSize: OffsetSizeTrait> std::fmt::Debug for
GenericListArray<OffsetSiz
let prefix = OffsetSize::PREFIX;
write!(f, "{prefix}ListArray\n[\n")?;
- print_long_array(self, f, |array, index, f| {
- std::fmt::Debug::fmt(&array.value(index), f)
+ print_long_array(self, f, &mut |index, f| {
+ std::fmt::Debug::fmt(&self.value(index), f)
})?;
write!(f, "]")
}
diff --git a/arrow-array/src/array/list_view_array.rs
b/arrow-array/src/array/list_view_array.rs
index 9a44fcbd6e..1983e1a841 100644
--- a/arrow-array/src/array/list_view_array.rs
+++ b/arrow-array/src/array/list_view_array.rs
@@ -568,8 +568,8 @@ impl<OffsetSize: OffsetSizeTrait> std::fmt::Debug for
GenericListViewArray<Offse
fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
let prefix = OffsetSize::PREFIX;
write!(f, "{prefix}ListViewArray\n[\n")?;
- print_long_array(self, f, |array, index, f| {
- std::fmt::Debug::fmt(&array.value(index), f)
+ print_long_array(self, f, &mut |index, f| {
+ std::fmt::Debug::fmt(&self.value(index), f)
})?;
write!(f, "]")
}
diff --git a/arrow-array/src/array/map_array.rs
b/arrow-array/src/array/map_array.rs
index 9c7207210a..9c7996cab3 100644
--- a/arrow-array/src/array/map_array.rs
+++ b/arrow-array/src/array/map_array.rs
@@ -617,8 +617,8 @@ impl ArrayAccessor for &MapArray {
impl std::fmt::Debug for MapArray {
fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
write!(f, "MapArray\n[\n")?;
- print_long_array(self, f, |array, index, f| {
- std::fmt::Debug::fmt(&array.value(index), f)
+ print_long_array(self, f, &mut |index, f| {
+ std::fmt::Debug::fmt(&self.value(index), f)
})?;
write!(f, "]")
}
diff --git a/arrow-array/src/array/mod.rs b/arrow-array/src/array/mod.rs
index b3eb5eeaf2..6379b1f341 100644
--- a/arrow-array/src/array/mod.rs
+++ b/arrow-array/src/array/mod.rs
@@ -1060,11 +1060,16 @@ unsafe fn get_offsets_from_buffer<O: ArrowNativeType>(
}
/// Helper function for printing potentially long arrays.
-fn print_long_array<A, F>(array: &A, f: &mut std::fmt::Formatter, print_item:
F) -> std::fmt::Result
-where
- A: Array,
- F: Fn(&A, usize, &mut std::fmt::Formatter) -> std::fmt::Result,
-{
+///
+/// Note this function is deliberately not generic over the array or closure
+/// type: it is called from `Debug` impls that are instantiated for many
+/// concrete array types, and using dynamic dispatch here avoids duplicating
+/// this function's code in each of those instantiations
+fn print_long_array(
+ array: &dyn Array,
+ f: &mut std::fmt::Formatter,
+ print_item: &mut dyn FnMut(usize, &mut std::fmt::Formatter) ->
std::fmt::Result,
+) -> std::fmt::Result {
let head = std::cmp::min(10, array.len());
for i in 0..head {
@@ -1072,7 +1077,7 @@ where
writeln!(f, " null,")?;
} else {
write!(f, " ")?;
- print_item(array, i, f)?;
+ print_item(i, f)?;
writeln!(f, ",")?;
}
}
@@ -1088,7 +1093,7 @@ where
writeln!(f, " null,")?;
} else {
write!(f, " ")?;
- print_item(array, i, f)?;
+ print_item(i, f)?;
writeln!(f, ",")?;
}
}
diff --git a/arrow-array/src/array/primitive_array.rs
b/arrow-array/src/array/primitive_array.rs
index 09ab889f73..c0d000e7b7 100644
--- a/arrow-array/src/array/primitive_array.rs
+++ b/arrow-array/src/array/primitive_array.rs
@@ -19,7 +19,8 @@ use crate::array::print_long_array;
use crate::builder::{BooleanBufferBuilder, PrimitiveBuilder};
use crate::iterator::PrimitiveIter;
use crate::temporal_conversions::{
- as_date, as_datetime, as_datetime_with_timezone, as_duration, as_time,
+ as_datetime, as_datetime_with_data_type, as_datetime_with_timezone,
+ as_datetime_with_timezone_and_data_type, as_duration, as_time,
as_time_with_data_type,
};
use crate::timezone::Tz;
use crate::trusted_len::trusted_len_unzip;
@@ -1370,73 +1371,90 @@ where
}
}
-impl<T: ArrowPrimitiveType> std::fmt::Debug for PrimitiveArray<T> {
- fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
- let data_type = self.data_type();
-
- write!(f, "PrimitiveArray<{data_type}>\n[\n")?;
- print_long_array(self, f, |array, index, f| match data_type {
- DataType::Date32 | DataType::Date64 => {
- let v = self.value(index).to_i64().unwrap();
- match as_date::<T>(v) {
- Some(date) => write!(f, "{date:?}"),
- None => {
- write!(
- f,
- "Cast error: Failed to convert {v} to temporal for
{data_type}"
- )
- }
+/// Writes the `Debug` representation of a single temporal value (converted to
+/// `i64`) of the given [`DataType`] to `f`
+fn write_temporal_value(
+ f: &mut std::fmt::Formatter,
+ data_type: &DataType,
+ v: i64,
+) -> std::fmt::Result {
+ match data_type {
+ DataType::Date32 | DataType::Date64 => {
+ match as_datetime_with_data_type(data_type, v).map(|datetime|
datetime.date()) {
+ Some(date) => write!(f, "{date:?}"),
+ None => {
+ write!(
+ f,
+ "Cast error: Failed to convert {v} to temporal for
{data_type}"
+ )
}
}
- DataType::Time32(_) | DataType::Time64(_) => {
- let v = self.value(index).to_i64().unwrap();
- match as_time::<T>(v) {
- Some(time) => write!(f, "{time:?}"),
- None => {
- write!(
- f,
- "Cast error: Failed to convert {v} to temporal for
{data_type}"
- )
- }
- }
+ }
+ DataType::Time32(_) | DataType::Time64(_) => match
as_time_with_data_type(data_type, v) {
+ Some(time) => write!(f, "{time:?}"),
+ None => {
+ write!(
+ f,
+ "Cast error: Failed to convert {v} to temporal for
{data_type}"
+ )
}
- DataType::Timestamp(_, tz_string_opt) => {
- let v = self.value(index).to_i64().unwrap();
- match tz_string_opt {
- // for Timestamp with TimeZone
- Some(tz_string) => {
- match tz_string.parse::<Tz>() {
- // if the time zone is valid, construct a
DateTime<Tz> and format it as rfc3339
- Ok(tz) => match as_datetime_with_timezone::<T>(v,
tz) {
- Some(datetime) => write!(f, "{}",
datetime.to_rfc3339()),
- None => write!(
- f,
- "Cast error: Failed to convert {v} to
timestamp for {data_type}"
- ),
- },
- // if the time zone is invalid, shows
NaiveDateTime with an error message
- Err(_) => match as_datetime::<T>(v) {
- Some(datetime) => {
- write!(f, "{datetime:?} (Unknown Time Zone
'{tz_string}')")
- }
- None => write!(
- f,
- "Cast error: Failed to convert {v} to
timestamp for {data_type}"
- ),
- },
- }
+ },
+ DataType::Timestamp(_, tz_string_opt) => {
+ match tz_string_opt {
+ // for Timestamp with TimeZone
+ Some(tz_string) => {
+ match tz_string.parse::<Tz>() {
+ // if the time zone is valid, construct a DateTime<Tz>
and format it as rfc3339
+ Ok(tz) => match
as_datetime_with_timezone_and_data_type(data_type, v, tz) {
+ Some(datetime) => write!(f, "{}",
datetime.to_rfc3339()),
+ None => write!(
+ f,
+ "Cast error: Failed to convert {v} to
timestamp for {data_type}"
+ ),
+ },
+ // if the time zone is invalid, shows NaiveDateTime
with an error message
+ Err(_) => match as_datetime_with_data_type(data_type,
v) {
+ Some(datetime) => {
+ write!(f, "{datetime:?} (Unknown Time Zone
'{tz_string}')")
+ }
+ None => write!(
+ f,
+ "Cast error: Failed to convert {v} to
timestamp for {data_type}"
+ ),
+ },
}
- // for Timestamp without TimeZone
- None => match as_datetime::<T>(v) {
- Some(datetime) => write!(f, "{datetime:?}"),
- None => write!(
- f,
- "Cast error: Failed to convert {v} to timestamp
for {data_type}"
- ),
- },
}
+ // for Timestamp without TimeZone
+ None => match as_datetime_with_data_type(data_type, v) {
+ Some(datetime) => write!(f, "{datetime:?}"),
+ None => write!(
+ f,
+ "Cast error: Failed to convert {v} to timestamp for
{data_type}"
+ ),
+ },
+ }
+ }
+ _ => unreachable!("write_temporal_value called with non-temporal type
{data_type}"),
+ }
+}
+
+impl<T: ArrowPrimitiveType> std::fmt::Debug for PrimitiveArray<T> {
+ fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+ let data_type = self.data_type();
+
+ write!(f, "PrimitiveArray<{data_type}>\n[\n")?;
+ // Keep the per-value closure as small as possible: temporal formatting
+ // is dispatched to the non-generic `write_temporal_value` so it is not
+ // instantiated for every primitive type (see #10889)
+ print_long_array(self, f, &mut |index, f| match data_type {
+ DataType::Date32
+ | DataType::Date64
+ | DataType::Time32(_)
+ | DataType::Time64(_)
+ | DataType::Timestamp(_, _) => {
+ write_temporal_value(f, data_type,
self.value(index).to_i64().unwrap())
}
- _ => std::fmt::Debug::fmt(&array.value(index), f),
+ _ => std::fmt::Debug::fmt(&self.value(index), f),
})?;
write!(f, "]")
}
diff --git a/arrow-array/src/array/struct_array.rs
b/arrow-array/src/array/struct_array.rs
index 767cb94fed..fa2718c7fd 100644
--- a/arrow-array/src/array/struct_array.rs
+++ b/arrow-array/src/array/struct_array.rs
@@ -571,7 +571,7 @@ impl std::fmt::Debug for StructArray {
writeln!(f, "StructArray")?;
writeln!(f, "-- validity:")?;
writeln!(f, "[")?;
- print_long_array(self, f, |_array, _index, f| write!(f, "valid"))?;
+ print_long_array(self, f, &mut |_index, f| write!(f, "valid"))?;
writeln!(f, "]\n[")?;
for (child_index, name) in self.column_names().iter().enumerate() {
let column = self.column(child_index);
diff --git a/arrow-array/src/temporal_conversions.rs
b/arrow-array/src/temporal_conversions.rs
index 375c19bc87..0197acdc51 100644
--- a/arrow-array/src/temporal_conversions.rs
+++ b/arrow-array/src/temporal_conversions.rs
@@ -241,7 +241,17 @@ pub fn duration_ns_to_duration(v: i64) -> Duration {
/// Converts an [`ArrowPrimitiveType`] to [`NaiveDateTime`]
pub fn as_datetime<T: ArrowPrimitiveType>(v: i64) -> Option<NaiveDateTime> {
- match T::DATA_TYPE {
+ as_datetime_with_data_type(&T::DATA_TYPE, v)
+}
+
+/// Converts a value of the given [`DataType`] to [`NaiveDateTime`]
+///
+/// Non-generic counterpart of [`as_datetime`], driven by the runtime
+/// [`DataType`] so callers that already dispatch on the data type (e.g.
`Debug`
+/// impls) do not monomorphize this logic for every primitive type
+#[inline]
+pub(crate) fn as_datetime_with_data_type(data_type: &DataType, v: i64) ->
Option<NaiveDateTime> {
+ match data_type {
DataType::Date32 => date32_to_datetime(v as i32),
DataType::Date64 => date64_to_datetime(v),
DataType::Time32(_) | DataType::Time64(_) => None,
@@ -259,7 +269,20 @@ pub fn as_datetime<T: ArrowPrimitiveType>(v: i64) ->
Option<NaiveDateTime> {
/// Converts an [`ArrowPrimitiveType`] to [`DateTime<Tz>`]
pub fn as_datetime_with_timezone<T: ArrowPrimitiveType>(v: i64, tz: Tz) ->
Option<DateTime<Tz>> {
- let naive = as_datetime::<T>(v)?;
+ as_datetime_with_timezone_and_data_type(&T::DATA_TYPE, v, tz)
+}
+
+/// Converts a value of the given [`DataType`] to [`DateTime<Tz>`]
+///
+/// Non-generic counterpart of [`as_datetime_with_timezone`], see
+/// [`as_datetime_with_data_type`]
+#[inline]
+pub(crate) fn as_datetime_with_timezone_and_data_type(
+ data_type: &DataType,
+ v: i64,
+ tz: Tz,
+) -> Option<DateTime<Tz>> {
+ let naive = as_datetime_with_data_type(data_type, v)?;
Some(Utc.from_utc_datetime(&naive).with_timezone(&tz))
}
@@ -270,7 +293,15 @@ pub fn as_date<T: ArrowPrimitiveType>(v: i64) ->
Option<NaiveDate> {
/// Converts an [`ArrowPrimitiveType`] to [`NaiveTime`]
pub fn as_time<T: ArrowPrimitiveType>(v: i64) -> Option<NaiveTime> {
- match T::DATA_TYPE {
+ as_time_with_data_type(&T::DATA_TYPE, v)
+}
+
+/// Converts a value of the given [`DataType`] to [`NaiveTime`]
+///
+/// Non-generic counterpart of [`as_time`], see [`as_datetime_with_data_type`]
+#[inline]
+pub(crate) fn as_time_with_data_type(data_type: &DataType, v: i64) ->
Option<NaiveTime> {
+ match data_type {
DataType::Time32(unit) => {
// safe to immediately cast to u32 as `self.value(i)` is positive
i32
let v = v as u32;
@@ -285,7 +316,9 @@ pub fn as_time<T: ArrowPrimitiveType>(v: i64) ->
Option<NaiveTime> {
TimeUnit::Nanosecond => time64ns_to_time(v),
_ => None,
},
- DataType::Timestamp(_, _) => as_datetime::<T>(v).map(|datetime|
datetime.time()),
+ DataType::Timestamp(_, _) => {
+ as_datetime_with_data_type(data_type, v).map(|datetime|
datetime.time())
+ }
DataType::Date32 | DataType::Date64 => NaiveTime::from_hms_opt(0, 0,
0),
DataType::Interval(_) => None,
_ => None,
