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


##########
datafusion/spark/src/function/string/format_string.rs:
##########
@@ -2896,17 +2900,26 @@ mod tests {
 
     #[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
+        test_scalar_function!(

Review Comment:
   Nice addition to the regression tests. Would it be worth adding a Decimal256 
case for the same `%(...)` decimal formatting regression as well?
   
   I believe the implementation path is shared between Decimal128 and 
Decimal256, so I do not think this is required for correctness. That said, 
having both public decimal scalar variants covered would help lock the behavior 
in and guard against future regressions.



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