Support extract week in temporal.rs
#1376
Conversation
Ted-Jiang
force-pushed
the
ISSUE_1375
branch
2 times, most recently
from
39b53e3
to
34df948
Compare
|
@alamb @Dandandan @liukun4515 PTAL |
Thanks. @Ted-Jiang |
| let mut b = Int32Builder::new(array.len()); | ||
|
|
||
| match array.data_type() { | ||
| &DataType::Date32 | &DataType::Date64 | &DataType::Timestamp(_, None) => { |
There was a problem hiding this comment.
we need to consider the timezone when support the Timestamp type.
There was a problem hiding this comment.
I agree it is important (eventually) to handle the timezone
This function seems to follow the model of existing functions such as second which also don't handle timezone
I suggest we merge this PR in as is (without timezone support) and file a ticket for proper handling of timezone support for all the extract functions
There was a problem hiding this comment.
Thanks，I will create an issue follow up the timezone.
There was a problem hiding this comment.
we can add the enhancement pull request to resolve the time zone issue. @alamb
| assert_eq!(9, b.value(0)); | ||
| assert!(!b.is_valid(1)); | ||
| assert_eq!(1, b.value(2)); | ||
| assert_eq!(52, b.value(3)); |
There was a problem hiding this comment.
I don't understand how a date of 2022.01.01 would have a week of 52. I wonder if the comments on what date 1640998800000 corresponds to is incorrect?
There was a problem hiding this comment.
Sorry for forgetting provide enough info.
PG has the same behavior.
postgres=# select extract('week' from date '2022-01-01');
date_part
-----------
52
(1 row)
It may cause by 2019/12/31 is Tuesday, i use chronno to calculate the week it followed ISO standard.
There was a problem hiding this comment.
I don't understand how a date of
2022.01.01would have a week of 52. I wonder if the comments on what date1640998800000corresponds to is incorrect?
@alamb
the week of 2022.01.01 belongs to the last year.
There was a problem hiding this comment.
thank you for the clarification. Sorry for my confusion
| let mut b = Int32Builder::new(array.len()); | ||
|
|
||
| match array.data_type() { | ||
| &DataType::Date32 | &DataType::Date64 | &DataType::Timestamp(_, None) => { |
There was a problem hiding this comment.
I agree it is important (eventually) to handle the timezone
This function seems to follow the model of existing functions such as second which also don't handle timezone
I suggest we merge this PR in as is (without timezone support) and file a ticket for proper handling of timezone support for all the extract functions
|
Thank you @Ted-Jiang and @liukun4515 |
alamb
changed the title
week in temporal.rs
Which issue does this PR close?
Closes #1375.
Add a method Extracts the week of a given temporal array as an array of integers.
For apache/datafusion#1896
will implement
week :week number starting from 1 in the year.