[SPARK-24457][SQL] Improving performance of stringToTimestamp #21505

ssonker · 2018-06-07T08:56:06Z

What changes were proposed in this pull request?

As of now, stringToTimestamp function in DateTimeUtils creates a calendar instance on each call. This change maintains a thread-local timezone to calendar map, and creates just one calendar for each timezone. Whenever a calendar instance is queried given a timezone, it is looked-up inside the map, reinitialized and returned.

How was this patch tested?

Using existing test cases.

Please review http://spark.apache.org/contributing.html before opening a pull request.

viirya · 2018-06-07T09:01:02Z

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

When you get the calendar instance next time, isn't its time out of date?

Please refer line 130 for this. Before returning the calendar instance, it is reinitialized to the time it was originally created.

@viirya ^ Does that answer you question, or you mean something else?

Isn't timeInMillis also stored when you first update this map entity? So next time you access this calendar, you just set it with the old timeInMillis. Isn't it?

Yes, correct.

kiszk · 2018-06-07T09:14:25Z

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

I agree with @viirya 's comment. Do we need to set the value of System.currentTimeMillis()?

@kiszk Thanks, I'm updating that. BTW, can you please help me understand a scenario where that is absolutely needed.

@kiszk @viirya I've updated the code. Please review.

viirya · 2018-06-07T09:44:19Z

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

hmm, I think System.currentTimeMillis() is UTC-based?

oh, Calendar.getTimeInMillis and setTimeInMillis are also UTC-based.

viirya · 2018-06-07T09:48:55Z

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

Doesn't clear reset the timezone of that Calendar instance?

Nope. It clears all the fields and zone is not a field.

kiszk · 2018-06-07T09:49:29Z

We would appreciate it if you put the performance before and after this PR?
It would be good to use Benchmark class.

viirya · 2018-06-07T10:19:26Z

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

Do we need to keep Calendar for many timezone? Since getCalendar takes a time zone input, we can just keep one Calendar instance, and set it with given timezone in getCalendar. WDYT? Regarding performance, is there big difference?

Usually, only the default time zone is used. To execute Cast regarding date is called with a timezone may use another timezone. For the correctness, I think that it is necessary to support multiple timezones.

To enable caching for default time zone and to create an instance for other time zones would also work correctly.

@kiszk I think @viirya meant having just one thread-local calendar instance. That should also work, isn't it?

Yes, it should work functionally if we check a given time zone every time.
Do you know the typical access pattern of time zone? If there is temporal locality regarding time zone, we do not have to use mutale.Map.

@kiszk @viirya I've tried running benchmarks for with/without mutable.Map implementation. Looks like setting timezone in a calendar instance is a costly operation and it drags the performance down. As the number of timezones cannot be large, maintaining a map will not be a huge memory overhead. So, I suggest going with the mutable.Map approach. Comments?

How much performance down without a map here?

I ran a benchmark and following is the output:

string to timestamp calendar caching: Best/Avg Time(ms) Rate(M/s) Per Row(ns) Relative ------------------------------------------------------------------------------------------------ with map 8 / 9 12.9 77.7 1.0X without map 11 / 12 9.4 106.3 0.7X

I'm fine with the map approach.

+1 for map approach

ssonker · 2018-06-11T05:18:53Z

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

@viirya @kiszk Do you agree with this commit?

Seems setTimeInMillis can result in all fields set. If so, clear is redundant.

ssonker · 2018-06-11T06:11:08Z

@kiszk I've added a benchmark according to your recommendation, please review.

…ing Calendar instances for input timezones instead of creating new everytime

ssonker · 2018-06-12T08:16:12Z

@kiszk @viirya Do you have more review comments that need to be incorporated? If not, can you please get this merged?

viirya · 2018-06-12T08:50:25Z

The PR title is too long and truncated. Can you shorten it?

viirya · 2018-06-12T08:51:45Z

cc @cloud-fan

ssonker · 2018-06-12T08:58:51Z

@viirya Done.
cc: @cloud-fan

kiszk · 2018-06-12T12:38:32Z

LGTM

cloud-fan · 2018-06-12T18:41:48Z

sql/catalyst/src/test/scala/org/apache/spark/sql/StringToTimestampBenchmark.scala

+import org.apache.spark.sql.catalyst.util.{DateTimeTestUtils, DateTimeUtils}
+import org.apache.spark.util.Benchmark
+
+object StringToTimestampBenchmark {


This benchmark tests Calendar creation, not string to timestamp. how about DateTimeUtilsBenchmark?

cloud-fan · 2018-06-12T18:42:47Z

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

+
+  def getCalendar(timeZone: TimeZone): Calendar = {
+    val c = threadLocalComputedCalendarsMap.get()
+      .getOrElseUpdate(timeZone, {


nit: getOrElseUpdate(timeZone, Calendar.getInstance(timeZone))

HyukjinKwon · 2018-07-13T09:04:58Z

ok to test

SparkQA · 2018-07-13T09:09:59Z

Test build #92969 has finished for PR 21505 at commit c940381.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

srowen

How big a performance impact are we talking?

srowen · 2018-07-15T17:24:38Z

sql/catalyst/src/test/scala/org/apache/spark/sql/StringToTimestampBenchmark.scala

@@ -0,0 +1,53 @@
+/*


Does this really need to go in the code base?

Yea, that's what I was thinking.

srowen

What about other calls to Calendar.getInstance in this module -- can, should, others use this utility? I'm OK with this, but have a few outstanding comments above.

srowen · 2018-07-31T22:09:40Z

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

+      }
+    }
+
+  def getCalendar(timeZone: TimeZone): Calendar = {


Can this be private?

HyukjinKwon · 2018-08-01T02:47:14Z

ping @ssonker to update or close.

kiszk · 2018-08-09T10:02:59Z

gentle ping @ssonker

ssonker · 2018-08-09T12:25:12Z

I need to update this PR with more changes, will open a new one! Closing this.

viirya reviewed Jun 7, 2018

View reviewed changes

kiszk reviewed Jun 7, 2018

View reviewed changes

viirya reviewed Jun 7, 2018

View reviewed changes

ssonker commented Jun 11, 2018

View reviewed changes

[SPARK-24457][SQL] Improving performance of stringToTimestamp by cach…

c940381

…ing Calendar instances for input timezones instead of creating new everytime

ssonker force-pushed the master branch from eb8a99e to c940381 Compare June 12, 2018 08:13

ssonker changed the title ~~[SPARK-24457][SQL] Improving performance of stringToTimestamp by cach…~~ [SPARK-24457][SQL] Improving performance of stringToTimestamp Jun 12, 2018

cloud-fan reviewed Jun 12, 2018

View reviewed changes

srowen reviewed Jul 15, 2018

View reviewed changes

srowen requested changes Jul 31, 2018

View reviewed changes

ssonker closed this Aug 9, 2018

[SPARK-24457][SQL] Improving performance of stringToTimestamp #21505

[SPARK-24457][SQL] Improving performance of stringToTimestamp #21505

Uh oh!

Conversation

ssonker commented Jun 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kiszk commented Jun 7, 2018

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!

Choose a reason for hiding this comment

Uh oh!

ssonker Jun 11, 2018 • 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!

ssonker commented Jun 11, 2018

Uh oh!

ssonker commented Jun 12, 2018

Uh oh!

viirya commented Jun 12, 2018

Uh oh!

viirya commented Jun 12, 2018

Uh oh!

ssonker commented Jun 12, 2018

Uh oh!

kiszk commented Jun 12, 2018

Uh oh!

cloud-fan Jun 12, 2018 • 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!

HyukjinKwon commented Jul 13, 2018

Uh oh!

SparkQA commented Jul 13, 2018

ssonker commented Jun 7, 2018 •

edited

Loading

ssonker Jun 11, 2018 •

edited

Loading

cloud-fan Jun 12, 2018 •

edited

Loading