[SPARK-29605][SQL] Optimize string to interval casting #26256

MaxGekk · 2019-10-25T15:33:31Z

What changes were proposed in this pull request?

In the PR, I propose new function stringToInterval() in IntervalUtils for converting UTF8String to CalendarInterval. The function is used in casting a STRING column to an INTERVAL column.

Why are the changes needed?

The proposed implementation is ~10 times faster. For example, parsing 9 interval units on JDK 8:
Before:

9 units w/ interval                               14004          14125         116          0.1       14003.6       0.0X
9 units w/o interval                              13785          14056         290          0.1       13784.9       0.0X

After:

9 units w/ interval                                1343           1344           1          0.7        1343.0       0.3X
9 units w/o interval                               1345           1349           8          0.7        1344.6       0.3X

Does this PR introduce any user-facing change?

No

How was this patch tested?

By new tests for stringToInterval in IntervalUtilsSuite
By existing tests

SparkQA · 2019-10-25T17:28:38Z

Test build #112677 has finished for PR 26256 at commit 479d5bd.

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

MaxGekk · 2019-10-25T18:01:55Z

jenkins, retest this, please

SparkQA · 2019-10-25T20:29:23Z

Test build #112684 has finished for PR 26256 at commit 479d5bd.

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

MaxGekk · 2019-10-25T20:38:20Z

@cloud-fan @srowen @dongjoon-hyun @maropu May I ask you to review this PR.

maropu · 2019-10-26T02:42:27Z

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

+  private final val minuteStr = UTF8String.fromString("minute")
+  private final val secondStr = UTF8String.fromString("second")
+  private final val millisStr = UTF8String.fromString("millisecond")
+  private final val microsStr = UTF8String.fromString("microsecond")


I know you added final for performance reasons, but do we have actual performance diffs of the benchmarks below with/without this final? (Just out of curiosity...

I re-ran IntervalBenchmark without finals, there is no difference, actually.

maropu · 2019-10-26T13:34:19Z

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

+  private final val millisStr = UTF8String.fromString("millisecond")
+  private final val microsStr = UTF8String.fromString("microsecond")
+
+  def stringToInterval(input: UTF8String): Option[CalendarInterval] = {


Why did you use Option here? If this method has the same return type with safeFromString, we could make diff in Cast.scala much less.

nit: safeFromString -> safeAndFastFromString? Also, could you leave some comments here like This is the fast version of safeFromString...?

Why did you use Option here?

The same reason as stringToDate and stringToTimestamp return Option[...]. I believe all 3 functions should follow similar convention - None for errors and Some[] for valid values.

Returning null instead of None looks slightly ugly in Scala but this should produce less garbage. Let me think of ...

nit: safeFromString -> safeAndFastFromString?

I would remain the same name for consistency with stringToDate and stringToTimestamp, or rename it to safeFromUTF8String.

cloud-fan · 2019-10-28T04:53:28Z

This might be different from INTERVAL'...'. Does casting string to interval type allows leading "interval" in the string?

MaxGekk · 2019-10-28T08:01:04Z

Does casting string to interval type allows leading "interval" in the string?

yes, it does

cloud-fan · 2019-10-28T08:33:09Z

Yes it does allow in Spark now, but I'm asking if it's also allowed in others.

I checked pgsql

cloud0fan=# select 'interval 1 day'::interval;
ERROR:  invalid input syntax for type interval: "interval 1 day"
LINE 1: select 'interval 1 day'::interval;
               ^
cloud0fan=# select '1 day'::interval;
 interval 
----------
 1 day
(1 row)

Can you check more?

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

MaxGekk · 2019-10-28T08:43:46Z

Can you check more?

I can check but the purpose of this PR is optimize existing functionality. I don't think changing behavior here is right choice.

cloud-fan · 2019-10-28T13:46:47Z

If we need to change behavior, I'd suggest we wait for it to happen before writing a new implementation to optimize it.

cloud-fan · 2019-10-28T14:33:55Z

I've open #26283 to change the behavior.

…nterval # Conflicts: # 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

SparkQA · 2019-10-29T21:57:03Z

Test build #112865 has finished for PR 26256 at commit d68f41e.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
class _ImputerParams(HasInputCol, HasInputCols, HasOutputCol, HasOutputCols):
class _OneHotEncoderParams(HasInputCol, HasInputCols, HasOutputCol, HasOutputCols,
abstract class BitAggregate extends DeclarativeAggregate with ExpectsInputTypes
case class BitAndAgg(child: Expression) extends BitAggregate
case class BitOrAgg(child: Expression) extends BitAggregate
case class BitXorAgg(child: Expression) extends BitAggregate
case class Version() extends LeafExpression with CodegenFallback
case class AlterTableRecoverPartitionsStatement(
case class DropNamespaceStatement(
case class LoadDataStatement(
case class ShowCreateTableStatement(tableName: Seq[String]) extends ParsedStatement
case class UncacheTableStatement(
case class DropNamespace(
case class LocalShuffleReaderExec(child: QueryStageExec) extends UnaryExecNode
case class DropNamespaceExec(

…nterval # Conflicts: # 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

SparkQA · 2019-11-02T10:20:37Z

Test build #113124 has finished for PR 26256 at commit 9dfb45d.

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

MaxGekk · 2019-11-02T11:23:56Z

jenkins, retest this, please

cloud-fan · 2019-11-06T11:24:04Z

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

+            case _ if '0' <= b && b <= '9' && fractionScale > 0 =>
+              fraction = Math.addExact(fraction, Math.multiplyExact(fractionScale, (b - '0')))
+              fractionScale /= 10
+            case ' ' =>


how about 1. hour and 1. second?

spark-sql> select cast('1. seconds' as interval); interval 1 seconds spark-sql> select cast('1. days' as interval); interval 1 days

SparkQA · 2019-11-06T12:51:00Z

Test build #113313 has finished for PR 26256 at commit 107d16c.

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

SparkQA · 2019-11-06T13:06:10Z

Test build #113312 has finished for PR 26256 at commit 98dd44f.

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

cloud-fan · 2019-11-06T13:38:37Z

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

+            case ' ' =>
+              state = BEGIN_UNIT_NAME
+            case '.' =>
+              fractionScale = 100000


the antlr version(IntervalUtils.parseNanos) supports up to 9 digits in the fraction part. Shall we follow?

seems the behavior is:

if it's less than 9 digits, take the first 6 and parse to microseconds

if it's more than 9 digits, fail.

Changed the behavior

cloud-fan · 2019-11-06T13:47:51Z

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

-    val input = "+1 year -1 day"
-    val result = new CalendarInterval(12, -1, 0)
-    assert(fromString(input) == result)
+  test("string to interval: special cases") {


can we also test 1. second, 1. hour and 1.111111111 seconds here?

cloud-fan

LGTM except 2 comments.

cloud-fan

LGTM except 2 comments.

cloud-fan

LGTM except 2 comments.

cloud-fan

LGTM except 2 comments.

SparkQA · 2019-11-06T17:31:42Z

Test build #113324 has finished for PR 26256 at commit 8dd9518.

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

SparkQA · 2019-11-06T17:41:31Z

Test build #113325 has finished for PR 26256 at commit 464eacc.

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

MaxGekk · 2019-11-06T17:49:03Z

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

+        case FRACTIONAL_PART =>
+          b match {
+            case _ if '0' <= b && b <= '9' && fractionScale > 0 =>
+              fraction = Math.addExact(fraction, Math.multiplyExact(fractionScale, (b - '0')))


The fractional part cannot overflow. The max number is 999999999 fits to Int. I will remove the exact functions.

MaxGekk · 2019-11-06T19:23:54Z

jenkins, retest this, please

SparkQA · 2019-11-06T20:44:47Z

Test build #113339 has finished for PR 26256 at commit 2222f13.

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

SparkQA · 2019-11-06T21:55:56Z

Test build #113341 has finished for PR 26256 at commit 2222f13.

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

cloud-fan · 2019-11-07T04:40:05Z

thanks, merging to master!

… signs and values ### What changes were proposed in this pull request? With the latest string to literal optimization #26256, some interval strings can not be cast when there are some spaces between signs and unit values. After state `PARSE_SIGN`, it directly goes to `PARSE_UNIT_VALUE` when takes a space character as the end. So when there are some white spaces come before the real unit value, it fails to parse, we should add a new state like `TRIM_VALUE` to trim all these spaces. How to re-produce, which aim the revisions since #26256 is merged ```sql select cast(v as interval) from values ('+ 1 second') t(v); select cast(v as interval) from values ('- 1 second') t(v); ``` ### Why are the changes needed? bug fix ### Does this PR introduce any user-facing change? no ### How was this patch tested? 1. ut 2. new benchmark test Closes #26449 from yaooqinn/SPARK-29605. Authored-by: Kent Yao <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

MaxGekk added 8 commits October 24, 2019 20:50

Generate benchmark results for JDK 8

b22f243

Generate benchmark results for JDK 11

c33710f

Initial implementation

ffa1bee

Add tests to IntervalUtilsSuite

a4c16a1

Optimize unit name matching

f249f49

Catch ArithmeticException

d077c87

Generate benchmark results for JDK 8

80f5a06

Generate benchmark results for JDK 11

479d5bd

MaxGekk mentioned this pull request Oct 25, 2019

[SPARK-29524][SQL] Support unordered interval units in casting from strings #26180

Closed

MaxGekk changed the title ~~[WIP][SPARK-29605][SQL] Optimize string to interval casting~~ [SPARK-29605][SQL] Optimize string to interval casting Oct 25, 2019

dongjoon-hyun added the SQL label Oct 25, 2019

maropu reviewed Oct 26, 2019

View reviewed changes

cloud-fan reviewed Oct 28, 2019

View reviewed changes

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

Merge remote-tracking branch 'remotes/origin/master' into string-to-i…

d68f41e

…nterval # Conflicts: # 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

MaxGekk added 2 commits November 2, 2019 10:27

Merge remote-tracking branch 'remotes/origin/master' into string-to-i…

8e8e539

…nterval # Conflicts: # 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

Rebase on interval with days

9dfb45d

MaxGekk added 2 commits November 6, 2019 14:04

Regen benchmark results for JDK 8

0cd7e88

Regen benchmark results for JDK 11

107d16c

cloud-fan reviewed Nov 6, 2019

View reviewed changes

cloud-fan approved these changes Nov 6, 2019

View reviewed changes

MaxGekk added 2 commits November 6, 2019 18:01

Address Wenchen's comments + change behavior for fractions

8dd9518

Improve comments

464eacc

MaxGekk commented Nov 6, 2019

View reviewed changes

MaxGekk added 4 commits November 6, 2019 20:54

Unify structure

dbad971

Remove exact functions in fraction parsing

527b00e

Replace magic numbers

5e96a0e

Replace 100000000 by NANOS_PER_SECOND / 10

2222f13

cloud-fan closed this in 29dc59a Nov 7, 2019

yaooqinn mentioned this pull request Nov 11, 2019

[SPARK-29822][SQL] Fix cast error when there are white spaces between signs and values #26449

Closed

MaxGekk deleted the string-to-interval branch June 5, 2020 19:41

[SPARK-29605][SQL] Optimize string to interval casting #26256

[SPARK-29605][SQL] Optimize string to interval casting #26256

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!

SparkQA commented Oct 25, 2019

Uh oh!

MaxGekk commented Oct 25, 2019

Uh oh!

SparkQA commented Oct 25, 2019

Uh oh!

MaxGekk commented Oct 25, 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!

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Oct 28, 2019

Uh oh!

MaxGekk commented Oct 28, 2019

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

MaxGekk commented Oct 28, 2019

Uh oh!

cloud-fan commented Oct 28, 2019

Uh oh!

cloud-fan commented Oct 28, 2019

Uh oh!

SparkQA commented Oct 29, 2019

Uh oh!

SparkQA commented Nov 2, 2019

Uh oh!

MaxGekk commented Nov 2, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 6, 2019

Uh oh!

SparkQA commented Nov 6, 2019

Uh oh!

cloud-fan Nov 6, 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!

cloud-fan left a comment

Choose a reason for hiding this comment

Uh oh!

cloud-fan left a comment

Choose a reason for hiding this comment

Uh oh!

cloud-fan left a comment

Choose a reason for hiding this comment

Uh oh!

cloud-fan left a comment

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Oct 28, 2019 •

edited

Loading

cloud-fan Nov 6, 2019 •

edited

Loading

MaxGekk Nov 6, 2019 •

edited

Loading