You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "aleksandar-trajkovic-db (via GitHub)" <gi...@apache.org> on 2026/10/20 09:37:27 UTC
[PR] [SPARK-60385][SQL] Pass JDBCOptions to JdbcUtils.getSchema instead of individual timestamp flags [spark]
aleksandar-trajkovic-db opened a new pull request, #59634:
URL: https://github.com/apache/spark/pull/59634
### What changes were proposed in this pull request?
3. Solution: `JdbcUtils.getSchema` and the private `JdbcUtils.getCatalystType` take `JDBCOptions` instead of the `isTimestampNTZ` and `preferTimestampNanos` booleans. `getSchema` still writes the same column metadata keys for dialects.
### Why are the changes needed?
1. Goal: Read options that affect JDBC type mapping reach `getSchema` without widening its signature.
2. Problem: Both callers, `JdbcUtils.getSchemaOption` and `JDBCRDD.getQueryOutputSchema`, already hold a `JDBCOptions` and unpack the same two fields from it. Each new option means another parameter plus an edit at both call sites.
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Existing tests in `JDBCSuite` and `JDBCV2Suite`.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Anthropic Claude Opus 5.5)
This pull request and its description were 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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60385][SQL] Pass JDBCOptions to JdbcUtils.getSchema instead of individual timestamp flags [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on PR #59634:
URL: https://github.com/apache/spark/pull/59634#issuecomment-6291651982
**Merge Summary:**
- merged into master https://github.com/apache/spark/commit/e9d51c694ec2e6c5d6376fc1b7c3433ec4c6de73
- merged into branch-4.x https://github.com/apache/spark/commit/26d242e472cbf05a05cc8d8299fd4232c76cac31
*Posted by `merge_spark_pr.py`*
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60385][SQL] Pass JDBCOptions to JdbcUtils.getSchema instead of individual timestamp flags [spark]
Posted by "urosstan-db (via GitHub)" <gi...@apache.org>.
urosstan-db commented on code in PR #59634:
URL: https://github.com/apache/spark/pull/59634#discussion_r4323875705
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JdbcUtils.scala:
##########
@@ -304,18 +301,17 @@ object JdbcUtils extends Logging with SQLConfHelper {
/**
* Takes a [[ResultSet]] and returns its Catalyst schema.
*
+ * @param options The JDBC options of the read, which control how column types are mapped.
* @param alwaysNullable If true, all the columns are nullable.
- * @param isTimestampNTZ If true, all timestamp columns are interpreted as TIMESTAMP_NTZ.
* @return A [[StructType]] giving the Catalyst schema.
* @throws SQLException if the schema contains an unsupported type.
*/
def getSchema(
conn: Connection,
resultSet: ResultSet,
dialect: JdbcDialect,
- alwaysNullable: Boolean = false,
- isTimestampNTZ: Boolean = false,
- preferTimestampNanos: Boolean = false): StructType = {
+ options: JDBCOptions,
Review Comment:
You probably mean on timestamp ntz wallclock getter/setter, while I though on legacyOracleTimestampMappingEnabled, which is conf currently, but it makes sense to have option which will be applicable on all dialects after this PR. Since many dialects today maps timestamp ntz to pure timestamp.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60385][SQL] Pass JDBCOptions to JdbcUtils.getSchema instead of individual timestamp flags [spark]
Posted by "aleksandar-trajkovic-db (via GitHub)" <gi...@apache.org>.
aleksandar-trajkovic-db commented on code in PR #59634:
URL: https://github.com/apache/spark/pull/59634#discussion_r4325209001
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JdbcUtils.scala:
##########
@@ -304,18 +301,17 @@ object JdbcUtils extends Logging with SQLConfHelper {
/**
* Takes a [[ResultSet]] and returns its Catalyst schema.
*
+ * @param options The JDBC options of the read, which control how column types are mapped.
* @param alwaysNullable If true, all the columns are nullable.
- * @param isTimestampNTZ If true, all timestamp columns are interpreted as TIMESTAMP_NTZ.
* @return A [[StructType]] giving the Catalyst schema.
* @throws SQLException if the schema contains an unsupported type.
*/
def getSchema(
conn: Connection,
resultSet: ResultSet,
dialect: JdbcDialect,
- alwaysNullable: Boolean = false,
- isTimestampNTZ: Boolean = false,
- preferTimestampNanos: Boolean = false): StructType = {
+ options: JDBCOptions,
Review Comment:
Makes sense, I'll add a new conf for the wallclock getter
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60385][SQL] Pass JDBCOptions to JdbcUtils.getSchema instead of individual timestamp flags [spark]
Posted by "aleksandar-trajkovic-db (via GitHub)" <gi...@apache.org>.
aleksandar-trajkovic-db commented on code in PR #59634:
URL: https://github.com/apache/spark/pull/59634#discussion_r4323774438
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JdbcUtils.scala:
##########
@@ -304,18 +301,17 @@ object JdbcUtils extends Logging with SQLConfHelper {
/**
* Takes a [[ResultSet]] and returns its Catalyst schema.
*
+ * @param options The JDBC options of the read, which control how column types are mapped.
* @param alwaysNullable If true, all the columns are nullable.
- * @param isTimestampNTZ If true, all timestamp columns are interpreted as TIMESTAMP_NTZ.
* @return A [[StructType]] giving the Catalyst schema.
* @throws SQLException if the schema contains an unsupported type.
*/
def getSchema(
conn: Connection,
resultSet: ResultSet,
dialect: JdbcDialect,
- alwaysNullable: Boolean = false,
- isTimestampNTZ: Boolean = false,
- preferTimestampNanos: Boolean = false): StructType = {
+ options: JDBCOptions,
Review Comment:
Everything inside JdbcUtils should be part of the internal API, so should not require a release note. I am not sure I entirely understand the second question. A JdbcOption already exists for Oracle's new TimestampNTZ (or for that matter any dialect, as the option is dialect agnostic). It is currently not propagated to getCatalystType, because that was too broad a change for a single PR, but I think a follow up PR might be worth it for that. Right now, it dictates the getter that gets picked during execution. Does that answer the second question?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60385][SQL] Pass JDBCOptions to JdbcUtils.getSchema instead of individual timestamp flags [spark]
Posted by "aleksandar-trajkovic-db (via GitHub)" <gi...@apache.org>.
aleksandar-trajkovic-db commented on code in PR #59634:
URL: https://github.com/apache/spark/pull/59634#discussion_r4323987676
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JdbcUtils.scala:
##########
@@ -304,18 +301,17 @@ object JdbcUtils extends Logging with SQLConfHelper {
/**
* Takes a [[ResultSet]] and returns its Catalyst schema.
*
+ * @param options The JDBC options of the read, which control how column types are mapped.
* @param alwaysNullable If true, all the columns are nullable.
- * @param isTimestampNTZ If true, all timestamp columns are interpreted as TIMESTAMP_NTZ.
* @return A [[StructType]] giving the Catalyst schema.
* @throws SQLException if the schema contains an unsupported type.
*/
def getSchema(
conn: Connection,
resultSet: ResultSet,
dialect: JdbcDialect,
- alwaysNullable: Boolean = false,
- isTimestampNTZ: Boolean = false,
- preferTimestampNanos: Boolean = false): StructType = {
+ options: JDBCOptions,
Review Comment:
Yes the conf you mention does exist, but it gets mapped to a JdbcOption:
https://github.com/apache/spark/blob/d36ebf3147fd79dcb159b9daa02743fc389b7b5b/sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JDBCOptions.scala#L269-L273
The option is intentionally dialect-agnostic, for the other dialects to be able to use it in the future to remap their types to TimestampNTZ
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60385][SQL] Pass JDBCOptions to JdbcUtils.getSchema instead of individual timestamp flags [spark]
Posted by "aleksandar-trajkovic-db (via GitHub)" <gi...@apache.org>.
aleksandar-trajkovic-db commented on code in PR #59634:
URL: https://github.com/apache/spark/pull/59634#discussion_r4324160833
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JdbcUtils.scala:
##########
@@ -304,18 +301,17 @@ object JdbcUtils extends Logging with SQLConfHelper {
/**
* Takes a [[ResultSet]] and returns its Catalyst schema.
*
+ * @param options The JDBC options of the read, which control how column types are mapped.
* @param alwaysNullable If true, all the columns are nullable.
- * @param isTimestampNTZ If true, all timestamp columns are interpreted as TIMESTAMP_NTZ.
* @return A [[StructType]] giving the Catalyst schema.
* @throws SQLException if the schema contains an unsupported type.
*/
def getSchema(
conn: Connection,
resultSet: ResultSet,
dialect: JdbcDialect,
- alwaysNullable: Boolean = false,
- isTimestampNTZ: Boolean = false,
- preferTimestampNanos: Boolean = false): StructType = {
+ options: JDBCOptions,
Review Comment:
The JdbcOption I mentioned should affect mapping as well, not just execution (right now doesn't, I will refactor that, as the current way of reading the config is proving a hassle outside of OSS). For the mapping of Oracle's Date to Spark's Timestamp, there exists the option oracle.jdbc.mapDateToTimestamp, which is default true and doesn't exist as a defined JdbcOptions, but does get passed through JdbcOptions to the driver.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60385][SQL] Pass JDBCOptions to JdbcUtils.getSchema instead of individual timestamp flags [spark]
Posted by "urosstan-db (via GitHub)" <gi...@apache.org>.
urosstan-db commented on code in PR #59634:
URL: https://github.com/apache/spark/pull/59634#discussion_r4325009031
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JdbcUtils.scala:
##########
@@ -304,18 +301,17 @@ object JdbcUtils extends Logging with SQLConfHelper {
/**
* Takes a [[ResultSet]] and returns its Catalyst schema.
*
+ * @param options The JDBC options of the read, which control how column types are mapped.
* @param alwaysNullable If true, all the columns are nullable.
- * @param isTimestampNTZ If true, all timestamp columns are interpreted as TIMESTAMP_NTZ.
* @return A [[StructType]] giving the Catalyst schema.
* @throws SQLException if the schema contains an unsupported type.
*/
def getSchema(
conn: Connection,
resultSet: ResultSet,
dialect: JdbcDialect,
- alwaysNullable: Boolean = false,
- isTimestampNTZ: Boolean = false,
- preferTimestampNanos: Boolean = false): StructType = {
+ options: JDBCOptions,
Review Comment:
Current option is called timestampNTZAsWallclock which is not good name for type mapping change, it would be better to have different option. While your option will then mandate how mapped timestamp NTZ is read.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60385][SQL] Pass JDBCOptions to JdbcUtils.getSchema instead of individual timestamp flags [spark]
Posted by "urosstan-db (via GitHub)" <gi...@apache.org>.
urosstan-db commented on code in PR #59634:
URL: https://github.com/apache/spark/pull/59634#discussion_r4323626010
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JdbcUtils.scala:
##########
@@ -304,18 +301,17 @@ object JdbcUtils extends Logging with SQLConfHelper {
/**
* Takes a [[ResultSet]] and returns its Catalyst schema.
*
+ * @param options The JDBC options of the read, which control how column types are mapped.
* @param alwaysNullable If true, all the columns are nullable.
- * @param isTimestampNTZ If true, all timestamp columns are interpreted as TIMESTAMP_NTZ.
* @return A [[StructType]] giving the Catalyst schema.
* @throws SQLException if the schema contains an unsupported type.
*/
def getSchema(
conn: Connection,
resultSet: ResultSet,
dialect: JdbcDialect,
- alwaysNullable: Boolean = false,
- isTimestampNTZ: Boolean = false,
- preferTimestampNanos: Boolean = false): StructType = {
+ options: JDBCOptions,
Review Comment:
Just check whether change in this method needs some release note, I am not sure whether this is part of API?
Can we add some option for newly added config for Oracle TimestampNTZ mapping change in next PR. Since `isTimestampNTZ` affects both timestamp in JDBC data source.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60385][SQL] Pass JDBCOptions to JdbcUtils.getSchema instead of individual timestamp flags [spark]
Posted by "urosstan-db (via GitHub)" <gi...@apache.org>.
urosstan-db commented on code in PR #59634:
URL: https://github.com/apache/spark/pull/59634#discussion_r4324036383
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JdbcUtils.scala:
##########
@@ -304,18 +301,17 @@ object JdbcUtils extends Logging with SQLConfHelper {
/**
* Takes a [[ResultSet]] and returns its Catalyst schema.
*
+ * @param options The JDBC options of the read, which control how column types are mapped.
* @param alwaysNullable If true, all the columns are nullable.
- * @param isTimestampNTZ If true, all timestamp columns are interpreted as TIMESTAMP_NTZ.
* @return A [[StructType]] giving the Catalyst schema.
* @throws SQLException if the schema contains an unsupported type.
*/
def getSchema(
conn: Connection,
resultSet: ResultSet,
dialect: JdbcDialect,
- alwaysNullable: Boolean = false,
- isTimestampNTZ: Boolean = false,
- preferTimestampNanos: Boolean = false): StructType = {
+ options: JDBCOptions,
Review Comment:
I did not mean on timestampNTZAsWallClock, that affects TZ being used when reading already mapped type, I mean on type mapping (Oracle's Date -> timestamp), does JDBC option exist for that?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60385][SQL] Pass JDBCOptions to JdbcUtils.getSchema instead of individual timestamp flags [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b closed pull request #59634: [SPARK-60385][SQL] Pass JDBCOptions to JdbcUtils.getSchema instead of individual timestamp flags
URL: https://github.com/apache/spark/pull/59634
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-60385][SQL] Pass JDBCOptions to JdbcUtils.getSchema instead of individual timestamp flags [spark]
Posted by "uros-b (via GitHub)" <gi...@apache.org>.
uros-b commented on PR #59634:
URL: https://github.com/apache/spark/pull/59634#issuecomment-6291638694
Thank you @aleksandar-trajkovic-db and @urosstan-db!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org