urosstan-db commented on PR #59990: URL: https://github.com/apache/spark/pull/59990#issuecomment-6546306459
**Review: no blocking findings were identified.** ## Review summary No defects were found. The only production change is `OracleDialect.timestampNTZAsWallClock` becoming the constant `true`, which is the value the removed flag's default already produced, so Oracle's default TimestampNTZ type mapping and wall-clock read and write conversion are unchanged on every V1 and V2 entry path, and explicit `zonelessTimestampAsNTZ` and `timestampNTZAsWallClock` values are honored exactly as before. No reference to the removed key, its `SQLConf` val, or its getter remains anywhere in the repository. The key has never shipped in a release (no v4.4 tag exists and the newest tags are v4.3.0-rc*, while branch-4.x carries the introducing SPARK-60420 commit and branch-4.3 does not), so deleting it without a `RemovedConfig` entry, deprecation period, or alternative key is consistent with the existing `removedSQLConfigs` convention, whose entries all name previously released keys. The removed JVM symbols are internal and MiMa-excluded, and the public `JdbcDialect.tim estampNTZAsWallClock` hook keeps its signature, default, and ScalaDoc, so no compatibility contract is affected. The rewritten documentation matches the code: the Oracle type-mapping table rows agree with `OracleDialect.getCatalystType` plus the shared JDBC `TIMESTAMP` fallback, the new `preferTimestampNTZ` note about `spark.sql.timestampType` and the Oracle override is accurate, the option descriptions no longer tie their defaults to a configuration that does not exist, and the migration-guide entry correctly sits under the 4.3-to-4.4 section with its restore instruction now pointing at both options. The reshaped tests exercise unset, `true`, and `false` for both options across schema mapping, the read getter, and the write setter, including the eight-hour shift under a Los Angeles JVM zone when `timestampNTZAsWallClock` is `false`, so a regression of the Oracle default or of explicit option handling would fail `JDBCSuite`. The Oracle Docker suite repeats those decision points against the real driver with value assertions, but as the description notes it was only compile-checked locally. This review comment was written by Isaac. -- 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]
