[SPARK-29688][SQL] Support average for interval type values #26347

yaooqinn · 2019-10-31T14:17:41Z

What changes were proposed in this pull request?

avg aggregate support interval type 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, we can do avg on intervals

How was this patch tested?

add ut

SparkQA · 2019-10-31T16:31:52Z

Test build #113031 has finished for PR 26347 at commit a62d9db.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class IntervalDivide(left: Expression, right: Expression)

SparkQA · 2019-11-01T05:59:50Z

Test build #113058 has finished for PR 26347 at commit 91792ea.

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

yaooqinn · 2019-11-01T06:03:44Z

cc @cloud-fan @wangyum @gatorsmile

SparkQA · 2019-11-04T11:15:18Z

Test build #113206 has finished for PR 26347 at commit 0102f37.

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

maropu · 2019-11-04T22:53:25Z

also cc: @MaxGekk

maropu · 2019-11-04T22:53:49Z

@yaooqinn Can you fix the build failure first?

yaooqinn · 2019-11-05T02:16:41Z

@maropu thanks for your review, I have had it rebased with newest master branch, thanks. I appreciate for your time to take another look.

maropu · 2019-11-05T02:35:43Z

btw, this pr includes SPARK-29387, too? If so, how about fixing more basic operators (div and mul) for intervals then implementing avg in follow-up?

yaooqinn · 2019-11-05T02:40:59Z

Yes, avg needs interval divide feature support.

yaooqinn · 2019-11-05T03:51:21Z

#26345 I proposed this to support multiply and divide without noticing spark-29378，@maropu，intended to make these step by step

maropu · 2019-11-05T04:30:19Z

I will revisit this after #26132 merged.

SparkQA · 2019-11-05T06:02:19Z

Test build #113232 has finished for PR 26347 at commit 3c76add.

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

MaxGekk · 2019-11-05T08:00:51Z

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

+      Decimal(interval.days) / divisor
+    val milliseconds = days.remainder(Decimal.ONE) * Decimal(DateTimeUtils.MICROS_PER_DAY) +
+      Decimal(interval.microseconds) / divisor
+    new CalendarInterval(months.toInt, days.toInt, milliseconds.toLong)


milliseconds?

microseconds. thanks.

this won't be needed when your pr will be merged. I'd like to leave it for a while.

SparkQA · 2019-11-07T13:37:03Z

Test build #113385 has finished for PR 26347 at commit 161d742.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
public class DateTimeConstants
case class LocalShuffleReaderExec(child: SparkPlan) extends UnaryExecNode
class ContinuousRecordEndpoint(buckets: Seq[Seq[UnsafeRow]], lock: Object)

SparkQA · 2019-11-07T16:07:01Z

Test build #113386 has finished for PR 26347 at commit 6ac44e4.

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

yaooqinn · 2019-11-08T03:31:30Z

ping @maropu @MaxGekk @cloud-fan, this have been rebased with lastest interval divide behavior change, thanks very much for review one more time, sorry for the delay.

cloud-fan · 2019-11-08T04:10:14Z

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

+
+-- average with interval type
+-- null
+select avg(cast(v as interval)) from VALUES ('1 seconds'), ('2 seconds'), (null) t(v) where v is null;


is it the same with select avg(cast(v as interval)) from VALUES (null) t(v)?

yes, I changed it in your way. thanks.

SparkQA · 2019-11-08T06:47:31Z

Test build #113425 has finished for PR 26347 at commit 3443c18.

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

maropu · 2019-11-08T07:34:27Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Average.scala

  private lazy val resultType = child.dataType match {
    case DecimalType.Fixed(p, s) =>
      DecimalType.bounded(p + 4, s + 4)
+    case CalendarIntervalType => CalendarIntervalType


nit: case interval: CalendarIntervalType => interval?

ok, I will change this and the one in the sumDataType

yaooqinn · 2019-11-08T08:23:31Z

@cloud-fan @maropu thanks for your review, I will update with the minor revision in a minute. Sorry for the delay, focused on the spark unnest implementation just now

SparkQA · 2019-11-08T12:39:07Z

Test build #113452 has finished for PR 26347 at commit 2e06305.

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

cloud-fan · 2019-11-08T13:55:19Z

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 Nov 3, 2019

yaooqinn mentioned this pull request Nov 5, 2019

[SPARK-29387][SQL] Support * and / operators for intervals #26132

Closed

MaxGekk reviewed Nov 5, 2019

View reviewed changes

yaooqinn added 8 commits November 6, 2019 14:20

[SPARK-29688][SQL] Support average for interval type values

8ae0ced

fix ut

20c094d

style

f1e197b

rebase, mv tests to groupby.sql

3726762

fix interval divide example's result

2ba835d

rebase

998bd95

Merge branch 'master' into SPARK-29688

161d742

rm useless test and regen golden file

6ac44e4

fix example result

3443c18

cloud-fan reviewed Nov 8, 2019

View reviewed changes

maropu reviewed Nov 8, 2019

View reviewed changes

maropu approved these changes Nov 8, 2019

View reviewed changes

address comments

2e06305

cloud-fan closed this in e7f7990 Nov 8, 2019

cloud-fan mentioned this pull request Feb 18, 2020

revert SPARK-29663 and SPARK-29688 #27619

Closed

[SPARK-29688][SQL] Support average for interval type values #26347

[SPARK-29688][SQL] Support average for interval type values #26347

Uh oh!

Conversation

yaooqinn commented Oct 31, 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 31, 2019

Uh oh!

SparkQA commented Nov 1, 2019

Uh oh!

yaooqinn commented Nov 1, 2019

Uh oh!

SparkQA commented Nov 4, 2019

Uh oh!

maropu commented Nov 4, 2019

Uh oh!

maropu commented Nov 4, 2019

Uh oh!

yaooqinn commented Nov 5, 2019

Uh oh!

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

Uh oh!

Uh oh!

yaooqinn commented Nov 5, 2019

Uh oh!

yaooqinn commented Nov 5, 2019

Uh oh!

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

Uh oh!

Uh oh!

SparkQA commented Nov 5, 2019

Uh oh!

MaxGekk Nov 5, 2019

Choose a reason for hiding this comment

Uh oh!

yaooqinn Nov 5, 2019

Choose a reason for hiding this comment

Uh oh!

yaooqinn Nov 5, 2019

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 7, 2019

Uh oh!

SparkQA commented Nov 7, 2019

Uh oh!

yaooqinn commented Nov 8, 2019

Uh oh!

cloud-fan Nov 8, 2019

Choose a reason for hiding this comment

Uh oh!

yaooqinn Nov 8, 2019

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 8, 2019

Uh oh!

maropu Nov 8, 2019

Choose a reason for hiding this comment

Uh oh!

yaooqinn Nov 8, 2019

Choose a reason for hiding this comment

Uh oh!

yaooqinn commented Nov 8, 2019

Uh oh!

SparkQA commented Nov 8, 2019

Uh oh!

cloud-fan commented Nov 8, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

maropu commented Nov 5, 2019 •

edited

Loading

maropu commented Nov 5, 2019 •

edited

Loading