charliec05 commented on code in PR #58536:
URL: https://github.com/apache/spark/pull/58536#discussion_r4109154068
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCRelation.scala:
##########
@@ -216,26 +217,22 @@ private[sql] object JDBCRelation extends Logging {
private def toBoundValueInWhereClause(
value: Long,
columnType: DataType,
- timeZoneId: String): String = {
- def dateTimeToString(): String = {
- val dateTimeStr = columnType match {
+ timeZoneId: String,
+ dialect: JdbcDialect): String = {
+ def compileDateTimeValue(): String = {
+ val dateTimeValue = columnType match {
case DateType =>
- DateFormatter().format(value.toInt)
+ java.sql.Date.valueOf(DateTimeUtils.daysToLocalDate(value.toInt))
Review Comment:
Addressed in `d55ac41a6d6`: the `java.sql.Date` conversion is now removed
entirely in favor of `DateTimeUtils.daysToLocalDate`. The
`LocalDate`/`LocalDateTime` imports needed in the Oracle dialect are in the
file header.
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCRelation.scala:
##########
@@ -216,26 +217,22 @@ private[sql] object JDBCRelation extends Logging {
private def toBoundValueInWhereClause(
value: Long,
columnType: DataType,
- timeZoneId: String): String = {
- def dateTimeToString(): String = {
- val dateTimeStr = columnType match {
+ timeZoneId: String,
+ dialect: JdbcDialect): String = {
+ def compileDateTimeValue(): String = {
+ val dateTimeValue = columnType match {
case DateType =>
- DateFormatter().format(value.toInt)
+ java.sql.Date.valueOf(DateTimeUtils.daysToLocalDate(value.toInt))
Review Comment:
Thanks for catching this. Fixed in `d55ac41a6d6`: partition bounds now stay
as `LocalDate`/`LocalDateTime`, and the Oracle dialect wraps the base dialect's
proleptic-Gregorian formatter output instead of converting a `LocalDateTime`
through `Timestamp.valueOf`.
The new regression reproduces the old code changing the midpoint
`1582-10-10` to `1582-10-20`. It also covers ancient timestamps, a zoneless
time in a DST gap, fractional seconds, both datetime Java 8 API settings, and
different JVM/session/bound time zones. All 153 JDBCSuite tests, Docker
integration compilation, and Scala lint pass locally; fresh full CI is pending.
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCRelation.scala:
##########
@@ -216,26 +217,22 @@ private[sql] object JDBCRelation extends Logging {
private def toBoundValueInWhereClause(
value: Long,
columnType: DataType,
- timeZoneId: String): String = {
- def dateTimeToString(): String = {
- val dateTimeStr = columnType match {
+ timeZoneId: String,
+ dialect: JdbcDialect): String = {
+ def compileDateTimeValue(): String = {
+ val dateTimeValue = columnType match {
Review Comment:
Thanks, I checked the standard `CatalystTypeConverters.convertToScala` path.
There are two constraints here: with
`spark.sql.datetime.java8API.enabled=false`, it returns
`java.sql.Date`/`Timestamp`, whereas this path needs proleptic-Gregorian
`java.time` values regardless of the external Row API setting; and timestamp
bounds must honor the explicit `timeZoneId` argument, which need not match
`SQLConf` or the JVM default.
I've therefore retained the explicit `DateTimeUtils` conversion, removed
`java.sql.Date.valueOf`, and reused the base dialect's standard literal
formatters in Oracle's `LocalDate`/`LocalDateTime` cases. The regression now
covers both API settings, differing session/bound/JVM time zones, the 1582
cutover, a DST gap, and fractional seconds. The old code reproduced
`1582-10-10` becoming `1582-10-20`; the updated code passes all 153 JDBCSuite
tests.
Does retaining this explicit conversion while sharing the formatter address
your concern? I'm leaving this thread open for your confirmation rather than
forcing the Row API configuration in this code path.
--
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]
