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 all commits
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
10 changes: 7 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,7 +505,9 @@ object IntervalUtils {
var days: Int = 0
var microseconds: Long = 0
var fractionScale: Int = 0
val initialFractionScale = (NANOS_PER_SECOND / 10).toInt
Copy link
Contributor

@cloud-fan cloud-fan Nov 19, 2019

Choose a reason for hiding this comment

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

do we still need it now?

var fraction: Int = 0
var pointPrefixed: Boolean = false

def trimToNextState(b: Byte, next: ParseState): Unit = {
b match {
Expand Down Expand Up @@ -545,6 +547,7 @@ object IntervalUtils {
// We preset the scale to an invalid value to track fraction presence in the UNIT_BEGIN
// state. If we meet '.', the scale become valid for the VALUE_FRACTIONAL_PART state.
fractionScale = -1
pointPrefixed = false
b match {
case '-' =>
isNegative = true
Expand All @@ -556,7 +559,8 @@ object IntervalUtils {
isNegative = false
case '.' =>
isNegative = false
fractionScale = (NANOS_PER_SECOND / 10).toInt
fractionScale = initialFractionScale
pointPrefixed = true
i += 1
state = VALUE_FRACTIONAL_PART
case _ => throwIAE( s"unrecognized number '$currentWord'")
Expand All @@ -572,7 +576,7 @@ object IntervalUtils {
}
case ' ' => state = TRIM_BEFORE_UNIT
case '.' =>
fractionScale = (NANOS_PER_SECOND / 10).toInt
fractionScale = initialFractionScale
state = VALUE_FRACTIONAL_PART
case _ => throwIAE(s"invalid value '$currentWord'")
}
Expand All @@ -582,7 +586,7 @@ object IntervalUtils {
case _ if '0' <= b && b <= '9' && fractionScale > 0 =>
fraction += (b - '0') * fractionScale
fractionScale /= 10
case ' ' =>
case ' ' if !pointPrefixed || fractionScale < initialFractionScale =>
Copy link
Contributor

@cloud-fan cloud-fan Nov 19, 2019

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@yaooqinn yaooqinn Nov 19, 2019

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@yaooqinn yaooqinn Nov 19, 2019

Choose a reason for hiding this comment

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

rm fractionScale < initialFractionScale forbiding .1 case

Copy link
Contributor

@cloud-fan cloud-fan Nov 19, 2019

Choose a reason for hiding this comment

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

ah i see!

fraction /= NANOS_PER_MICROS.toInt
state = TRIM_BEFORE_UNIT
case _ if '0' <= b && b <= '9' =>
Expand Down
2 changes: 1 addition & 1 deletion 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 @@ -115,7 +115,7 @@ class IntervalUtilsSuite extends SparkFunSuite {
checkFromInvalidString("2234567890 days", "integer overflow")
checkFromInvalidString("\n", "Error parsing '\n' to interval")
checkFromInvalidString("\t", "Error parsing '\t' to interval")

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

test("string to interval: seconds with fractional part") {
Expand Down