Re: [PR] [SPARK-47424][SQL] Add getDatabaseCalendar method to the JdbcDialect [spark]
beliefer commented on code in PR #45537:
URL: https://github.com/apache/spark/pull/45537#discussion_r1623902893
##
connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/DB2IntegrationSuite.scala:
##
@@ -45,15 +45,18 @@ class DB2IntegrationSuite extends
DockerJDBCIntegrationV2Suite with V2JDBCTest {
"scan with aggregate push-down: REGR_INTERCEPT with DISTINCT",
"scan with aggregate push-down: REGR_SLOPE with DISTINCT",
"scan with aggregate push-down: REGR_R2 with DISTINCT",
-"scan with aggregate push-down: REGR_SXY with DISTINCT")
+"scan with aggregate push-down: REGR_SXY with DISTINCT",
+"simple timestamps roundtrip",
+"simple timestamps pushdown")
override val catalogName: String = "db2"
override val namespaceOpt: Option[String] = Some("DB2INST1")
override val db = new DB2DatabaseOnDocker
+ override def url: String = db.getJdbcUrl(dockerIp, externalPort)
Review Comment:
Or we put it into `DockerJDBCIntegrationV2Suite` ?
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47424][SQL] Add getDatabaseCalendar method to the JdbcDialect [spark]
PetarVasiljevic-DB commented on code in PR #45537:
URL: https://github.com/apache/spark/pull/45537#discussion_r1609798248
##
connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/DB2IntegrationSuite.scala:
##
@@ -45,15 +45,18 @@ class DB2IntegrationSuite extends
DockerJDBCIntegrationV2Suite with V2JDBCTest {
"scan with aggregate push-down: REGR_INTERCEPT with DISTINCT",
"scan with aggregate push-down: REGR_SLOPE with DISTINCT",
"scan with aggregate push-down: REGR_R2 with DISTINCT",
-"scan with aggregate push-down: REGR_SXY with DISTINCT")
+"scan with aggregate push-down: REGR_SXY with DISTINCT",
+"simple timestamps roundtrip",
+"simple timestamps pushdown")
override val catalogName: String = "db2"
override val namespaceOpt: Option[String] = Some("DB2INST1")
override val db = new DB2DatabaseOnDocker
+ override def url: String = db.getJdbcUrl(dockerIp, externalPort)
Review Comment:
Can't really because `db.getJdbcUrl(dockerIp, externalPort)` is dependent on
`DockerJDBCIntegrationV2Suite`.
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47424][SQL] Add getDatabaseCalendar method to the JdbcDialect [spark]
beliefer commented on code in PR #45537:
URL: https://github.com/apache/spark/pull/45537#discussion_r1607799864
##
connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/DB2IntegrationSuite.scala:
##
@@ -45,15 +45,18 @@ class DB2IntegrationSuite extends
DockerJDBCIntegrationV2Suite with V2JDBCTest {
"scan with aggregate push-down: REGR_INTERCEPT with DISTINCT",
"scan with aggregate push-down: REGR_SLOPE with DISTINCT",
"scan with aggregate push-down: REGR_R2 with DISTINCT",
-"scan with aggregate push-down: REGR_SXY with DISTINCT")
+"scan with aggregate push-down: REGR_SXY with DISTINCT",
+"simple timestamps roundtrip",
+"simple timestamps pushdown")
override val catalogName: String = "db2"
override val namespaceOpt: Option[String] = Some("DB2INST1")
override val db = new DB2DatabaseOnDocker
+ override def url: String = db.getJdbcUrl(dockerIp, externalPort)
Review Comment:
How about put `def url: String = db.getJdbcUrl(dockerIp, externalPort)` into
`V2JDBCTest` and avoid the duplicate code ?
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47424][SQL] Add getDatabaseCalendar method to the JdbcDialect [spark]
PetarVasiljevic-DB commented on code in PR #45537: URL: https://github.com/apache/spark/pull/45537#discussion_r1603513988 ## connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/OracleIntegrationSuite.scala: ## @@ -125,6 +126,29 @@ class OracleIntegrationSuite extends DockerJDBCIntegrationV2Suite with V2JDBCTes override def caseConvert(tableName: String): String = tableName.toUpperCase(Locale.ROOT) + override protected val timestampNTZType: String = "TIMESTAMP WITH LOCAL TIME ZONE" Review Comment: Yep, thanks -- 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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47424][SQL] Add getDatabaseCalendar method to the JdbcDialect [spark]
cloud-fan commented on code in PR #45537: URL: https://github.com/apache/spark/pull/45537#discussion_r1603428926 ## connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/OracleIntegrationSuite.scala: ## @@ -125,6 +126,29 @@ class OracleIntegrationSuite extends DockerJDBCIntegrationV2Suite with V2JDBCTes override def caseConvert(tableName: String): String = tableName.toUpperCase(Locale.ROOT) + override protected val timestampNTZType: String = "TIMESTAMP WITH LOCAL TIME ZONE" Review Comment: isn't it LTZ? -- 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-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47424][SQL] Add getDatabaseCalendar method to the JdbcDialect [spark]
cloud-fan commented on code in PR #45537:
URL: https://github.com/apache/spark/pull/45537#discussion_r1603426755
##
connector/docker-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/v2/DB2IntegrationSuite.scala:
##
@@ -102,4 +105,7 @@ class DB2IntegrationSuite extends
DockerJDBCIntegrationV2Suite with V2JDBCTest {
}
override def caseConvert(tableName: String): String =
tableName.toUpperCase(Locale.ROOT)
+
+ override protected val timestampNTZType: String = "TIMESTAMP WITHOUT TIME
ZONE"
+ override protected val timestampTZType: String = "TIMESTAMP"
Review Comment:
do you mean NTZ? TZ usually means WITH TIME ZONE
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47424][SQL] Add getDatabaseCalendar method to the JdbcDialect [spark]
PetarVasiljevic-DB commented on code in PR #45537:
URL: https://github.com/apache/spark/pull/45537#discussion_r1601528824
##
sql/core/src/main/scala/org/apache/spark/sql/jdbc/MySQLDialect.scala:
##
@@ -232,6 +232,8 @@ private case class MySQLDialect() extends JdbcDialect with
SQLConfHelper {
case _ => JdbcUtils.getCommonJDBCType(dt)
}
+ override def getDatabaseCalendar: Option[Calendar] = None
Review Comment:
Not sure what you mean. Like running it through mysql jdbc driver and not
mariadb?
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47424][SQL] Add getDatabaseCalendar method to the JdbcDialect [spark]
beliefer commented on code in PR #45537:
URL: https://github.com/apache/spark/pull/45537#discussion_r1601523317
##
sql/core/src/main/scala/org/apache/spark/sql/jdbc/MySQLDialect.scala:
##
@@ -232,6 +232,8 @@ private case class MySQLDialect() extends JdbcDialect with
SQLConfHelper {
case _ => JdbcUtils.getCommonJDBCType(dt)
}
+ override def getDatabaseCalendar: Option[Calendar] = None
Review Comment:
Why not consider MySQL directly?
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47424][SQL] Add getDatabaseCalendar method to the JdbcDialect [spark]
PetarVasiljevic-DB commented on code in PR #45537:
URL: https://github.com/apache/spark/pull/45537#discussion_r1601500893
##
sql/core/src/main/scala/org/apache/spark/sql/jdbc/MySQLDialect.scala:
##
@@ -232,6 +232,8 @@ private case class MySQLDialect() extends JdbcDialect with
SQLConfHelper {
case _ => JdbcUtils.getCommonJDBCType(dt)
}
+ override def getDatabaseCalendar: Option[Calendar] = None
Review Comment:
Because MariaDB driver is used for MySQL and as far as I can see here
[https://github.dev/mariadb-corporation/mariadb-connector-j/blob/75b1201bf57f2685bf[…]/internal/com/read/resultset/rowprotocol/BinaryRowProtocol.java](https://github.dev/mariadb-corporation/mariadb-connector-j/blob/75b1201bf57f2685bf49a11e7aa55f1e2e57f65d/src/main/java/org/mariadb/jdbc/internal/com/read/resultset/rowprotocol/BinaryRowProtocol.java#L1209)
calendar is not considered (this corresponds to 2.7.12 that we use).
Also, on their master they don't consider calendar too:
[https://github.dev/mariadb-corporation/mariadb-connector-j/blob/0db5648c1b90303937[…]c/main/java/org/mariadb/jdbc/client/column/TimestampColumn.java](https://github.dev/mariadb-corporation/mariadb-connector-j/blob/0db5648c1b90303937b12929b592b8af96498bea/src/main/java/org/mariadb/jdbc/client/column/TimestampColumn.java#L99)
+ tested locally, results don't change no matter what calendar is provided
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47424][SQL] Add getDatabaseCalendar method to the JdbcDialect [spark]
PetarVasiljevic-DB commented on code in PR #45537:
URL: https://github.com/apache/spark/pull/45537#discussion_r1601500893
##
sql/core/src/main/scala/org/apache/spark/sql/jdbc/MySQLDialect.scala:
##
@@ -232,6 +232,8 @@ private case class MySQLDialect() extends JdbcDialect with
SQLConfHelper {
case _ => JdbcUtils.getCommonJDBCType(dt)
}
+ override def getDatabaseCalendar: Option[Calendar] = None
Review Comment:
Because MariaDB driver is used for MySQL and as far as I can see here
[https://github.dev/mariadb-corporation/mariadb-connector-j/blob/75b1201bf57f2685bf[…]/internal/com/read/resultset/rowprotocol/BinaryRowProtocol.java](https://github.dev/mariadb-corporation/mariadb-connector-j/blob/75b1201bf57f2685bf49a11e7aa55f1e2e57f65d/src/main/java/org/mariadb/jdbc/internal/com/read/resultset/rowprotocol/BinaryRowProtocol.java#L1209)
calendar is not considered (this corresponds to 2.7.9 that we use).
Also, on their master they don't consider calendar too:
[https://github.dev/mariadb-corporation/mariadb-connector-j/blob/0db5648c1b90303937[…]c/main/java/org/mariadb/jdbc/client/column/TimestampColumn.java](https://github.dev/mariadb-corporation/mariadb-connector-j/blob/0db5648c1b90303937b12929b592b8af96498bea/src/main/java/org/mariadb/jdbc/client/column/TimestampColumn.java#L99)
+ tested locally, results don't change no matter what calendar is provided
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47424][SQL] Add getDatabaseCalendar method to the JdbcDialect [spark]
PetarVasiljevic-DB commented on code in PR #45537:
URL: https://github.com/apache/spark/pull/45537#discussion_r1601500893
##
sql/core/src/main/scala/org/apache/spark/sql/jdbc/MySQLDialect.scala:
##
@@ -232,6 +232,8 @@ private case class MySQLDialect() extends JdbcDialect with
SQLConfHelper {
case _ => JdbcUtils.getCommonJDBCType(dt)
}
+ override def getDatabaseCalendar: Option[Calendar] = None
Review Comment:
Because MariaDB driver is used for MySQL and as far as I can see here
[https://github.dev/mariadb-corporation/mariadb-connector-j/blob/75b1201bf57f2685bf[…]/internal/com/read/resultset/rowprotocol/BinaryRowProtocol.java](https://github.dev/mariadb-corporation/mariadb-connector-j/blob/75b1201bf57f2685bf49a11e7aa55f1e2e57f65d/src/main/java/org/mariadb/jdbc/internal/com/read/resultset/rowprotocol/BinaryRowProtocol.java#L1209)
calendar is not considered (this corresponds to 2.7.9 that we use).
Also, on their master they don't consider calendar too:
[https://github.dev/mariadb-corporation/mariadb-connector-j/blob/0db5648c1b90303937[…]c/main/java/org/mariadb/jdbc/client/column/TimestampColumn.java](https://github.dev/mariadb-corporation/mariadb-connector-j/blob/0db5648c1b90303937b12929b592b8af96498bea/src/main/java/org/mariadb/jdbc/client/column/TimestampColumn.java#L99)
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47424][SQL] Add getDatabaseCalendar method to the JdbcDialect [spark]
beliefer commented on code in PR #45537:
URL: https://github.com/apache/spark/pull/45537#discussion_r1601494207
##
sql/core/src/main/scala/org/apache/spark/sql/jdbc/MySQLDialect.scala:
##
@@ -232,6 +232,8 @@ private case class MySQLDialect() extends JdbcDialect with
SQLConfHelper {
case _ => JdbcUtils.getCommonJDBCType(dt)
}
+ override def getDatabaseCalendar: Option[Calendar] = None
Review Comment:
Why MySQL dialect also return None here? the same as H2?
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47424][SQL] Add getDatabaseCalendar method to the JdbcDialect [spark]
cloud-fan commented on code in PR #45537:
URL: https://github.com/apache/spark/pull/45537#discussion_r1596901652
##
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##
@@ -3992,6 +3992,16 @@ object SQLConf {
.booleanConf
.createWithDefault(false)
+ val USE_LOCAL_SESSION_CALENDAR =
buildConf("spark.sql.jdbc.useLocalSessionCalendar")
Review Comment:
let's turn it into a legacy config
```
spark.sql.legacy.jdbc.useNullCalendar
```
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47424][SQL] Add getDatabaseCalendar method to the JdbcDialect [spark]
cloud-fan commented on code in PR #45537:
URL: https://github.com/apache/spark/pull/45537#discussion_r1596899527
##
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JdbcUtils.scala:
##
@@ -499,8 +500,14 @@ object JdbcUtils extends Logging with SQLConfHelper {
}
case TimestampType =>
+ val calendar = if (conf.useLocalSessionCalendar) {
+dialect.getDatabaseCalendar.get
Review Comment:
```suggestion
dialect.getDatabaseCalendar.orNull
```
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47424][SQL] Add getDatabaseCalendar method to the JdbcDialect [spark]
cloud-fan commented on code in PR #45537:
URL: https://github.com/apache/spark/pull/45537#discussion_r1596898967
##
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##
@@ -3992,6 +3992,16 @@ object SQLConf {
.booleanConf
.createWithDefault(false)
+ val USE_LOCAL_SESSION_CALENDAR =
buildConf("spark.sql.jdbc.useLocalSessionCalendar")
+.doc("When retrieving timestamp NTZ columns from JDBC data source, user
will see the " +
+ "values as they where shown in data source only if the JVM time zone is
equal " +
+ "to the spark local session time zone. Since these two can differ, if
this " +
+ "configuration property is set to true, rs.getTimestamp and
rs.setTimestamp " +
+ "API calls will use calendar with spark local session time zone as the
second argument")
+.version("4.0.0")
+.booleanConf
Review Comment:
let's add `.internal`. Users should not set this. It's only for unknown bugs.
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47424][SQL] Add getDatabaseCalendar method to the JdbcDialect [spark]
cloud-fan commented on code in PR #45537:
URL: https://github.com/apache/spark/pull/45537#discussion_r1596897736
##
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##
@@ -3992,6 +3992,16 @@ object SQLConf {
.booleanConf
.createWithDefault(false)
+ val USE_LOCAL_SESSION_CALENDAR =
buildConf("spark.sql.jdbc.useLocalSessionCalendar")
+.doc("When retrieving timestamp NTZ columns from JDBC data source, user
will see the " +
Review Comment:
```suggestion
.doc("When read timestamp NTZ columns from JDBC data source as Spark
timestamp LTZ, user will see the " +
```
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47424][SQL] Add getDatabaseCalendar method to the JdbcDialect [spark]
cloud-fan commented on code in PR #45537:
URL: https://github.com/apache/spark/pull/45537#discussion_r1596897400
##
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##
@@ -3992,6 +3992,16 @@ object SQLConf {
.booleanConf
.createWithDefault(false)
+ val USE_LOCAL_SESSION_CALENDAR =
buildConf("spark.sql.jdbc.useLocalSessionCalendar")
+.doc("When retrieving timestamp NTZ columns from JDBC data source, user
will see the " +
+ "values as they where shown in data source only if the JVM time zone is
equal " +
Review Comment:
```suggestion
"values as they were shown in data source only if the JVM time zone is
equal " +
```
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47424][SQL] Add getDatabaseCalendar method to the JdbcDialect [spark]
cloud-fan commented on code in PR #45537:
URL: https://github.com/apache/spark/pull/45537#discussion_r1596819133
##
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JdbcUtils.scala:
##
@@ -500,7 +501,7 @@ object JdbcUtils extends Logging with SQLConfHelper {
case TimestampType =>
(rs: ResultSet, row: InternalRow, pos: Int) =>
-val t = rs.getTimestamp(pos + 1)
+val t = rs.getTimestamp(pos + 1, dialect.getDatabaseCalendar.get)
Review Comment:
let's avoid per row access
```
val calendar = if (conf) {
dialect.getDatabaseCalendar
} else {
null
}
(rs: ResultSet, row: InternalRow, pos: Int) => ...
```
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47424][SQL] Add getDatabaseCalendar method to the JdbcDialect [spark]
cloud-fan commented on code in PR #45537:
URL: https://github.com/apache/spark/pull/45537#discussion_r1596818126
##
sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala:
##
@@ -215,6 +215,21 @@ abstract class JdbcDialect extends Serializable with
Logging {
toJavaTimestampNoRebase(micros)
}
+ /**
+ * Returns a calendar that will be used when reading (rs.getTimestamp) or
+ * writing to the database (setTimestamp). If the calendar is None, old code
path
+ * in [[JdbcUtils]] will be hit and calendar won't be used.
+ * @return
+ */
+ @Since("3.5.0")
+ def getDatabaseCalendar: Option[Calendar] = {
+if (SQLConf.get.useLocalSessionCalendar) {
Review Comment:
we should not access conf here. If a dialect overrides it, the conf won't be
affective.
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47424][SQL] Add getDatabaseCalendar method to the JdbcDialect [spark]
cloud-fan commented on code in PR #45537:
URL: https://github.com/apache/spark/pull/45537#discussion_r1596817507
##
sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala:
##
@@ -3992,6 +3992,16 @@ object SQLConf {
.booleanConf
.createWithDefault(false)
+ val USE_LOCAL_SESSION_CALENDAR =
buildConf("spark.sql.jdbc.useLocalSessionCalendar")
+.doc("When retrieving timestamp NTZ columns from JDBC data source, user
will see the " +
+ "values as they where shown in data source only if the JVM time zone is
equal " +
+ "to the spark local session time zone. Since these two can differ, if
this " +
+ "configuration property is set to true, rs.getTimestamp and
rs.setTimestamp " +
+ "API calls will use calendar with spark local session time zone as the
second argument")
+.version("3.5.0")
Review Comment:
4.0.0
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47424][SQL] Add getDatabaseCalendar method to the JdbcDialect [spark]
cloud-fan commented on code in PR #45537:
URL: https://github.com/apache/spark/pull/45537#discussion_r1596816731
##
sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala:
##
@@ -215,6 +215,21 @@ abstract class JdbcDialect extends Serializable with
Logging {
toJavaTimestampNoRebase(micros)
}
+ /**
+ * Returns a calendar that will be used when reading (rs.getTimestamp) or
+ * writing to the database (setTimestamp). If the calendar is None, old code
path
+ * in [[JdbcUtils]] will be hit and calendar won't be used.
+ * @return
+ */
+ @Since("3.5.0")
Review Comment:
4.0.0
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47424][SQL] Add getDatabaseCalendar method to the JdbcDialect [spark]
milastdbx commented on code in PR #45537:
URL: https://github.com/apache/spark/pull/45537#discussion_r1593776643
##
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JdbcUtils.scala:
##
@@ -500,7 +501,11 @@ object JdbcUtils extends Logging with SQLConfHelper {
case TimestampType =>
(rs: ResultSet, row: InternalRow, pos: Int) =>
-val t = rs.getTimestamp(pos + 1)
+val t = dialect.getDatabaseCalendar match {
+ case Some(cal) => rs.getTimestamp(pos + 1, cal)
Review Comment:
Unforntunatelly it really depends on driver implementation whether they
handle difference between TZ and NTZ in their getTimestamp/setTimestamp codepath
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
Re: [PR] [SPARK-47424][SQL] Add getDatabaseCalendar method to the JdbcDialect [spark]
cloud-fan commented on code in PR #45537:
URL: https://github.com/apache/spark/pull/45537#discussion_r1593536803
##
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/jdbc/JdbcUtils.scala:
##
@@ -500,7 +501,11 @@ object JdbcUtils extends Logging with SQLConfHelper {
case TimestampType =>
(rs: ResultSet, row: InternalRow, pos: Int) =>
-val t = rs.getTimestamp(pos + 1)
+val t = dialect.getDatabaseCalendar match {
+ case Some(cal) => rs.getTimestamp(pos + 1, cal)
Review Comment:
shall we always call `get/setTimestamp` with a calendar instance of the
session local timezone? We can add a config to restore the old behavior,
instead of adding a new API in Dialect.
--
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-unsubscr...@spark.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
