kosiew commented on code in PR #22388:
URL: https://github.com/apache/datafusion/pull/22388#discussion_r3301846504


##########
datafusion/spark/src/function/string/format_string.rs:
##########
@@ -891,23 +891,82 @@ fn unsigned_to_char(value: u64) -> Result<char> {
     codepoint_to_char(codepoint)
 }
 
-/// Convert a non-null integer scalar to a [`char`] for the `%c` conversion.
-fn integer_scalar_to_char(scalar: &ScalarValue) -> Result<char> {
-    match scalar {
-        ScalarValue::Int8(Some(value)) => signed_to_char(*value as i64),
-        ScalarValue::Int16(Some(value)) => signed_to_char(*value as i64),
-        ScalarValue::Int32(Some(value)) => signed_to_char(*value as i64),
-        ScalarValue::Int64(Some(value)) => signed_to_char(*value),
-        ScalarValue::UInt8(Some(value)) => unsigned_to_char(*value as u64),
-        ScalarValue::UInt16(Some(value)) => unsigned_to_char(*value as u64),
-        ScalarValue::UInt32(Some(value)) => unsigned_to_char(*value as u64),
-        ScalarValue::UInt64(Some(value)) => unsigned_to_char(*value),
-        _ => datafusion_common::internal_err!(
-            "integer_scalar_to_char expects a non-null integer scalar, got 
{scalar:?}"
-        ),
+/// Normalizes integer scalar payloads while preserving Spark formatting 
semantics:
+/// signed values format as decimal for `%d` / `%s` / `%c`, but use their 
original
+/// bit width for `%x` / `%o` via `unsigned_bits`.
+#[derive(Debug, Clone, Copy)]
+enum IntegerValue {

Review Comment:
   💡 Good idea!
   
   I will replace this with a narrow local trait implemented directly for the 
primitive integer types (`i8`/`i16`/`i32`/`i64` and `u8`/`u16`/`u32`/`u64`). 
The shared `format_integer` helper can then be generic over that trait, while 
each primitive supplies the few operations where signedness/width matter: 
decimal formatting, `%x`/`%o` unsigned bit representation, `%c`, and `%s` 
string rendering.
   
   This should preserve the current behavior and the added width/null 
regression coverage, but avoid the intermediary enum and make the dispatch 
intent more direct.



-- 
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]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to