Skip to content

Commit 265473e

Browse files
kosiewkosiew
authored
Refactor Spark format_string numeric %c conversion dispatch (#22166)
## Which issue does this PR close? * Part of #22163 ## Rationale for this change The `%c` conversion path in Spark `format_string` duplicated nearly identical logic across all signed and unsigned integer scalar variants. This duplication increased maintenance overhead and the risk of future `%c` fixes being applied inconsistently. This change centralizes numeric scalar-to-character conversion in a single helper while preserving existing behavior and error propagation semantics. ## What changes are included in this PR? * Added a local `integer_scalar_to_char` helper to centralize integer scalar `%c` conversion handling. * Routed all non-null integer `%c` conversion paths through the new helper. * Removed duplicated `%c` conversion branches from individual integer scalar match arms. * Preserved existing signed/unsigned conversion semantics by continuing to use the existing `signed_to_char` and `unsigned_to_char` helpers. * Left all non-`%c` formatting behavior unchanged. ## Are these changes tested? No new tests were added in this PR. This refactor is intended to preserve existing behavior, and existing `%c` formatting tests continue to validate valid and invalid code point handling. Suggested validation command: ```bash cargo test -p datafusion-spark format_char ``` ## Are there any user-facing changes? No. This is a maintainability refactor only and is not intended to change observable behavior or error messages. ## LLM-generated code disclosure This PR includes LLM-generated code and comments. All LLM-generated content has been manually reviewed and tested.
1 parent e80076b commit 265473e

1 file changed

Lines changed: 32 additions & 24 deletions

File tree

‎datafusion/spark/src/function/string/format_string.rs‎

Lines changed: 32 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -891,6 +891,23 @@ fn unsigned_to_char(value: u64) -> Result<char> {
891891
codepoint_to_char(codepoint)
892892
}
893893

894+
/// Convert a non-null integer scalar to a [`char`] for the `%c` conversion.
895+
fn integer_scalar_to_char(scalar: &ScalarValue) -> Result<char> {
896+
match scalar {
897+
ScalarValue::Int8(Some(value)) => signed_to_char(*value as i64),
898+
ScalarValue::Int16(Some(value)) => signed_to_char(*value as i64),
899+
ScalarValue::Int32(Some(value)) => signed_to_char(*value as i64),
900+
ScalarValue::Int64(Some(value)) => signed_to_char(*value),
901+
ScalarValue::UInt8(Some(value)) => unsigned_to_char(*value as u64),
902+
ScalarValue::UInt16(Some(value)) => unsigned_to_char(*value as u64),
903+
ScalarValue::UInt32(Some(value)) => unsigned_to_char(*value as u64),
904+
ScalarValue::UInt64(Some(value)) => unsigned_to_char(*value),
905+
_ => datafusion_common::internal_err!(
906+
"integer_scalar_to_char expects a non-null integer scalar, got {scalar:?}"
907+
),
908+
}
909+
}
910+
894911
impl ConversionSpecifier {
895912
/// Validates that the grouping separator flag is not used with scientific
896913
/// notation conversions, matching Java/Spark behavior which throws
@@ -923,6 +940,21 @@ impl ConversionSpecifier {
923940

924941
_ => self.format_boolean(string, value),
925942
},
943+
ScalarValue::Int8(Some(_))
944+
| ScalarValue::Int16(Some(_))
945+
| ScalarValue::Int32(Some(_))
946+
| ScalarValue::Int64(Some(_))
947+
| ScalarValue::UInt8(Some(_))
948+
| ScalarValue::UInt16(Some(_))
949+
| ScalarValue::UInt32(Some(_))
950+
| ScalarValue::UInt64(Some(_))
951+
if matches!(
952+
self.conversion_type,
953+
ConversionType::CharLower | ConversionType::CharUpper
954+
) =>
955+
{
956+
self.format_char(string, integer_scalar_to_char(value)?)
957+
}
926958
ScalarValue::Int8(value) => match (self.conversion_type, value) {
927959
(ConversionType::DecInt, Some(value)) => {
928960
self.format_signed(string, *value as i64)
@@ -933,9 +965,6 @@ impl ConversionSpecifier {
933965
| ConversionType::OctInt,
934966
Some(value),
935967
) => self.format_unsigned(string, (*value as u8) as u64),
936-
(ConversionType::CharLower | ConversionType::CharUpper, Some(value)) => {
937-
self.format_char(string, signed_to_char(*value as i64)?)
938-
}
939968
(
940969
ConversionType::StringLower | ConversionType::StringUpper,
941970
Some(value),
@@ -952,9 +981,6 @@ impl ConversionSpecifier {
952981
(ConversionType::DecInt, Some(value)) => {
953982
self.format_signed(string, *value as i64)
954983
}
955-
(ConversionType::CharLower | ConversionType::CharUpper, Some(value)) => {
956-
self.format_char(string, signed_to_char(*value as i64)?)
957-
}
958984
(
959985
ConversionType::HexIntLower
960986
| ConversionType::HexIntUpper
@@ -983,9 +1009,6 @@ impl ConversionSpecifier {
9831009
| ConversionType::OctInt,
9841010
Some(value),
9851011
) => self.format_unsigned(string, (*value as u32) as u64),
986-
(ConversionType::CharLower | ConversionType::CharUpper, Some(value)) => {
987-
self.format_char(string, signed_to_char(*value as i64)?)
988-
}
9891012
(
9901013
ConversionType::StringLower | ConversionType::StringUpper,
9911014
Some(value),
@@ -1008,9 +1031,6 @@ impl ConversionSpecifier {
10081031
| ConversionType::OctInt,
10091032
Some(value),
10101033
) => self.format_unsigned(string, *value as u64),
1011-
(ConversionType::CharLower | ConversionType::CharUpper, Some(value)) => {
1012-
self.format_char(string, signed_to_char(*value)?)
1013-
}
10141034
(
10151035
ConversionType::StringLower | ConversionType::StringUpper,
10161036
Some(value),
@@ -1031,9 +1051,6 @@ impl ConversionSpecifier {
10311051
| ConversionType::OctInt,
10321052
Some(value),
10331053
) => self.format_unsigned(string, *value as u64),
1034-
(ConversionType::CharLower | ConversionType::CharUpper, Some(value)) => {
1035-
self.format_char(string, unsigned_to_char(*value as u64)?)
1036-
}
10371054
(
10381055
ConversionType::StringLower | ConversionType::StringUpper,
10391056
Some(value),
@@ -1054,9 +1071,6 @@ impl ConversionSpecifier {
10541071
| ConversionType::OctInt,
10551072
Some(value),
10561073
) => self.format_unsigned(string, *value as u64),
1057-
(ConversionType::CharLower | ConversionType::CharUpper, Some(value)) => {
1058-
self.format_char(string, unsigned_to_char(*value as u64)?)
1059-
}
10601074
(
10611075
ConversionType::StringLower | ConversionType::StringUpper,
10621076
Some(value),
@@ -1077,9 +1091,6 @@ impl ConversionSpecifier {
10771091
| ConversionType::OctInt,
10781092
Some(value),
10791093
) => self.format_unsigned(string, *value as u64),
1080-
(ConversionType::CharLower | ConversionType::CharUpper, Some(value)) => {
1081-
self.format_char(string, unsigned_to_char(*value as u64)?)
1082-
}
10831094
(
10841095
ConversionType::StringLower | ConversionType::StringUpper,
10851096
Some(value),
@@ -1100,9 +1111,6 @@ impl ConversionSpecifier {
11001111
| ConversionType::OctInt,
11011112
Some(value),
11021113
) => self.format_unsigned(string, *value),
1103-
(ConversionType::CharLower | ConversionType::CharUpper, Some(value)) => {
1104-
self.format_char(string, unsigned_to_char(*value)?)
1105-
}
11061114
(
11071115
ConversionType::StringLower | ConversionType::StringUpper,
11081116
Some(value),

0 commit comments

Comments
 (0)