-
Notifications
You must be signed in to change notification settings - Fork 28.2k
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-8195] [SPARK-8196] [SQL] udf next_day last_day #6986
Conversation
Test build #35677 has finished for PR 6986 at commit
|
Close it because #6782 (comment) for now |
Test build #38195 has finished for PR 6986 at commit
|
Test build #38197 has finished for PR 6986 at commit
|
Test build #38203 has finished for PR 6986 at commit
|
Test build #38382 has finished for PR 6986 at commit
|
/** | ||
* Returns the last day of the month which the date belongs to. | ||
*/ | ||
case class LastDay(startDate: Expression) extends UnaryExpression with ImplicitCastInputTypes { |
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.
add a prettyName last_day
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
Test build #38533 has finished for PR 6986 at commit
|
@@ -246,4 +247,19 @@ class DateExpressionsSuite extends SparkFunSuite with ExpressionEvalHelper { | |||
} | |||
} | |||
|
|||
test("last_day") { | |||
checkEvaluation(LastDay(Literal(Date.valueOf("2015-07-23"))), Date.valueOf("2015-07-31")) |
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 please add more unit tests?
you should test one for each month, and also test the leap years.
@adrian-wang I think this one is pretty close - we need to add more user facing documentation and internal comments, as well as unit tests. Thanks. |
* @group datetime_funcs | ||
* @since 1.5.0 | ||
*/ | ||
def next_day(sd: Column, dayOfWeek: Column): Column = NextDay(sd.expr, dayOfWeek.expr) |
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 think it is ok to not support that in the data frame api, since it is very unlikely somebody will be using a non literal.
Can you address these two issues? |
@@ -2392,6 +2399,16 @@ object functions { | |||
def minute(columnName: String): Column = minute(Column(columnName)) | |||
|
|||
/** | |||
* Returns the first date which is later than given date sd and named as dow. | |||
* For example, next_day('2015-07-27', 'Sunday') would return 2015-08-02, which is the |
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.
quote next_day ... in backticks
done |
LGTM |
Test build #38634 has finished for PR 6986 at commit
|
Test build #38645 has finished for PR 6986 at commit
|
Test build #38627 has finished for PR 6986 at commit
|
Thanks - I've merged this. |
Seq(Row(Date.valueOf("2015-07-27")), Row(Date.valueOf("2015-07-27")))) | ||
checkAnswer( | ||
df2.select(next_day(col("t"), "th")), | ||
Seq(Row(Date.valueOf("2015-07-30")), Row(null))) |
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.
note that this actually fails unit tests. I thought it passed but it was from a previous commit.
Can you please pay a little bit more attention in the future and at least run your own unit tests locally? Thanks.
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.
Sorry for that...
next_day, returns next certain dayofweek.
last_day, returns the last day of the month which given date belongs to.