This is an automated email from the ASF dual-hosted git repository.
Jefffrey 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 15449e9447 fix: Correct two bugs when formatting decimal values
(#10869)
15449e9447 is described below
commit 15449e9447bc0b0a1548527d9f517134ba105bf4
Author: Neil Conway <[email protected]>
AuthorDate: Sat Aug 29 03:51:17 2026 -0400
fix: Correct two bugs when formatting decimal values (#10869)
# Which issue does this PR close?
- Closes #10866.
- Closes #10865.
# Rationale for this change
By default, decimal values are not validated against their type's
declared precision. Such out-of-precision values must therefore be
handled correctly through arrow-rs. `format_decimal_str` attempted to do
so by truncating the digit string to the declared precision, but that is
not correct: for example, the value `12345` in a `Decimal128(3, 1)`
context was formatted as `12.3`, i.e., it silently produced incorrect
values.
There are two principled ways to handle such values: format them in
their full precision or return an error. Returning an error is
impractical: (1) the formatting code doesn't have an easy way to surface
such errors (2) this would be inconsistent with how out-of-precision
decimals are handled throughout the rest of the Arrow ecosystem. Hence,
this PR arranges to format out-of-precision values without truncation;
the declared precision no longer affects formatting.
Along the way, fix a second bug: a `0` decimal value with a negative
scale was zero-padded; for example, formatting with scale `-2` resulted
in the string "000". That is not a legal number in JSON, and is also
just a surprising and likely unexpected behavior.
# What changes are included in this PR?
* Fix decimal formatting bugs as described above
* Add/update tests
# Are these changes tested?
Yes; new tests added.
# Are there any user-facing changes?
Yes, but only in corner-cases. Both scenarios are rare to begin with,
and the implemented behavior is unlikely to be what a user would desire.
`format_decimal_str` still takes a `precision` parameter that is now
unused. I've kept that in the function signature to avoid breaking a
public API.
---
arrow-cast/src/cast/mod.rs | 19 +++++++---
arrow-data/src/decimal.rs | 89 ++++++++++++++++++++++++++------------------
arrow-json/src/writer/mod.rs | 25 +++++++++++++
3 files changed, 92 insertions(+), 41 deletions(-)
diff --git a/arrow-cast/src/cast/mod.rs b/arrow-cast/src/cast/mod.rs
index 52733422ea..459b47ac1c 100644
--- a/arrow-cast/src/cast/mod.rs
+++ b/arrow-cast/src/cast/mod.rs
@@ -11942,6 +11942,19 @@ mod tests {
assert_eq!(1672531200000000000, c.value(0));
}
+ #[test]
+ fn test_cast_out_of_precision_decimal_to_string() {
+ // Decimal values are not validated against their type's declared
+ // precision by default. Check that out-of-precision values are
rendered
+ // in full when cast to strings, rather than truncated to the declared
+ // precision (https://github.com/apache/arrow-rs/issues/10866)
+ let array = create_decimal128_array(vec![Some(123456789),
Some(-123456789)], 7, 3).unwrap();
+ let b = cast(&array, &DataType::Utf8).unwrap();
+ let c = b.as_string::<i32>();
+ assert_eq!("123456.789", c.value(0));
+ assert_eq!("-123456.789", c.value(1));
+ }
+
#[test]
fn test_cast_decimal_to_string() {
assert!(can_cast_types(
@@ -11970,9 +11983,7 @@ mod tests {
assert_eq!("-3123.456", c.value(3));
assert_eq!("0.000", c.value(4));
assert_eq!("0.123", c.value(5));
- assert_eq!("1234.567", c.value(6));
- assert_eq!("-1234.567", c.value(7));
- assert!(c.is_null(8));
+ assert!(c.is_null(6));
};
}
@@ -12003,8 +12014,6 @@ mod tests {
Some(-3123456),
Some(0),
Some(123),
- Some(123456789),
- Some(-123456789),
None,
];
let array64: Vec<Option<i64>> = array32.iter().map(|num| num.map(|x| x
as i64)).collect();
diff --git a/arrow-data/src/decimal.rs b/arrow-data/src/decimal.rs
index de692a3418..c03dfe2f07 100644
--- a/arrow-data/src/decimal.rs
+++ b/arrow-data/src/decimal.rs
@@ -932,8 +932,7 @@ pub fn validate_decimal32_precision(
)));
}
if value > MAX_DECIMAL32_FOR_EACH_PRECISION[precision as usize] {
- let unscaled_value =
- format_decimal_str_internal(&value.to_string(), precision.into(),
scale, false);
+ let unscaled_value = format_decimal_str_internal(&value.to_string(),
scale);
let unscale_max_value = format_decimal_str(
&MAX_DECIMAL32_FOR_EACH_PRECISION[precision as usize].to_string(),
precision.into(),
@@ -943,8 +942,7 @@ pub fn validate_decimal32_precision(
"{unscaled_value} is too large to store in a Decimal32 of
precision {precision}. Max is {unscale_max_value}"
)))
} else if value < MIN_DECIMAL32_FOR_EACH_PRECISION[precision as usize] {
- let unscaled_value =
- format_decimal_str_internal(&value.to_string(), precision.into(),
scale, false);
+ let unscaled_value = format_decimal_str_internal(&value.to_string(),
scale);
let unscale_min_value = format_decimal_str(
&MIN_DECIMAL32_FOR_EACH_PRECISION[precision as usize].to_string(),
precision.into(),
@@ -985,8 +983,7 @@ pub fn validate_decimal64_precision(
)));
}
if value > MAX_DECIMAL64_FOR_EACH_PRECISION[precision as usize] {
- let unscaled_value =
- format_decimal_str_internal(&value.to_string(), precision.into(),
scale, false);
+ let unscaled_value = format_decimal_str_internal(&value.to_string(),
scale);
let unscaled_max_value = format_decimal_str(
&MAX_DECIMAL64_FOR_EACH_PRECISION[precision as usize].to_string(),
precision.into(),
@@ -996,8 +993,7 @@ pub fn validate_decimal64_precision(
"{unscaled_value} is too large to store in a Decimal64 of
precision {precision}. Max is {unscaled_max_value}"
)))
} else if value < MIN_DECIMAL64_FOR_EACH_PRECISION[precision as usize] {
- let unscaled_value =
- format_decimal_str_internal(&value.to_string(), precision.into(),
scale, false);
+ let unscaled_value = format_decimal_str_internal(&value.to_string(),
scale);
let unscaled_min_value = format_decimal_str(
&MIN_DECIMAL64_FOR_EACH_PRECISION[precision as usize].to_string(),
precision.into(),
@@ -1034,8 +1030,7 @@ pub fn validate_decimal_precision(value: i128, precision:
u8, scale: i8) -> Resu
)));
}
if value > MAX_DECIMAL128_FOR_EACH_PRECISION[precision as usize] {
- let unscaled_value =
- format_decimal_str_internal(&value.to_string(), precision.into(),
scale, false);
+ let unscaled_value = format_decimal_str_internal(&value.to_string(),
scale);
let unscaled_max_value = format_decimal_str(
&MAX_DECIMAL128_FOR_EACH_PRECISION[precision as usize].to_string(),
precision.into(),
@@ -1045,8 +1040,7 @@ pub fn validate_decimal_precision(value: i128, precision:
u8, scale: i8) -> Resu
"{unscaled_value} is too large to store in a Decimal128 of
precision {precision}. Max is {unscaled_max_value}"
)))
} else if value < MIN_DECIMAL128_FOR_EACH_PRECISION[precision as usize] {
- let unscaled_value =
- format_decimal_str_internal(&value.to_string(), precision.into(),
scale, false);
+ let unscaled_value = format_decimal_str_internal(&value.to_string(),
scale);
let unscaled_min_value = format_decimal_str(
&MIN_DECIMAL128_FOR_EACH_PRECISION[precision as usize].to_string(),
precision.into(),
@@ -1088,8 +1082,7 @@ pub fn validate_decimal256_precision(
}
if value > MAX_DECIMAL256_FOR_EACH_PRECISION[precision as usize] {
- let unscaled_value =
- format_decimal_str_internal(&value.to_string(), precision.into(),
scale, false);
+ let unscaled_value = format_decimal_str_internal(&value.to_string(),
scale);
let unscaled_max_value = format_decimal_str(
&MAX_DECIMAL256_FOR_EACH_PRECISION[precision as usize].to_string(),
precision.into(),
@@ -1099,8 +1092,7 @@ pub fn validate_decimal256_precision(
"{unscaled_value} is too large to store in a Decimal256 of
precision {precision}. Max is {unscaled_max_value}"
)))
} else if value < MIN_DECIMAL256_FOR_EACH_PRECISION[precision as usize] {
- let unscaled_value =
- format_decimal_str_internal(&value.to_string(), precision.into(),
scale, false);
+ let unscaled_value = format_decimal_str_internal(&value.to_string(),
scale);
let unscaled_min_value = format_decimal_str(
&MIN_DECIMAL256_FOR_EACH_PRECISION[precision as usize].to_string(),
precision.into(),
@@ -1126,36 +1118,31 @@ pub fn is_validate_decimal256_precision(value: i256,
precision: u8) -> bool {
}
#[inline]
-/// Formats a decimal string given the precision and scale.
-pub fn format_decimal_str(value_str: &str, precision: usize, scale: i8) ->
String {
- format_decimal_str_internal(value_str, precision, scale, true)
+/// Formats a decimal string given the scale.
+///
+/// The value is always formatted in full: `_precision` is unused and retained
+/// only for API compatibility.
+pub fn format_decimal_str(value_str: &str, _precision: usize, scale: i8) ->
String {
+ format_decimal_str_internal(value_str, scale)
}
-// Format a decimal string given the precision and scale.
-// If `safe_decimal` is true, the function will ensure that the output string
-// does not exceed the specified precision.
-fn format_decimal_str_internal(
- value_str: &str,
- precision: usize,
- scale: i8,
- safe_decimal: bool,
-) -> String {
+// Format a decimal string given the scale.
+fn format_decimal_str_internal(value_str: &str, scale: i8) -> String {
let (sign, rest) = match value_str.strip_prefix('-') {
Some(stripped) => ("-", stripped),
None => ("", value_str),
};
- let bound = if safe_decimal {
- precision.min(rest.len()) + sign.len()
- } else {
- value_str.len()
- };
- let value_str = &value_str[0..bound];
if scale == 0 {
value_str.to_string()
} else if scale < 0 {
- let padding = value_str.len() + scale.unsigned_abs() as usize;
- format!("{value_str:0<padding$}")
+ 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$}")
+ }
} 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);
@@ -1165,3 +1152,33 @@ fn format_decimal_str_internal(
format!("{}0.{:0>width$}", sign, rest, width = scale as usize)
}
}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+
+ #[test]
+ fn test_format_decimal_str() {
+ assert_eq!(format_decimal_str("12345", 7, 0), "12345");
+ assert_eq!(format_decimal_str("12345", 7, 2), "123.45");
+ assert_eq!(format_decimal_str("-12345", 7, 2), "-123.45");
+ assert_eq!(format_decimal_str("45", 7, 3), "0.045");
+ assert_eq!(format_decimal_str("-45", 7, 3), "-0.045");
+ assert_eq!(format_decimal_str("0", 7, 2), "0.00");
+ assert_eq!(format_decimal_str("12345", 7, 5), "0.12345");
+
+ // negative scales multiply the value by 10^|scale|
+ assert_eq!(format_decimal_str("12", 7, -2), "1200");
+ assert_eq!(format_decimal_str("-12", 7, -2), "-1200");
+ // a zero value is not padded
+ // https://github.com/apache/arrow-rs/issues/10865
+ assert_eq!(format_decimal_str("0", 7, -2), "0");
+
+ // values exceeding the declared precision are still formatted in full
+ // https://github.com/apache/arrow-rs/issues/10866
+ assert_eq!(format_decimal_str("12345", 3, 1), "1234.5");
+ assert_eq!(format_decimal_str("12345", 3, 3), "12.345");
+ assert_eq!(format_decimal_str("-12345", 3, 3), "-12.345");
+ assert_eq!(format_decimal_str("12345", 2, 4), "1.2345");
+ }
+}
diff --git a/arrow-json/src/writer/mod.rs b/arrow-json/src/writer/mod.rs
index 8f1eefe548..6937d2d498 100644
--- a/arrow-json/src/writer/mod.rs
+++ b/arrow-json/src/writer/mod.rs
@@ -2216,6 +2216,31 @@ mod tests {
);
}
+ #[test]
+ fn test_decimal_encoder_negative_scale() {
+ // https://github.com/apache/arrow-rs/issues/10865
+ let array = Decimal128Array::from_iter([Some(0), Some(12), Some(-12)])
+ .with_precision_and_scale(10, -2)
+ .unwrap();
+ let field = Arc::new(Field::new("decimal", array.data_type().clone(),
true));
+ let schema = Schema::new(vec![field]);
+ let batch = RecordBatch::try_new(Arc::new(schema),
vec![Arc::new(array)]).unwrap();
+
+ let mut buf = Vec::new();
+ {
+ let mut writer = LineDelimitedWriter::new(&mut buf);
+ writer.write_batches(&[&batch]).unwrap();
+ }
+
+ assert_json_eq(
+ &buf,
+ r#"{"decimal":0}
+{"decimal":1200}
+{"decimal":-1200}
+"#,
+ );
+ }
+
#[test]
fn write_structs_as_list() {
let schema = Schema::new(vec