[SPARK-29371][SQL] Support interval field values with fractional parts by yaooqinn · Pull Request #26314 · apache/spark

yaooqinn · 2019-10-30T08:14:42Z

What changes were proposed in this pull request?

Support fraction representation fro interval field values.

the fraction part of the year only affects the month. WE OBEY THAT.
pg not support millisecond or microsecond. WE DONT FOLLOW.

postgres=# select interval '1.41 years 2.51 months 2.21 weeks 15.24 days 3.31 hours 5.38 minutes 12.3456789 seconds';
interval
---------------------------------------1 year 6 mons 45 days 27:38:35.145679
(1 row)postgres=# select interval '1.42 years 2.51 months 2.21 weeks 15.24 days 3.31 hours 5.38 minutes 12.3456789 seconds';
interval
---------------------------------------1 year 7 mons 45 days 27:38:35.145679
(1 row)postgres=# select interval '1.42 years 2.51 months 2.21 weeks 15.24 days 3.31 hours 5.38 minutes 12.3456789 seconds 2.1 milliseconds ';
ERROR: invalid input syntax for type interval: "1.42 years 2.51 months 2.21 weeks 15.24 days 3.31 hours 5.38 minutes 12.3456789 seconds 2.1 milliseconds "LINE1: select interval '1.42 years 2.51 months 2.21 weeks 15.24 day...

Why are the changes needed?

In PostgreSQL, interval field values can have fractional parts. See https://www.postgresql.org/docs/current/datatype-datetime.html#DATATYPE-INTERVAL-INPUT

Does this PR introduce any user-facing change?

yes, add fraction input support for interval values.

How was this patch tested?

add uts

yaooqinn · 2019-10-30T09:05:42Z

cc @cloud-fan @MaxGekk

cloud-fan · 2019-10-30T09:14:05Z

 DateTimeUtils.MILLIS_PER_MINUTE * DateTimeUtils.MICROS_PER_MILLIS
 final val DAYS_PER_MONTH: Byte = 30
- final val MICROS_PER_MONTH: Long = DAYS_PER_MONTH * DateTimeUtils.SECONDS_PER_DAY
+ final val MICROS_PER_MONTH: Long = DAYS_PER_MONTH * DateTimeUtils.MICROS_PER_DAY


ah good catch! do we have a test case?

MaxGekk

In general, LGTM

MaxGekk · 2019-10-30T09:08:30Z

 DateTimeUtils.MILLIS_PER_MINUTE * DateTimeUtils.MICROS_PER_MILLIS
 final val DAYS_PER_MONTH: Byte = 30
- final val MICROS_PER_MONTH: Long = DAYS_PER_MONTH * DateTimeUtils.SECONDS_PER_DAY
+ final val MICROS_PER_MONTH: Long = DAYS_PER_MONTH * DateTimeUtils.MICROS_PER_DAY


dongjoon-hyun · 2019-10-30T09:19:19Z

This PR had better have the one-line fix on final val MICROS_PER_MONTH: Long = DAYS_PER_MONTH * DateTimeUtils.MICROS_PER_DAY and test cases.

SparkQA · 2019-10-30T09:26:45Z

Test build #112906 has finished for PR 26314 at commit 6231fb4.

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

SparkQA · 2019-10-30T09:51:29Z

Test build #112910 has finished for PR 26314 at commit bba0ad3.

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

SparkQA · 2019-10-30T10:10:19Z

Test build #112903 has finished for PR 26314 at commit 612bfca.

This patch fails Spark unit tests.
This patch does not merge cleanly.
This patch adds no public classes.

yaooqinn · 2019-10-30T11:14:12Z

With this pr, outside nanosecond will be omitted in silence as pg

postgres=# select interval '.1111111111' second
postgres-# ;
interval
-----------------00:00:00.111111
(1 row)

SparkQA · 2019-10-30T11:55:10Z

Test build #112921 has finished for PR 26314 at commit 25b78bb.

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

SparkQA · 2019-10-31T06:11:33Z

Test build #112982 has finished for PR 26314 at commit 652d01c.

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

yaooqinn · 2019-10-31T06:17:38Z

@MaxGekk @cloud-fan @dongjoon-hyun rebased with master, please take a look again.thanks.

MaxGekk

My only concern is the big decimal ops are relatively expensive. If we will bulky load interval strings or cast from strings, throughput could be low. @yaooqinn Could you run IntervalBenchmark to estimate performance.

yaooqinn · 2019-10-31T07:24:29Z

@MaxGekk Yes, thanks, will do a benchmark comparison right now.

yaooqinn · 2019-10-31T08:04:31Z

Before

[info] JavaHotSpot(TM) 64-BitServerVM1.8.0_65-b17 on MacOSX10.14.6
[info] Intel(R) Core(TM) i5-5287U CPU@2.90GHz
[info] cast strings to intervals: BestTime(ms) AvgTime(ms) Stdev(ms) Rate(M/s) PerRow(ns) Relative
[info] ------------------------------------------------------------------------------------------------------------------------
[info] prepare string w/ interval 493566772.0493.01.0X
[info] prepare string w/o interval 410437322.4410.01.2X
[info] 1 units w/ interval 523753401220.25236.60.1X
[info] 1 units w/o interval 506351361230.25062.60.1X
[info] 2 units w/ interval 69646993280.16964.00.1X
[info] 2 units w/o interval 66386665380.26637.60.1X
[info] 3 units w/ interval 83448444880.18343.90.1X
[info] 3 units w/o interval 787780341660.17876.80.1X
[info] 4 units w/ interval 930694201000.19305.70.1X
[info] 4 units w/o interval 836386352390.18363.00.1X
[info] 5 units w/ interval 10037103403010.110036.60.0X
[info] 5 units w/o interval 10071101361070.110071.50.0X
[info] 6 units w/ interval 10818115246120.110818.50.0X
[info] 6 units w/o interval 10726112894900.110725.80.0X
[info] 7 units w/ interval 11498115731040.111498.10.0X
[info] 7 units w/o interval 1129211311280.111292.10.0X
[info] 8 units w/ interval 12792129281890.112792.40.0X
[info] 8 units w/o interval 1238712399120.112386.60.0X
[info] 9 units w/ interval 14307145413940.114307.50.0X
[info] 9 units w/o interval 1398614015490.113985.50.0X

AFTER

[info] JavaHotSpot(TM) 64-BitServerVM1.8.0_65-b17 on MacOSX10.14.6
[info] Intel(R) Core(TM) i5-5287U CPU@2.90GHz
[info] cast strings to intervals: BestTime(ms) AvgTime(ms) Stdev(ms) Rate(M/s) PerRow(ns) Relative
[info] ------------------------------------------------------------------------------------------------------------------------
[info] prepare string w/ interval 6437261081.6643.01.0X
[info] prepare string w/o interval 468501352.1468.41.4X
[info] 1 units w/ interval 50458939NaN0.25044.80.1X
[info] 1 units w/o interval 559158193790.25590.60.1X
[info] 2 units w/ interval 646269387650.26461.90.1X
[info] 2 units w/o interval 602261491470.26021.70.1X
[info] 3 units w/ interval 72237319900.17223.20.1X
[info] 3 units w/o interval 697270811520.16972.30.1X
[info] 4 units w/ interval 818083332620.18179.80.1X
[info] 4 units w/o interval 78177877940.17817.50.1X
[info] 5 units w/ interval 91719201290.19170.60.1X
[info] 5 units w/o interval 911995676840.19118.70.1X
[info] 6 units w/ interval 10255105384880.110255.20.1X
[info] 6 units w/o interval 10122106045430.110121.90.1X
[info] 7 units w/ interval 12154123342220.112153.50.1X
[info] 7 units w/o interval 1135211359100.111352.10.1X
[info] 8 units w/ interval 127741277860.112774.20.1X
[info] 8 units w/o interval 12627128914080.112627.20.1X
[info] 9 units w/ interval 14087142091840.114087.50.0X
[info] 9 units w/o interval 13856140433100.113855.90.0X

yaooqinn · 2019-10-31T08:09:59Z

@MaxGekk the performances look the same

MaxGekk · 2019-10-31T09:52:08Z

I modified slightly the benchmark and append fractions:

diff --git a/sql/core/src/test/scala/org/apache/spark/sql/execution/benchmark/IntervalBenchmark.scala b/sql/core/src/test/scala/org/apache/spark/sql/execution/benchmark/IntervalBenchmark.scala
index d75cb1040f..ec32340b2b 100644
--- a/sql/core/src/test/scala/org/apache/spark/sql/execution/benchmark/IntervalBenchmark.scala+++ b/sql/core/src/test/scala/org/apache/spark/sql/execution/benchmark/IntervalBenchmark.scala@@ -83,8 +83,8 @@ object IntervalBenchmark extends SqlBasedBenchmark {
override def runBenchmarkSuite(mainArgs: Array[String]): Unit = {
val N = 1000000
val timeUnits = Seq(
- "13 months", "100 weeks", "9 days", "12 hours",- "5 minutes", "45 seconds", "123 milliseconds", "567 microseconds")+ "13.123 months", "100.123 weeks", "9.123 days", "12.123 hours",+ "5.123 minutes", "45.123 seconds", "123.456 milliseconds", "567 microseconds")
val intervalToTest = ListBuffer[String]()

and the results look not so nice:

Java HotSpot(TM) 64-Bit Server VM 1.8.0_231-b11 on Mac OS X 10.15
Intel(R) Core(TM) i7-4850HQ CPU @ 2.30GHz
cast strings to intervals: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------------------------------
prepare string w/ interval 440 497 51 2.3 440.3 1.0X
prepare string w/o interval 370 392 21 2.7 370.3 1.2X
1 units w/ interval 5322 5414 142 0.2 5322.4 0.1X
1 units w/o interval 5002 5038 35 0.2 5002.2 0.1X
2 units w/ interval 18172 18285 189 0.1 18172.0 0.0X
2 units w/o interval 17899 18278 511 0.1 17899.4 0.0X
3 units w/ interval 30881 31481 665 0.0 30880.5 0.0X
3 units w/o interval 30839 31600 664 0.0 30838.5 0.0X
4 units w/ interval 44694 45302 547 0.0 44693.9 0.0X
4 units w/o interval 42490 42599 99 0.0 42489.5 0.0X
5 units w/ interval 54424 54469 67 0.0 54424.5 0.0X
5 units w/o interval 54879 55015 161 0.0 54879.1 0.0X
6 units w/ interval 69155 69281 140 0.0 69154.8 0.0X
6 units w/o interval 68877 69018 156 0.0 68876.7 0.0X
7 units w/ interval 82116 82204 112 0.0 82116.4 0.0X
7 units w/o interval 81495 81641 144 0.0 81495.4 0.0X
8 units w/ interval 94609 94630 22 0.0 94608.7 0.0X
8 units w/o interval 93915 94099 180 0.0 93914.8 0.0X
9 units w/ interval 93132 95566 2126 0.0 93132.2 0.0X
9 units w/o interval 91703 94179 NaN 0.0 91703.1 0.0X

The throughput is roughly ~10 interval strings with 8-9 units per second.

yaooqinn · 2019-10-31T10:21:11Z

the worst-case gives the worst result. also, my benchmark test shows no performance regression to existing cases. Personally, I am ok with the result.

SparkQA · 2019-10-31T12:16:16Z

Test build #113007 has finished for PR 26314 at commit b071e09.

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

yaooqinn · 2019-10-31T14:37:15Z

retest this please

SparkQA · 2019-10-31T17:45:00Z

Test build #113032 has finished for PR 26314 at commit b071e09.

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

yaooqinn · 2019-11-01T03:56:09Z

The throughput is roughly ~10 interval strings with 8-9 units per second.

@MaxGekk is this per ms not per second?

yaooqinn · 2019-11-01T03:59:57Z

retest this please

SparkQA · 2019-11-01T07:51:32Z

Test build #113068 has finished for PR 26314 at commit b071e09.

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

dongjoon-hyun added the SQL label Oct 30, 2019

yaooqinn mentioned this pull request Oct 30, 2019
[SPARK-29532][SQL] Simplify interval string parsing #26190
Closed

yaooqinn mentioned this pull request Oct 30, 2019
[SPARK-29651][SQL] Fix parsing of interval seconds fraction #26313
Closed

cloud-fan reviewed Oct 30, 2019
View reviewed changes

MaxGekk reviewed Oct 30, 2019
View reviewed changes

cloud-fan reviewed Oct 30, 2019
View reviewed changes

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

MaxGekk reviewed Oct 31, 2019
View reviewed changes

yaooqinn added 3 commits October 31, 2019 16:13

[SPARK-29371][SQL] Support interval field values with fractional parts
ef460aa

add negetive interval tests
0196eaa

fix DDLParserSuite and activate interval.sql
b071e09

yaooqinn mentioned this pull request Nov 19, 2019
[SPARK-29371][SQL] Fractional representation for interval string #26592
Closed

yaooqinn closed this Dec 11, 2019

Uh oh!

Conversation

yaooqinn commented Oct 30, 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!

yaooqinn commented Oct 30, 2019

Uh oh!

cloud-fanOct 30, 2019

Choose a reason for hiding this comment

Uh oh!

MaxGekk left a comment

Choose a reason for hiding this comment

Uh oh!

MaxGekkOct 30, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dongjoon-hyun commented Oct 30, 2019

Uh oh!

SparkQA commented Oct 30, 2019

Uh oh!

SparkQA commented Oct 30, 2019

Uh oh!

SparkQA commented Oct 30, 2019

Uh oh!

yaooqinn commented Oct 30, 2019

Uh oh!

SparkQA commented Oct 30, 2019

Uh oh!

SparkQA commented Oct 31, 2019

Uh oh!

yaooqinn commented Oct 31, 2019

Uh oh!

MaxGekk left a comment

Choose a reason for hiding this comment

Uh oh!

yaooqinn commented Oct 31, 2019

Uh oh!

yaooqinn commented Oct 31, 2019

Before

AFTER

Uh oh!

yaooqinn commented Oct 31, 2019

Uh oh!

MaxGekk commented Oct 31, 2019

Uh oh!

yaooqinn commented Oct 31, 2019

Uh oh!

SparkQA commented Oct 31, 2019

Uh oh!

yaooqinn commented Oct 31, 2019

Uh oh!

SparkQA commented Oct 31, 2019

Uh oh!

yaooqinn commented Nov 1, 2019

Uh oh!

yaooqinn commented Nov 1, 2019

Uh oh!

SparkQA commented Nov 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants