[SPARK-32785][SQL] Interval with dangling parts should not results null #29635

yaooqinn · 2020-09-03T09:33:25Z

What changes were proposed in this pull request?

bugfix for incomplete interval values, e.g. interval '1', interval '1 day 2', currently these cases will result null, but actually we should fail them with IllegalArgumentsException

Why are the changes needed?

correctness

Does this PR introduce any user-facing change?

yes, incomplete intervals will throw exception now

before

bin/spark-sql -S -e "select interval '1', interval '+', interval '1 day -'"

NULL NULL NULL

after

-- !query
select interval '1'
-- !query schema
struct<>
-- !query output
org.apache.spark.sql.catalyst.parser.ParseException

Cannot parse the INTERVAL value: 1(line 1, pos 7)

== SQL ==
select interval '1'

How was this patch tested?

unit tests added

yaooqinn · 2020-09-03T09:37:57Z

cc @cloud-fan @MaxGekk @maropu @HyukjinKwon thanks for reviewing

SparkQA · 2020-09-03T10:17:12Z

Test build #128248 has finished for PR 29635 at commit 6d27597.

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

cloud-fan · 2020-09-03T12:04:39Z

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

@@ -77,6 +77,19 @@ class IntervalUtilsSuite extends SparkFunSuite with SQLHelper {
    }
  }

+  test("string to interval: interval with dangling parts should not results null") {
+    checkFromInvalidString("+", "dangling interval value sign '+'")
+    checkFromInvalidString("-", "dangling interval value sign '-'")


how about Expect a number after '-' but hit EOL

cloud-fan · 2020-09-03T12:05:01Z

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

+    checkFromInvalidString("-", "dangling interval value sign '-'")
+    checkFromInvalidString("+ 2", "dangling interval value '2'")
+    checkFromInvalidString("- 1", "dangling interval value '1'")
+    checkFromInvalidString("1", "dangling interval value '1'")


how about Expect a unit name after '1' but hit EOL

done, but looks a bit weird for interval '-.' to expect a unit name

SparkQA · 2020-09-03T15:31:05Z

Test build #128254 has finished for PR 29635 at commit eb99bd5.

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

SparkQA · 2020-09-03T19:58:25Z

Test build #128261 has finished for PR 29635 at commit ae6d9be.

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

cloud-fan · 2020-09-04T07:50:59Z

can we add a migration guide?

yaooqinn · 2020-09-04T07:53:38Z

OK, are you targeting this to 3.0 or not?

cloud-fan · 2020-09-04T09:14:22Z

yea we should target 3.0. Let's add migration guide for 3.1 in this PR, and do the same for 3.0.2 in the backport PR.

SparkQA · 2020-09-04T15:05:04Z

Test build #128293 has finished for PR 29635 at commit daed4e6.

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

cloud-fan · 2020-09-07T05:11:28Z

thanks, merging to master!

…t null THIS PR brings #29635 to branch-3.0 and targets v3.0.2 ### What changes were proposed in this pull request? bugfix for incomplete interval values, e.g. interval '1', interval '1 day 2', currently these cases will result null, but actually we should fail them with IllegalArgumentsException ### Why are the changes needed? correctness ### Does this PR introduce _any_ user-facing change? yes, incomplete intervals will throw exception now #### before ``` bin/spark-sql -S -e "select interval '1', interval '+', interval '1 day -'" NULL NULL NULL ``` #### after ``` -- !query select interval '1' -- !query schema struct<> -- !query output org.apache.spark.sql.catalyst.parser.ParseException Cannot parse the INTERVAL value: 1(line 1, pos 7) == SQL == select interval '1' ``` ### How was this patch tested? unit tests added Closes #29658 from yaooqinn/SPARK-32785-30. Authored-by: Kent Yao <yaooqinn@hotmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

gatorsmile · 2020-10-18T23:04:20Z

docs/sql-migration-guide.md

@@ -40,6 +40,8 @@ license: |

  - In Spark 3.1, `path` option cannot coexist when the following methods are called with path parameter(s): `DataFrameReader.load()`, `DataFrameWriter.save()`, `DataStreamReader.load()`, or `DataStreamWriter.start()`. In addition, `paths` option cannot coexist for `DataFrameReader.load()`. For example, `spark.read.format("csv").option("path", "/tmp").load("/tmp2")` or `spark.read.option("path", "/tmp").csv("/tmp2")` will throw `org.apache.spark.sql.AnalysisException`. In Spark version 3.0 and below, `path` option is overwritten if one path parameter is passed to above methods; `path` option is added to the overall paths if multiple path parameters are passed to `DataFrameReader.load()`. To restore the behavior before Spark 3.1, you can set `spark.sql.legacy.pathOptionBehavior.enabled` to `true`.

+  - In Spark 3.1, incomplete interval literals, e.g. `INTERVAL '1'`, `INTERVAL '1 DAY 2'` will fail with IllegalArgumentException. In Spark 3.0, they result `NULL`s.


In Spark 3.1, incomplete interval literals, e.g. INTERVAL '1', INTERVAL '1 DAY 2' will fail with IllegalArgumentException. In Spark 3.0, they result NULLs.

=>

In Spark 3.1, IllegalArgumentException is returned for the incomplete interval literals, e.g. INTERVAL '1', INTERVAL '1 DAY 2', which are invalid. In Spark 3.0, these literals result in NULLs.

Could you fix it in followup, @yaooqinn ?

OK, I will raise a PR today~

…lete interval literals ### What changes were proposed in this pull request? Address comments #29635 (comment) to improve migration guide ### Why are the changes needed? improve migration guide ### Does this PR introduce _any_ user-facing change? NO，only doc update ### How was this patch tested? passing GitHub action Closes #30113 from yaooqinn/SPARK-32785-F. Authored-by: Kent Yao <yaooqinn@hotmail.com> Signed-off-by: Takeshi Yamamuro <yamamuro@apache.org>

…complete interval literals ### What changes were proposed in this pull request? Address comments #29635 (comment) to improve migration guide ### Why are the changes needed? improve migration guide ### Does this PR introduce _any_ user-facing change? NO，only doc update ### How was this patch tested? passing GitHub action Closes #30117 from yaooqinn/SPARK-32785-F30. Authored-by: Kent Yao <yaooqinn@hotmail.com> Signed-off-by: Takeshi Yamamuro <yamamuro@apache.org>

…t null THIS PR brings apache#29635 to branch-3.0 and targets v3.0.2 ### What changes were proposed in this pull request? bugfix for incomplete interval values, e.g. interval '1', interval '1 day 2', currently these cases will result null, but actually we should fail them with IllegalArgumentsException ### Why are the changes needed? correctness ### Does this PR introduce _any_ user-facing change? yes, incomplete intervals will throw exception now #### before ``` bin/spark-sql -S -e "select interval '1', interval '+', interval '1 day -'" NULL NULL NULL ``` #### after ``` -- !query select interval '1' -- !query schema struct<> -- !query output org.apache.spark.sql.catalyst.parser.ParseException Cannot parse the INTERVAL value: 1(line 1, pos 7) == SQL == select interval '1' ``` ### How was this patch tested? unit tests added Closes apache#29658 from yaooqinn/SPARK-32785-30. Authored-by: Kent Yao <yaooqinn@hotmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

…complete interval literals ### What changes were proposed in this pull request? Address comments apache#29635 (comment) to improve migration guide ### Why are the changes needed? improve migration guide ### Does this PR introduce _any_ user-facing change? NO，only doc update ### How was this patch tested? passing GitHub action Closes apache#30117 from yaooqinn/SPARK-32785-F30. Authored-by: Kent Yao <yaooqinn@hotmail.com> Signed-off-by: Takeshi Yamamuro <yamamuro@apache.org>

[SPARK-32785][SQL] Interval with dangling parts should not results null

6d27597

probot-autolabeler bot added the SQL label Sep 3, 2020

nit

eb99bd5

cloud-fan reviewed Sep 3, 2020

View reviewed changes

address comment

ae6d9be

cloud-fan approved these changes Sep 3, 2020

View reviewed changes

m g

10f10ce

probot-autolabeler bot added the DOCS label Sep 4, 2020

m g

daed4e6

cloud-fan closed this in de44e9c Sep 7, 2020

yaooqinn mentioned this pull request Sep 7, 2020

[SPARK-32785][SQL][3.0] Interval with dangling parts should not result null #29658

Closed

gatorsmile reviewed Oct 18, 2020

View reviewed changes

yaooqinn mentioned this pull request Oct 21, 2020

[SPARK-32785][SQL][DOCS][FOLLOWUP] Update migaration guide for incomplete interval literals #30113

Closed

yaooqinn mentioned this pull request Oct 21, 2020

[SPARK-32785][SQL][DOCS][FOLLOWUP][3.0] Update migration guide for incomplete interval literals #30117

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-32785][SQL] Interval with dangling parts should not results null #29635

[SPARK-32785][SQL] Interval with dangling parts should not results null #29635

yaooqinn commented Sep 3, 2020

yaooqinn commented Sep 3, 2020

SparkQA commented Sep 3, 2020

cloud-fan Sep 3, 2020

cloud-fan Sep 3, 2020 •

edited

Loading

yaooqinn Sep 3, 2020

SparkQA commented Sep 3, 2020

SparkQA commented Sep 3, 2020

cloud-fan commented Sep 4, 2020

yaooqinn commented Sep 4, 2020

cloud-fan commented Sep 4, 2020

SparkQA commented Sep 4, 2020

cloud-fan commented Sep 7, 2020

gatorsmile Oct 18, 2020

maropu Oct 21, 2020

yaooqinn Oct 21, 2020

		@@ -40,6 +40,8 @@ license: \|

		- In Spark 3.1, `path` option cannot coexist when the following methods are called with path parameter(s): `DataFrameReader.load()`, `DataFrameWriter.save()`, `DataStreamReader.load()`, or `DataStreamWriter.start()`. In addition, `paths` option cannot coexist for `DataFrameReader.load()`. For example, `spark.read.format("csv").option("path", "/tmp").load("/tmp2")` or `spark.read.option("path", "/tmp").csv("/tmp2")` will throw `org.apache.spark.sql.AnalysisException`. In Spark version 3.0 and below, `path` option is overwritten if one path parameter is passed to above methods; `path` option is added to the overall paths if multiple path parameters are passed to `DataFrameReader.load()`. To restore the behavior before Spark 3.1, you can set `spark.sql.legacy.pathOptionBehavior.enabled` to `true`.

		- In Spark 3.1, incomplete interval literals, e.g. `INTERVAL '1'`, `INTERVAL '1 DAY 2'` will fail with IllegalArgumentException. In Spark 3.0, they result `NULL`s.

[SPARK-32785][SQL] Interval with dangling parts should not results null #29635

[SPARK-32785][SQL] Interval with dangling parts should not results null #29635

Conversation

yaooqinn commented Sep 3, 2020

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

before

after

How was this patch tested?

yaooqinn commented Sep 3, 2020

SparkQA commented Sep 3, 2020

cloud-fan Sep 3, 2020

Choose a reason for hiding this comment

cloud-fan Sep 3, 2020 • edited Loading

Choose a reason for hiding this comment

yaooqinn Sep 3, 2020

Choose a reason for hiding this comment

SparkQA commented Sep 3, 2020

SparkQA commented Sep 3, 2020

cloud-fan commented Sep 4, 2020

yaooqinn commented Sep 4, 2020

cloud-fan commented Sep 4, 2020

SparkQA commented Sep 4, 2020

cloud-fan commented Sep 7, 2020

gatorsmile Oct 18, 2020

Choose a reason for hiding this comment

maropu Oct 21, 2020

Choose a reason for hiding this comment

yaooqinn Oct 21, 2020

Choose a reason for hiding this comment

cloud-fan Sep 3, 2020 •

edited

Loading