[SPARK-29074][SQL] Optimize `date_format` for foldable `fmt` #25782

MaxGekk · 2019-09-13T09:48:03Z

What changes were proposed in this pull request?

In the PR, I propose to create an instance of TimestampFormatter only once at the initialization, and reuse it inside of nullSafeEval() and doGenCode() in the case when the fmt parameter is foldable.

Why are the changes needed?

The changes improve performance of the date_format() function.

Before:

format date:                             Best/Avg Time(ms)    Rate(M/s)   Per Row(ns)   Relative
------------------------------------------------------------------------------------------------
format date wholestage off                    7180 / 7181          1.4         718.0       1.0X
format date wholestage on                     7051 / 7194          1.4         705.1       1.0X

After:

format date:                             Best/Avg Time(ms)    Rate(M/s)   Per Row(ns)   Relative
------------------------------------------------------------------------------------------------
format date wholestage off                    4787 / 4839          2.1         478.7       1.0X
format date wholestage on                     4736 / 4802          2.1         473.6       1.0X

Does this PR introduce any user-facing change?

No.

How was this patch tested?

By existing test suites DateExpressionsSuite and DateFunctionsSuite.

MaxGekk · 2019-09-13T09:48:36Z

@dongjoon-hyun @maropu @srowen Please, take a look at the PR.

srowen

I don't quite know the logic of foldable enough to comment, but seems plausible.

srowen · 2019-09-13T13:09:29Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala

  override protected def nullSafeEval(timestamp: Any, format: Any): Any = {
-    val df = TimestampFormatter(format.toString, zoneId)
-    UTF8String.fromString(df.format(timestamp.asInstanceOf[Long]))
+    val tf = if (formatter.isEmpty) {


How about .getOrElse?

.getOrElse has some overhead of calling the lambda function. I explicitly avoided its usage in the interpreted mode. For consistency, I could do the same in the codegen function but I don't think it does matter.

SparkQA · 2019-09-13T13:27:40Z

Test build #110566 has finished for PR 25782 at commit 5ab324f.

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/expressions/datetimeExpressions.scala

SparkQA · 2019-09-13T20:00:05Z

Test build #110567 has finished for PR 25782 at commit ee2a7d2.

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/expressions/datetimeExpressions.scala

sql/core/benchmarks/DateTimeBenchmark-results.txt

dongjoon-hyun · 2019-09-13T22:20:04Z

Thank you for pinging me, @MaxGekk . In general, this PR looks fine. I left a few minor comments. I'll take a look later again.

SparkQA · 2019-09-14T07:05:02Z

Test build #110586 has finished for PR 25782 at commit 3b5a8c2.

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

maropu · 2019-09-14T12:18:21Z

retest this please

SparkQA · 2019-09-14T15:53:26Z

Test build #110590 has finished for PR 25782 at commit 3b5a8c2.

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/expressions/datetimeExpressions.scala

HyukjinKwon · 2019-09-16T05:14:46Z

LGTM if tests pass.

MaxGekk · 2019-09-16T06:03:08Z

jenkins, retest this, please

HyukjinKwon · 2019-09-16T07:12:44Z

Seems like Jenkins is down.

SparkQA · 2019-09-16T22:02:48Z

Test build #4870 has finished for PR 25782 at commit a9fd8e4.

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

maropu · 2019-09-16T23:55:13Z

retest this please

SparkQA · 2019-09-17T00:22:21Z

Test build #110639 has finished for PR 25782 at commit a9fd8e4.

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

SparkQA · 2019-09-17T04:29:36Z

Test build #110678 has finished for PR 25782 at commit a9fd8e4.

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

HyukjinKwon · 2019-09-17T06:59:49Z

Merged to master.

MaxGekk added 2 commits September 13, 2019 14:13

Optimize date_format for foldable fmt

de8dafb

Re-gen results of DateTimeBenchmark

5ab324f

srowen reviewed Sep 13, 2019

View reviewed changes

maropu reviewed Sep 13, 2019

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala Outdated Show resolved Hide resolved

MaxGekk added 2 commits September 13, 2019 21:05

Bug fix: copy formatter

85a290e

Re-gen results of DateTimeBenchmark

ee2a7d2

dongjoon-hyun added the SQL label Sep 13, 2019

dongjoon-hyun reviewed Sep 13, 2019

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala Outdated Show resolved Hide resolved

dongjoon-hyun reviewed Sep 13, 2019

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala Outdated Show resolved Hide resolved

dongjoon-hyun reviewed Sep 13, 2019

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala Show resolved Hide resolved

dongjoon-hyun reviewed Sep 13, 2019

View reviewed changes

sql/core/benchmarks/DateTimeBenchmark-results.txt Show resolved Hide resolved

MaxGekk added 2 commits September 14, 2019 09:52

Use default locale

3292acc

Skip DateTimeUtils

3b5a8c2

HyukjinKwon reviewed Sep 16, 2019

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala Outdated Show resolved Hide resolved

HyukjinKwon reviewed Sep 16, 2019

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/datetimeExpressions.scala Show resolved Hide resolved

MaxGekk added 2 commits September 16, 2019 09:44

Use @transient private lazy val formatter

0242852

Handle null from right.eval()

a9fd8e4

HyukjinKwon approved these changes Sep 16, 2019

View reviewed changes

HyukjinKwon closed this in db996cc Sep 17, 2019

MaxGekk deleted the date_format-foldable branch October 5, 2019 19:17

[SPARK-29074][SQL] Optimize date_format for foldable fmt #25782

[SPARK-29074][SQL] Optimize date_format for foldable fmt #25782

Uh oh!

Conversation

MaxGekk commented Sep 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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 Sep 13, 2019

Uh oh!

srowen left a comment

Choose a reason for hiding this comment

Uh oh!

srowen Sep 13, 2019

Choose a reason for hiding this comment

Uh oh!

MaxGekk Sep 13, 2019

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Sep 13, 2019

Uh oh!

Uh oh!

SparkQA commented Sep 13, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dongjoon-hyun commented Sep 13, 2019

Uh oh!

SparkQA commented Sep 14, 2019

Uh oh!

maropu commented Sep 14, 2019

Uh oh!

SparkQA commented Sep 14, 2019

Uh oh!

Uh oh!

Uh oh!

HyukjinKwon commented Sep 16, 2019

Uh oh!

MaxGekk commented Sep 16, 2019

Uh oh!

HyukjinKwon commented Sep 16, 2019

Uh oh!

SparkQA commented Sep 16, 2019

Uh oh!

maropu commented Sep 16, 2019

Uh oh!

SparkQA commented Sep 17, 2019

Uh oh!

SparkQA commented Sep 17, 2019

Uh oh!

HyukjinKwon commented Sep 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

[SPARK-29074][SQL] Optimize `date_format` for foldable `fmt` #25782

[SPARK-29074][SQL] Optimize `date_format` for foldable `fmt` #25782

MaxGekk commented Sep 13, 2019 •

edited

Loading