spark-test commented on code in PR #59990:
URL: https://github.com/apache/spark/pull/59990#discussion_r4432696192
##########
sql/core/src/test/scala/org/apache/spark/sql/jdbc/JDBCSuite.scala:
##########
@@ -1774,20 +1774,15 @@ class JDBCSuite extends SharedSparkSession {
}
}
- test("SPARK-58876: Oracle NTZ wall-clock follows the legacy flag, even
wrapped") {
+ test("SPARK-58876: Oracle defaults to NTZ wall-clock conversion, even
wrapped") {
val custom = new JdbcDialect {
override def canHandle(url: String): Boolean =
url.startsWith("jdbc:oracle")
}
val dialects = Seq[JdbcDialect](
OracleDialect(),
new AggregatedDialect(List(custom, OracleDialect())),
new AggregatedDialect(List(OracleDialect(), custom)))
- withSQLConf(SQLConf.LEGACY_ORACLE_TIMESTAMP_NTZ_MAPPING_ENABLED.key ->
"false") {
- dialects.foreach(d => assert(d.timestampNTZAsWallClock, s"dialect=$d"))
- }
- withSQLConf(SQLConf.LEGACY_ORACLE_TIMESTAMP_NTZ_MAPPING_ENABLED.key ->
"true") {
- dialects.foreach(d => assert(!d.timestampNTZAsWallClock, s"dialect=$d"))
- }
+ dialects.foreach(d => assert(d.timestampNTZAsWallClock, s"dialect=$d"))
Review Comment:
**Nit (P3):** Before this PR, the flag-on block also checked that both
`AggregatedDialect` orders report `false` when no member opts in. With
`OracleDialect` now constant `true`, every aggregate here includes an opting
member, so only the `true` side of `dialects.exists(_.timestampNTZAsWallClock)`
is still asserted, and no other test checks the `false` side. If the
composition regressed to a constant `true` or to `!dialects.forall(...)`,
nothing would fail. Yet a URL served by a user-registered dialect alongside a
non-opting built-in such as PostgreSQL would then silently switch its
TimestampNTZ reads and writes to `LocalDateTime` binding. Consider adding an
assertion next to this one that an `AggregatedDialect` of dialects that keep
the default, for example `custom` and a second plain dialect, reports `false`.
**Verification:**
- **Regression:** An aggregate of dialects that all keep the default reports
false, so the test fails if the composition regresses to a constant true or a
negated forall.
- **Behavior:** Aggregates that include the Oracle dialect, in either order,
still report true.
##########
connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/OracleIntegrationSuite.scala:
##########
@@ -636,26 +639,16 @@ class OracleIntegrationSuite extends
SharedJDBCIntegrationSuite
checkAnswer(noWallClockDf.select("T"), Row(LocalDateTime.of(1996, 1, 1,
9, 23, 45)))
}
- // Legacy flag on: both fall back to TimestampType.
- withSQLConf(SQLConf.LEGACY_ORACLE_TIMESTAMP_NTZ_MAPPING_ENABLED.key ->
"true") {
- val legacyDf = readDatetime()
- assert(legacyDf.schema("D").dataType === TimestampType)
- assert(legacyDf.schema("T").dataType === TimestampType)
- val legacyRow = legacyDf.select("D", "T").collect().head
- assert(legacyRow.getTimestamp(0) === Timestamp.valueOf("1991-11-09
00:00:00"))
- assert(legacyRow.getTimestamp(1) === Timestamp.valueOf("1996-01-01
01:23:45"))
-
- withDefaultTimeZone(LA) {
- val optInDf = readDatetime(
- Map("zonelessTimestampAsNTZ" -> "true", "timestampNTZAsWallClock" ->
"true"))
- assert(optInDf.schema("T").dataType === TimestampNTZType)
- checkAnswer(optInDf.select("D", "T"), ntzRow)
- withTable("datetime_ntz") {
- sql(s"CREATE TABLE datetime_ntz USING org.apache.spark.sql.jdbc
OPTIONS (url " +
- s"'$jdbcUrl', dbtable 'datetime', zonelessTimestampAsNTZ 'true', "
+
- "timestampNTZAsWallClock 'true')")
- checkAnswer(sql("SELECT D, T FROM datetime_ntz"), ntzRow)
- }
+ withDefaultTimeZone(LA) {
+ val optInDf = readDatetime(
+ Map("zonelessTimestampAsNTZ" -> "true", "timestampNTZAsWallClock" ->
"true"))
+ assert(optInDf.schema("T").dataType === TimestampNTZType)
+ checkAnswer(optInDf.select("D", "T"), ntzRow)
+ withTable("datetime_ntz") {
+ sql(s"CREATE TABLE datetime_ntz USING org.apache.spark.sql.jdbc
OPTIONS (url " +
+ s"'$jdbcUrl', dbtable 'datetime', zonelessTimestampAsNTZ 'true', " +
Review Comment:
**Nit (P3):** With the legacy-flag wrapper gone, these table OPTIONS just
repeat Oracle's defaults (`true` for both). So this `CREATE TABLE ... USING
jdbc` check returns `ntzRow` whether or not the table OPTIONS reach the
relation. Before this PR the block ran with
`spark.sql.legacy.oracle.timestampNTZMapping.enabled=true`, so both defaults
were `false` and the check failed unless the OPTIONS were honored. No other
test passes a non-default value of either option through a persistent table's
OPTIONS. Yet for `USING jdbc` tables, the migration guide gives table OPTIONS
as the restore, and the only reliable one now that the session flag is gone.
Consider creating the table with non-default values instead, matching the
DataFrame-reader checks above. With `zonelessTimestampAsNTZ 'false'`, `D` and
`T` should read as `TimestampType` with `1991-11-09 00:00:00` and `1996-01-01
01:23:45`. With `timestampNTZAsWallClock 'false'`, `T` should read as
`1996-01-01T09:23:45` under Los Angeles.
**Verification:**
- **Regression:** Against a live Oracle, the persistent-table checks pass
with the table OPTIONS honored. If either option were ignored when the table is
created or read, they would fail: with a schema mismatch on read, with
TimestampNTZType columns, or with unshifted wall-clock values.
- **Behavior:** The default reads in the same test still return
TimestampNTZType columns with their wall-clock values.
--
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]
