[SPARK-35664][SQL] Support java.time.LocalDateTime as an external type of TimestampWithoutTZ type by gengliangwang · Pull Request #32814 · apache/spark

gengliangwang · 2021-06-08T04:58:58Z

What changes were proposed in this pull request?

In the PR, I propose to extend Spark SQL API to accept java.time.LocalDateTime as an external type of recently added new Catalyst type - TimestampWithoutTZ. The Java class java.time.LocalDateTime has a similar semantic to ANSI SQL timestamp without timezone type, and it is the most suitable to be an external type for TimestampWithoutTZType. In more details:

Added TimestampWithoutTZConverter which converts java.time.LocalDateTime instances to/from internal representation of the Catalyst type TimestampWithoutTZType (to Long type). The TimestampWithoutTZConverter object uses new methods of DateTimeUtils:
- localDateTimeToMicros() converts the input date time to the total length in microseconds.
- microsToLocalDateTime() obtains a java.time.LocalDateTime
Support new type TimestampWithoutTZType in RowEncoder via the methods createDeserializerForLocalDateTime() and createSerializerForLocalDateTime().
Extended the Literal API to construct literals from java.time.LocalDateTime instances.

Why are the changes needed?

To allow users parallelization of java.time.LocalDateTime collections, and construct timestamp without time zone columns. Also to collect such columns back to the driver side.

Does this PR introduce any user-facing change?

The PR extends existing functionality. So, users can parallelize instances of the java.time.LocalDateTime class and collect them back.

scala> val ds = Seq(java.time.LocalDateTime.parse("1970-01-01T00:00:00")).toDS
ds: org.apache.spark.sql.Dataset[java.time.LocalDateTime] = [value: timestampwithouttz]
scala> ds.collect()
res0: Array[java.time.LocalDateTime] = Array(1970-01-01T00:00)

How was this patch tested?

New unit tests

SparkQA · 2021-06-08T06:07:23Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43970/

SparkQA · 2021-06-08T07:39:32Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43970/

SparkQA · 2021-06-08T09:07:33Z

Test build #139447 has finished for PR 32814 at commit 1101f55.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2021-06-08T10:05:41Z

Kubernetes integration test unable to build dist.

exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43990/

SparkQA · 2021-06-08T11:00:45Z

Test build #139469 has finished for PR 32814 at commit ba44cb1.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2021-06-08T11:03:53Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43992/

SparkQA · 2021-06-08T11:36:58Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/43992/

MaxGekk · 2021-06-08T12:51:34Z

@gengliangwang Have you done similar changes as in #31729, correct? Just want to avoid comparing of this PR to similar PRs for intervals.

gengliangwang · 2021-06-08T12:52:59Z

@MaxGekk Yes I follow #31729

MaxGekk · 2021-06-08T13:38:11Z

+ localDateTime.toEpochSecond(ZoneOffset.UTC) * MICROS_PER_SECOND +
+ localDateTime.getNano / NANOS_PER_MICROS


Potentially, * and + can overflow. Could you use Math.multiplyExact

Also, please, add tests to check long overflow.

BTW, I replaced * by multiplyExact, and got:
java.lang.ArithmeticException: long overflow at java.lang.Math.multiplyExact(Math.java:892) at org.apache.spark.sql.catalyst.util.DateTimeUtils$.localDateTimeToMicros(DateTimeUtils.scala:79) at org.apache.spark.sql.catalyst.util.DateTimeUtilsSuite.$anonfun$new$51(DateTimeUtilsSuite.scala:660)
on your new test:
assert(DateTimeUtils.localDateTimeToMicros(LocalDateTime.parse("-290308-12-21T19:59:05.224192")) ==Long.MinValue)

…/DateTimeUtils.scala Co-authored-by: Maxim Gekk <max.gekk@gmail.com>

SparkQA · 2021-06-08T14:31:33Z

Test build #139487 has finished for PR 32814 at commit 0418133.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

MaxGekk · 2021-06-08T14:39:02Z

@gengliangwang I wonder why did you remove the checks for Long.MaxValue/MinValue:

Does instantToMicros overflow on the values?

gengliangwang · 2021-06-08T15:12:05Z

Does instantToMicros overflow on the values?

@MaxGekk Yes. The Long.MinValue case contains negative seconds and positive nanoseconds. Thus it overflows when we try to convert it back. We can fix it but it is too corner. The fix makes performance worse, too.

scala> import org.apache.spark.sql.catalyst.util.DateTimeUtils._
import org.apache.spark.sql.catalyst.util.DateTimeUtils._
scala> instantToMicros(microsToInstant(Long.MinValue))
java.lang.ArithmeticException: long overflow
at java.lang.Math.multiplyExact(Math.java:892)
at org.apache.spark.sql.catalyst.util.DateTimeUtils$.instantToMicros(DateTimeUtils.scala:389)
... 49 elided
scala> microsToInstant(Long.MinValue).getNano
res10: Int = 224192000

SparkQA · 2021-06-08T15:13:21Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44012/

SparkQA · 2021-06-08T15:15:55Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44010/

MaxGekk · 2021-06-08T15:35:23Z

Yes. The Long.MinValue case contains negative seconds and positive nanoseconds.

@gengliangwang Could you open an JIRA for the issue, so, we will fix this separately from this PR.

gengliangwang · 2021-06-08T15:46:18Z

@MaxGekk https://issues.apache.org/jira/browse/SPARK-35679

SparkQA · 2021-06-08T15:47:07Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44012/

SparkQA · 2021-06-08T15:51:36Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44010/

…UserDefinedTypeSuite ### What changes were proposed in this pull request? Refactor LocalDateTimeUDT as YearUDT in UserDefinedTypeSuite ### Why are the changes needed? As we are going to support java.time.LocalDateTime as an external type of TimestampWithoutTZ type #32814, registering java.time.LocalDateTime as UDT will cause test failures: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/139469/testReport/ This PR is to unblock #32814. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Unit test. Closes#32824 from gengliangwang/UDTFollowUp. Authored-by: Gengliang Wang <gengliang@apache.org> Signed-off-by: Gengliang Wang <gengliang@apache.org>

SparkQA · 2021-06-09T03:25:36Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44045/

SparkQA · 2021-06-09T04:00:53Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44045/

SparkQA · 2021-06-09T05:34:10Z

Test build #139538 has finished for PR 32814 at commit a2326e0.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2021-06-09T06:29:36Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44062/

SparkQA · 2021-06-09T06:57:24Z

Test build #139520 has finished for PR 32814 at commit 35b78f1.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

gengliangwang · 2021-06-09T06:58:48Z

@cloud-fan @MaxGekk Thanks for the review.
Merging to master.

SparkQA · 2021-06-09T07:04:47Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44062/

github-actionsBot added the SQL label Jun 8, 2021

gengliangwang changed the title ~~[SPARK-35664][SQL] Support java.time. LocalDateTime as an external type of TimestampWithoutTZ type~~[SPARK-35664][SQL] Support java.time.LocalDateTime as an external type of TimestampWithoutTZ typeJun 8, 2021

gengliangwang requested review from MaxGekk and cloud-fan June 8, 2021 05:44

use LocalDateTime as external type for TimestampWithoutTZType
0533fb8

cloud-fan reviewed Jun 8, 2021
View reviewed changes

Comment threadsql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/DateTimeUtilsSuite.scala Outdated

cloud-fan approved these changes Jun 8, 2021
View reviewed changes

address comment
c2ebb06

gengliangwang force-pushed the LocalDateTime branch from 1101f55 to c2ebb06Compare June 8, 2021 09:23

revise test case
ba44cb1

MaxGekk reviewed Jun 8, 2021
View reviewed changes

gengliangwangand others added 2 commits June 8, 2021 21:48

Update sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util…
264c693
…/DateTimeUtils.scala Co-authored-by: Maxim Gekk <max.gekk@gmail.com>

exact
0418133

gengliangwang mentioned this pull request Jun 8, 2021
[SPARK-30993][FOLLOWUP][SQL] Refactor LocalDateTimeUDT as YearUDT in UserDefinedTypeSuite #32824
Closed

retrigger test
35b78f1

MaxGekk reviewed Jun 9, 2021
View reviewed changes

Comment threadsql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/DateTimeUtilsSuite.scala Outdated

MaxGekk reviewed Jun 9, 2021
View reviewed changes

Comment threadsql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/DateTimeUtilsSuite.scala Outdated

update jira id
a2326e0

MaxGekk approved these changes Jun 9, 2021
View reviewed changes

gengliangwang closed this in 84c5ca3Jun 9, 2021

gengliangwang mentioned this pull request Jun 9, 2021
[SPARK-35679][SQL] instantToMicros overflow #32839
Closed

		localDateTime.toEpochSecond(ZoneOffset.UTC) * MICROS_PER_SECOND +
		localDateTime.getNano / NANOS_PER_MICROS

Uh oh!

Conversation

gengliangwang commented Jun 8, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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?

Uh oh!

SparkQA commented Jun 8, 2021

Uh oh!

SparkQA commented Jun 8, 2021

Uh oh!

Uh oh!

SparkQA commented Jun 8, 2021

Uh oh!

SparkQA commented Jun 8, 2021

Uh oh!

SparkQA commented Jun 8, 2021

Uh oh!

SparkQA commented Jun 8, 2021

Uh oh!

SparkQA commented Jun 8, 2021

Uh oh!

MaxGekk commented Jun 8, 2021

Uh oh!

gengliangwang commented Jun 8, 2021

Uh oh!

Uh oh!

MaxGekkJun 8, 2021

Choose a reason for hiding this comment

Uh oh!

MaxGekkJun 8, 2021

Choose a reason for hiding this comment

Uh oh!

MaxGekkJun 8, 2021

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jun 8, 2021

Uh oh!

MaxGekk commented Jun 8, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gengliangwang commented Jun 8, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Jun 8, 2021

Uh oh!

SparkQA commented Jun 8, 2021

Uh oh!

MaxGekk commented Jun 8, 2021

Uh oh!

gengliangwang commented Jun 8, 2021

Uh oh!

SparkQA commented Jun 8, 2021

Uh oh!

SparkQA commented Jun 8, 2021

Uh oh!

SparkQA commented Jun 9, 2021

Uh oh!

SparkQA commented Jun 9, 2021

Uh oh!

Uh oh!

Uh oh!

SparkQA commented Jun 9, 2021

Uh oh!

SparkQA commented Jun 9, 2021

Uh oh!

SparkQA commented Jun 9, 2021

Uh oh!

gengliangwang commented Jun 9, 2021

Uh oh!

SparkQA commented Jun 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

gengliangwang commented Jun 8, 2021•
edited
Loading

MaxGekk commented Jun 8, 2021•
edited
Loading

gengliangwang commented Jun 8, 2021•
edited
Loading