[SPARK-36445][SQL] ANSI type coercion rule for date time operations by gengliangwang · Pull Request #33666 · apache/spark

gengliangwang · 2021-08-06T09:15:12Z

What changes were proposed in this pull request?

Implement a new rule for the date-time operations in the ANSI type coercion system:

Date will be converted to Timestamp when it is in the subtraction with Timestmap.
Promote string literals in date_add/date_sub/time_add

Why are the changes needed?

Currently the type coercion rule DateTimeOperations doesn't match the design of the ANSI type coercion system:

For date_add/date_sub, if the input is timestamp type, Spark should not convert it into date type since date type is narrower than the timestamp type.
For date_add/date_sub/time_add, string value can be implicit cast to date/timestamp only when it is literal.

Thus, we need to have a new rule for the date-time operations in the ANSI type coercion system.

Does this PR introduce any user-facing change?

No, the ANSI type coercion rules are not releaesd.

How was this patch tested?

New UT

SparkQA · 2021-08-06T09:52:45Z

Test build #142155 has finished for PR 33666 at commit 21391c4.

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

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/AnsiTypeCoercion.scala

cloud-fan · 2021-08-06T14:40:55Z

sql/core/src/test/resources/sql-tests/inputs/date.sql

 select date_sub('2011-11-11', str) from v;

+-- non-literal string column add/sub with integer
+create or replace temp view v2 as select '2011-11-11' str;


nit: let's remove or replace. It's always error-prone if we replace a temp view in the test, as we may change the rest of the test cases.

cloud-fan · 2021-08-06T14:41:16Z

sql/core/src/test/resources/sql-tests/inputs/timestamp.sql

 select '2011-11-11 11:11:11' - interval '2' second;
 select '1' - interval '2' second;
 select 1 - interval '2' second;
+create or replace temp view v as select '2011-11-11' str;


sql/core/src/test/resources/sql-tests/inputs/timestamp.sql

gengliangwang · 2021-08-06T15:27:19Z

sql/core/src/test/resources/sql-tests/inputs/timestamp.sql

 select '2011-11-11 11:11:11' - interval '2' second;
 select '1' - interval '2' second;
 select 1 - interval '2' second;
+select '2011-11-11 11:11:11' - date'2011-11-11';


how about string - date/timestamp?

I have added a new test case. But it seems that string - date will be cast(string as date) - date. Should we move it to date.sql or just leave it here?

let's put it in date.sql, as we don't need to test this query 4 times with different timestamp settings.

SparkQA · 2021-08-06T17:00:54Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/46674/

SparkQA · 2021-08-06T17:19:30Z

Test build #142160 has finished for PR 33666 at commit 653d0c6.

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

SparkQA · 2021-08-06T17:56:11Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/46674/

SparkQA · 2021-08-06T18:54:07Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/46681/

SparkQA · 2021-08-06T19:11:35Z

Kubernetes integration test unable to build dist.

exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/46668/

SparkQA · 2021-08-06T19:48:23Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/46681/

SparkQA · 2021-08-06T20:50:42Z

Test build #142168 has finished for PR 33666 at commit 17470db.

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

cloud-fan · 2021-08-09T09:21:45Z

sql/core/src/test/resources/sql-tests/results/ansi/timestamp.sql.out

+struct<>
+-- !query output
+org.apache.spark.sql.AnalysisException
+cannot resolve '('2011-11-11 11:11:11' - TIMESTAMP '2011-11-11 11:11:10')' due to data type mismatch: argument 1 requires (timestamp or timestamp without time zone) type, however, ''2011-11-11 11:11:11'' is of string type.; line 1 pos 7


why does this fail under ansi mode?

We don't have a rule for this one

shall we fix it under ansi mode? this is inconsistent: string_literal - date works but string_literal - timestamp does not.

cloud-fan · 2021-08-09T09:24:18Z

sql/core/src/test/resources/sql-tests/results/datetime-legacy.sql.out

+struct<>
+-- !query output
+org.apache.spark.sql.catalyst.analysis.TempTableAlreadyExistsException
+Temporary view 'v' already exists


seems we need to change the temp view name

sql/core/src/test/resources/sql-tests/results/timestamp.sql.out

SparkQA · 2021-08-09T15:21:57Z

Test build #142230 has started for PR 33666 at commit d8e468f.

SparkQA · 2021-08-09T16:14:14Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/46741/

SparkQA · 2021-08-09T17:13:32Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/46741/

SparkQA · 2021-08-10T07:27:04Z

Test build #142261 has finished for PR 33666 at commit eb0b080.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

gengliangwang · 2021-08-10T07:35:14Z

jenkins, retest this please

cloud-fan · 2021-08-10T07:48:02Z

sql/core/src/test/resources/sql-tests/inputs/date.sql

 select null - date '2019-10-06';
 select date '2001-10-01' - date '2001-09-28';
+select '2011-11-11 11:11:11' - date'2011-11-11';



nit: we can put select str - date'2011-11-11' from v2; here.

cloud-fan · 2021-08-10T07:50:27Z

sql/core/src/test/resources/sql-tests/results/date.sql.out

+
+
+-- !query
+select date_add(timestamp_ntz'2011-11-11 12:12:12', 1)


if timestamp_ntz becomes the default in the future, it's better to support this query as well to not break anything.

The LTZ one will fail in ANSI mode. We tend to make the NTZ behavior consistent in default/ANSI mode, right?

It's a different topic. Breaking change means it's very hard to enable NTZ by default in the future.

I think we tend to make LTZ/NTZ behavior consistent within default mode, or within ANSI mode, but not between default and ANSI mode.

I will have a follow-up for NTZ behavior.

SparkQA · 2021-08-10T08:03:45Z

Kubernetes integration test unable to build dist.

exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/46768/

SparkQA · 2021-08-10T08:54:10Z

Kubernetes integration test unable to build dist.

exiting with code: 1
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/46770/

SparkQA · 2021-08-10T10:56:49Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/46778/

SparkQA · 2021-08-10T11:51:11Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/46778/

SparkQA · 2021-08-10T13:06:31Z

Test build #142262 has finished for PR 33666 at commit eb0b080.

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

SparkQA · 2021-08-10T13:53:55Z

Test build #142270 has finished for PR 33666 at commit 0e7ce1d.

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

gengliangwang · 2021-08-11T03:54:40Z

Merging to master/3.2

### What changes were proposed in this pull request? Implement a new rule for the date-time operations in the ANSI type coercion system: 1. Date will be converted to Timestamp when it is in the subtraction with Timestmap. 2. Promote string literals in date_add/date_sub/time_add ### Why are the changes needed? Currently the type coercion rule `DateTimeOperations` doesn't match the design of the ANSI type coercion system: 1. For date_add/date_sub, if the input is timestamp type, Spark should not convert it into date type since date type is narrower than the timestamp type. 2. For date_add/date_sub/time_add, string value can be implicit cast to date/timestamp only when it is literal. Thus, we need to have a new rule for the date-time operations in the ANSI type coercion system. ### Does this PR introduce _any_ user-facing change? No, the ANSI type coercion rules are not releaesd. ### How was this patch tested? New UT Closes #33666 from gengliangwang/datetimeOp. Authored-by: Gengliang Wang <gengliang@apache.org> Signed-off-by: Gengliang Wang <gengliang@apache.org> (cherry picked from commit 3029e62) Signed-off-by: Gengliang Wang <gengliang@apache.org>

gengliangwang added 2 commits August 6, 2021 15:51

ansi datetime operation

ef96875

add tests

21391c4

gengliangwang requested a review from cloud-fan August 6, 2021 09:15

github-actions bot added the SQL label Aug 6, 2021

fix

653d0c6

cloud-fan reviewed Aug 6, 2021

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/AnsiTypeCoercion.scala Show resolved Hide resolved

cloud-fan reviewed Aug 6, 2021

View reviewed changes

sql/core/src/test/resources/sql-tests/inputs/timestamp.sql Outdated Show resolved Hide resolved

address comment

d9b1f3f

gengliangwang commented Aug 6, 2021

View reviewed changes

update comments

17470db

cloud-fan reviewed Aug 9, 2021

View reviewed changes

sql/core/src/test/resources/sql-tests/results/timestamp.sql.out Show resolved Hide resolved

address comments

d8e468f

allow str - timestamp

eb0b080

cloud-fan reviewed Aug 10, 2021

View reviewed changes

cloud-fan approved these changes Aug 10, 2021

View reviewed changes

add one case

0e7ce1d

gengliangwang closed this in 3029e62 Aug 11, 2021



		-- !query
		select date_add(timestamp_ntz'2011-11-11 12:12:12', 1)

Conversation

gengliangwang commented Aug 6, 2021

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!

SparkQA commented Aug 6, 2021

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 6, 2021

Uh oh!

SparkQA commented Aug 6, 2021

Uh oh!

SparkQA commented Aug 6, 2021

Uh oh!

SparkQA commented Aug 6, 2021

Uh oh!

SparkQA commented Aug 6, 2021

Uh oh!

SparkQA commented Aug 6, 2021

Uh oh!

SparkQA commented Aug 6, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SparkQA commented Aug 9, 2021

Uh oh!

SparkQA commented Aug 9, 2021

Uh oh!

SparkQA commented Aug 9, 2021

Uh oh!

SparkQA commented Aug 10, 2021

Uh oh!

gengliangwang commented Aug 10, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 10, 2021

Uh oh!

SparkQA commented Aug 10, 2021

Uh oh!

SparkQA commented Aug 10, 2021

Uh oh!

SparkQA commented Aug 10, 2021

Uh oh!

SparkQA commented Aug 10, 2021

Uh oh!

SparkQA commented Aug 10, 2021

Uh oh!

gengliangwang commented Aug 11, 2021

Uh oh!