MaxGekk commented on PR #56884:
URL: https://github.com/apache/spark/pull/56884#issuecomment-4841826002
I reviewed the change and didn't find correctness issues.
The config semantics look right to me: JDBC `TIME` maps to `TimeType` only
when `spark.sql.timeType.enabled=true` and the new legacy escape hatch is
false; when the escape hatch is true it falls back to the existing timestamp
mapping path, preserving `preferTimestampNTZ` behavior through
`getTimestampType(isTimestampNTZ)`.
I also checked that dialect-specific mappings such as PostgreSQL `timetz`
and H2 `TIME_WITH_TIMEZONE` are not affected by this default mapper change,
which seems intentional. The `.version("4.3.0")` also looks consistent with the
expected branch policy for this change.
Only minor note: the new test does not explicitly cover
`spark.sql.timeType.enabled=false` together with the escape hatch enabled, but
the existing legacy TIME test already covers the disabled-TIME path and the
code path is straightforward. So this is not a blocker from my side.
--
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]
