-
Notifications
You must be signed in to change notification settings - Fork 28.1k
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-8186][SPARK-8187][SPARK-8194][SPARK-8198][SPARK-9133] [SPARK-9290] [SQL] functions: date_add, date_sub, add_months, months_between, time-interval calculation #7754
Conversation
Conflicts: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/util/DateTimeUtils.scala sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DateExpressionsSuite.scala sql/core/src/main/scala/org/apache/spark/sql/functions.scala sql/core/src/test/scala/org/apache/spark/sql/DateFunctionsSuite.scala
Test build #38865 has finished for PR 7754 at commit
|
Test build #38866 has finished for PR 7754 at commit
|
I'm going to look through this. You will need to rebase because of #7745 |
Add(right, left) // switch the order | ||
|
||
case Add(left, right) if right.dataType == IntervalType => | ||
Cast(TimeAdd(Cast(left, TimestampType), right), left.dataType) |
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.
this implicitly casts all types to TimestampType. How about requiring left to be string, date, or timestamp?
* Turns Add/Subtract of DateType/TimestampType/StringType and CalendarIntervalType | ||
* to TimeAdd/TimeSub | ||
*/ | ||
object DateTimeOperations extends Rule[LogicalPlan] { |
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.
can you add a unit test for this rule?
make sure there is one test case just for this rule, and another that tests DateTimeOperations and ImplicitTypeCasts in combination to make sure they still work. I worry if we change ImplicitTypeCasts in the future, DateTimeOperations might break (e.g. the interval got converted to string type)
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.
done, the combination of them will be covered by function tests.
Test build #38901 has finished for PR 7754 at commit
|
cc @yjshen to take a look at the date time function implementations too |
|
||
/** | ||
* Returns number of months between time1 and time2. time1 and time2 are expressed in | ||
* microseconds since 1.1.1970 |
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 should document that this returns an integer if it is the same day in two different month
Test build #38902 has finished for PR 7754 at commit
|
Test build #38909 has finished for PR 7754 at commit
|
Test build #38929 has finished for PR 7754 at commit
|
} else { | ||
dayOfMonth | ||
} | ||
firstDayOfMonth(absoluteMonth) + currentDayInMonth - 1 |
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.
getYear
,getMonth
, getDayOfMonth
will call getYearAndDayInYear
first, therefore 3 times in total. If performance is a consideration here, I think we could have a function (date) => (year, month, dayOfMonth)
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.
yes, we can do that later.
Test build #39001 has finished for PR 7754 at commit
|
127596c
to
237452b
Compare
Test build #39017 has finished for PR 7754 at commit
|
Test build #39011 has finished for PR 7754 at commit
|
Test build #39020 has finished for PR 7754 at commit
|
Test build #1239 has finished for PR 7754 at commit
|
Test build #39070 has finished for PR 7754 at commit
|
Conflicts: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/DateExpressionsSuite.scala
@rxin If no more objection, I will merge this once it pass the tests. |
LGTM |
Test build #39083 has finished for PR 7754 at commit
|
Test build #39084 has finished for PR 7754 at commit
|
This PR is based on #7589 , thanks to @adrian-wang
Added SQL function date_add, date_sub, add_months, month_between, also add a rule for
add/subtract of date/timestamp and interval.
Closes #7589
cc @rxin