[SPARK-29663][SQL] Support sum with interval type values #26325

yaooqinn · 2019-10-30T16:53:31Z

What changes were proposed in this pull request?

sum support interval values

Why are the changes needed?

Part of SPARK-27764 Feature Parity between PostgreSQL and Spark

Does this PR introduce any user-facing change?

yes, sum can evaluate intervals

How was this patch tested?

add ut

SparkQA · 2019-10-30T18:53:39Z

Test build #112937 has finished for PR 26325 at commit a09ec61.

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

SparkQA · 2019-10-31T05:53:31Z

Test build #112981 has finished for PR 26325 at commit 9f1ce88.

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

yaooqinn · 2019-10-31T06:18:40Z

cc @cloud-fan @MaxGekk @dongjoon-hyun @wangyum

MaxGekk

Could you add an example with intervals to https://github.com/apache/spark/pull/26325/files#diff-84a56c8f7c77992f9f7294cea56734ceL30-L36

yaooqinn · 2019-10-31T07:16:32Z

@MaxGekk good advice, thanks.

SparkQA · 2019-10-31T10:02:46Z

Test build #113003 has finished for PR 26325 at commit 1ff5135.

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

SparkQA · 2019-10-31T14:58:10Z

Test build #113021 has finished for PR 26325 at commit e17dc68.

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

yaooqinn · 2019-11-01T06:07:51Z

retest this please

SparkQA · 2019-11-01T09:56:07Z

Test build #113074 has finished for PR 26325 at commit e17dc68.

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

yaooqinn · 2019-11-01T10:25:00Z

@MaxGekk please recheck thanks

yaooqinn · 2019-11-01T18:23:30Z

@MaxGekk Can we have this merged？ Or you may help cc some other reviewer. thank youvery much.

MaxGekk · 2019-11-01T19:05:17Z

@yaooqinn I don't have permissions to merge this. You need someone from the list https://spark.apache.org/committers.html

yaooqinn · 2019-11-04T06:37:51Z

cc @maropu @HyukjinKwon thanks

cloud-fan · 2019-11-04T09:30:22Z

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

@@ -0,0 +1,32 @@
+-- sum


shall we make it clear that this only test interval sum?

or you can add tests to group-by.sql

I can move them to group-by.sql

SparkQA · 2019-11-04T10:21:04Z

Test build #113205 has finished for PR 26325 at commit e9e4b59.

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

cloud-fan · 2019-11-04T10:25:55Z

sql/core/src/test/resources/sql-tests/inputs/group-by.sql

+-- empty set
+select sum(cast(v as interval)) from VALUES ('1 seconds'), ('2 seconds'), (null) t(v) where 1=0;
+
+--


basic interval sum

cloud-fan · 2019-11-04T10:26:08Z

sql/core/src/test/resources/sql-tests/inputs/group-by.sql

+select sum(cast(v as interval)) from VALUES ('-1 seconds'), ('-2 seconds'), (null) t(v);
+select sum(cast(v as interval)) from VALUES ('-1 weeks'), ('2 seconds'), (null) t(v);
+
+--group by


one space after --

cloud-fan · 2019-11-04T10:26:14Z

sql/core/src/test/resources/sql-tests/inputs/group-by.sql

+from VALUES (1, '-1 weeks'), (2, '2 seconds'), (3, null), (1, '5 days') t(i, v)
+group by i;
+
+--having


updated with all above comments, thank for reviewing

SparkQA · 2019-11-04T11:40:37Z

Test build #113207 has finished for PR 26325 at commit 5415a85.

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

SparkQA · 2019-11-04T15:36:02Z

Test build #113209 has finished for PR 26325 at commit b3c975f.

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

cloud-fan · 2019-11-04T17:08:06Z

thanks, merging to master!

### What changes were proposed in this pull request? This PR reverts #26325 and #26347 ### Why are the changes needed? When we do sum/avg, we need a wider type of input to hold the sum value, to reduce the possibility of overflow. For example, we use long to hold the sum of integral inputs, use double to hold the sum of float/double. However, we don't have a wider type of interval. Also the semantic is unclear: what if the days field overflows but the months field doesn't? Currently the avg of `1 month` and `2 month` is `1 month 15 days`, which assumes 1 month has 30 days and we should avoid this assumption. ### Does this PR introduce any user-facing change? yes, remove 2 features added in 3.0 ### How was this patch tested? N/A Closes #27619 from cloud-fan/revert. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: herman <[email protected]>

### What changes were proposed in this pull request? This PR reverts #26325 and #26347 ### Why are the changes needed? When we do sum/avg, we need a wider type of input to hold the sum value, to reduce the possibility of overflow. For example, we use long to hold the sum of integral inputs, use double to hold the sum of float/double. However, we don't have a wider type of interval. Also the semantic is unclear: what if the days field overflows but the months field doesn't? Currently the avg of `1 month` and `2 month` is `1 month 15 days`, which assumes 1 month has 30 days and we should avoid this assumption. ### Does this PR introduce any user-facing change? yes, remove 2 features added in 3.0 ### How was this patch tested? N/A Closes #27619 from cloud-fan/revert. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: herman <[email protected]> (cherry picked from commit 1b67d54) Signed-off-by: herman <[email protected]>

### What changes were proposed in this pull request? This PR reverts apache#26325 and apache#26347 ### Why are the changes needed? When we do sum/avg, we need a wider type of input to hold the sum value, to reduce the possibility of overflow. For example, we use long to hold the sum of integral inputs, use double to hold the sum of float/double. However, we don't have a wider type of interval. Also the semantic is unclear: what if the days field overflows but the months field doesn't? Currently the avg of `1 month` and `2 month` is `1 month 15 days`, which assumes 1 month has 30 days and we should avoid this assumption. ### Does this PR introduce any user-facing change? yes, remove 2 features added in 3.0 ### How was this patch tested? N/A Closes apache#27619 from cloud-fan/revert. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: herman <[email protected]>

dongjoon-hyun added the SQL label Oct 30, 2019

MaxGekk reviewed Oct 31, 2019

View reviewed changes

MaxGekk approved these changes Nov 1, 2019

View reviewed changes

cloud-fan reviewed Nov 4, 2019

View reviewed changes

yaooqinn added 7 commits November 4, 2019 19:45

[SPARK-29663][SQL] Support sum with interval type values

fc9a75c

fix ut

7603817

add an example for doc

efb4a0d

missing colon

4b9f517

mv tests

b9893e7

style

ac112d3

rebase and regen golden file

b3c975f

maropu approved these changes Nov 4, 2019

View reviewed changes

cloud-fan closed this in 44b8fbc Nov 4, 2019

yaooqinn deleted the SPARK-29663 branch November 5, 2019 00:51

cloud-fan mentioned this pull request Feb 18, 2020

revert SPARK-29663 and SPARK-29688 #27619

Closed

MaxGekk mentioned this pull request Apr 11, 2021

[SPARK-34716][SQL] Support ANSI SQL intervals by the aggregate function sum #32107

Closed

[SPARK-29663][SQL] Support sum with interval type values #26325

[SPARK-29663][SQL] Support sum with interval type values #26325

Uh oh!

Conversation

yaooqinn commented Oct 30, 2019

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 Oct 30, 2019

Uh oh!

SparkQA commented Oct 31, 2019

Uh oh!

yaooqinn commented Oct 31, 2019

Uh oh!

MaxGekk left a comment

Choose a reason for hiding this comment

Uh oh!

yaooqinn commented Oct 31, 2019

Uh oh!

SparkQA commented Oct 31, 2019

Uh oh!

SparkQA commented Oct 31, 2019

Uh oh!

yaooqinn commented Nov 1, 2019

Uh oh!

SparkQA commented Nov 1, 2019

Uh oh!

yaooqinn commented Nov 1, 2019

Uh oh!

yaooqinn commented Nov 1, 2019

Uh oh!

MaxGekk commented Nov 1, 2019

Uh oh!

yaooqinn commented Nov 4, 2019

Uh oh!

cloud-fan Nov 4, 2019

Choose a reason for hiding this comment

Uh oh!

cloud-fan Nov 4, 2019

Choose a reason for hiding this comment

Uh oh!

yaooqinn Nov 4, 2019

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 4, 2019

Uh oh!

cloud-fan Nov 4, 2019

Choose a reason for hiding this comment

Uh oh!

cloud-fan Nov 4, 2019

Choose a reason for hiding this comment

Uh oh!

cloud-fan Nov 4, 2019

Choose a reason for hiding this comment

Uh oh!

yaooqinn Nov 4, 2019

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 4, 2019

Uh oh!

SparkQA commented Nov 4, 2019

Uh oh!

cloud-fan commented Nov 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants