MaxGekk commented on code in PR #56904:
URL: https://github.com/apache/spark/pull/56904#discussion_r3499944652
##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/PostgresDialect.scala:
##########
@@ -124,7 +124,10 @@ private case class PostgresDialect()
Some(StringType)
case "bytea" => Some(BinaryType)
case "timestamptz" | "timetz" => Some(TimestampType)
- case "timestamp" | "time" => Some(getTimestampType(md.build()))
+ case "timestamp" => Some(getTimestampType(md.build()))
+ case "time" =>
+ if (conf.isTimeTypeEnabled) Some(TimeType(TimeType.DEFAULT_PRECISION))
Review Comment:
This `case "time"` branch is reached only for `time[]` array elements —
`toCatalystType` is called solely from the `Types.ARRAY` arm of
`getCatalystType`. Scalar `time` columns return `None` here and fall through to
the generic `JdbcUtils` TIME path, so the new scalar `c36` read test exercises
that generic path rather than this branch. The new array mapping isn't covered
by either added test; a `time[]` array read test (analogous to the
`SPARK-22291` array test) would close the gap.
Separately (nit): the hardcoded `TimeType.DEFAULT_PRECISION` here diverges
from the scalar path's `TimeType(scale)` (JdbcUtils). It's defensible —
Postgres doesn't reliably report element typmod for arrays, and
`DEFAULT_PRECISION` (6) is Postgres `time`'s max so nothing is lost — but a
one-line comment saying so would keep it from reading as an accidental
divergence.
##########
sql/core/src/main/scala/org/apache/spark/sql/jdbc/PostgresDialect.scala:
##########
@@ -159,6 +162,7 @@ private case class PostgresDialect()
case ShortType | ByteType => Some(JdbcType("SMALLINT", Types.SMALLINT))
case TimestampType if !conf.legacyPostgresDatetimeMappingEnabled =>
Some(JdbcType("TIMESTAMP WITH TIME ZONE", Types.TIMESTAMP))
+ case _: TimeType => Some(JdbcType("TIME", Types.TIME))
Review Comment:
This overrides the base `JdbcUtils.getCommonJDBCType` mapping `TimeType ->
TIME(${t.precision})` with bare `TIME` (= Postgres `TIME(6)`), so the declared
precision is dropped for every `TimeType`: a `TimeType(3)` writes as `TIME(6)`,
and a write/read round-trip widens `TimeType(3)` -> `TimeType(6)` (values are
preserved). Postgres accepts `TIME(p)` for p<=6, so
`TIME(${math.min(t.precision, 6)})` would preserve the declared precision for
p<=6 while still avoiding the out-of-range `TIME(7..9)` the base mapping would
emit for nanos `TimeType`s. If uniform bare `TIME` is a deliberate idiom
choice, a short comment noting that would be helpful.
--
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]
