HyukjinKwon commented on PR #58786:
URL: https://github.com/apache/spark/pull/58786#issuecomment-5672847659
Reviewed as part of a batch pass (read-only). The change is reasonable and
consistent with
other dialects (Teradata uses `SYSTEM_DEFAULT` = `Decimal(38,18)` for the
same "unknown scale"
situation), and it does reduce fractional truncation as described.
One compatibility point worth calling out explicitly, since the description
frames this as pure
upside: this is a two-sided trade-off, not just a precision gain.
`Decimal(38,10)` has 28
integer digits; `Decimal(38,18)` has only 20. So while fractions gain 8
digits, the integer
range loses 8.
Failure scenario: an Oracle bare `NUMBER` column holding a large-magnitude
value such as
`12345678901234567890123` (23 integer digits) previously fit in
`Decimal(38,10)` and read back
correctly; it now overflows `Decimal(38,18)` (max 20 integer digits) and
comes back as `NULL`
(non-ANSI) or raises (ANSI). Bare `NUMBER` used as a large integer
key/counter is a common
Oracle pattern, so this can silently break existing reads in the opposite
direction from the
one the PR fixes.
That may well be an acceptable trade (18 is Spark's own default and the old
10 was admittedly
arbitrary), but given it's an unconditional change to a long-standing JDBC
type mapping, please
consider:
1. A note in `docs/sql-migration-guide.md` documenting the `Decimal(38,10)`
-> `Decimal(38,18)`
change for Oracle bare `NUMBER` / `FLOAT`, including the integer-range
reduction.
2. Whether a legacy config (to restore `10`) is warranted for users with
large-magnitude bare
`NUMBER` columns — Spark usually gates behavior changes of this kind.
Minor: the new test duplicates the two assertions that the existing
`OracleDialect jdbc type mapping` test (just above) was already updated to
cover; the only net-new
assertion is the explicit-precision `== None` case. Could fold that one case
into the existing
test rather than adding a near-duplicate suite entry. Not blocking.
--
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]
