alamb commented on code in PR #11002:
URL: https://github.com/apache/arrow-rs/pull/11002#discussion_r4031033340
##########
arrow-array/src/types.rs:
##########
@@ -1418,8 +1418,12 @@ pub trait DecimalType:
/// "Decimal32", "Decimal64", "Decimal128" or "Decimal256", for use in
error messages
const PREFIX: &'static str;
- /// Formats the decimal value with the provided precision and scale
- fn format_decimal(value: Self::Native, precision: u8, scale: i8) -> String;
+ /// Formats `value` with `scale` fractional digits. The value is always
Review Comment:
A follow on improvement might be to deprecate this method and add one that
formatted into an already allocated buffer 🤔 Or maybe we can have it implement
`Display` so it could be written into any buffer directly (which is how it is
implemented internally anyways) 🤔
##########
arrow-data/src/decimal.rs:
##########
@@ -1126,37 +1103,149 @@ pub fn format_decimal_str(value_str: &str, _precision:
usize, scale: i8) -> Stri
format_decimal_str_internal(value_str, scale)
}
-// Format a decimal string given the scale.
+/// The native value of a decimal type: `i32`, `i64`, `i128` or `i256`
+pub trait DecimalNativeType: Display + sealed::DecimalNativeTypeSealed {}
+
+mod sealed {
+ pub trait DecimalNativeTypeSealed {}
Review Comment:
👍 for sealing the type
##########
arrow-array/src/types.rs:
##########
@@ -1400,7 +1400,7 @@ mod decimal {
/// [`Decimal128Array`]: crate::array::Decimal128Array
/// [`Decimal256Array`]: crate::array::Decimal256Array
pub trait DecimalType:
- 'static + Send + Sync + ArrowPrimitiveType + decimal::DecimalTypeSealed
+ 'static + Send + Sync + ArrowPrimitiveType<Native: DecimalNativeType> +
decimal::DecimalTypeSealed
Review Comment:
what is the implication of this new trait bound? I assume that nothing other
than what is in this crate will be effected?
##########
arrow-array/src/types.rs:
##########
@@ -1400,7 +1400,7 @@ mod decimal {
/// [`Decimal128Array`]: crate::array::Decimal128Array
/// [`Decimal256Array`]: crate::array::Decimal256Array
pub trait DecimalType:
- 'static + Send + Sync + ArrowPrimitiveType + decimal::DecimalTypeSealed
+ 'static + Send + Sync + ArrowPrimitiveType<Native: DecimalNativeType> +
decimal::DecimalTypeSealed
Review Comment:
I double checked and this trait is indeed sealed (DecimalTypeSealed)
https://github.com/apache/arrow-rs/blob/acca3ef48a11f7a28d0d04a5cdf4449308b63a09/arrow-array/src/types.rs#L1378-L1389
##########
arrow-data/src/decimal.rs:
##########
@@ -1126,37 +1103,149 @@ pub fn format_decimal_str(value_str: &str, _precision:
usize, scale: i8) -> Stri
format_decimal_str_internal(value_str, scale)
}
-// Format a decimal string given the scale.
+/// The native value of a decimal type: `i32`, `i64`, `i128` or `i256`
+pub trait DecimalNativeType: Display + sealed::DecimalNativeTypeSealed {}
+
+mod sealed {
+ pub trait DecimalNativeTypeSealed {}
+}
+
+macro_rules! decimal_native {
+ ($($t:ty),+) => {
+ $(
+ impl sealed::DecimalNativeTypeSealed for $t {}
+ impl DecimalNativeType for $t {}
+ )+
+ };
+}
+
+decimal_native!(i32, i64, i128, i256);
+
+/// Formats the unscaled decimal `value` with `scale` fractional digits: the
+/// decimal point is inserted `scale` digits from the right, with leading
+/// zeros as needed, and a negative scale appends zeros instead. The value is
+/// always formatted in full, whatever its precision.
+pub fn format_decimal<V: DecimalNativeType>(value: V, scale: i8) -> String {
+ format_decimal_str_internal(DigitBuffer::from_value(&value).as_str(),
scale)
+}
+
+/// Like [`format_decimal`], but writes the result to `f` instead of returning
+/// a new `String`.
+pub fn write_decimal<V: DecimalNativeType>(
+ f: &mut dyn Write,
+ value: V,
+ scale: i8,
+) -> std::fmt::Result {
+ write_decimal_str(f, DigitBuffer::from_value(&value).as_str(), scale)
+}
+
+/// Formats `value_str` as [`write_decimal_str`] does, into a `String` with
+/// enough capacity to avoid reallocation
fn format_decimal_str_internal(value_str: &str, scale: i8) -> String {
- let (sign, rest) = match value_str.strip_prefix('-') {
- Some(stripped) => ("-", stripped),
+ let mut out = String::with_capacity(value_str.len() + scale.unsigned_abs()
as usize + 2);
+ write_decimal_str(&mut out, value_str, scale).expect("writing to a String
cannot fail");
+ out
+}
+
+/// The length of the longest decimal native value when formatted: `i256::MIN`
+/// has 77 digits and a sign
+const MAX_DECIMAL_VALUE_LEN: usize = 78;
+
+/// The formatted digits of a decimal native value, with its sign
+struct DigitBuffer {
+ bytes: [u8; MAX_DECIMAL_VALUE_LEN],
+ len: usize,
+}
+
+impl DigitBuffer {
+ /// Formats `value`
+ fn from_value(value: &dyn DecimalNativeType) -> Self {
+ let mut buf = Self {
+ bytes: [0; MAX_DECIMAL_VALUE_LEN],
+ len: 0,
+ };
+ write!(buf, "{value}").expect("a decimal native value fits the digit
buffer");
+ buf
+ }
+
+ fn as_str(&self) -> &str {
+ // `write_str` copies complete strings, so the bytes are valid UTF-8
+ std::str::from_utf8(&self.bytes[..self.len]).expect("DigitBuffer
contains valid UTF-8")
+ }
+}
+
+impl Write for DigitBuffer {
+ fn write_str(&mut self, s: &str) -> std::fmt::Result {
+ let end = self.len + s.len();
+ let target = self.bytes.get_mut(self.len..end).ok_or(std::fmt::Error)?;
+ target.copy_from_slice(s.as_bytes());
+ self.len = end;
+ Ok(())
+ }
+}
+
+/// Writes `value_str`, the digits of an unscaled decimal value with an
optional
+/// leading `-`, to `f` as a decimal with `scale` fractional digits: the
decimal
+/// point is inserted `scale` digits from the right, with leading zeros as
+/// needed, and a negative scale appends zeros instead. The value is always
+/// written in full, whatever its precision.
+fn write_decimal_str(f: &mut dyn Write, value_str: &str, scale: i8) ->
std::fmt::Result {
+ let (sign, digits) = match value_str.strip_prefix('-') {
+ Some(digits) => ("-", digits),
None => ("", value_str),
};
if scale == 0 {
- value_str.to_string()
+ f.write_str(value_str)
} else if scale < 0 {
- if rest == "0" {
- // Zero must not be zero-padded ("000" is not a valid number)
- value_str.to_string()
- } else {
- let padding = value_str.len() + scale.unsigned_abs() as usize;
- format!("{value_str:0<padding$}")
+ f.write_str(value_str)?;
+ // Zero must not be zero-padded ("000" is not a valid number)
+ if digits != "0" {
+ for _ in 0..scale.unsigned_abs() {
+ f.write_char('0')?;
+ }
}
- } else if rest.len() > scale as usize {
- // Decimal separator is in the middle of the string
- let (whole, decimal) = value_str.split_at(value_str.len() - scale as
usize);
- format!("{whole}.{decimal}")
+ Ok(())
+ } else if digits.len() > scale as usize {
+ // The decimal point is in the middle of the digits
+ let (whole, fraction) = value_str.split_at(value_str.len() - scale as
usize);
+ f.write_str(whole)?;
+ f.write_char('.')?;
+ f.write_str(fraction)
} else {
- // String has to be padded
- format!("{}0.{:0>width$}", sign, rest, width = scale as usize)
+ // The digits are all fractional and may need leading zeros
+ f.write_str(sign)?;
+ f.write_str("0.")?;
+ for _ in digits.len()..scale as usize {
+ f.write_char('0')?;
+ }
+ f.write_str(digits)
}
}
#[cfg(test)]
mod tests {
use super::*;
+ #[test]
+ fn test_format_decimal() {
+ assert_eq!(format_decimal(12345_i32, 2), "123.45");
+ assert_eq!(format_decimal(-5_i64, 3), "-0.005");
+ assert_eq!(format_decimal(0_i128, -2), "0");
+ assert_eq!(
+ format_decimal(i128::MIN, 38),
+ "-1.70141183460469231731687303715884105728"
+ );
+ assert_eq!(
+ format_decimal(i256::MIN, 0),
+
"-57896044618658097711785492504343953926634992332820282019728792003956564819968"
Review Comment:
Shoudl we do it for the other decimal types (32, 64, 128, etc?)
##########
arrow-data/src/decimal.rs:
##########
@@ -1126,37 +1103,149 @@ pub fn format_decimal_str(value_str: &str, _precision:
usize, scale: i8) -> Stri
format_decimal_str_internal(value_str, scale)
}
-// Format a decimal string given the scale.
+/// The native value of a decimal type: `i32`, `i64`, `i128` or `i256`
+pub trait DecimalNativeType: Display + sealed::DecimalNativeTypeSealed {}
+
+mod sealed {
+ pub trait DecimalNativeTypeSealed {}
+}
+
+macro_rules! decimal_native {
+ ($($t:ty),+) => {
+ $(
+ impl sealed::DecimalNativeTypeSealed for $t {}
+ impl DecimalNativeType for $t {}
+ )+
+ };
+}
+
+decimal_native!(i32, i64, i128, i256);
+
+/// Formats the unscaled decimal `value` with `scale` fractional digits: the
+/// decimal point is inserted `scale` digits from the right, with leading
+/// zeros as needed, and a negative scale appends zeros instead. The value is
+/// always formatted in full, whatever its precision.
+pub fn format_decimal<V: DecimalNativeType>(value: V, scale: i8) -> String {
+ format_decimal_str_internal(DigitBuffer::from_value(&value).as_str(),
scale)
+}
+
+/// Like [`format_decimal`], but writes the result to `f` instead of returning
+/// a new `String`.
+pub fn write_decimal<V: DecimalNativeType>(
+ f: &mut dyn Write,
+ value: V,
+ scale: i8,
+) -> std::fmt::Result {
+ write_decimal_str(f, DigitBuffer::from_value(&value).as_str(), scale)
+}
+
+/// Formats `value_str` as [`write_decimal_str`] does, into a `String` with
+/// enough capacity to avoid reallocation
fn format_decimal_str_internal(value_str: &str, scale: i8) -> String {
- let (sign, rest) = match value_str.strip_prefix('-') {
- Some(stripped) => ("-", stripped),
+ let mut out = String::with_capacity(value_str.len() + scale.unsigned_abs()
as usize + 2);
+ write_decimal_str(&mut out, value_str, scale).expect("writing to a String
cannot fail");
+ out
+}
+
+/// The length of the longest decimal native value when formatted: `i256::MIN`
+/// has 77 digits and a sign
+const MAX_DECIMAL_VALUE_LEN: usize = 78;
+
+/// The formatted digits of a decimal native value, with its sign
Review Comment:
👍
As I mentioned above, it would be great to find some way to write into a
pre-allocated buffer too (as a follow on PR)
##########
arrow-data/src/decimal.rs:
##########
@@ -1126,37 +1103,149 @@ pub fn format_decimal_str(value_str: &str, _precision:
usize, scale: i8) -> Stri
format_decimal_str_internal(value_str, scale)
}
-// Format a decimal string given the scale.
+/// The native value of a decimal type: `i32`, `i64`, `i128` or `i256`
+pub trait DecimalNativeType: Display + sealed::DecimalNativeTypeSealed {}
+
+mod sealed {
+ pub trait DecimalNativeTypeSealed {}
+}
+
+macro_rules! decimal_native {
+ ($($t:ty),+) => {
+ $(
+ impl sealed::DecimalNativeTypeSealed for $t {}
+ impl DecimalNativeType for $t {}
+ )+
+ };
+}
+
+decimal_native!(i32, i64, i128, i256);
+
+/// Formats the unscaled decimal `value` with `scale` fractional digits: the
+/// decimal point is inserted `scale` digits from the right, with leading
+/// zeros as needed, and a negative scale appends zeros instead. The value is
+/// always formatted in full, whatever its precision.
+pub fn format_decimal<V: DecimalNativeType>(value: V, scale: i8) -> String {
+ format_decimal_str_internal(DigitBuffer::from_value(&value).as_str(),
scale)
+}
+
+/// Like [`format_decimal`], but writes the result to `f` instead of returning
+/// a new `String`.
+pub fn write_decimal<V: DecimalNativeType>(
+ f: &mut dyn Write,
+ value: V,
+ scale: i8,
+) -> std::fmt::Result {
+ write_decimal_str(f, DigitBuffer::from_value(&value).as_str(), scale)
+}
+
+/// Formats `value_str` as [`write_decimal_str`] does, into a `String` with
+/// enough capacity to avoid reallocation
fn format_decimal_str_internal(value_str: &str, scale: i8) -> String {
- let (sign, rest) = match value_str.strip_prefix('-') {
- Some(stripped) => ("-", stripped),
+ let mut out = String::with_capacity(value_str.len() + scale.unsigned_abs()
as usize + 2);
+ write_decimal_str(&mut out, value_str, scale).expect("writing to a String
cannot fail");
+ out
+}
+
+/// The length of the longest decimal native value when formatted: `i256::MIN`
+/// has 77 digits and a sign
+const MAX_DECIMAL_VALUE_LEN: usize = 78;
+
+/// The formatted digits of a decimal native value, with its sign
+struct DigitBuffer {
+ bytes: [u8; MAX_DECIMAL_VALUE_LEN],
+ len: usize,
+}
+
+impl DigitBuffer {
+ /// Formats `value`
+ fn from_value(value: &dyn DecimalNativeType) -> Self {
+ let mut buf = Self {
+ bytes: [0; MAX_DECIMAL_VALUE_LEN],
+ len: 0,
+ };
+ write!(buf, "{value}").expect("a decimal native value fits the digit
buffer");
+ buf
+ }
+
+ fn as_str(&self) -> &str {
+ // `write_str` copies complete strings, so the bytes are valid UTF-8
+ std::str::from_utf8(&self.bytes[..self.len]).expect("DigitBuffer
contains valid UTF-8")
+ }
+}
+
+impl Write for DigitBuffer {
+ fn write_str(&mut self, s: &str) -> std::fmt::Result {
+ let end = self.len + s.len();
+ let target = self.bytes.get_mut(self.len..end).ok_or(std::fmt::Error)?;
+ target.copy_from_slice(s.as_bytes());
+ self.len = end;
+ Ok(())
+ }
+}
+
+/// Writes `value_str`, the digits of an unscaled decimal value with an
optional
+/// leading `-`, to `f` as a decimal with `scale` fractional digits: the
decimal
+/// point is inserted `scale` digits from the right, with leading zeros as
+/// needed, and a negative scale appends zeros instead. The value is always
+/// written in full, whatever its precision.
+fn write_decimal_str(f: &mut dyn Write, value_str: &str, scale: i8) ->
std::fmt::Result {
+ let (sign, digits) = match value_str.strip_prefix('-') {
+ Some(digits) => ("-", digits),
None => ("", value_str),
};
if scale == 0 {
- value_str.to_string()
+ f.write_str(value_str)
} else if scale < 0 {
- if rest == "0" {
- // Zero must not be zero-padded ("000" is not a valid number)
- value_str.to_string()
- } else {
- let padding = value_str.len() + scale.unsigned_abs() as usize;
- format!("{value_str:0<padding$}")
+ f.write_str(value_str)?;
+ // Zero must not be zero-padded ("000" is not a valid number)
+ if digits != "0" {
+ for _ in 0..scale.unsigned_abs() {
+ f.write_char('0')?;
+ }
}
- } else if rest.len() > scale as usize {
- // Decimal separator is in the middle of the string
- let (whole, decimal) = value_str.split_at(value_str.len() - scale as
usize);
- format!("{whole}.{decimal}")
+ Ok(())
+ } else if digits.len() > scale as usize {
+ // The decimal point is in the middle of the digits
+ let (whole, fraction) = value_str.split_at(value_str.len() - scale as
usize);
+ f.write_str(whole)?;
+ f.write_char('.')?;
+ f.write_str(fraction)
} else {
- // String has to be padded
- format!("{}0.{:0>width$}", sign, rest, width = scale as usize)
+ // The digits are all fractional and may need leading zeros
+ f.write_str(sign)?;
+ f.write_str("0.")?;
+ for _ in digits.len()..scale as usize {
+ f.write_char('0')?;
+ }
+ f.write_str(digits)
}
}
#[cfg(test)]
mod tests {
use super::*;
+ #[test]
+ fn test_format_decimal() {
+ assert_eq!(format_decimal(12345_i32, 2), "123.45");
+ assert_eq!(format_decimal(-5_i64, 3), "-0.005");
+ assert_eq!(format_decimal(0_i128, -2), "0");
+ assert_eq!(
+ format_decimal(i128::MIN, 38),
+ "-1.70141183460469231731687303715884105728"
+ );
+ assert_eq!(
+ format_decimal(i256::MIN, 0),
+
"-57896044618658097711785492504343953926634992332820282019728792003956564819968"
Review Comment:
👍
--
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]
