[SPARK-44280][SQL] Add convertJavaTimestampToTimestamp in JDBCDialect API #41843

mingkangli-db · 2023-07-04T05:52:49Z

What changes were proposed in this pull request?

This PR fixes an overflow issue upon reading an "infinity" timestamp from PostgreSQL. It addresses the issue by adding a new function, convertJavaTimestampToTimestamp, to the JDBCDialects API, and overrides it in PostgresDialect.scala by clamping the special "infinity" timestamps to long.max and long.min.

Why are the changes needed?

The pre-existing default behavior of timestamp conversion potentially triggers an overflow due to these special values (i.e. The executor would crash if you select a column that contains infinity timestamps in PostgreSQL.) By integrating this new function, we can mitigate such issues, enabling more versatile and robust timestamp value conversions across various JDBC-based connectors.

Does this PR introduce any user-facing change?

A new function, convertJavaTimestampToTimestamp, is added to the JDBCDialects API to allow JDBC dialects to override the default behavior of converting Java timestamps.

How was this patch tested?

An integration test was added in PostgresIntegrationSuite.scala to verify it can handle +infinity and -infinity timestamps in PostgreSQL.

mingkangli-db · 2023-07-04T05:58:14Z

cc @cloud-fan @HyukjinKwon

MaxGekk · 2023-07-04T06:02:40Z

sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala

+   * @throws IllegalArgumentException if t is null
+   */
+  def convertJavaTimestampToTimestamp(t: Timestamp): Long = {
+    require(t != null, "timestamp must be non-null")


Is this an user-facing error? If so, please, add an error class to error-classes.json. See for instance:

spark/sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala

Line 478 in 7bc28d5

throw QueryCompilationErrors.unsupportedTableChangeInJDBCCatalogError(change)

It is not an user facing error but is a precondition: the timestamp passed into this function will never be null. I removed the null check because a similar function convertJavaTimestampToTimestampNTZ in JdbcDialects.scala does not have the check either.

HyukjinKwon · 2023-07-05T07:27:37Z

cc @sadikovi FYI

ShreyeshArangath

LGTM!

yaooqinn · 2023-07-06T04:07:21Z

Can we provide more information about the overflow issue？IIRC, Spark doesn't accommodate infinite timestamps. It may be more appropriate to fail on overflow or unsupported values instead of converting them to maximum values.

sadikovi · 2023-07-06T04:11:03Z

sql/core/src/main/scala/org/apache/spark/sql/jdbc/PostgresDialect.scala

+   * PostgreSQL has four special "infinity values" that we need clamp to avoid overflow.
+   * If it is not one of the infinity values, fall back to default behavior. */
+  override def convertJavaTimestampToTimestamp(t: Timestamp): Long = {
+


nit: no new line is needed.

sadikovi · 2023-07-06T04:11:29Z

sql/core/src/main/scala/org/apache/spark/sql/jdbc/PostgresDialect.scala

+   * If it is not one of the infinity values, fall back to default behavior. */
+  override def convertJavaTimestampToTimestamp(t: Timestamp): Long = {
+
+    // variable names come from PostgreSQL "constant field docs":


nit: Please update to this: // Variable names come from ...

sadikovi · 2023-07-06T04:14:51Z

@mingkangli-db Please update the title of the PR. It is not [CORE] but [SQL].

sadikovi · 2023-07-06T04:21:25Z

@yaooqinn Additional information is provided in the PR description: if Postgres table contains infinity timestamps, users cannot read such tables in Spark.

I don't think failing such queries is an option - they already fail even without this change, albeit with an overflow error. Postgres driver handles infinity values as mentioned in the PR, Spark simply fails to convert them. This PR adds the ability to bypass Spark conversion for such values.

org.apache.spark.sql.catalyst.util.DateTimeUtils.fromJavaTimestamp(new java.sql.Timestamp(-9223372036832400000L))

would fail with

java.lang.ArithmeticException: long overflow
  java.lang.Math.multiplyExact(Math.java:892)
  org.apache.spark.sql.catalyst.util.DateTimeUtils$.millisToMicros(DateTimeUtils.scala:257)
  org.apache.spark.sql.catalyst.util.DateTimeUtils$.fromJavaTimestampNoRebase(DateTimeUtils.scala:209)
  org.apache.spark.sql.catalyst.util.DateTimeUtils$.fromJavaTimestamp(DateTimeUtils.scala:199)

IMHO, it could be beneficial to refactor code and add a special conversion function since other JDBC dialects could potentially require timestamp handling, like in this case.

Also, this infinity timestamp fix is only for Postgres, not all dialects.

cloud-fan · 2023-07-06T20:53:54Z

sql/core/src/main/scala/org/apache/spark/sql/jdbc/PostgresDialect.scala

+
+    if (time == POSTGRESQL_DATE_POSITIVE_INFINITY ||
+      time == POSTGRESQL_DATE_DATE_POSITIVE_SMALLER_INFINITY) {
+      Long.MaxValue


If we query a infinite timestamp column in pgsql, what does pgsql display?

It will be displayed as "+infinity" or "-infinity". See: here "The values infinity and -infinity are specially represented inside the system and will be displayed unchanged"

cloud-fan · 2023-07-06T21:16:37Z

sql/core/src/main/scala/org/apache/spark/sql/jdbc/PostgresDialect.scala

+    val POSTGRESQL_DATE_POSITIVE_INFINITY = 9223372036825200000L
+    val POSTGRESQL_DATE_DATE_POSITIVE_SMALLER_INFINITY = 185543533774800000L
+
+    val time = t.getTime


So the range of java Timestamp is larger than Spark SQL timestamp?

What is the Spark SQL timestamp range?

From Long.MinValue microseconds before UTC epoch to Long.MaxValue microseconds after UTC epoch.

Then it is the same range for java Timestamp.

then how does the overflow happen? because the calendar is different?

The problem is that java.sql timestamps are measured with millisecond accuracy (from Long.MinValue milliseconds to Long.MaxValue milliseconds), see here while Spark timestamps are measured at microseconds accuracy. So we would get an overflow exception when we call MultiplyExact by 1000 in Java.

The stacktrace would look something like this:
at java.lang.Math.multiplyExact(Math.java:892)
at org.apache.spark.sql.catalyst.util.DateTimeUtils$.millisToMicros(DateTimeUtils.scala:xxx)
at org.apache.spark.sql.catalyst.util.DateTimeUtils$.fromJavaTimestampNoRebase(DateTimeUtils.scala:xxx)
at org.apache.spark.sql.catalyst.util.DateTimeUtils$.fromJavaTimestamp(DateTimeUtils.scala:xxx)
at org.apache.spark.sql.execution.datasources.jdbc.JdbcUtils$.$anonfun$makeGetter$15(JdbcUtils.scala:xxx)

To simplify, Long.MaxValue should be the min value in microseconds to not overflow, is that right @mingkangli-db?

Yes, if you mean Long.MaxValue itself is the maximum value in microseconds that can be stored to not cause overflow. I added some comments in PostgresDialect.scala, hopefully this would make it clearer.

sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala

…into SPARK-44280

yaooqinn · 2023-07-26T02:34:25Z

...er-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/PostgresIntegrationSuite.scala

@@ -432,4 +437,18 @@ class PostgresIntegrationSuite extends DockerJDBCIntegrationSuite {
    assert(row(0).getSeq[String](0) == Seq("1", "fds", "fdsa"))
    assert(row(0).getString(1) == "fdasfasdf")
  }
+
+  test("SPARK-44280: infinity timestamp test") {
+    val df = sqlContext.read.jdbc(jdbcUrl, "infinity_timestamp", new Properties)


Can we also use the write API to play a roundtrip here？

The goal here is upon reading one of the infinity timestamps in Postgresql, it is cast into reasonable values in Spark SQL instead of throwing an overflow error. However, from the other direction, since there is no built-in "infinity" values in Spark SQL, we can't write an infinity timestamp value to Postgresql, so I don't think a roundtrip test would be possible here.

cloud-fan · 2023-08-02T06:47:31Z

thanks, merging to master/3.5 (fix pgsql dialect)!

… API ### What changes were proposed in this pull request? This PR fixes an overflow issue upon reading an "infinity" timestamp from PostgreSQL. It addresses the issue by adding a new function, convertJavaTimestampToTimestamp, to the JDBCDialects API, and overrides it in PostgresDialect.scala by clamping the special "infinity" timestamps to long.max and long.min. ### Why are the changes needed? The pre-existing default behavior of timestamp conversion potentially triggers an overflow due to these special values (i.e. The executor would crash if you select a column that contains infinity timestamps in PostgreSQL.) By integrating this new function, we can mitigate such issues, enabling more versatile and robust timestamp value conversions across various JDBC-based connectors. ### Does this PR introduce _any_ user-facing change? A new function, convertJavaTimestampToTimestamp, is added to the JDBCDialects API to allow JDBC dialects to override the default behavior of converting Java timestamps. ### How was this patch tested? An integration test was added in PostgresIntegrationSuite.scala to verify it can handle +infinity and -infinity timestamps in PostgreSQL. Closes #41843 from mingkangli-db/SPARK-44280. Authored-by: Mingkang Li <mingkang.li@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit e91c024) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

… API ### What changes were proposed in this pull request? This PR fixes an overflow issue upon reading an "infinity" timestamp from PostgreSQL. It addresses the issue by adding a new function, convertJavaTimestampToTimestamp, to the JDBCDialects API, and overrides it in PostgresDialect.scala by clamping the special "infinity" timestamps to long.max and long.min. ### Why are the changes needed? The pre-existing default behavior of timestamp conversion potentially triggers an overflow due to these special values (i.e. The executor would crash if you select a column that contains infinity timestamps in PostgreSQL.) By integrating this new function, we can mitigate such issues, enabling more versatile and robust timestamp value conversions across various JDBC-based connectors. ### Does this PR introduce _any_ user-facing change? A new function, convertJavaTimestampToTimestamp, is added to the JDBCDialects API to allow JDBC dialects to override the default behavior of converting Java timestamps. ### How was this patch tested? An integration test was added in PostgresIntegrationSuite.scala to verify it can handle +infinity and -infinity timestamps in PostgreSQL. Closes apache#41843 from mingkangli-db/SPARK-44280. Authored-by: Mingkang Li <mingkang.li@databricks.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

yruslan · 2024-03-19T15:06:08Z

...er-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/PostgresIntegrationSuite.scala

+    val minTimeStamp = LocalDateTime.of(1, 1, 1, 0, 0, 0).toEpochSecond(ZoneOffset.UTC)
+    val maxTimestamp = LocalDateTime.of(9999, 12, 31, 23, 59, 59).toEpochSecond(ZoneOffset.UTC)
+
+    assert(infinity.getTime == maxTimestamp)
+    assert(negativeInfinity.getTime == minTimeStamp)


ts.getTime()' returns epoch in milliseconds, and LocalDateTime.toEpochSecond()` returns values in seconds.

Shouldn't this be

val minTimeStamp = LocalDateTime.of(1, 1, 1, 0, 0, 0).toEpochSecond(ZoneOffset.UTC) * 1000 val maxTimestamp = LocalDateTime.of(9999, 12, 31, 23, 59, 59).toEpochSecond(ZoneOffset.UTC) * 1000

Otherwise, convertJavaTimestampToTimestamp() does not return infinites as expected

yruslan · 2024-03-19T15:06:25Z

sql/core/src/main/scala/org/apache/spark/sql/jdbc/PostgresDialect.scala

+    val minTimeStamp = LocalDateTime.of(1, 1, 1, 0, 0, 0).toEpochSecond(ZoneOffset.UTC)
+    val maxTimestamp = LocalDateTime.of(9999, 12, 31, 23, 59, 59).toEpochSecond(ZoneOffset.UTC)
+
+    val time = t.getTime


@yaooqinn, @mingkangli-db, @cloud-fan

ts.getTime() returns epoch in milliseconds, new Timestamp(ts) takes time in milliseconds , and LocalDateTime.toEpochSecond() returns values in seconds.

Shouldn't this be

val minTimeStamp = LocalDateTime.of(1, 1, 1, 0, 0, 0).toEpochSecond(ZoneOffset.UTC) * 1000 val maxTimestamp = LocalDateTime.of(9999, 12, 31, 23, 59, 59).toEpochSecond(ZoneOffset.UTC) * 1000

e.g. * 1000

Otherwise:

val tsLong = LocalDateTime.of(9999, 12, 31, 23, 59, 59).toEpochSecond(ZoneOffset.UTC) val ts = new Timestamp(tsLong)

gives:

tsLong: Long = 253402300799 ts: java.sql.Timestamp = 1978-01-11 22:31:40.799

not 9999-12-31

and time is used later to create a new instance of Timestamp

Nice catch! We shall fix this...

…ITY timestamps ### What changes were proposed in this pull request? This PR fixes a bug involved with #41843 that Epoch Second is used instead of epoch millis to create a timestamp value ### Why are the changes needed? bugfix ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? revised tests ### Was this patch authored or co-authored using generative AI tooling? no Closes #45599 from yaooqinn/SPARK-47473. Authored-by: Kent Yao <yao@apache.org> Signed-off-by: Kent Yao <yao@apache.org>

…ITY timestamps This PR fixes a bug involved with #41843 that Epoch Second is used instead of epoch millis to create a timestamp value bugfix no revised tests no Closes #45599 from yaooqinn/SPARK-47473. Authored-by: Kent Yao <yao@apache.org> Signed-off-by: Kent Yao <yao@apache.org> (cherry picked from commit ad8ac17) Signed-off-by: Kent Yao <yao@apache.org>

…estampToTimestamp for JdbcDialect ### What changes were proposed in this pull request? Add convertDateToDate like the existing convertTimestampToTimestamp for JdbcDialect ### Why are the changes needed? The date '±infinity' values cause overflows like timestamp '±infinity' in #41843 ### Does this PR introduce _any_ user-facing change? fix expected overflow for dates to align with the timestamps of PostgreSQL ### How was this patch tested? new tests ### Was this patch authored or co-authored using generative AI tooling? no Closes #45638 from yaooqinn/SPARK-47501. Authored-by: Kent Yao <yao@apache.org> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

…ITY timestamps ### What changes were proposed in this pull request? This PR fixes a bug involved with apache#41843 that Epoch Second is used instead of epoch millis to create a timestamp value ### Why are the changes needed? bugfix ### Does this PR introduce _any_ user-facing change? no ### How was this patch tested? revised tests ### Was this patch authored or co-authored using generative AI tooling? no Closes apache#45599 from yaooqinn/SPARK-47473. Authored-by: Kent Yao <yao@apache.org> Signed-off-by: Kent Yao <yao@apache.org>

…estampToTimestamp for JdbcDialect ### What changes were proposed in this pull request? Add convertDateToDate like the existing convertTimestampToTimestamp for JdbcDialect ### Why are the changes needed? The date '±infinity' values cause overflows like timestamp '±infinity' in apache#41843 ### Does this PR introduce _any_ user-facing change? fix expected overflow for dates to align with the timestamps of PostgreSQL ### How was this patch tested? new tests ### Was this patch authored or co-authored using generative AI tooling? no Closes apache#45638 from yaooqinn/SPARK-47501. Authored-by: Kent Yao <yao@apache.org> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

add support for fromJavaTimeStamptoTimeStamp

bdb2be3

github-actions bot added the SQL label Jul 4, 2023

retrigger workflow

2ed6130

MaxGekk reviewed Jul 4, 2023

View reviewed changes

remove requires in fromJavaTimestampToTimestamp

6f9c93a

ShreyeshArangath approved these changes Jul 6, 2023

View reviewed changes

sadikovi reviewed Jul 6, 2023

View reviewed changes

mingkangli-db changed the title ~~[SPARK-44280][CORE] Add convertJavaTimestampToTimestamp in JDBCDialect API~~ [SPARK-44280][SQL] Add convertJavaTimestampToTimestamp in JDBCDialect API Jul 6, 2023

mingkangli-db added 2 commits July 6, 2023 06:50

Update PostgresDialect.scala

adf23ce

Update PostgresDialect.scala

9445d37

cloud-fan reviewed Jul 6, 2023

View reviewed changes

cloud-fan approved these changes Jul 7, 2023

View reviewed changes

mingkangli-db added 12 commits July 6, 2023 18:00

further clarify why clamping is needed in PostgresDialect.scala

2e05df7

fix style

f2f77e8

Update PostgresDialect.scala

a2da4ff

Update PostgresDialect.scala

304de8f

fix formatting

f29b4de

Merge branch 'master' into SPARK-44280

d287b29

format once more

6041a43

Update PostgresIntegrationSuite.scala

9471d6a

Update PostgresIntegrationSuite.scala

40597af

Merge branch 'apache:master' into SPARK-44280

4b07018

add fix attempt

2916930

refactor

700fc72

mingkangli-db added 10 commits July 20, 2023 13:26

explicit import UTC

34ecd58

remove duplicate import

c7c84d7

import ZoneOffset

bf88f33

Update PostgresDialect.scala

a4ea4b5

Update PostgresDialect.scala

ffb9ea9

Merge branch 'apache:master' into SPARK-44280

18bb802

Empty-Commit

ec204c4

Update PostgresIntegrationSuite.scala

ace6182

Update PostgresIntegrationSuite.scala

d1f5578

Update PostgresIntegrationSuite.scala

d752feb

cloud-fan reviewed Jul 25, 2023

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala Show resolved Hide resolved

cloud-fan reviewed Jul 25, 2023

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/jdbc/JdbcDialects.scala Show resolved Hide resolved

cloud-fan approved these changes Jul 25, 2023

View reviewed changes

mingkangli-db added 2 commits July 25, 2023 10:37

address comments

fa52e05

Merge branch 'SPARK-44280' of https://github.com/mingkangli-db/spark …

e3fd600

…into SPARK-44280

yaooqinn reviewed Jul 26, 2023

View reviewed changes

Merge branch 'master' into SPARK-44280

30db0bb

cloud-fan closed this in e91c024 Aug 2, 2023

yruslan reviewed Mar 19, 2024

View reviewed changes

yaooqinn mentioned this pull request Mar 20, 2024

[SPARK-47473][SQL] Fix correctness issue of converting postgres INFINITY timestamps #45599

Closed

yaooqinn mentioned this pull request Mar 21, 2024

[SPARK-47501][SQL] Add convertDateToDate like the existing convertTimestampToTimestamp for JdbcDialect #45638

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-44280][SQL] Add convertJavaTimestampToTimestamp in JDBCDialect API #41843

[SPARK-44280][SQL] Add convertJavaTimestampToTimestamp in JDBCDialect API #41843

mingkangli-db commented Jul 4, 2023

mingkangli-db commented Jul 4, 2023

MaxGekk Jul 4, 2023

mingkangli-db Jul 6, 2023

HyukjinKwon commented Jul 5, 2023

ShreyeshArangath left a comment

yaooqinn commented Jul 6, 2023

sadikovi Jul 6, 2023

sadikovi Jul 6, 2023

sadikovi commented Jul 6, 2023

sadikovi commented Jul 6, 2023 •

edited

Loading

cloud-fan Jul 6, 2023

mingkangli-db Jul 6, 2023

cloud-fan Jul 6, 2023

sadikovi Jul 6, 2023

cloud-fan Jul 6, 2023

sadikovi Jul 7, 2023

cloud-fan Jul 7, 2023

mingkangli-db Jul 7, 2023 •

edited

Loading

sadikovi Jul 7, 2023

mingkangli-db Jul 7, 2023 •

edited

Loading

yaooqinn Jul 26, 2023

mingkangli-db Jul 28, 2023

cloud-fan commented Aug 2, 2023

yruslan Mar 19, 2024

yruslan Mar 19, 2024 •

edited

Loading

yaooqinn Mar 19, 2024

[SPARK-44280][SQL] Add convertJavaTimestampToTimestamp in JDBCDialect API #41843

[SPARK-44280][SQL] Add convertJavaTimestampToTimestamp in JDBCDialect API #41843

Conversation

mingkangli-db commented Jul 4, 2023

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

mingkangli-db commented Jul 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HyukjinKwon commented Jul 5, 2023

ShreyeshArangath left a comment

Choose a reason for hiding this comment

yaooqinn commented Jul 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sadikovi commented Jul 6, 2023

sadikovi commented Jul 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mingkangli-db Jul 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mingkangli-db Jul 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan commented Aug 2, 2023

Choose a reason for hiding this comment

yruslan Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sadikovi commented Jul 6, 2023 •

edited

Loading

mingkangli-db Jul 7, 2023 •

edited

Loading

mingkangli-db Jul 7, 2023 •

edited

Loading

yruslan Mar 19, 2024 •

edited

Loading