Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
beliefer wants to merge 9 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d1373fb
Support ANSI SQL intervals by the aggregate function `sum`
beliefer Apr 9, 2021
7897af4
Update sql/core/src/test/scala/org/apache/spark/sql/DataFrameAggregat…
HyukjinKwon Apr 11, 2021
75f3836
Add tests
beliefer Apr 13, 2021
a5bf107
Merge branch 'SPARK-34716' of github.com:beliefer/spark into SPARK-34716
beliefer Apr 13, 2021
9b8c461
Add tests
beliefer Apr 13, 2021
a4d1214
extend BufferSetterGetterUtils
beliefer Apr 13, 2021
18c17ff
test
beliefer Apr 14, 2021
01effb8
Update code
beliefer Apr 15, 2021
d022492
Merge branch 'master' into SPARK-34716
beliefer Apr 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Sum.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,15 +46,20 @@ case class Sum(child: Expression) extends DeclarativeAggregate with ImplicitCast
// Return data type.
override def dataType: DataType = resultType

override def inputTypes: Seq[AbstractDataType] = Seq(NumericType)
override def inputTypes: Seq[AbstractDataType] =
Seq(TypeCollection(NumericType, YearMonthIntervalType, DayTimeIntervalType))

override def checkInputDataTypes(): TypeCheckResult =
TypeUtils.checkForNumericExpr(child.dataType, "function sum")
override def checkInputDataTypes(): TypeCheckResult = child.dataType match {
case YearMonthIntervalType | DayTimeIntervalType => TypeCheckResult.TypeCheckSuccess
case _ => TypeUtils.checkForNumericExpr(child.dataType, "function sum")
}

private lazy val resultType = child.dataType match {
case DecimalType.Fixed(precision, scale) =>
DecimalType.bounded(precision + 10, scale)
case _: IntegralType => LongType
case _: YearMonthIntervalType => YearMonthIntervalType
case _: DayTimeIntervalType => DayTimeIntervalType
case _ => DoubleType
}

Expand Down
10 changes: 10 additions & 0 deletions sql/core/src/test/scala/org/apache/spark/sql/DataFrameAggregateSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@

package org.apache.spark.sql

import java.time.{Duration, Period}

import scala.util.Random

import org.scalatest.matchers.must.Matchers.the
Expand Down Expand Up @@ -1110,6 +1112,14 @@ class DataFrameAggregateSuite extends QueryTest
val e = intercept[AnalysisException](arrayDF.groupBy(struct($"col.a")).count())
assert(e.message.contains("requires integral type"))
}

test("SPARK-34716: Support ANSI SQL intervals by the aggregate function `sum`") {
Copy link
Member

@MaxGekk MaxGekk Apr 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you test more cases:

  1. Negative tests such as overflow
  2. Aggregate nulls and non-nulls
  3. Looking at [SPARK-29663][SQL] Support sum with interval type values #26325, we will need to add more tests as soon as we support construction of the intervals in SQL via cast or make_interval

Copy link
Contributor Author

@beliefer beliefer Apr 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the third suggestion, If we support intervals in SQL, we will add new test cases.

val df = Seq((Period.ofMonths(10), Duration.ofDays(10)),
(Period.ofMonths(1), Duration.ofDays(1)))
.toDF("year-month", "day-time")
val sumDF = df.select(sum($"year-month"), sum($"day-time"))
checkAnswer(sumDF, Row(Period.ofMonths(11), Duration.ofDays(11)))
Copy link
Member

@MaxGekk MaxGekk Apr 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You modified the resultType but don't check the type in schema. Could you add such check, please.

Copy link
Contributor Author

@beliefer beliefer Apr 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

}
}

case class B(c: Option[Double])
Expand Down