[SPARK-29864][SPARK-29920][SQL] Strict parsing of day-time strings to intervals #26473

MaxGekk · 2019-11-11T20:17:10Z

What changes were proposed in this pull request?

In the PR, I propose new implementation of fromDayTimeString which strictly parses strings in day-time formats to intervals. New implementation accepts only strings that match to a pattern defined by the from and to. Here is the mapping of user's bounds and patterns:

[+|-]D+ H[H]:m[m]:s[s][.SSSSSSSSS] for DAY TO SECOND
[+|-]D+ H[H]:m[m] for DAY TO MINUTE
[+|-]D+ H[H] for DAY TO HOUR
[+|-]H[H]:m[m]s[s][.SSSSSSSSS] for HOUR TO SECOND
[+|-]H[H]:m[m] for HOUR TO MINUTE
[+|-]m[m]:s[s][.SSSSSSSSS] for MINUTE TO SECOND

Closes #26327
Closes #26358

Why are the changes needed?

Improve user experience with Spark SQL, and respect to the bound specified by users.
Behave the same as other broadly used DBMS - Oracle and MySQL.

Does this PR introduce any user-facing change?

Yes, before:

spark-sql> SELECT INTERVAL '10 11:12:13.123' HOUR TO MINUTE;
interval 1 weeks 3 days 11 hours 12 minutes

After:

spark-sql> SELECT INTERVAL '10 11:12:13.123' HOUR TO MINUTE;
Error in query:
requirement failed: Interval string must match day-time format of '^(?<sign>[+|-])?(?<hour>\d{1,2}):(?<minute>\d{1,2})$': 10 11:12:13.123(line 1, pos 16)

== SQL ==
SELECT INTERVAL '10 11:12:13.123' HOUR TO MINUTE
----------------^^^

How was this patch tested?

Added tests to IntervalUtilsSuite
By ExpressionParserSuite
Updated literals.sql

MaxGekk · 2019-11-11T20:21:37Z

@cloud-fan I cannot easily integrate old implementation to new one because of #26327 (comment). What I am considering is to just keep old implementation of fromDayTimeString and rename it to let's say to legacyFromDayTimeString. And to introduce a SQL flag to fallback to the old implementation. WDYT?

SparkQA · 2019-11-12T00:48:25Z

Test build #113600 has finished for PR 26473 at commit e88c8e0.

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

cloud-fan · 2019-11-12T05:38:03Z

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

+    check("-12:40", HOUR, MINUTE, "-12 hours -40 minutes")
+    checkFail("5 12:40", HOUR, MINUTE, "must match day-time format")
+
+    check("12:40:30.999999999", HOUR, SECOND, "12 hours 40 minutes 30.999999 seconds")


can we test 12:40:30.0123456789 as well?

cloud-fan · 2019-11-12T05:59:05Z

+1 to keep the old implementation and add a legacy config to fallback

SparkQA · 2019-11-12T11:07:26Z

Test build #113624 has finished for PR 26473 at commit 988d532.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-11-12T16:10:50Z

Test build #113628 has finished for PR 26473 at commit 58ba7b4.

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/IntervalUtils.scala

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

…-daytime-string # Conflicts: # sql/core/src/test/resources/sql-tests/results/literals.sql.out

MaxGekk · 2019-11-22T06:49:36Z

@cloud-fan I ran ThriftServerQueryTestSuite locally. It failed by the the same reasons.

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

Test build #114278 has finished for PR 26473 at commit 73ef32f.

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

MaxGekk · 2019-11-22T08:05:57Z

jenkins, retest this, please

cloud-fan · 2019-11-22T11:42:18Z

@MaxGekk do we have a simple example to demonstrate the bug of ThriftServerQueryTestSuite? We can create a JIRA to look into it.

In the meanwhile, we don't add tests to pgsql/interval.sql to avoid the bug.

SparkQA · 2019-11-22T12:01:30Z

Test build #114282 has finished for PR 26473 at commit 73ef32f.

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

SparkQA · 2019-11-22T12:09:58Z

Test build #114286 has finished for PR 26473 at commit 73ef32f.

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

MaxGekk · 2019-11-22T15:40:28Z

@MaxGekk do we have a simple example to demonstrate the bug of ThriftServerQueryTestSuite?

If this PR will be merged to the master, the bug can be reproduced by removing set ... in ansi/interval.sql pretty easy. Otherwise I don't know how to reproduce it easier.

We can create a JIRA to look into it.

It has been already created: https://issues.apache.org/jira/browse/SPARK-29933

In the meanwhile, we don't add tests to pgsql/interval.sql to avoid the bug.

Actually, it is not enough because wrong settings cause failure ansi/interval.sql due to https://issues.apache.org/jira/browse/SPARK-29920 and postgresql/interval.sql due to ThriftServerQueryTestSuite uses new implementation which is much stronger.

I could remove the tests from *interval.sql but I would prefer to leave them there to have reproducible examples for SPARK-29933 and to don't forget about the test in the future.

MaxGekk · 2019-11-22T15:42:41Z

For some reasons, the last 2 builds finished successfully #26473 (comment) & #26473 (comment) . Probably, the order of tests was changed.

…-daytime-string # Conflicts: # docs/sql-migration-guide.md

MaxGekk · 2019-11-22T16:06:13Z

@cloud-fan Hmm, when I run all tests via build/sbt "hive-thriftserver/test-only *ThriftServerQueryTestSuite" -Phive-thriftserver, they all passed. When I run only interval.sql via build/sbt "hive-thriftserver/test-only *ThriftServerQueryTestSuite -- -z interval.sql" -Phive-thriftserver, they fail.

SparkQA · 2019-11-22T16:49:13Z

Test build #114304 has finished for PR 26473 at commit d27d434.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2019-11-26T17:07:45Z

I've figured out the problem: The Spark thrift server creates many SparkSessions to serve requests, and the thrift server serves requests using a single thread. One thread can only have one active SparkSession, so SQLCong.get can't get the proper conf from the session that runs the query.

For this particular problem, a simple fix in Dataset is

def sql(sqlText: String): DataFrame = {
     val tracker = new QueryPlanningTracker
+    SparkSession.setActiveSession(this)
     val plan = tracker.measurePhase(QueryPlanningTracker.PARSING) {
       sessionState.sqlParser.parsePlan(sqlText)
     }

But we should really think about how to set active session correctly.

MaxGekk · 2019-12-09T14:22:25Z

I don't know when this bug #26473 (comment) will be fixed but can we merge this PR with the workaround?

I just want to remind you that this PR fixes the bug #26473 (comment)

cloud-fan · 2019-12-10T12:55:44Z

It has been a while, can you remind me what's your workaround? BTW I feel it's also OK to use my one-line fix: just add SparkSession.setActiveSession(this) in SparkSession/sql

MaxGekk · 2019-12-10T17:09:09Z

can you remind me what's your workaround?

I mean explicit set ca46f44

cloud-fan · 2019-12-10T17:24:37Z

The dialect config has been removed. Can you try and see if the test still fail? If it does then maybe just use my one-line fix.

…-daytime-string # Conflicts: # sql/core/src/test/resources/sql-tests/results/ansi/interval.sql.out # sql/core/src/test/resources/sql-tests/results/interval.sql.out

SparkQA · 2019-12-10T23:47:29Z

Test build #115122 has finished for PR 26473 at commit f9510e3.

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

cloud-fan · 2019-12-11T08:33:44Z

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

+
+    check("12:40:30.999999999", HOUR, SECOND, "12 hours 40 minutes 30.999999 seconds")
+    check("+12:40:30.999999999", HOUR, SECOND, "12 hours 40 minutes 30.999999 seconds")
+    check("-12:40:30.999999999", HOUR, SECOND, "-12 hours -40 minutes -30.999999 seconds")


let's avoid using 99999. It's hard for reviewers to count the number of digits. Let's use 123456 to ease the digits counting.

I just wanted to test the maximum possible value in the fractional part. I think we need at least one test for 999999999

I replaced 999999999 by 123456789 everywhere except one check

cloud-fan

LGTM except one minor comment

SparkQA · 2019-12-11T14:37:41Z

Test build #115177 has finished for PR 26473 at commit c16f2a7.

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

cloud-fan · 2019-12-11T16:34:51Z

thanks, merging to master!

MaxGekk added 6 commits November 11, 2019 21:55

Strict fromDayTimeString

800f6e5

Add tests

8cf38db

Update tests in IntervalUtilsSuite

3a1a710

Fix ExpressionParserSuite

4e051d6

Add tests for invalid input into literals.sql

a1ef591

Regen interval.sql.out

e88c8e0

MaxGekk mentioned this pull request Nov 11, 2019

[SPARK-29712][SQL] Take into account the left bound in fromDayTimeString() #26358

Closed

cloud-fan reviewed Nov 12, 2019

View reviewed changes

MaxGekk added 5 commits November 12, 2019 12:58

Add the config spark.sql.legacy.fromDayTimeString.enabled

c87cf8c

Restore previous version of fromDayTimeString

3f66881

Restore old tests in IntervalUtilsSuite

da9b801

Enable legacy behavior for postgresql tests

c8afd33

Regen interval.sql.out

988d532

MaxGekk added 2 commits November 12, 2019 14:47

Set LEGACY_FROM_DAYTIME_STRING to true in ThriftServerQueryTestSuite

ce28745

Address Wenchen's review comment about a test

58ba7b4

MaxGekk added 3 commits November 12, 2019 19:30

Add comments for parseDayTimeLegacy

60fe0c1

Avoid unnecessary catching of exceptions

e95068e

Add comments for parseDayTime()

880e7ed

cloud-fan reviewed Nov 12, 2019

View reviewed changes

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

cloud-fan reviewed Nov 12, 2019

View reviewed changes

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

cloud-fan reviewed Nov 12, 2019

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala Show resolved Hide resolved

MaxGekk added 4 commits November 12, 2019 20:21

Merge remote-tracking branch 'remotes/origin/master' into strict-from…

d00c95d

…-daytime-string # Conflicts: # sql/core/src/test/resources/sql-tests/results/literals.sql.out

Regen literals.sql.out

a37bad4

Remove exact ops

8e733c1

Update the SQL migration guide

6b5b7ef

Regen interval.sql.out

73ef32f

Merge remote-tracking branch 'remotes/origin/master' into strict-from…

d27d434

…-daytime-string # Conflicts: # docs/sql-migration-guide.md

MaxGekk mentioned this pull request Dec 9, 2019

[SPARK-30189][SQL] Interval from year-month/date-time string should handle whitespaces #26815

Closed

MaxGekk added 3 commits December 10, 2019 21:25

Merge remote-tracking branch 'remotes/origin/master' into strict-from…

9d8394e

…-daytime-string # Conflicts: # sql/core/src/test/resources/sql-tests/results/ansi/interval.sql.out # sql/core/src/test/resources/sql-tests/results/interval.sql.out

Remove usage of usePostgreSQLDialect

ef2cbe1

Regen interval.sql.out

f9510e3

cloud-fan reviewed Dec 11, 2019

View reviewed changes

cloud-fan approved these changes Dec 11, 2019

View reviewed changes

Replace 999999999 by 123456789 in seconds fractions in tests

c16f2a7

cloud-fan closed this in e933539 Dec 11, 2019

MaxGekk deleted the strict-from-daytime-string branch June 5, 2020 19:41

[SPARK-29864][SPARK-29920][SQL] Strict parsing of day-time strings to intervals #26473

[SPARK-29864][SPARK-29920][SQL] Strict parsing of day-time strings to intervals #26473

Uh oh!

Conversation

MaxGekk commented Nov 11, 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!

MaxGekk commented Nov 11, 2019

Uh oh!

SparkQA commented Nov 12, 2019

Uh oh!

cloud-fan Nov 12, 2019

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Nov 12, 2019

Uh oh!

SparkQA commented Nov 12, 2019

Uh oh!

SparkQA commented Nov 12, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MaxGekk commented Nov 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Nov 22, 2019

Uh oh!

MaxGekk commented Nov 22, 2019

Uh oh!

cloud-fan commented Nov 22, 2019

Uh oh!

SparkQA commented Nov 22, 2019

Uh oh!

SparkQA commented Nov 22, 2019

Uh oh!

MaxGekk commented Nov 22, 2019

Uh oh!

MaxGekk commented Nov 22, 2019

Uh oh!

MaxGekk commented Nov 22, 2019

Uh oh!

SparkQA commented Nov 22, 2019

Uh oh!

cloud-fan commented Nov 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MaxGekk commented Dec 9, 2019

Uh oh!

cloud-fan commented Dec 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MaxGekk commented Dec 10, 2019

Uh oh!

cloud-fan commented Dec 10, 2019

Uh oh!

SparkQA commented Dec 10, 2019

Uh oh!

cloud-fan Dec 11, 2019

Choose a reason for hiding this comment

Uh oh!

MaxGekk Dec 11, 2019

Choose a reason for hiding this comment

Uh oh!

MaxGekk Dec 11, 2019

Choose a reason for hiding this comment

Uh oh!

cloud-fan left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 11, 2019

Uh oh!

cloud-fan commented Dec 11, 2019

Uh oh!

Reviewers

Assignees

MaxGekk commented Nov 11, 2019 •

edited

Loading

MaxGekk commented Nov 22, 2019 •

edited

Loading

cloud-fan commented Nov 26, 2019 •

edited

Loading

cloud-fan commented Dec 10, 2019 •

edited

Loading