Spark dayname function implementation#20825
Spark dayname function implementation#20825kazantsev-maksim wants to merge 15 commits intoapache:mainfrom
Conversation
|
|
||
| query T | ||
| SELECT dayname('2008-02-20'::DATE); | ||
| ---- |
There was a problem hiding this comment.
Should we also verify that the data type matches the spark data types (Itf8 / LargeUtf8 / Utf8View)
| fn spark_day_name(days: i32) -> String { | ||
| let weekday = Date32Type::to_naive_date_opt(days).unwrap().weekday(); | ||
| let display_name = get_display_name(weekday.num_days_from_monday()); | ||
| display_name.unwrap() |
There was a problem hiding this comment.
potential panic in unwrap ?
There was a problem hiding this comment.
I tried to improve it
| } | ||
|
|
||
| fn spark_day_name(days: i32) -> String { | ||
| let weekday = Date32Type::to_naive_date_opt(days).unwrap().weekday(); |
There was a problem hiding this comment.
here as well ? Could we guarantee that unwrap is never going to panic ?
There was a problem hiding this comment.
I tried to improve it
|
|
||
| query T | ||
| SELECT dayname('2008-02-20'::DATE); | ||
| ---- |
There was a problem hiding this comment.
may be we could also add empty strings test cases as well ?
|
Test failures : |
|
Thanks for the review @coderfender. Could you please another see, when you have a time? |
coderfender
left a comment
coderfender
left a comment
There was a problem hiding this comment.
I think the changes are looking good @kazantsev-maksim . Left a couple of questions for further review . Thank you very much for your patience :)
|
|
||
| query T | ||
| SELECT dayname('2010-04-24'::TIMESTAMP); | ||
| ---- |
There was a problem hiding this comment.
Could we also tests to add actual timestamps instead of dates parsed as timestamps here ?
|
|
||
| query T | ||
| SELECT dayname(NULL::DATE); | ||
| ---- |
There was a problem hiding this comment.
We might also want to check NULL cases for timestamps here as well ?
| 4 => Some(String::from("Fri")), | ||
| 5 => Some(String::from("Sat")), | ||
| 6 => Some(String::from("Sun")), | ||
| _ => None, |
There was a problem hiding this comment.
nit : we might also want to add a comment saying that the default local is english @kazantsev-maksim (similar to spark)
| DataType::Date32 | DataType::Timestamp(_, _) => spark_day_name_inner(array), | ||
| DataType::Utf8 | DataType::Utf8View | DataType::LargeUtf8 => { | ||
| let date_array = | ||
| cast_with_options(array, &DataType::Date32, &CastOptions::default())?; |
There was a problem hiding this comment.
What would happen in case of an invalid string / error here ? Could probably handle it here and throw an error if it is a malformed input ?
| 6 => Some(String::from("Sun")), | ||
| _ => None, | ||
| } | ||
| } |
There was a problem hiding this comment.
I dont think we are handling timezone's support here ? Spark handles timezone in current implementation and this could produce wrong results
| 6 => Some(String::from("Sun")), | ||
| _ => None, | ||
| } | ||
| } |
There was a problem hiding this comment.
nit : could we also add unit tests apart from the SQL tests to test out method signatures , invalid inputs , error handling , timezone support etc ?
| TypeSignatureClass::Timestamp, | ||
| )]), | ||
| TypeSignature::Coercible(vec
Which issue does this PR close?
N/A
Rationale for this change
Add new spark function: https://spark.apache.org/docs/latest/api/sql/index.html#dayname
What changes are included in this PR?
Are these changes tested?
Yes, tests added as part of this PR.
Are there any user-facing changes?
No, these are new function.