You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@datafusion.apache.org by gi...@apache.org on 2026/04/30 11:20:57 UTC
(datafusion) branch gh-readonly-queue/main/pr-20268-014457014d28806cee34c51ebf3587148188a303 created (now 13642865b5)
This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a change to branch gh-readonly-queue/main/pr-20268-014457014d28806cee34c51ebf3587148188a303
in repository https://gitbox.apache.org/repos/asf/datafusion.git
at 13642865b5 fix: grouping separator for float and decimal (#20268)
This branch includes the following new commits:
new 13642865b5 fix: grouping separator for float and decimal (#20268)
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datafusion.apache.org
For additional commands, e-mail: commits-help@datafusion.apache.org
(datafusion) 01/01: fix: grouping separator for float and decimal (#20268)
Posted by gi...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-20268-014457014d28806cee34c51ebf3587148188a303
in repository https://gitbox.apache.org/repos/asf/datafusion.git
commit 13642865b526b32fbef8d6c29c1f5db4abe8c6b6
Author: Druva <dd...@gmail.com>
AuthorDate: Thu Apr 30 16:50:18 2026 +0530
fix: grouping separator for float and decimal (#20268)
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and
enhancements and this helps us generate change logs for our releases.
You can link an issue to this PR using the GitHub syntax. For example
`Closes #123` indicates that this PR will close issue #123.
-->
- Closes https://github.com/apache/datafusion/issues/20266
<!--
Why are you proposing this change? If this is already explained clearly
in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand
your changes and offer better suggestions for fixes.
-->
## What changes are included in this PR?
Created a new function to insert the separator (,) into the numbers if
the flag is enabled
<!--
There is no need to duplicate the description in the issue here but it
is sometimes worth providing a summary of the individual changes in this
PR.
-->
## Are these changes tested?
Tests passed locally
Added unit tests to verify functionality
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example,
are they covered by existing tests)?
-->
## Are there any user-facing changes?
No
<!--
If there are user-facing changes then we may require documentation to be
updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api
change` label.
-->
No changes to public api
## Additional Info
Claude was used to assist in identifying the source of the issue.
---
.../spark/src/function/string/format_string.rs | 417 ++++++++++++++++++++-
1 file changed, 401 insertions(+), 16 deletions(-)
diff --git a/datafusion/spark/src/function/string/format_string.rs b/datafusion/spark/src/function/string/format_string.rs
index 8a03c7ee38..ad97841c4a 100644
--- a/datafusion/spark/src/function/string/format_string.rs
+++ b/datafusion/spark/src/function/string/format_string.rs
@@ -862,6 +862,28 @@ fn take_numeric_param(s: &str, zero: bool) -> (NumericParam, &str) {
}
impl ConversionSpecifier {
+ /// Validates that the grouping separator flag is not used with scientific
+ /// notation conversions, matching Java/Spark behavior which throws
+ /// `FormatFlagsConversionMismatchException` for `%,e` / `%,E`.
+ fn validate_grouping_separator(&self) -> Result<()> {
+ if self.grouping_separator
+ && matches!(
+ self.conversion_type,
+ ConversionType::SciFloatLower | ConversionType::SciFloatUpper
+ )
+ {
+ return exec_err!(
+ "Grouping separator ',' flag is not compatible with scientific notation conversion '{}'",
+ if self.conversion_type == ConversionType::SciFloatUpper {
+ 'E'
+ } else {
+ 'e'
+ }
+ );
+ }
+ Ok(())
+ }
+
pub fn format(&self, string: &mut String, value: &ScalarValue) -> Result<()> {
match value {
ScalarValue::Boolean(value) => match self.conversion_type {
@@ -1690,6 +1712,8 @@ impl ConversionSpecifier {
}
fn format_float(&self, writer: &mut String, value: f64) -> Result<()> {
+ self.validate_grouping_separator()?;
+
let mut prefix = String::new();
let mut suffix = String::new();
let mut number = String::new();
@@ -1770,6 +1794,9 @@ impl ConversionSpecifier {
if strip_trailing_0s {
number = trim_trailing_0s(&number).to_owned();
}
+ if self.grouping_separator {
+ number = insert_thousands_separator(&number);
+ }
}
if self.alt_form && !number.contains('.') {
number += ".";
@@ -1882,20 +1909,11 @@ impl ConversionSpecifier {
match self.conversion_type {
ConversionType::DecInt => {
let num_str = format!("{value}");
- if self.grouping_separator {
- // Add thousands separators
- let mut result = String::new();
- let chars: Vec<char> = num_str.chars().collect();
- for (i, c) in chars.iter().enumerate() {
- if i > 0 && (chars.len() - i).is_multiple_of(3) {
- result.push(',');
- }
- result.push(*c);
- }
- s = result;
+ s = if self.grouping_separator {
+ insert_thousands_separator(&num_str)
} else {
- s = num_str;
- }
+ num_str
+ };
}
ConversionType::HexIntLower => {
alt_prefix = "0x";
@@ -2000,6 +2018,8 @@ impl ConversionSpecifier {
}
fn format_decimal(&self, writer: &mut String, value: &str, scale: i64) -> Result<()> {
+ self.validate_grouping_separator()?;
+
let mut prefix = String::new();
let upper = self.conversion_type.is_upper();
@@ -2010,6 +2030,10 @@ impl ConversionSpecifier {
let decimal = BigDecimal::from_bigint(decimal, scale);
// Handle sign
+ // TODO: `negative_in_parentheses` (the `(` flag) is not implemented here.
+ // Java/Spark wrap negative values in parentheses when this flag is set
+ // (e.g. `%(,.2f` with -1234.5 → "(1,234.50)"), but this path always
+ // uses a minus sign. See `format_float` for the correct implementation.
let is_negative = decimal.sign() == Sign::Minus;
let abs_decimal = decimal.abs();
@@ -2033,7 +2057,15 @@ impl ConversionSpecifier {
let number = match self.conversion_type {
ConversionType::DecFloatLower => {
// Format as fixed-point decimal
- self.format_decimal_fixed(&abs_decimal, precision, strip_trailing_0s)?
+ let mut n = self.format_decimal_fixed(
+ &abs_decimal,
+ precision,
+ strip_trailing_0s,
+ )?;
+ if self.grouping_separator {
+ n = insert_thousands_separator(&n);
+ }
+ n
}
ConversionType::SciFloatLower => self.format_decimal_scientific(
&abs_decimal,
@@ -2062,11 +2094,15 @@ impl ConversionSpecifier {
strip_trailing_0s,
)?
} else {
- self.format_decimal_fixed(
+ let mut n = self.format_decimal_fixed(
&abs_decimal,
precision - 1 - log10_val.floor() as i32,
strip_trailing_0s,
- )?
+ )?;
+ if self.grouping_separator {
+ n = insert_thousands_separator(&n);
+ }
+ n
}
}
_ => {
@@ -2332,6 +2368,24 @@ impl FloatBits for f64 {
}
}
+/// Inserts thousands separators (`,`) into the integer part of a numeric string.
+/// For example, `"1234567.89"` becomes `"1,234,567.89"`.
+fn insert_thousands_separator(number: &str) -> String {
+ let (int_part, frac_part) = match number.find('.') {
+ Some(pos) => (&number[..pos], &number[pos..]),
+ None => (number, ""),
+ };
+ let mut result = String::with_capacity(number.len() + number.len() / 3);
+ for (i, c) in int_part.char_indices() {
+ if i > 0 && (int_part.len() - i) % 3 == 0 {
+ result.push(',');
+ }
+ result.push(c);
+ }
+ result.push_str(frac_part);
+ result
+}
+
fn trim_trailing_0s(number: &str) -> &str {
if number.contains('.') {
for (i, c) in number.chars().rev().enumerate() {
@@ -2355,6 +2409,8 @@ fn trim_trailing_0s_hex(number: &str) -> &str {
#[cfg(test)]
mod tests {
use super::*;
+ use crate::function::utils::test::test_scalar_function;
+ use arrow::array::StringArray;
use arrow::datatypes::DataType::Utf8;
#[test]
@@ -2385,4 +2441,333 @@ mod tests {
Ok(())
}
+
+ #[test]
+ fn test_insert_thousands_separator() {
+ assert_eq!(insert_thousands_separator("1234567.89"), "1,234,567.89");
+ assert_eq!(insert_thousands_separator("123.45"), "123.45");
+ assert_eq!(insert_thousands_separator("1234"), "1,234");
+ assert_eq!(insert_thousands_separator("12"), "12");
+ assert_eq!(insert_thousands_separator("0.5"), "0.5");
+ assert_eq!(
+ insert_thousands_separator("1234567890.1234"),
+ "1,234,567,890.1234"
+ );
+ assert_eq!(insert_thousands_separator("1000"), "1,000");
+ assert_eq!(insert_thousands_separator("100"), "100");
+ }
+
+ #[test]
+ fn test_grouping_separator_float() -> Result<()> {
+ test_scalar_function!(
+ FormatStringFunc::new(),
+ vec![
+ ColumnarValue::Scalar(ScalarValue::Utf8(Some("%,.2f".to_string()))),
+ ColumnarValue::Scalar(ScalarValue::Float64(Some(1234567.89))),
+ ],
+ Ok(Some("1,234,567.89")),
+ &str,
+ Utf8,
+ StringArray
+ );
+ Ok(())
+ }
+
+ #[test]
+ fn test_grouping_separator_decimal() -> Result<()> {
+ test_scalar_function!(
+ FormatStringFunc::new(),
+ vec![
+ ColumnarValue::Scalar(ScalarValue::Utf8(Some("%,.2f".to_string()))),
+ ColumnarValue::Scalar(ScalarValue::Decimal128(Some(123456789), 10, 2)),
+ ],
+ Ok(Some("1,234,567.89")),
+ &str,
+ Utf8,
+ StringArray
+ );
+ Ok(())
+ }
+
+ #[test]
+ fn test_grouping_separator_scientific_float() -> Result<()> {
+ // %,e — Java/Spark reject grouping separator with scientific notation
+ test_scalar_function!(
+ FormatStringFunc::new(),
+ vec![
+ ColumnarValue::Scalar(ScalarValue::Utf8(Some("%,e".to_string()))),
+ ColumnarValue::Scalar(ScalarValue::Float64(Some(1234567.89))),
+ ],
+ Err(DataFusionError::Execution(
+ "Grouping separator ',' flag is not compatible with scientific notation conversion 'e'".to_string(),
+ )),
+ &str,
+ Utf8,
+ StringArray
+ );
+ // %,E — uppercase scientific also rejected
+ test_scalar_function!(
+ FormatStringFunc::new(),
+ vec![
+ ColumnarValue::Scalar(ScalarValue::Utf8(Some("%,E".to_string()))),
+ ColumnarValue::Scalar(ScalarValue::Float64(Some(1234567.89))),
+ ],
+ Err(DataFusionError::Execution(
+ "Grouping separator ',' flag is not compatible with scientific notation conversion 'E'".to_string(),
+ )),
+ &str,
+ Utf8,
+ StringArray
+ );
+ // %,.0e — precision 0 scientific with grouping also rejected
+ test_scalar_function!(
+ FormatStringFunc::new(),
+ vec![
+ ColumnarValue::Scalar(ScalarValue::Utf8(Some("%,.0e".to_string()))),
+ ColumnarValue::Scalar(ScalarValue::Float64(Some(1234567.89))),
+ ],
+ Err(DataFusionError::Execution(
+ "Grouping separator ',' flag is not compatible with scientific notation conversion 'e'".to_string(),
+ )),
+ &str,
+ Utf8,
+ StringArray
+ );
+ Ok(())
+ }
+
+ #[test]
+ fn test_grouping_separator_compact_float() -> Result<()> {
+ // %,g with large number — triggers scientific, no commas
+ test_scalar_function!(
+ FormatStringFunc::new(),
+ vec![
+ ColumnarValue::Scalar(ScalarValue::Utf8(Some("%,g".to_string()))),
+ ColumnarValue::Scalar(ScalarValue::Float64(Some(1234567.89))),
+ ],
+ Ok(Some("1.23457e+06")),
+ &str,
+ Utf8,
+ StringArray
+ );
+ // %,g with small number — triggers fixed-point, commas in integer part
+ test_scalar_function!(
+ FormatStringFunc::new(),
+ vec![
+ ColumnarValue::Scalar(ScalarValue::Utf8(Some("%,g".to_string()))),
+ ColumnarValue::Scalar(ScalarValue::Float64(Some(12345.6))),
+ ],
+ Ok(Some("12,345.6")),
+ &str,
+ Utf8,
+ StringArray
+ );
+ // %,.0g — precision 0 compact with grouping (large number, scientific)
+ test_scalar_function!(
+ FormatStringFunc::new(),
+ vec![
+ ColumnarValue::Scalar(ScalarValue::Utf8(Some("%,.0g".to_string()))),
+ ColumnarValue::Scalar(ScalarValue::Float64(Some(1234567.89))),
+ ],
+ Ok(Some("1e+06")),
+ &str,
+ Utf8,
+ StringArray
+ );
+ // %,G — uppercase compact
+ test_scalar_function!(
+ FormatStringFunc::new(),
+ vec![
+ ColumnarValue::Scalar(ScalarValue::Utf8(Some("%,G".to_string()))),
+ ColumnarValue::Scalar(ScalarValue::Float64(Some(1234567.89))),
+ ],
+ Ok(Some("1.23457E+06")),
+ &str,
+ Utf8,
+ StringArray
+ );
+ Ok(())
+ }
+
+ #[test]
+ fn test_grouping_separator_scientific_decimal() -> Result<()> {
+ // %,e on decimal — Java/Spark reject grouping separator with scientific notation
+ test_scalar_function!(
+ FormatStringFunc::new(),
+ vec![
+ ColumnarValue::Scalar(ScalarValue::Utf8(Some("%,e".to_string()))),
+ ColumnarValue::Scalar(ScalarValue::Decimal128(Some(123456789), 10, 2)),
+ ],
+ Err(DataFusionError::Execution(
+ "Grouping separator ',' flag is not compatible with scientific notation conversion 'e'".to_string(),
+ )),
+ &str,
+ Utf8,
+ StringArray
+ );
+ // %,.0e on decimal — also rejected
+ test_scalar_function!(
+ FormatStringFunc::new(),
+ vec![
+ ColumnarValue::Scalar(ScalarValue::Utf8(Some("%,.0e".to_string()))),
+ ColumnarValue::Scalar(ScalarValue::Decimal128(Some(123456789), 10, 2)),
+ ],
+ Err(DataFusionError::Execution(
+ "Grouping separator ',' flag is not compatible with scientific notation conversion 'e'".to_string(),
+ )),
+ &str,
+ Utf8,
+ StringArray
+ );
+ Ok(())
+ }
+
+ #[test]
+ fn test_grouping_separator_compact_decimal() -> Result<()> {
+ // %,g on decimal — large number triggers scientific, no commas
+ test_scalar_function!(
+ FormatStringFunc::new(),
+ vec![
+ ColumnarValue::Scalar(ScalarValue::Utf8(Some("%,g".to_string()))),
+ ColumnarValue::Scalar(ScalarValue::Decimal128(Some(123456789), 10, 2)),
+ ],
+ Ok(Some("1.23457e+06")),
+ &str,
+ Utf8,
+ StringArray
+ );
+ // %,g on decimal — small number triggers fixed-point, commas expected
+ test_scalar_function!(
+ FormatStringFunc::new(),
+ vec![
+ ColumnarValue::Scalar(ScalarValue::Utf8(Some("%,g".to_string()))),
+ ColumnarValue::Scalar(ScalarValue::Decimal128(Some(1234560), 10, 2)),
+ ],
+ Ok(Some("12,345.6")),
+ &str,
+ Utf8,
+ StringArray
+ );
+ // %,.0g on decimal — precision 0 compact with grouping (scientific)
+ test_scalar_function!(
+ FormatStringFunc::new(),
+ vec![
+ ColumnarValue::Scalar(ScalarValue::Utf8(Some("%,.0g".to_string()))),
+ ColumnarValue::Scalar(ScalarValue::Decimal128(Some(123456789), 10, 2)),
+ ],
+ Ok(Some("1e+06")),
+ &str,
+ Utf8,
+ StringArray
+ );
+ Ok(())
+ }
+
+ #[test]
+ fn test_grouping_separator_width_sign_float() -> Result<()> {
+ // %0,15.2f — zero-pad + grouping + width
+ test_scalar_function!(
+ FormatStringFunc::new(),
+ vec![
+ ColumnarValue::Scalar(ScalarValue::Utf8(Some("%0,15.2f".to_string()))),
+ ColumnarValue::Scalar(ScalarValue::Float64(Some(1234567.89))),
+ ],
+ Ok(Some("0001,234,567.89")),
+ &str,
+ Utf8,
+ StringArray
+ );
+ // %+,15.2f — force-sign + grouping + width (space-padded)
+ test_scalar_function!(
+ FormatStringFunc::new(),
+ vec![
+ ColumnarValue::Scalar(ScalarValue::Utf8(Some("%+,15.2f".to_string()))),
+ ColumnarValue::Scalar(ScalarValue::Float64(Some(1234567.89))),
+ ],
+ Ok(Some(" +1,234,567.89")),
+ &str,
+ Utf8,
+ StringArray
+ );
+ // %-,15.2f — left-adjust + grouping + width
+ test_scalar_function!(
+ FormatStringFunc::new(),
+ vec![
+ ColumnarValue::Scalar(ScalarValue::Utf8(Some("%-,15.2f".to_string()))),
+ ColumnarValue::Scalar(ScalarValue::Float64(Some(1234567.89))),
+ ],
+ Ok(Some("1,234,567.89 ")),
+ &str,
+ Utf8,
+ StringArray
+ );
+ Ok(())
+ }
+
+ #[test]
+ fn test_grouping_separator_width_sign_decimal() -> Result<()> {
+ // %0,15.2f — zero-pad + grouping + width on decimal
+ test_scalar_function!(
+ FormatStringFunc::new(),
+ vec![
+ ColumnarValue::Scalar(ScalarValue::Utf8(Some("%0,15.2f".to_string()))),
+ ColumnarValue::Scalar(ScalarValue::Decimal128(Some(123456789), 10, 2)),
+ ],
+ Ok(Some("0001,234,567.89")),
+ &str,
+ Utf8,
+ StringArray
+ );
+ // %+,15.2f — force-sign + grouping + width on decimal
+ test_scalar_function!(
+ FormatStringFunc::new(),
+ vec![
+ ColumnarValue::Scalar(ScalarValue::Utf8(Some("%+,15.2f".to_string()))),
+ ColumnarValue::Scalar(ScalarValue::Decimal128(Some(123456789), 10, 2)),
+ ],
+ Ok(Some(" +1,234,567.89")),
+ &str,
+ Utf8,
+ StringArray
+ );
+ Ok(())
+ }
+
+ #[test]
+ fn test_grouping_separator_parentheses_float() -> Result<()> {
+ // %(,15.2f with negative — parentheses + grouping + width
+ // Java: String.format("%(,15.2f", -1234.5) → " (1,234.50)"
+ test_scalar_function!(
+ FormatStringFunc::new(),
+ vec![
+ ColumnarValue::Scalar(ScalarValue::Utf8(Some("%(,15.2f".to_string()))),
+ ColumnarValue::Scalar(ScalarValue::Float64(Some(-1234.5))),
+ ],
+ Ok(Some(" (1,234.50)")),
+ &str,
+ Utf8,
+ StringArray
+ );
+ Ok(())
+ }
+
+ #[test]
+ fn test_grouping_separator_parentheses_decimal() -> Result<()> {
+ // %(,15.2f on negative decimal — format_decimal ignores negative_in_parentheses,
+ // always uses '-'. Check TODO in fn format_decimal
+ // Java: String.format("%(,15.2f", -1234.5) → " (1,234.50)"
+ // Ours: " -1,234.50" (minus sign, no parens)
+ test_scalar_function!(
+ FormatStringFunc::new(),
+ vec![
+ ColumnarValue::Scalar(ScalarValue::Utf8(Some("%(,15.2f".to_string()))),
+ ColumnarValue::Scalar(ScalarValue::Decimal128(Some(-123450), 10, 2)),
+ ],
+ Ok(Some(" -1,234.50")),
+ &str,
+ Utf8,
+ StringArray
+ );
+ Ok(())
+ }
}
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datafusion.apache.org
For additional commands, e-mail: commits-help@datafusion.apache.org