aleksandar-trajkovic-db commented on PR #59671:
URL: https://github.com/apache/spark/pull/59671#issuecomment-6417564213
The spark-dev:review:
Review summary
This review found no defects. Both earlier review threads are addressed at
head. Loading with state options now goes through TableCatalog's default
dispatch, so write privileges and subclass loadTable overrides still apply, and
the counting-catalog test covers reads and writes with the option.
zonelessTimestampAsNTZ now reaches the Oracle dialect through the existing
column-metadata channel instead of a new getCatalystType overload. One design
question for the author follows.
Decision challenges
One dialect hook defaults both the NTZ type mapping and the wall-clock
conversion
This PR splits the Oracle behavior into two options, zonelessTimestampAsNTZ
(type mapping) and timestampNTZAsWallClock (value conversion). Both still
default to the single @DeveloperApi hook JdbcDialect.timestampNTZAsWallClock.
Its scaladoc now reads "Default of the timestampNTZAsWallClock and
zonelessTimestampAsNTZ JDBC options." JDBCOptions,
JDBCTableCatalog.withPlanTimeOptions, and the OracleDialect fallback all derive
the zoneless default from that hook. Default behavior is unchanged, and nothing
is broken today.
The cost falls on third-party dialects. A dialect that overrides the hook to
true for LocalDateTime conversion is now documented as also defaulting
zonelessTimestampAsNTZ to true, even though only Oracle honors that option.
Such a dialect cannot turn conversion on by default while keeping the mapping
default off. If another dialect or the shared mapping later honors the option,
those dialects' inferred schemas would switch to TimestampNTZ.
The hook is @Since("4.4.0") and not yet released, so it is cheap to reshape
now. Would you prefer a separate per-dialect default for the type mapping? The
alternative is to keep this hook documented as the conversion default only and
describe the zoneless default as Oracle-specific. Or is the shared default
intended as a long-term contract?
FINDING COUNTS: first review; findings in this review: 0.
OUTCOME: no-changes
--
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]
