cloud-fan commented on code in PR #58408:
URL: https://github.com/apache/spark/pull/58408#discussion_r3904348405
##########
docs/sql-migration-guide.md:
##########
@@ -24,6 +24,7 @@ license: |
## Upgrading from Spark SQL 4.3 to 4.4
+- Since Spark 4.4, `format_string` and `printf` pass a decimal argument to
`java.util.Formatter` as a `java.math.BigDecimal`, so the floating-point
conversions (`%f`, `%e`, `%g`, `%a`) format decimals instead of failing with
`IllegalFormatConversionException`. As a side effect, `%h` on a decimal prints
a different hash than earlier releases, because it now hashes the
`java.math.BigDecimal` (unscaled value and scale) rather than Spark's internal
`Decimal` (which hashes consistently with `Double`). `%s` output and `NULL`
handling are unchanged.
Review Comment:
**Non-blocking (P2):** `java.util.Formatter` explicitly does not support
`%a` for `BigDecimal`. Both new paths pass a `BigDecimal`, so
`format_string('%a', CAST(1.5 AS DECIMAL(2,1)))` still throws
`IllegalFormatConversionException`. Given how specialized this conversion is,
please list only `%e`, `%f`, and `%g` here and in the source comment, and add a
focused assertion that decimal `%a` remains unsupported so the limitation
cannot be advertised accidentally.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala:
##########
@@ -2404,7 +2411,10 @@ case class FormatString(children: Expression*) extends
Expression with ImplicitC
val numArgLists = argListGen.length
val argListCode = argListGen.zipWithIndex.map { case(v, index) =>
val value =
- if (CodeGenerator.boxedType(v._1) != CodeGenerator.javaType(v._1)) {
+ if (v._1.isInstanceOf[DecimalType]) {
Review Comment:
**Blocking (P1):** Please unwrap a UDT before deciding whether this is a
decimal. `CodeGenerator.javaType` and the generated row accessors use a
`UserDefinedType`'s underlying `sqlType`, but this direct predicate sees only
the declared UDT. For a UDT backed by `DecimalType`, interpreted evaluation
sees the physical `Decimal` and converts it, while codegen passes the raw
Catalyst `Decimal` to `Formatter`; `%f` therefore succeeds interpreted and
throws under codegen. Using the underlying SQL type here and adding a
decimal-backed UDT case to the existing interpreted/codegen test would keep the
two paths aligned.
--
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]
