sarutak commented on PR #58786:
URL: https://github.com/apache/spark/pull/58786#issuecomment-5674830279
Thank you for the fix, @shrirangmhalgi.
One correction to the overflow behavior, since it makes the case for gating
stronger. I traced the read path and it does not degrade to `NULL` in non-ANSI
mode; it is an unconditional failure regardless of ANSI:
- `JDBCValueGetter.DecimalGetter.apply` ->
`nullSafeConvert(rs.getBigDecimal(...), d => Decimal(d, precision, scale))`
- `Decimal(d, 38, 18)` -> `Decimal.set(BigDecimal, 38, 18)`, which does
`setScale(18, ROUND_HALF_UP)` and then throws
`SparkArithmeticException("NUMERIC_VALUE_OUT_OF_RANGE...")` whenever
`decimalVal.precision > 38`.
`nullSafeConvert` only guards the `null` input case, so the exception
propagates. With `Decimal(38,18)` the integer part shrinks from 28 to 20
digits, so a bare `NUMBER` holding a 21-28 digit integer (large IDs / counters
/ epoch nanos, common in Oracle) that reads fine today would fail the query
after this change, in both ANSI and non-ANSI. So the regression is a hard read
failure, not a silent `NULL`.
Given that, I would second the two requests and add a couple:
1. Gate behind a legacy config. There is a direct precedent in this same
dialect: `spark.sql.legacy.oracle.timestampMapping.enabled` /
`spark.sql.legacy.oracle.timestampNTZMapping.enabled` (`SQLConf`), for exactly
this kind of Oracle type-mapping change. A
`spark.sql.legacy.oracle.numberMapping`-style flag that restores `scale=10`
would let users with large-magnitude bare `NUMBER` columns opt out.
2. Document in `docs/sql-migration-guide.md`, mirroring the existing Oracle
`TIMESTAMP` entries, and explicitly note the integer-range reduction (28 -> 20
digits), not just the fractional gain.
3. On the Teradata comparison: `TeradataDialect` uses `SYSTEM_DEFAULT` to
work around JDBC reporting precision 40 (above `MAX_PRECISION`), which is a
different situation from Oracle's `NUMBER` that already fits in 38. The closer
precedent is the gated `TIMESTAMP` change in this dialect.
4. Tests: beyond folding the duplicated assertions into the existing
`OracleDialect jdbc type mapping` test as noted, the most valuable missing
coverage is the boundary itself: a 21-28 digit integer now failing, and an
11-18 digit fraction now preserved. The current additions do not exercise the
actual behavior change.
Net: the one-line change is mechanically correct, but as an unconditional
change to a long-standing JDBC mapping that can break existing reads, I think
it needs the config gate + migration note before merge.
--
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]
