Skip to content

Conversation

@cloud-fan
Copy link
Contributor

What changes were proposed in this pull request?

CatalystTypeConverters is useful when the type of the input data classes are not known statically (otherwise we can use ExpressionEncoder). However, the current CatalystTypeConverters requires you to know the datetime data class statically, which makes it hard to use.

This PR improves the CatalystTypeConverters for date/timestamp, to support the old and new Java time classes at the same time.

Why are the changes needed?

Make CatalystTypeConverters easier to use.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

new test

@cloud-fan
Copy link
Contributor Author

@brkyvz @MaxGekk

@github-actions github-actions bot added the SQL label Apr 23, 2021
@SparkQA
Copy link

SparkQA commented Apr 23, 2021

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

@SparkQA
Copy link

SparkQA commented Apr 23, 2021

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

case l: LocalDate => DateTimeUtils.localDateToDays(l)
case other => throw new IllegalArgumentException(
s"The value (${other.toString}) of the type (${other.getClass.getCanonicalName}) "
+ s"cannot be converted to the date type")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s is not needed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to the date type

How about to call a method of DateType to render type name.

case i: Instant => DateTimeUtils.instantToMicros(i)
case other => throw new IllegalArgumentException(
s"The value (${other.toString}) of the type (${other.getClass.getCanonicalName}) "
+ s"cannot be converted to the timestamp type")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same - s is not needed

val i = Instant.ofEpochSecond(seconds)
val result1 = converter(i)

val t = new java.sql.Timestamp(seconds * 1000)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use MILLIS_PER_SECOND

@SparkQA
Copy link

SparkQA commented Apr 23, 2021

Test build #137857 has finished for PR 32312 at commit 4d6e4b6.

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

@SparkQA
Copy link

SparkQA commented Apr 23, 2021

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

@SparkQA
Copy link

SparkQA commented Apr 23, 2021

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

@MaxGekk
Copy link
Member

MaxGekk commented Apr 23, 2021

+1, LGTM. GA passed. Merging to master.
Screenshot 2021-04-23 at 19 19 14
Thank you, @cloud-fan .

@MaxGekk MaxGekk closed this in a9345a0 Apr 23, 2021
@SparkQA
Copy link

SparkQA commented Apr 23, 2021

Test build #137865 has finished for PR 32312 at commit c8dbc1a.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants