Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
yaooqinn wants to merge 4 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cdc7b2d
[SPARK-29926][SQL] Fix weird interval string whose value end with dan…
yaooqinn Nov 18, 2019
42afa6f
naming
yaooqinn Nov 18, 2019
982b63d
nit
yaooqinn Nov 18, 2019
f6b0edd
only fibid '. seconds'
yaooqinn Nov 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/IntervalUtils.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -505,6 +505,7 @@ object IntervalUtils {
var days: Int = 0
var microseconds: Long = 0
var fractionScale: Int = 0
val validOriginFractionScale = (NANOS_PER_SECOND / 10).toInt

@cloud-fan cloud-fan Nov 18, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about just initialFractionScale?

var fraction: Int = 0

def trimToNextState(b: Byte, next: ParseState): Unit = {
Expand Down Expand Up @@ -556,7 +557,7 @@ object IntervalUtils {
isNegative = false
case '.' =>
isNegative = false
fractionScale = (NANOS_PER_SECOND / 10).toInt
fractionScale = validOriginFractionScale
i += 1
state = VALUE_FRACTIONAL_PART
case _ => throwIAE( s"unrecognized number '$currentWord'")
Expand All @@ -572,7 +573,7 @@ object IntervalUtils {
}
case ' ' => state = TRIM_BEFORE_UNIT
case '.' =>
fractionScale = (NANOS_PER_SECOND / 10).toInt
fractionScale = validOriginFractionScale
state = VALUE_FRACTIONAL_PART
case _ => throwIAE(s"invalid value '$currentWord'")
}
Expand All @@ -582,7 +583,7 @@ object IntervalUtils {
case _ if '0' <= b && b <= '9' && fractionScale > 0 =>
fraction += (b - '0') * fractionScale
fractionScale /= 10
case ' ' =>
case ' ' if fractionScale != validOriginFractionScale =>
fraction /= NANOS_PER_MICROS.toInt
state = TRIM_BEFORE_UNIT
case _ if '0' <= b && b <= '9' =>
Expand Down
6 changes: 3 additions & 3 deletions sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/IntervalUtilsSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ class IntervalUtilsSuite extends SparkFunSuite {
checkFromString("1 day 10 day", new CalendarInterval(0, 11, 0))
// Only the seconds units can have the fractional part
checkFromInvalidString("1.5 days", "'days' cannot have fractional part")
checkFromInvalidString("1. hour", "'hour' cannot have fractional part")
checkFromInvalidString("1. hour", "invalid value '1.'")
checkFromInvalidString("1 hourX", "invalid unit 'hourx'")
checkFromInvalidString("~1 hour", "unrecognized number '~1'")
checkFromInvalidString("1 Mour", "invalid unit 'mour'")
Expand All @@ -115,12 +115,12 @@ class IntervalUtilsSuite extends SparkFunSuite {
checkFromInvalidString("2234567890 days", "integer overflow")
checkFromInvalidString("\n", "Error parsing '\n' to interval")
checkFromInvalidString("\t", "Error parsing '\t' to interval")

checkFromInvalidString("1. seconds", "invalid value '1.'")
checkFromInvalidString(". seconds", "invalid value '.'")
}

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))

@HyukjinKwon HyukjinKwon Nov 19, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@yaooqinn yaooqinn Nov 19, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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

@cloud-fan cloud-fan Nov 19, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@HyukjinKwon HyukjinKwon Nov 19, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

^ +1!

@yaooqinn yaooqinn Nov 19, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated, please recheck, thanks.

checkFromString("123.001 seconds", new CalendarInterval(0, 0, 123001000))
checkFromString("1.001001 seconds", new CalendarInterval(0, 0, 1001001))
checkFromString("1 minute 1.001001 seconds", new CalendarInterval(0, 0, 61001001))
Expand Down