aleksandar-trajkovic-db commented on PR #59671:
URL: https://github.com/apache/spark/pull/59671#issuecomment-6523406136
Spark-dev:review
Review summary
This review found one blocking regression and one documentation fix.
Removing the loadTable pin lets V2 JDBC catalog tables pick the TimestampNTZ
value conversion when the plan is optimized rather than when the table loads. A
V2 Oracle query analyzed before a legacy-flag change then reads and writes
shifted values under an NTZ schema, and the V2 test that covered this was
deleted. The fix restores the load-time default for V2 tables through an
internal option key. This keeps per-query and catalog-level
timestampNTZAsWallClock in control and leaves the JDBCTable option merge
unchanged. It also restores the flag-flip coverage. Separately, the new
timestampNTZAsWallClock doc row should tie the java.sql.Timestamp conversion to
the option being false. The earlier review threads are addressed at head. One
open design question follows for the author.
Self-fix decision
Recommendation
- repair-v2-wallclock-snapshot: Restore the load-time snapshot of the
dialect's TimestampNTZ default for V2 JDBC catalog tables, so that query-level
and catalog-level timestampNTZAsWallClock still override it. Add back a V2
Oracle integration test (or a non-Docker JDBC catalog test) that plans a read
and a write under one legacy-flag value, executes them under the other, and
asserts wall-clock values.
- Finding coverage: finding:7ce4f818ba7c06ef9521
- repair-wallclock-doc-wording: In the JDBC options table, say that the
java.sql.Timestamp conversion in the JVM default time zone applies when the
option is false, for example by starting that sentence with 'When it is false,'
or by moving the driver requirement after it.
- Finding coverage: pass-b:wallclock-doc-otherwise-ambiguous
General approach
repair-v2-wallclock-snapshot
Mechanism: Register an internal option key with newOption (so it is filtered
from driver connection properties) that holds the dialect's
timestampNTZAsWallClock value. In JDBCOptions, resolve
dialectTimestampNTZAsWallClock from that key when present, and call the dialect
only otherwise. In loadTable, add the key with dialect.timestampNTZAsWallClock
to the table options. Both public options keep their explicit-value-first
lookup. Rebuilt JDBCOptions in newScanBuilder, newWriteBuilder, and join
pushdown then inherit the load-time default unless the catalog or query sets
the public option.
- Root cause: The V2 JDBC table no longer retains the wall-clock conversion
decided when its schema was resolved. Every JDBCOptions rebuilt for a scan,
write, or join pushdown re-derives the default from the legacy flag at
optimization time, and the regression test for this was deleted together with
the pin.
- Owner: JDBCTableCatalog.loadTable and the JDBCOptions default resolution
it feeds - loadTable is where the schema is resolved under a given flag value.
The JDBCTable's retained options are the only state every later scan, write,
and join-pushdown rebuild starts from, and JDBCOptions is where the
unset-option default is resolved. So the snapshot belongs at those two points
rather than in each consumer.
- Alternative considered: Re-adding the merge-base pin under the now-public
timestampNTZAsWallClock key restores the snapshot. But a per-query key whose
casing differs from the pinned key then collides with it in the case-sensitive
originalMap merge in JDBCTable, so precedence becomes nondeterministic. The
author gave this as the reason the pin was dropped. Making that merge
case-insensitive fixes the collision, but a reviewer asked to keep merge
changes out of this PR because they affect every option. Storing the resolved
boolean as a JDBCTable field avoids the merge too, but it adds a second channel
next to the options map that the join-pushdown rebuild would also have to
carry. Pinning the dialect default under a separate internal option key avoids
all three problems. The key never collides with a user-set public key. Query
and catalog values of the public option still win through the existing lookup.
JDBCTable and the merge stay unchanged, and every rebuild from originalMap in
herits the key, including join pushdown.
Change scope
repair-v2-wallclock-snapshot
Pin the dialect's NTZ default under an internal option key in V2 table
loading, have JDBCOptions use it before asking the dialect, and restore the
flag-flip regression coverage.
Implementation areas (3):
- V2 JDBC catalog table loading
- JDBC option default resolution for the TimestampNTZ options
- V2 JDBC Oracle NTZ integration tests
Integration points (2):
- JDBCOptions default resolution for timestampNTZAsWallClock and
zonelessTimestampAsNTZ
- Per-query V2 options supplied through WITH or DataFrame options
Supporting areas (2):
- JDBC catalog tests
- Docker Oracle V2 integration tests
Representative key files, non-exhaustive (3):
-
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/v2/jdbc/JDBCTableCatalog.scala
-
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCOptions.scala
-
connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/OracleIntegrationSuite.scala
repair-wallclock-doc-wording
Reword one sentence of the timestampNTZAsWallClock option description.
Implementation areas (1):
- JDBC data source user documentation
Integration points (1):
- JDBC options reference table
Supporting areas (1):
- Documentation build
Representative key files, non-exhaustive (1):
- docs/sql-data-sources-jdbc.md
Expected behavior
repair-v2-wallclock-snapshot
- Observable postcondition: A V2 Oracle catalog read or write planned with
the legacy flag false and executed with it true still uses the LocalDateTime
wall-clock conversion for TimestampNTZ columns, unless the query sets
timestampNTZAsWallClock.
- Observable postcondition: An explicit per-query timestampNTZAsWallClock,
in any key casing, determines the V2 read and write conversion.
repair-wallclock-doc-wording
- Observable postcondition: The description states that the
java.sql.Timestamp conversion applies when the option is false and no longer
reads as a driver-capability fallback.
Material considerations
- Compatibility: Default V2 behavior with no option set and an unchanged
legacy flag stays the same; per-query and catalog-level option precedence is
unchanged, and the JDBCTable option merge is untouched. Preserve behavior
outside the local correction.
- Risk: The internal key reappears in table options, so tests that assert
the exact contents of a loaded JDBCTable's options may need updating, as they
did when the merge base pinned the earlier internal key.
- Constraint: Per-query timestampNTZAsWallClock must continue to apply to V2
reads and writes.
- Constraint: A catalog-level timestampNTZAsWallClock must still take
precedence over the load-time dialect default.
- Constraint: The JDBCTable option merge stays unchanged.
Exclusions
- repair-v2-wallclock-snapshot: Supporting per-query zonelessTimestampAsNTZ
for V2 tables is not required: the V2 table schema is fixed at load time, and
per-query values are declared not to apply there.
Validation
- repair-v2-wallclock-snapshot:verification:1 [regression]: Cover a V2
Oracle catalog read whose plan is created under the legacy flag false and
collected under true. Assert that NTZ values read back as the stored wall-clock
in a non-UTC JVM zone, and cover the equivalent planned write.
- repair-v2-wallclock-snapshot:verification:2 [behavior]: Cover V2 reads and
writes with timestampNTZAsWallClock set per query, including a key whose casing
differs from the documented name, and assert that the query value decides the
conversion.
- repair-v2-wallclock-snapshot:verification:3 [compatibility]: Cover that a
catalog-level timestampNTZAsWallClock still overrides the dialect default for
V2 tables.
- repair-wallclock-doc-wording:verification:1 [inspection]: Check that the
timestampNTZAsWallClock description ties the java.sql.Timestamp conversion to
the option being false and keeps the LocalDateTime driver requirement attached
to the true case.
Approval boundary
Approval authorizes only the proposals explicitly bound to it and their
content under Recommendation, General approach when present, Change scope,
Expected behavior, Material considerations, Exclusions, and Validation.
Representative key files are non-exhaustive anchors. Concrete file placement,
line counts, local design, test selection, targets, actions, and command
arguments remain Apply-time judgments while the approved semantic subject stays
true. Content explicitly identified as supporting context may explain
diagnosis, evidence, rationale, and before-state without expanding authority.
Delivery selection
Route: self-fix
Review event: none
Selected discussion replies (provisional): 0 selected (none)
Existing feedback status
Existing feedback items: 12 resolved, 1 active (open or waiting); current
review items requiring fresh delivery: 3.
- waiting conversation: One JdbcDialect.timestampNTZAsWallClock hook
defaults both the NTZ type mapping and the wall-clock conversion, so
third-party dialects cannot default them independently. (discussion); semantic
response not required; current-route reply not available on this route
Use render-approval --full for the supporting review record, evidence, and
complete audit view.
Decision challenges
One JdbcDialect hook defaults both the NTZ type mapping and the wall-clock
conversion
Following up on the earlier question, which is still open. At head the
scaladoc still reads 'Default of the timestampNTZAsWallClock and
zonelessTimestampAsNTZ JDBC options', and JDBCOptions still derives both
defaults from one dialectTimestampNTZAsWallClock value. The earlier comment's
mention of JDBCTableCatalog.withPlanTimeOptions is stale (that code is gone),
but the coupling is unchanged. Only OracleDialect honors
zonelessTimestampAsNTZ, so nothing is broken today. Before the @Since("4.4.0")
hook ships, should the type-mapping default get its own dialect hook? Or should
this hook stay documented as the conversion default only, with the zoneless
default described as Oracle-specific?
--
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]
