[SPARK-29926][SQL] Fix weird interval string whose value is only a dangling decimal point #26573

yaooqinn · 2019-11-18T09:44:34Z

What changes were proposed in this pull request?

Currently, we support to parse '1. second' to 1s or even '. second' to 0s.

-- !query 118
select interval '1. seconds'
-- !query 118 schema
struct<1 seconds:interval>
-- !query 118 output
1 seconds


-- !query 119
select interval '. seconds'
-- !query 119 schema
struct<0 seconds:interval>
-- !query 119 output
0 seconds

postgres=# select interval '1. second';
ERROR:  invalid input syntax for type interval: "1. second"
LINE 1: select interval '1. second';

postgres=# select interval '. second';
ERROR:  invalid input syntax for type interval: ". second"
LINE 1: select interval '. second';

We fix this by fixing the new interval parser's VALUE_FRACTIONAL_PART state

With further digging, we found that 1. is valid in python, r, scala, and presto and so on... so this PR
ONLY forbid the invalid interval value in the form of '. seconds'.

Why are the changes needed?

bug fix

Does this PR introduce any user-facing change?

yes, now we treat '. second' .... as invalid intervals

How was this patch tested?

add ut

…gling decial point

yaooqinn · 2019-11-18T09:47:53Z

cc @cloud-fan @MaxGekk @maropu @HyukjinKwon thanks for reviewing in advance. Pre-discussion could be found here #26491 (comment)

cloud-fan · 2019-11-18T13:10:38Z

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

    var days: Int = 0
    var microseconds: Long = 0
    var fractionScale: Int = 0
+    val validOriginFractionScale = (NANOS_PER_SECOND / 10).toInt


how about just initialFractionScale?

SparkQA · 2019-11-18T13:50:11Z

Test build #114008 has finished for PR 26573 at commit cdc7b2d.

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

cloud-fan · 2019-11-18T15:05:36Z

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

              fraction += (b - '0') * fractionScale
              fractionScale /= 10
-            case ' ' =>
+            case ' ' if fractionScale != initialFractionScale =>


super nit: seems fractionScale < initialFractionScale is safer?

SparkQA · 2019-11-18T17:42:04Z

Test build #114020 has finished for PR 26573 at commit 42afa6f.

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

SparkQA · 2019-11-18T19:07:16Z

Test build #114024 has finished for PR 26573 at commit 982b63d.

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

HyukjinKwon · 2019-11-19T04:38:22Z

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


  test("string to interval: seconds with fractional part") {
    checkFromString("0.1 seconds", new CalendarInterval(0, 0, 100000))
-    checkFromString("1. seconds", new CalendarInterval(0, 0, 1000000))


Will we explicitly disable this case?

scala> sql("select 0.") res0: org.apache.spark.sql.DataFrame = [0: decimal(1,0)]

At least I know Python, Java, Scala and R support this way as well.

presto> select interval '1.' second; _col0 ---------------- 0 00:00:01.000 (1 row) Query 20191119_051438_00001_f5kcs, FINISHED, 1 node Splits: 17 total, 17 done (100.00%) 0:03 [0 rows, 0B] [0 rows/s, 0B/s] presto> select interval '.' second; Query 20191119_051452_00002_f5kcs failed: Invalid INTERVAL SECOND value: .

Also check with presto, '1.' seconds is valid

@HyukjinKwon good point. To be consistent with Spark's own parser, maybe we should allow interval '1.' second. But interval '.' second should be invalid.

updated, please recheck, thanks.

cloud-fan · 2019-11-19T08:02:37Z

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

    var days: Int = 0
    var microseconds: Long = 0
    var fractionScale: Int = 0
+    val initialFractionScale = (NANOS_PER_SECOND / 10).toInt


do we still need it now?

cloud-fan · 2019-11-19T08:03:20Z

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

              fraction += (b - '0') * fractionScale
              fractionScale /= 10
-            case ' ' =>
+            case ' ' if !pointPrefixed || fractionScale < initialFractionScale =>


since 1. is allowed, I don't think fractionScale < initialFractionScale is a valid check any more.

we need this, the cases are (1.0, 0.1, .1, 1.) + (.), this condition is equal to !(pointPrefixed && fractionScale == initialFractionScale)

rm fractionScale < initialFractionScale forbiding .1 case

SparkQA · 2019-11-19T12:23:27Z

Test build #114076 has finished for PR 26573 at commit f6b0edd.

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

cloud-fan · 2019-11-19T13:01:41Z

thanks, merging to master!

[SPARK-29926][SQL] Fix weird interval string whose value end with dan…

cdc7b2d

…gling decial point

cloud-fan reviewed Nov 18, 2019

View reviewed changes

cloud-fan approved these changes Nov 18, 2019

View reviewed changes

naming

42afa6f

cloud-fan reviewed Nov 18, 2019

View reviewed changes

nit

982b63d

dongjoon-hyun added the SQL label Nov 18, 2019

HyukjinKwon reviewed Nov 19, 2019

View reviewed changes

only fibid '. seconds'

f6b0edd

yaooqinn changed the title ~~[SPARK-29926][SQL] Fix weird interval string whose value end with a dangling decimal point~~ [SPARK-29926][SQL] Fix weird interval string whose value is only a dangling decimal point Nov 19, 2019

cloud-fan reviewed Nov 19, 2019

View reviewed changes

cloud-fan closed this in 79ed4ae Nov 19, 2019

[SPARK-29926][SQL] Fix weird interval string whose value is only a dangling decimal point #26573

[SPARK-29926][SQL] Fix weird interval string whose value is only a dangling decimal point #26573

Uh oh!

Conversation

yaooqinn commented Nov 18, 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!

yaooqinn commented Nov 18, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 18, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 18, 2019

Uh oh!

SparkQA commented Nov 18, 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!

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!

SparkQA commented Nov 19, 2019

Uh oh!

cloud-fan commented Nov 19, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

yaooqinn commented Nov 18, 2019 •

edited

Loading