Skip to content

Conversation

@MaxGekk
Copy link
Member

@MaxGekk MaxGekk commented Apr 6, 2021

What changes were proposed in this pull request?

  1. Added new method toDayTimeIntervalString() to IntervalUtils which converts a day-time interval as a number of microseconds to a string in the form "INTERVAL '[sign]days hours:minutes:secondsWithFraction' DAY TO SECOND".
  2. Extended the Cast expression to support casting of DayTimeIntervalType to StringType.

Why are the changes needed?

To conform the ANSI SQL standard which requires to support such casting.

Does this PR introduce any user-facing change?

Should not because new day-time interval has not been released yet.

How was this patch tested?

Added new tests for casting:

$ build/sbt "testOnly *CastSuite*"

@SparkQA
Copy link

SparkQA commented Apr 6, 2021

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

@SparkQA
Copy link

SparkQA commented Apr 6, 2021

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

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

SparkQA commented Apr 6, 2021

Test build #136965 has finished for PR 32070 at commit b8bfa9f.

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

Duration.ofMillis(1234) -> "0 0:0:1.234",
Duration.ofSeconds(-59).minus(999999, ChronoUnit.MICROS) -> "-0 0:0:59.999999",
Duration.ofMinutes(30).plusMillis(10) -> "0 0:30:0.01",
Duration.ofHours(-23).minusSeconds(59) -> "-0 23:0:59",
Copy link
Contributor

Choose a reason for hiding this comment

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

For times, I think it's more common to always have 2 digits. Have we checked with other databases?

Copy link
Member Author

Choose a reason for hiding this comment

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

Have we checked with other databases?

For example, Oracle doesn't prepend zero for hours, see the doc:

INTERVAL '4 5:12:10.222' DAY TO SECOND

Copy link
Contributor

Choose a reason for hiding this comment

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

that's the literal syntax, which is supposed to be more flexible. How about the cast behavior?

Copy link
Member Author

Choose a reason for hiding this comment

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

added leading zeros for time parts

@MaxGekk
Copy link
Member Author

MaxGekk commented Apr 7, 2021

@cloud-fan @gengliangwang @yaooqinn @AngersZhuuuu Could you review this PR, please.

val hours = rest % HOURS_PER_DAY
val days = rest / HOURS_PER_DAY
s"INTERVAL '$sign$days $hours:$minutes:$secondStr' DAY TO SECOND"
val leadSecZero = if (seconds < 10000000) "0" else ""
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: use MICROS_PER_SECONDS?

Copy link
Member

@yaooqinn yaooqinn left a comment

Choose a reason for hiding this comment

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

LGTM as always

var sign = ""
var rest = micros
if (micros < 0) {
if (micros == Long.MinValue) {
Copy link
Member

Choose a reason for hiding this comment

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

nice catch!

Copy link
Member

@gengliangwang gengliangwang left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@AngersZhuuuu AngersZhuuuu left a comment

Choose a reason for hiding this comment

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

LGTM

@SparkQA
Copy link

SparkQA commented Apr 7, 2021

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

@SparkQA
Copy link

SparkQA commented Apr 7, 2021

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

@SparkQA
Copy link

SparkQA commented Apr 7, 2021

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

@SparkQA
Copy link

SparkQA commented Apr 7, 2021

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

@SparkQA
Copy link

SparkQA commented Apr 7, 2021

Test build #137012 has finished for PR 32070 at commit 688c71b.

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

@SparkQA
Copy link

SparkQA commented Apr 7, 2021

Test build #137020 has finished for PR 32070 at commit 0bb44e3.

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

@AmplabJenkins
Copy link

Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/137020/

@cloud-fan
Copy link
Contributor

thanks, merging to master!

@cloud-fan cloud-fan closed this in 3dfd456 Apr 7, 2021
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.

7 participants