[SPARK-29494][SQL] Fix for ArrayOutofBoundsException while converting string to timestamp #26143

rahulsmahadev · 2019-10-16T18:18:42Z

What changes were proposed in this pull request?

Adding an additional check in stringToTimestamp to handle cases where the input has trailing ':'
Added a test to make sure this works.

Why are the changes needed?

In a couple of scenarios while converting from String to Timestamp DateTimeUtils.stringToTimestamp throws an array out of bounds exception if there is trailing ':'. The behavior of this method requires it to return None in case the format of the string is incorrect.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Added a test in the DateTimeTestUtils suite to test if my fix works.

rahulsmahadev · 2019-10-16T18:20:39Z

cc @zsxwing

MaxGekk · 2019-10-16T18:47:08Z

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

@@ -456,6 +456,12 @@ class DateTimeUtilsSuite extends SparkFunSuite with Matchers {
    }
  }

+  test("trailing characters while converting string to timestamp") {
+    val s = UTF8String.fromString("2019-10-31T10:59:23Z:::")


You described 2 cases in the PR discription ':' and ' '. This test checks the first one only. Could you add at least one more check for ' '.

zsxwing · 2019-10-16T20:00:17Z

ok to test

MaxGekk · 2019-10-16T20:37:21Z

Actually, trailing spaces shouldn't cause any errors because stringToTimestamp trims its input

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

Line 221 in d11cbf2

val bytes = s.trim.getBytes

MaxGekk · 2019-10-16T20:40:04Z

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

+  }
+
+  test("trailing space while converting string to timestamp") {
+    val s = UTF8String.fromString("2019-10-31T10:59:23ZZ   ")


stringToTimestamp returns None becase of invalid time zone ZZ at the end. If you replace the string "2019-10-31T10:59:23ZZ " by "2019-10-31T10:59:23Z ", the functions shouldn't return None

You are right this works accurately for trailing spaces, I first enounctered this issue on an older release branch and it did not have the .trim . But the fix should work for trailing spaces as well if backported.

SparkQA · 2019-10-16T20:57:48Z

Test build #112189 has finished for PR 26143 at commit 7f764d0.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-10-16T23:44:49Z

Test build #112185 has finished for PR 26143 at commit 17edc79.

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

SparkQA · 2019-10-16T23:53:40Z

Test build #112186 has finished for PR 26143 at commit 421138f.

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

SparkQA · 2019-10-17T00:53:56Z

Test build #112191 has finished for PR 26143 at commit 17edc79.

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

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

…/DateTimeUtils.scala Co-Authored-By: Maxim Gekk <max.gekk@gmail.com>

SparkQA · 2019-10-18T21:40:45Z

Test build #112291 has finished for PR 26143 at commit 5ef53b7.

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

… string to timestamp ### What changes were proposed in this pull request? * Adding an additional check in `stringToTimestamp` to handle cases where the input has trailing ':' * Added a test to make sure this works. ### Why are the changes needed? In a couple of scenarios while converting from String to Timestamp `DateTimeUtils.stringToTimestamp` throws an array out of bounds exception if there is trailing ':'. The behavior of this method requires it to return `None` in case the format of the string is incorrect. ### Does this PR introduce any user-facing change? No ### How was this patch tested? Added a test in the `DateTimeTestUtils` suite to test if my fix works. Closes #26143 from rahulsmahadev/SPARK-29494. Lead-authored-by: Rahul Mahadev <rahul.mahadev@databricks.com> Co-authored-by: Rahul Shivu Mahadev <51690557+rahulsmahadev@users.noreply.github.com> Signed-off-by: Sean Owen <sean.owen@databricks.com> (cherry picked from commit 4cfce3e) Signed-off-by: Sean Owen <sean.owen@databricks.com>

srowen · 2019-10-18T21:46:54Z

Merged to master/2.4

zsxwing · 2019-10-18T22:22:35Z

@rahulsmahadev this broke 2.4. I just reverted it from branch-2.4. Could you submit a new PR against branch-2.4? Thanks!

srowen · 2019-10-19T03:01:15Z

Shoot, thanks for catching that @zsxwing . Looks like some test utility code isn't present in 2.4:

[error] /home/jenkins/workspace/spark-branch-2.4-test-maven-hadoop-2.7/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/util/DateTimeUtilsSuite.scala:586: not found: value defaultZoneId
[error]     val time = DateTimeUtils.stringToTimestamp(s, defaultZoneId)
[error]                                                   ^
[error] one error found
[error] Compile failed at Oct 18, 2019 2:55:40 PM [15.246s]

rahulsmahadev added 2 commits October 16, 2019 11:02

Fix for ArrayOutofBoundsException while converting string to timestamp

23c01c5

Removed println

17edc79

MaxGekk reviewed Oct 16, 2019

View reviewed changes

rahulsmahadev force-pushed the SPARK-29494 branch from 7f764d0 to 17edc79 Compare October 16, 2019 21:10

dongjoon-hyun added the SQL label Oct 16, 2019

srowen approved these changes Oct 17, 2019

View reviewed changes

MaxGekk reviewed Oct 18, 2019

View reviewed changes

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

Update sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util…

5ef53b7

…/DateTimeUtils.scala Co-Authored-By: Maxim Gekk <max.gekk@gmail.com>

MaxGekk approved these changes Oct 18, 2019

View reviewed changes

srowen closed this in 4cfce3e Oct 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[SPARK-29494][SQL] Fix for ArrayOutofBoundsException while converting string to timestamp #26143

[SPARK-29494][SQL] Fix for ArrayOutofBoundsException while converting string to timestamp #26143

Uh oh!

rahulsmahadev commented Oct 16, 2019 •

edited

Loading

Uh oh!

rahulsmahadev commented Oct 16, 2019

Uh oh!

MaxGekk Oct 16, 2019

Uh oh!

zsxwing commented Oct 16, 2019

Uh oh!

MaxGekk commented Oct 16, 2019

Uh oh!

MaxGekk Oct 16, 2019

Uh oh!

rahulsmahadev Oct 16, 2019

Uh oh!

SparkQA commented Oct 16, 2019

Uh oh!

SparkQA commented Oct 16, 2019

Uh oh!

SparkQA commented Oct 16, 2019

Uh oh!

SparkQA commented Oct 17, 2019

Uh oh!

Uh oh!

SparkQA commented Oct 18, 2019

Uh oh!

srowen commented Oct 18, 2019

Uh oh!

zsxwing commented Oct 18, 2019

Uh oh!

srowen commented Oct 19, 2019

Uh oh!

Uh oh!

[SPARK-29494][SQL] Fix for ArrayOutofBoundsException while converting string to timestamp #26143

[SPARK-29494][SQL] Fix for ArrayOutofBoundsException while converting string to timestamp #26143

Uh oh!

Conversation

rahulsmahadev commented Oct 16, 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!

rahulsmahadev commented Oct 16, 2019

Uh oh!

MaxGekk Oct 16, 2019

Choose a reason for hiding this comment

Uh oh!

zsxwing commented Oct 16, 2019

Uh oh!

MaxGekk commented Oct 16, 2019

Uh oh!

MaxGekk Oct 16, 2019

Choose a reason for hiding this comment

Uh oh!

rahulsmahadev Oct 16, 2019

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Oct 16, 2019

Uh oh!

SparkQA commented Oct 16, 2019

Uh oh!

SparkQA commented Oct 16, 2019

Uh oh!

SparkQA commented Oct 17, 2019

Uh oh!

Uh oh!

SparkQA commented Oct 18, 2019

Uh oh!

srowen commented Oct 18, 2019

Uh oh!

zsxwing commented Oct 18, 2019

Uh oh!

srowen commented Oct 19, 2019

Uh oh!

Uh oh!

rahulsmahadev commented Oct 16, 2019 •

edited

Loading