feat(compute): Support week0 (PostgreSQL behaviour) for temporal #2052
Conversation
ovr
changed the title
There was a problem hiding this comment.
I did some research and implementations seem to be wildly inconsistent on whether the start of the week is Sunday or Monday, and whether the returned value start at 0 or 1.
postgres extract(dow ...) and datepart is 0-6 starting on Sunday
mysql weekday is 1-7 starting on Monday
SQL server lets you set the start date to anything, although its datepart starts at 1
I don't know if @alamb has any source on what the "correct" behaviour is. The ISO standard is 1-7 starting on Monday, but I understand America has its own date standards...
|
|
||
| fn quarter0(&self) -> u32 { | ||
| self.month0() / 3 | ||
| /// Extracts the day of week of a given temporal array as an array of integers |
There was a problem hiding this comment.
| /// Extracts the day of week of a given temporal array as an array of integers | |
| /// Extracts the day of week of a given temporal array as an array of integers | |
| /// with each integer representing the number of days from sunday. | |
| /// | |
| /// For example, Sunday 1st 2010, would return 1 |
There was a problem hiding this comment.
Oops! Sorry, I didnt saw your commit suggestion while I was working on the update for PR.
Anyway, if you think that updated comment is inadequate, feel free to add addition, I'll commit it.
Thanks
| fn quarter0(&self) -> u32 { | ||
| self.month0() / 3 | ||
| /// Extracts the day of week of a given temporal array as an array of integers | ||
| pub fn dow<T>(array: &PrimitiveArray<T>) -> Result<Int32Array> |
There was a problem hiding this comment.
I wonder how this is different than weekday: https://docs.rs/arrow/18.0.0/arrow/compute/kernels/temporal/fn.weekday.html (defined on line 304 of this same file)?
There was a problem hiding this comment.
Wow, I didn't saw this new method, because we use old DF & old Arrow.
|
My opinion is that DataFusion should follow the postgres behavior, but arrow is not thus constrained. If it would be more convenient to have kernels that started on 0 and one that started on 1 (e.g. |
👍 If it's okey, I can update this PR to reflect similar way. |
| @@ -654,6 +620,26 @@ mod tests { | |||
| assert_eq!(2, b.value(2)); | |||
| } | |||
|
|
|||
| #[test] | |||
| fn test_temporal_array_date64_weekday0() { | |||
There was a problem hiding this comment.
Same behaivour as PostgreSQL:
ovr
changed the title
ovr
changed the title
Codecov Report
@@ Coverage Diff @@
## master #2052 +/- ##
=======================================
Coverage 83.55% 83.56%
=======================================
Files 222 222
Lines 58216 58234 +18
=======================================
+ Hits 48645 48662 +17
- Misses 9571 9572 +1
Continue to review full report at Codecov.
|
| fn quarter0(&self) -> u32 { | ||
| self.month0() / 3 | ||
| fn weekday0(&self) -> i32 { | ||
| self.weekday().num_days_from_sunday() as i32 |
There was a problem hiding this comment.
🤔 now I see that chrono uses the names num_days_from_sunday and num_days_from_monday
https://docs.rs/chrono/0.4.19/chrono/enum.Weekday.html#method.num_days_from_sunday
https://docs.rs/chrono/0.4.19/chrono/enum.Weekday.html#method.num_days_from_monday
What would you think about using those names instead of weekday0 and weekday @ovr (I would be happy to rename things in a follow on PR prior to release)?
There was a problem hiding this comment.
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
|
Thanks @ovr |
|
Benchmark runs are scheduled for baseline = 50e285f and contender = 4444cb7. 4444cb7 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Rationale for this change
DataFusion uses temporal to operate with date like structures. I want to implement
EXTRACT(DOW from NOW()).What changes are included in this PR?
Implementation of
temporal::dowAre there any user-facing changes?
No
Thanks