-
Notifications
You must be signed in to change notification settings - Fork 28.3k
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-23905][SQL] Add UDF weekday #21009
Conversation
Jenkins, add to whitelist. |
Jenkins, test this please. |
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.
LGTM
Test build #89081 has finished for PR 21009 at commit
|
|
||
override def dataType: DataType = IntegerType | ||
|
||
@transient protected lazy val c: Calendar = { |
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.
nit: maybe calendar
instead of just c
.
@transient private lazy val c = { | ||
Calendar.getInstance(DateTimeUtils.getTimeZone("UTC")) | ||
} | ||
case class DayOfWeek(child: Expression) extends DayWeek { |
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.
Looks like DayOfWeek
is duplicate to WeekDay
, do we have plan to deprecate DayOfWeek
in maybe 3.0?
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.
They are not duplicate.
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.
They are different, see:
WeekDay: 0 = Monday, 1 = Tuesday, … 6 = Sunday
DayOfWeek: 1 = Sunday, 2 = Monday, ..., 7 = Saturday
Jenkins, retest this please |
Test build #89137 has finished for PR 21009 at commit
|
Test build #89138 has finished for PR 21009 at commit
|
I did the local testing, all pass. |
Jenkins, retest this please |
Test build #89161 has finished for PR 21009 at commit
|
Test build #89181 has finished for PR 21009 at commit
|
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.
change itself seems fine.
Test build #89229 has finished for PR 21009 at commit
|
retest this please |
Test build #89235 has finished for PR 21009 at commit
|
Test build #89242 has finished for PR 21009 at commit
|
Thanks! Merged to master. |
What changes were proposed in this pull request?
Add UDF weekday
How was this patch tested?
A new test