[SPARK-29607][SQL] Move static methods from CalendarInterval to IntervalUtils #26261

MaxGekk · 2019-10-25T20:08:49Z

What changes were proposed in this pull request?

In the PR, I propose to move all static methods from the CalendarInterval class to the IntervalUtils object. All those methods are rewritten from Java to Scala.

Why are the changes needed?

For consistency with other helper methods. Such methods were placed to the helper object IntervalUtils, see [SPARK-29532][SQL] Simplify interval string parsing #26190
Taking into account that CalendarInterval will be fully exposed to users in the future (see [SPARK-24695][SQL] Move CalendarInterval to org.apache.spark.sql.types package #25022), it would be nice to clean it up by moving service methods to an internal object.

Does this PR introduce any user-facing change?

No

How was this patch tested?

By moved tests from CalendarIntervalSuite to IntervalUtilsSuite
By existing test suites

MaxGekk · 2019-10-25T20:09:21Z

@cloud-fan @HyukjinKwon @dongjoon-hyun Please, take a look at the PR.

SparkQA · 2019-10-25T21:53:11Z

Test build #112688 has finished for PR 26261 at commit 21c30c6.

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

MaxGekk · 2019-10-25T22:00:31Z

jenkins, retest this, please

SparkQA · 2019-10-26T00:32:22Z

Test build #112692 has finished for PR 26261 at commit 21c30c6.

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

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/IntervalUtils.scala

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/IntervalUtilsSuite.scala

dongjoon-hyun · 2019-10-26T00:38:51Z

@MaxGekk . I left a few minor comments. In general, the logic is correctly migrated from Java to Scala.

And, I agree with @MaxGekk 's opinion. How do you think about this, @cloud-fan ?

SparkQA · 2019-10-26T12:41:50Z

Test build #112713 has finished for PR 26261 at commit a417cbe.

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

MaxGekk · 2019-10-26T13:06:20Z

jenkins, retest this, please

SparkQA · 2019-10-26T15:44:35Z

Test build #112717 has finished for PR 26261 at commit a417cbe.

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

srowen · 2019-10-26T17:36:34Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/IntervalUtils.scala

    }
  }
+
+  private val yearMonthPattern = Pattern.compile("^([+|-])?(\\d+)-(\\d+)$")


This can just use Scala regex primitives instead.

This will require rewriting of dependent code but I wanted to just move methods and keep them closer to original methods as much as possible. Maybe we will refactor the code in a follow up PR?

You're already moving the code and converting to scala in other ways, so I think it's not a big deal to do it here rather than make another one for pretty minor adjustments. Or is there an argument that git won't really track the history of this code after move if it's changed a lot?

To use Scala regexp, I will have to restructure the code significantly, so, it will be impossible to track anything.

Why does it need any restructuring? "...".r gives you an object with pretty much the same API. You don't have to use regex pattern matching in Scala if it's disruptive. Well, I don't feel strongly about it, just doesn't seem any more of a change than you've already made in adapting this code to Scala.

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/IntervalUtils.scala

srowen · 2019-10-26T17:39:06Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/IntervalUtils.scala

+   */
+  private def parseSecondNano(secondNano: String): Long = {
+    val parts = secondNano.split("\\.")
+    if (parts.length == 1) {


use a match statement maybe?

I would do this refactoring in a separate PR too if you don't mind.

SparkQA · 2019-10-26T21:50:26Z

Test build #112720 has finished for PR 26261 at commit e443921.

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

SparkQA · 2019-10-27T01:30:06Z

Test build #112721 has finished for PR 26261 at commit a6fbc4d.

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

dongjoon-hyun · 2019-10-27T05:33:39Z

The following failure happens in master branch, too. It seems that irrelevant to this PR.

org.scalatest.exceptions.TestFailedException: 1572217200000000 was not 1572213600000000 plus or minus 30000000

https://amplab.cs.berkeley.edu/jenkins/view/Spark%20QA%20Test%20(Dashboard)/job/spark-master-test-sbt-hadoop-3.2/653/

MaxGekk · 2019-10-27T08:31:25Z

jenkins, retest this, please

cloud-fan · 2019-10-28T16:45:27Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/IntervalUtils.scala

+    result
+  }
+
+  private val yearMonthPattern = "^\\s*([+|-])?(\\d+)-(\\d+)\\s*$".r


The previous regex does not have the \\s* at the beginning and end. This is because we get the string from parser which already trims the white spaces. Shall we change it back?

Previously, we trim the input explicitly: b482367 . I just trim by the regexp.

I will revert the regexp and add the assert: assert(input.length == input.trim.length)

I will do the same in fromDayTimeString()

cloud-fan · 2019-10-28T16:50:46Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/IntervalUtils.scala

+            s"Cannot support (interval '$s' $from to $to) expression")
+      }
+      new CalendarInterval(0, sign * (
+        days * DateTimeUtils.MICROS_PER_DAY + hours * MICROS_PER_HOUR +


inspired by https://github.com/apache/spark/pull/26261/files#diff-eba257f41b49f470321579875f054f00R152 , we should use exact math functions here too. This can be done in a followup.

cloud-fan

LGTM except a minor comment

dongjoon-hyun · 2019-10-28T19:09:30Z

Retest this please.

SparkQA · 2019-10-28T19:12:20Z

Test build #112791 has finished for PR 26261 at commit 38f7c78.

This patch fails build dependency tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-10-28T21:04:34Z

Test build #112792 has finished for PR 26261 at commit 38f7c78.

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

cloud-fan · 2019-10-29T04:06:27Z

retest this please

SparkQA · 2019-10-29T05:47:57Z

Test build #112817 has finished for PR 26261 at commit 38f7c78.

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

cloud-fan · 2019-10-29T06:06:14Z

retest this please

SparkQA · 2019-10-29T07:05:02Z

Test build #112826 has finished for PR 26261 at commit 38f7c78.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon

Looks fine to me too

HyukjinKwon · 2019-10-29T07:06:25Z

retest this please

cloud-fan · 2019-10-29T07:06:34Z

retest this please

SparkQA · 2019-10-29T09:18:14Z

Test build #112832 has finished for PR 26261 at commit 38f7c78.

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

cloud-fan · 2019-10-29T17:15:30Z

thanks, merging to master!

dongjoon-hyun · 2019-10-29T17:23:03Z

Thank you all!

Move static methods from CalendarInterval to IntervalUtils

21c30c6

dongjoon-hyun added the SQL label Oct 25, 2019

dongjoon-hyun reviewed Oct 26, 2019

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/IntervalUtils.scala Outdated Show resolved Hide resolved

dongjoon-hyun reviewed Oct 26, 2019

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/IntervalUtils.scala Show resolved Hide resolved

dongjoon-hyun reviewed Oct 26, 2019

View reviewed changes

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/IntervalUtilsSuite.scala Outdated Show resolved Hide resolved

dongjoon-hyun reviewed Oct 26, 2019

View reviewed changes

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/IntervalUtilsSuite.scala Outdated Show resolved Hide resolved

MaxGekk added 6 commits October 26, 2019 13:26

Remove empty line

ebd5a06

Fix coding style in IntervalUtilsSuite

55af403

Make changes in fromDayTimeString() closer to originals

b61b07b

Make toLongWithRange() private

25379fa

Fix coding style

67ea49e

Make parseSecondNano() private

a417cbe

srowen reviewed Oct 26, 2019

View reviewed changes

MaxGekk added 4 commits October 26, 2019 22:18

Refactor toLongWithRange()

e1c7be5

use string interpolation

b7e962e

Change require message

0a306b1

Use require()

e443921

Fix test failures

a6fbc4d

MaxGekk added 6 commits October 28, 2019 18:25

Use Regex in fromYearMonthString()

68e8cbe

Add a test to "from year-month string"

8d55b9f

Trim spaces by regexp

b482367

Use Regex in parseSecondNano()

58ebcaa

Replace Pattern.compile by Scala Regex

1d91e6a

Avoid magic constant

484ecf8

cloud-fan reviewed Oct 28, 2019

View reviewed changes

cloud-fan approved these changes Oct 28, 2019

View reviewed changes

MaxGekk added 3 commits October 28, 2019 21:48

Assume trimmed input in fromYearMonthString()

56923dc

Assume trimmed input in fromDayTimeString()

b2a8cbe

Use exact math functions in fromDayTimeString()

38f7c78

HyukjinKwon approved these changes Oct 29, 2019

View reviewed changes

cloud-fan closed this in 44c1c03 Oct 29, 2019

MaxGekk deleted the refactoring-calendar-interval branch June 5, 2020 19:40

[SPARK-29607][SQL] Move static methods from CalendarInterval to IntervalUtils #26261

[SPARK-29607][SQL] Move static methods from CalendarInterval to IntervalUtils #26261

Uh oh!

Conversation

MaxGekk commented Oct 25, 2019

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!

MaxGekk commented Oct 25, 2019

Uh oh!

SparkQA commented Oct 25, 2019

Uh oh!

MaxGekk commented Oct 25, 2019

Uh oh!

SparkQA commented Oct 26, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dongjoon-hyun commented Oct 26, 2019

Uh oh!

SparkQA commented Oct 26, 2019

Uh oh!

MaxGekk commented Oct 26, 2019

Uh oh!

SparkQA commented Oct 26, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Oct 26, 2019

Uh oh!

SparkQA commented Oct 27, 2019

Uh oh!

dongjoon-hyun commented Oct 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MaxGekk commented Oct 27, 2019

Uh oh!

cloud-fan Oct 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan left a comment

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Oct 28, 2019

Uh oh!

SparkQA commented Oct 28, 2019

Uh oh!

SparkQA commented Oct 28, 2019

Uh oh!

cloud-fan commented Oct 29, 2019

Uh oh!

SparkQA commented Oct 29, 2019

dongjoon-hyun commented Oct 27, 2019 •

edited

Loading

cloud-fan Oct 28, 2019 •

edited

Loading