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

yaooqinn · 2019-11-09T17:46:54Z

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

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?

ut
new benchmark test

…ween signs and values

yaooqinn · 2019-11-09T17:47:29Z

cc @MaxGekk @maropu @cloud-fan, thanks in advance.

MaxGekk

LGTM, could you regenerate benchmark results for JDK 11 - IntervalBenchmark-jdk11-results.txt

MaxGekk · 2019-11-09T18:17:39Z

sql/core/benchmarks/IntervalBenchmark-results.txt

@@ -1,25 +1,29 @@
-Java HotSpot(TM) 64-Bit Server VM 1.8.0_231-b11 on Mac OS X 10.15.1
-Intel(R) Core(TM) i7-4850HQ CPU @ 2.30GHz
+Java HotSpot(TM) 64-Bit Server VM 1.8.0_65-b17 on Mac OS X 10.14.6


So old jdk? ;-)

updated and regen-ed with both new jdk8 and jdk11, thanks.

SparkQA · 2019-11-09T21:28:17Z

Test build #113501 has finished for PR 26449 at commit 8f0dba0.

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

This reverts commit 65e2a1c.

SparkQA · 2019-11-10T05:52:33Z

Test build #113515 has finished for PR 26449 at commit 65e2a1c.

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

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

Test build #113516 has finished for PR 26449 at commit 8851f35.

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

maropu · 2019-11-10T07:08:56Z

Can you file a new jira for this for better traceability?

yaooqinn · 2019-11-10T08:33:13Z

Can you file a new jira for this for better traceability?

Done, thanks for your suggestion @maropu

Here is the ticket:
https://issues.apache.org/jira/browse/SPARK-29822

dongjoon-hyun · 2019-11-10T16:55:03Z

sql/core/src/test/resources/sql-tests/inputs/interval.sql

@@ -41,3 +41,12 @@ select max(cast(v as interval)) from VALUES ('1 seconds'), ('4 seconds'), ('3 se

 -- min
 select min(cast(v as interval)) from VALUES ('1 seconds'), ('4 seconds'), ('3 seconds') t(v);
+
+-- SPARK-29605: cast string to intervals


This should be SPARK-29822 because this is newly added test coverage for this PR SPARK-29822.

I guess I just rm it from here, thanks

dongjoon-hyun · 2019-11-10T17:07:25Z

Hi, @yaooqinn .
I know this PR is valid on master branch. Could you put the pointer in the PR description about what causes this? For 3.0-preview, it worked like the following.

spark-sql> select cast(v as interval) from values ('+     1 second') t(v);
interval 1 seconds
Time taken: 2.011 seconds, Fetched 1 row(s)

spark-sql> select cast(v as interval) from values ('-     1 second') t(v);
interval -1 seconds
Time taken: 0.033 seconds, Fetched 1 row(s)

spark-sql> select version();
3.0.0 007c873ae34f58651481ccba30e8e2ba38a692c4
Time taken: 1.181 seconds, Fetched 1 row(s)

yaooqinn · 2019-11-11T02:03:48Z

@dongjoon-hyun, thanks for review. The pr description is updated

SparkQA · 2019-11-11T05:47:07Z

Test build #113553 has finished for PR 26449 at commit 1e8272c.

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

cloud-fan · 2019-11-11T05:55:02Z

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

    val PREFIX,
        BEGIN_VALUE,
        PARSE_SIGN,
+        TRIM_VALUE,
        PARSE_UNIT_VALUE,
        FRACTIONAL_PART,
        BEGIN_UNIT_NAME,


In BEGIN_UNIT_NAME we also trim the spaces. Can we have a consistent way to do it? Now there are 2 ways:

have an intermedia state to trim the spaces

trim the spaces at the beginning of a state.

we can add a TRIM_UNIT state to unify these 2 ways

cloud-fan · 2019-11-11T06:33:01Z

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

    val PREFIX,
        BEGIN_VALUE,
        PARSE_SIGN,
+        TRIM_VALUE,


can we make the name clearer? e.g. TRIM_BEFORE_PARSE_UNIT_VALUE

cloud-fan · 2019-11-11T06:35:09Z

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

@@ -425,11 +425,15 @@ object IntervalUtils {
  }

  private object ParseState extends Enumeration {
+    type ParseState = Value
+
    val PREFIX,
        BEGIN_VALUE,


we can rename it to TRIM_BEFORE_PARSE_VALUE

so we may change PARSE_UNIT_VALUE to PARSE_VALUE too

How about renaming them all as,

val PREFIX, BODY, SIGN, TRIM_BEFORE_VALUE, VALUE, VALUE_FRACTIONAL_PART, TRIM_BEFORE_UNIT, UNIT_BEGIN, UNIT_SUFFIX, UNIT_END = Value

Seems loud and clear enough for each state

what does BODY mean? others LGTM

interval (PREFIX, BODY)
BODY (SIGN, VALUE , UNIT)+

Seems not persuadable enough

or NEXT_VALUE_UNIT? NEXT_VALUE_UNIT_PAIR

cloud-fan · 2019-11-11T07:36:48Z

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

-        BEGIN_UNIT_NAME,
-        UNIT_NAME_SUFFIX,
-        END_UNIT_NAME = Value
+        NEXT_VALUE_UNIT,


how about TRIM_BEFORE_SIGN to be consistent?

cool. good naming

SparkQA · 2019-11-11T08:05:01Z

Test build #113571 has finished for PR 26449 at commit 83b9ae3.

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

SparkQA · 2019-11-11T08:05:02Z

Test build #113568 has finished for PR 26449 at commit cb8aa6b.

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

SparkQA · 2019-11-11T08:05:02Z

Test build #113575 has finished for PR 26449 at commit 34bf719.

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

SparkQA · 2019-11-11T08:05:02Z

Test build #113569 has finished for PR 26449 at commit 0a301a1.

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

cloud-fan · 2019-11-11T08:16:21Z

sql/core/src/test/resources/sql-tests/inputs/interval.sql

@@ -65,3 +65,12 @@ select make_interval(1, 2, 3, 4);
 select make_interval(1, 2, 3, 4, 5);
 select make_interval(1, 2, 3, 4, 5, 6);
 select make_interval(1, 2, 3, 4, 5, 6, 7.008009);
+
+-- cast string to intervals
+select cast(v as interval) from values ('1 second') t(v);


nit: select cast('1 second' as interval)

SparkQA · 2019-11-11T12:22:28Z

Test build #113578 has finished for PR 26449 at commit cb83761.

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

cloud-fan · 2019-11-11T13:53:44Z

thanks, merging to master!

yaooqinn added 3 commits November 10, 2019 01:15

[SPARK-29605][SQL][FOLLOWUP] Fix cast error when there are spaces bet…

b1b2f7c

…ween signs and values

Merge remote-tracking branch 'origin/master' into SPARK-29605

71acf0a

benchmark and unit tests

8f0dba0

MaxGekk reviewed Nov 9, 2019

View reviewed changes

dongjoon-hyun added the SQL label Nov 9, 2019

yaooqinn added 4 commits November 10, 2019 09:53

benchmark with jdk 1.8.0_231-b11

925e8e7

benchmark with jdk-11.0.5

65e2a1c

Revert "benchmark with jdk-11.0.5"

200fd5d

This reverts commit 65e2a1c.

fix jdk11 result

8851f35

maropu approved these changes Nov 10, 2019

View reviewed changes

yaooqinn changed the title ~~[SPARK-29605][SQL][FOLLOWUP] Fix cast error when there are spaces between signs and values~~ [SPARK-29822][SQL] Fix cast error when there are spaces between signs and values Nov 10, 2019

dongjoon-hyun reviewed Nov 10, 2019

View reviewed changes

Merge branch 'master' into SPARK-29605

48d168d

yaooqinn changed the title ~~[SPARK-29822][SQL] Fix cast error when there are spaces between signs and values~~ [SPARK-29822][SQL] Fix cast error when there are white spaces between signs and values Nov 11, 2019

regen golden file

1e8272c

cloud-fan reviewed Nov 11, 2019

View reviewed changes

add TRIM_UNIT

cb8aa6b

cloud-fan reviewed Nov 11, 2019

View reviewed changes

naming

0a301a1

naming again

83b9ae3

cloud-fan reviewed Nov 11, 2019

View reviewed changes

naming again

34bf719

cloud-fan reviewed Nov 11, 2019

View reviewed changes

tests

cb83761

cloud-fan approved these changes Nov 11, 2019

View reviewed changes

cloud-fan closed this in d06a9cc Nov 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

yaooqinn commented Nov 9, 2019 •

edited

yaooqinn commented Nov 9, 2019

MaxGekk left a comment

MaxGekk Nov 9, 2019

yaooqinn Nov 10, 2019

SparkQA commented Nov 9, 2019

SparkQA commented Nov 10, 2019

SparkQA commented Nov 10, 2019

maropu commented Nov 10, 2019

yaooqinn commented Nov 10, 2019

dongjoon-hyun Nov 10, 2019

yaooqinn Nov 11, 2019

dongjoon-hyun commented Nov 10, 2019

yaooqinn commented Nov 11, 2019

SparkQA commented Nov 11, 2019

cloud-fan Nov 11, 2019

yaooqinn Nov 11, 2019

cloud-fan Nov 11, 2019 •

edited

cloud-fan Nov 11, 2019

yaooqinn Nov 11, 2019 •

edited

cloud-fan Nov 11, 2019

yaooqinn Nov 11, 2019

yaooqinn Nov 11, 2019 •

edited

cloud-fan Nov 11, 2019

yaooqinn Nov 11, 2019

SparkQA commented Nov 11, 2019

SparkQA commented Nov 11, 2019

SparkQA commented Nov 11, 2019

SparkQA commented Nov 11, 2019

cloud-fan Nov 11, 2019

SparkQA commented Nov 11, 2019

cloud-fan commented Nov 11, 2019

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

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

Conversation

yaooqinn commented Nov 9, 2019 • edited

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?

yaooqinn commented Nov 9, 2019

MaxGekk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Nov 9, 2019

SparkQA commented Nov 10, 2019

SparkQA commented Nov 10, 2019

maropu commented Nov 10, 2019

yaooqinn commented Nov 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongjoon-hyun commented Nov 10, 2019

yaooqinn commented Nov 11, 2019

SparkQA commented Nov 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan Nov 11, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaooqinn Nov 11, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaooqinn Nov 11, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Nov 11, 2019

SparkQA commented Nov 11, 2019

SparkQA commented Nov 11, 2019

SparkQA commented Nov 11, 2019

Choose a reason for hiding this comment

SparkQA commented Nov 11, 2019

cloud-fan commented Nov 11, 2019

yaooqinn commented Nov 9, 2019 •

edited

cloud-fan Nov 11, 2019 •

edited

yaooqinn Nov 11, 2019 •

edited

yaooqinn Nov 11, 2019 •

edited