-
Notifications
You must be signed in to change notification settings - Fork 28.5k
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.
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
[SPARK-34677][SQL] Support the +
/-
operators over ANSI SQL intervals
#31789
Conversation
Kubernetes integration test starting |
Kubernetes integration test status success |
Test build #135908 has finished for PR 31789 at commit
|
@HyukjinKwon @cloud-fan Could you take a look at this PR, please. |
@@ -173,6 +180,11 @@ abstract class BinaryArithmetic extends BinaryOperator with NullIntolerant { | |||
def calendarIntervalMethod: String = | |||
sys.error("BinaryArithmetics must override either calendarIntervalMethod or genCode") | |||
|
|||
/** Name of the function for this expression on [[DayTimeIntervalType]] and | |||
* [[YearMonthIntervalType]] types. */ | |||
def intervalMethod: String = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we reuse exactMathMethod
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, since exactMathMethod
is always defined in the expressions, we can directly invoke exactMathMethod.get
. Though I can add a guard if exactMathMethod.isDefined
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can use assert
to make sure it's defined for interval types.
Kubernetes integration test starting |
Kubernetes integration test status failure |
@cloud-fan @HyukjinKwon @yaooqinn Any more feedback for the changes? |
...yst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ArithmeticExpressionSuite.scala
Show resolved
Hide resolved
.toDF("year-month-A", "day-time-A", "year-month-B", "day-time-B") | ||
val negatedDF = df.select(-$"year-month-A", -$"day-time-A") | ||
checkAnswer(negatedDF, Row(Period.ofMonths(-10), Duration.ofDays(-10))) | ||
val sumDF = df.select($"year-month-A" + $"year-month-B", $"day-time-A" + $"day-time-B") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sumDF
-> addDF
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does sum
function accept the new interval types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does sum function accept the new interval types?
sum
operates over NumericType
only, see
spark/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/Sum.scala
Line 49 in 3a299aa
override def inputTypes: Seq[AbstractDataType] = Seq(NumericType) |
+
/-
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opened the JIRA SPARK-34716 to don't forget about it.
Kubernetes integration test starting |
Kubernetes integration test status failure |
thanks, merging to master! |
What changes were proposed in this pull request?
Extend the
Add
,Subtract
andUnaryMinus
expression to supportDayTimeIntervalType
andYearMonthIntervalType
added by #31614.Note: the expressions can throw the
overflow
exception independently from the SQL configspark.sql.ansi.enabled
. In this way, the modified expressions always behave in the ANSI mode for the intervals.Why are the changes needed?
To conform to the ANSI SQL standard which defines

-/+
over intervals:Does this PR introduce any user-facing change?
Should not since new types have not been released yet.
How was this patch tested?
By running new tests in the test suites: