spark-test commented on code in PR #59990:
URL: https://github.com/apache/spark/pull/59990#discussion_r4436321971
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/jdbc/JDBCTable.scala:
##########
@@ -59,15 +60,25 @@ case class JDBCTable(
util.EnumSet.of(BATCH_READ, V1_BATCH_WRITE, TRUNCATE)
}
- override def newScanBuilder(options: CaseInsensitiveStringMap):
JDBCScanBuilder = {
- val mergedOptions = new JDBCOptions(
+ private def mergeOptions(options: CaseInsensitiveStringMap):
CaseInsensitiveMap[String] = {
+ // Preserve case-sensitive driver properties when merging query options.
+ val merged = CaseInsensitiveMap(
jdbcOptions.parameters.originalMap ++
options.asCaseSensitiveMap().asScala)
+ // Only normalize the timestamp options, preserving the table defaults
unless overridden.
+ merged +
+ (JDBCOptions.JDBC_TIMESTAMP_NTZ_WALL_CLOCK -> options.getOrDefault(
+ JDBCOptions.JDBC_TIMESTAMP_NTZ_WALL_CLOCK,
jdbcOptions.timestampNTZAsWallClock.toString)) +
+ (JDBCOptions.JDBC_ZONELESS_TIMESTAMP_AS_NTZ -> options.getOrDefault(
Review Comment:
**Non-blocking (P2):** For a JDBC catalog table, the schema is resolved once
in `JDBCTableCatalog.loadTable` from the catalog options, and the docs say
per-query `zonelessTimestampAsNTZ` values do not apply there
(`docs/sql-data-sources-jdbc.md:389`, `docs/sql-migration-guide.md:50`). This
line still lets a per-query value replace the table's resolved one. In the
builder, the option's only reader is aggregate pushdown. `pushAggregation`
re-infers the pushed query's output types with it (`JDBCScanBuilder.scala:139`,
`JdbcUtils.scala:355-356`), and `buildScanWithPushedAggregate` silently casts
each column back to the schema's type in the session time zone.
Take a table that resolved `ts` as `TimestampNTZType`, and run `SELECT
MAX(ts) FROM ora.s.t WITH ('zonelessTimestampAsNTZ' = 'false')` with the
default `preferTimestampNTZ=false`. The pushed `MAX(ts)` is read as
`TimestampType` through `rs.getTimestamp` in the JVM default zone, then cast
back in the session zone. It comes back shifted by the offset between the two
zones, while the unpushed plan returns the exact wall clock. `MIN`, a `GROUP
BY` key on such a column, and the reverse case are affected the same way; the
reverse case is a per-query `true` on a catalog that sets `false`. Plain scans
and writes never read this option.
The base branch already behaved this way. But this PR now carries the
table's value into the builders to fix exactly this mismatch when the flag
flips, and it exempts these inputs. The new grid also asserts the per-query
precedence for this key (`JDBCTableCatalogSuite.scala:152-153`). Consider
always using the table's value for this key, regardless of the query options,
and keeping the per-query override only for `timestampNTZAsWallClock`, which
the docs allow per read or write:
```scala
(JDBCOptions.JDBC_ZONELESS_TIMESTAMP_AS_NTZ ->
jdbcOptions.zonelessTimestampAsNTZ.toString)
```
The grid would then expect the table's value whatever the query supplies.
This extends an [existing
discussion](https://github.com/apache/spark/pull/59990#issuecomment-6558990658).
**Verification:**
- **Behavior:** For a per-query zonelessTimestampAsNTZ of either value and
in either casing, both the scan builder and the write builder report the
table's resolved zonelessTimestampAsNTZ, in both legacy-flag flip directions.
- **Regression:** Against Oracle, with the session time zone different from
the JVM default zone and the default spark.sql.timestampType, pushed aggregates
return the same values as the unpushed plan when the per-query
zonelessTimestampAsNTZ differs from the table's resolved value, in both
directions: a per-query false on a table resolved as TimestampNTZ, and a
per-query true on a catalog that sets zonelessTimestampAsNTZ to false. Cover a
MIN or MAX over a zoneless DATE or TIMESTAMP column and a GROUP BY key on one.
- **Compatibility:** A per-query timestampNTZAsWallClock still changes read
and write conversion for that operation, and case-distinct driver properties
still reach the connection properties unchanged.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/v2/jdbc/JDBCTableCatalogSuite.scala:
##########
@@ -106,6 +108,73 @@ class JDBCTableCatalogSuite extends SharedSparkSession {
checkAnswer(sql("SHOW TABLES IN h2.bad_test"), Seq())
}
+ test("SPARK-60420: JDBC builders preserve timestamp defaults and honor query
overrides") {
+ val wallClockKey = JDBCOptions.JDBC_TIMESTAMP_NTZ_WALL_CLOCK
+ val zonelessKey = JDBCOptions.JDBC_ZONELESS_TIMESTAMP_AS_NTZ
+ val legacyKey = SQLConf.LEGACY_ORACLE_TIMESTAMP_NTZ_MAPPING_ENABLED.key
+ val schema = new StructType().add("t", TimestampNTZType)
+ for {
+ legacy <- Seq(false, true)
+ catalogValue <- Seq(None, Some(false), Some(true))
+ catalogUpperCase <- Seq(false, true)
+ } {
+ val table = withSQLConf(legacyKey -> legacy.toString) {
+ val parameters = Map(
+ "url" -> "jdbc:oracle:thin:@//host:1521/db",
+ "dbtable" -> "t",
+ "driver" -> "org.h2.Driver") ++ catalogValue.toSeq.flatMap { value =>
+ Seq(wallClockKey -> value.toString, zonelessKey -> (!value).toString)
Review Comment:
**Nit (P3):** Here and on the query side (lines 137-141), one generated
value always produces both `timestampNTZAsWallClock` and
`zonelessTimestampAsNTZ`, so every case sets both keys or neither. Suppose a
later change to `mergeOptions` injected the load-time values only when no
timestamp key is supplied. Every case here would still pass. Yet a catalog that
sets only `zonelessTimestampAsNTZ`, the documented catalog usage, would
re-derive `timestampNTZAsWallClock` from the flipped flag in each builder,
which brings back the shifted NTZ read this PR fixes. The Oracle Docker tests
don't cover it either: the flag-flip V2 test sets no timestamp option, and the
per-query V2 test never flips the flag.
Consider generating the two keys independently on each side. Set only
`zonelessTimestampAsNTZ` or only `timestampNTZAsWallClock` on the catalog, and
only one of them per query. Then assert that the omitted key keeps the table's
load-time value in both builders.
**Verification:**
- **Regression:** A variant of the option merge that pins the load-time
values only when neither timestamp key is supplied fails the extended grid, in
both flag-flip directions and for both builders.
- **Behavior:** With only zonelessTimestampAsNTZ set on the catalog and the
legacy flag flipped after table resolution, both builders report the catalog's
zonelessTimestampAsNTZ and the load-time timestampNTZAsWallClock. With only
timestampNTZAsWallClock set per query, both builders report the query's
timestampNTZAsWallClock and the table's zonelessTimestampAsNTZ. With only
zonelessTimestampAsNTZ set per query, both builders report the table's
load-time timestampNTZAsWallClock.
--
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]
