You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/03/01 07:57:38 UTC
[GitHub] [arrow-rs] Ted-Jiang opened a new pull request #1376: Add extract week in temporal.rs
Ted-Jiang opened a new pull request #1376:
URL: https://github.com/apache/arrow-rs/pull/1376
# 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 https://github.com/apache/arrow-datafusion/issues/1896
will implement
week :week number starting from 1 in the year.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] liukun4515 commented on a change in pull request #1376: Add extract week in temporal.rs
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on a change in pull request #1376:
URL: https://github.com/apache/arrow-rs/pull/1376#discussion_r816676265
##########
File path: arrow/src/compute/kernels/temporal.rs
##########
@@ -197,6 +211,24 @@ where
Ok(b.finish())
}
+/// Extracts the week of a given temporal array as an array of integers
+pub fn week<T>(array: &PrimitiveArray<T>) -> Result<Int32Array>
+where
+ T: ArrowTemporalType + ArrowNumericType,
+ i64: std::convert::From<T::Native>,
+{
+ let mut b = Int32Builder::new(array.len());
+
+ match array.data_type() {
+ &DataType::Date32 | &DataType::Date64 | &DataType::Timestamp(_, None) => {
Review comment:
we need to consider the `timezone` when support the `Timestamp` type.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] liukun4515 commented on a change in pull request #1376: Add extract week in temporal.rs
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on a change in pull request #1376:
URL: https://github.com/apache/arrow-rs/pull/1376#discussion_r817371291
##########
File path: arrow/src/compute/kernels/temporal.rs
##########
@@ -334,6 +366,46 @@ mod tests {
assert_eq!(44, b.value(2));
}
+ #[test]
+ fn test_temporal_array_date32_week() {
+ let a: PrimitiveArray<Date32Type> = vec![Some(0), None, Some(7)].into();
+
+ let b = week(&a).unwrap();
+ assert_eq!(1, b.value(0));
+ assert!(!b.is_valid(1));
+ assert_eq!(2, b.value(2));
+ }
+
+ #[test]
+ fn test_temporal_array_date64_week() {
+ // 1646116175000 -> 2022.03.01 , 1641171600000 -> 2022.01.03
+ // 1640998800000 -> 2022.01.01
+ let a: PrimitiveArray<Date64Type> = vec![
+ Some(1646116175000),
+ None,
+ Some(1641171600000),
+ Some(1640998800000),
+ ]
+ .into();
+
+ let b = week(&a).unwrap();
+ assert_eq!(9, b.value(0));
+ assert!(!b.is_valid(1));
+ assert_eq!(1, b.value(2));
+ assert_eq!(52, b.value(3));
Review 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?
@alamb
the week of `2022.01.01` belongs to the last year.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] Ted-Jiang commented on pull request #1376: Add extract week in temporal.rs
Posted by GitBox <gi...@apache.org>.
Ted-Jiang commented on pull request #1376:
URL: https://github.com/apache/arrow-rs/pull/1376#issuecomment-1055287113
@alamb @Dandandan @liukun4515 PTAL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on a change in pull request #1376: Add extract week in temporal.rs
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1376:
URL: https://github.com/apache/arrow-rs/pull/1376#discussion_r817873520
##########
File path: arrow/src/compute/kernels/temporal.rs
##########
@@ -334,6 +366,46 @@ mod tests {
assert_eq!(44, b.value(2));
}
+ #[test]
+ fn test_temporal_array_date32_week() {
+ let a: PrimitiveArray<Date32Type> = vec![Some(0), None, Some(7)].into();
+
+ let b = week(&a).unwrap();
+ assert_eq!(1, b.value(0));
+ assert!(!b.is_valid(1));
+ assert_eq!(2, b.value(2));
+ }
+
+ #[test]
+ fn test_temporal_array_date64_week() {
+ // 1646116175000 -> 2022.03.01 , 1641171600000 -> 2022.01.03
+ // 1640998800000 -> 2022.01.01
+ let a: PrimitiveArray<Date64Type> = vec![
+ Some(1646116175000),
+ None,
+ Some(1641171600000),
+ Some(1640998800000),
+ ]
+ .into();
+
+ let b = week(&a).unwrap();
+ assert_eq!(9, b.value(0));
+ assert!(!b.is_valid(1));
+ assert_eq!(1, b.value(2));
+ assert_eq!(52, b.value(3));
Review comment:
thank you for the clarification. Sorry for my confusion
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] Ted-Jiang removed a comment on pull request #1376: Add extract week in temporal.rs
Posted by GitBox <gi...@apache.org>.
Ted-Jiang removed a comment on pull request #1376:
URL: https://github.com/apache/arrow-rs/pull/1376#issuecomment-1055132052
@alamb @Dandandan @liukun4515 PTAL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] liukun4515 commented on pull request #1376: Add extract week in temporal.rs
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on pull request #1376:
URL: https://github.com/apache/arrow-rs/pull/1376#issuecomment-1055317522
> @alamb @Dandandan @liukun4515 PTAL
Thanks. @Ted-Jiang
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] liukun4515 commented on a change in pull request #1376: Add extract week in temporal.rs
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on a change in pull request #1376:
URL: https://github.com/apache/arrow-rs/pull/1376#discussion_r817588792
##########
File path: arrow/src/compute/kernels/temporal.rs
##########
@@ -197,6 +211,24 @@ where
Ok(b.finish())
}
+/// Extracts the week of a given temporal array as an array of integers
+pub fn week<T>(array: &PrimitiveArray<T>) -> Result<Int32Array>
+where
+ T: ArrowTemporalType + ArrowNumericType,
+ i64: std::convert::From<T::Native>,
+{
+ let mut b = Int32Builder::new(array.len());
+
+ match array.data_type() {
+ &DataType::Date32 | &DataType::Date64 | &DataType::Timestamp(_, None) => {
Review comment:
we can add the enhancement pull request to resolve the time zone issue. @alamb
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on a change in pull request #1376: Add extract week in temporal.rs
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #1376:
URL: https://github.com/apache/arrow-rs/pull/1376#discussion_r817006099
##########
File path: arrow/src/compute/kernels/temporal.rs
##########
@@ -334,6 +366,46 @@ mod tests {
assert_eq!(44, b.value(2));
}
+ #[test]
+ fn test_temporal_array_date32_week() {
+ let a: PrimitiveArray<Date32Type> = vec![Some(0), None, Some(7)].into();
+
+ let b = week(&a).unwrap();
+ assert_eq!(1, b.value(0));
+ assert!(!b.is_valid(1));
+ assert_eq!(2, b.value(2));
+ }
+
+ #[test]
+ fn test_temporal_array_date64_week() {
+ // 1646116175000 -> 2022.03.01 , 1641171600000 -> 2022.01.03
+ // 1640998800000 -> 2022.01.01
+ let a: PrimitiveArray<Date64Type> = vec![
+ Some(1646116175000),
+ None,
+ Some(1641171600000),
+ Some(1640998800000),
+ ]
+ .into();
+
+ let b = week(&a).unwrap();
+ assert_eq!(9, b.value(0));
+ assert!(!b.is_valid(1));
+ assert_eq!(1, b.value(2));
+ assert_eq!(52, b.value(3));
Review 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?
##########
File path: arrow/src/compute/kernels/temporal.rs
##########
@@ -197,6 +211,24 @@ where
Ok(b.finish())
}
+/// Extracts the week of a given temporal array as an array of integers
+pub fn week<T>(array: &PrimitiveArray<T>) -> Result<Int32Array>
+where
+ T: ArrowTemporalType + ArrowNumericType,
+ i64: std::convert::From<T::Native>,
+{
+ let mut b = Int32Builder::new(array.len());
+
+ match array.data_type() {
+ &DataType::Date32 | &DataType::Date64 | &DataType::Timestamp(_, None) => {
Review 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
##########
File path: arrow/src/compute/kernels/temporal.rs
##########
@@ -334,6 +366,46 @@ mod tests {
assert_eq!(44, b.value(2));
}
+ #[test]
+ fn test_temporal_array_date32_week() {
+ let a: PrimitiveArray<Date32Type> = vec![Some(0), None, Some(7)].into();
+
+ let b = week(&a).unwrap();
+ assert_eq!(1, b.value(0));
+ assert!(!b.is_valid(1));
+ assert_eq!(2, b.value(2));
+ }
+
+ #[test]
+ fn test_temporal_array_date64_week() {
+ // 1646116175000 -> 2022.03.01 , 1641171600000 -> 2022.01.03
+ // 1640998800000 -> 2022.01.01
+ let a: PrimitiveArray<Date64Type> = vec![
+ Some(1646116175000),
+ None,
+ Some(1641171600000),
+ Some(1640998800000),
+ ]
+ .into();
+
+ let b = week(&a).unwrap();
+ assert_eq!(9, b.value(0));
+ assert!(!b.is_valid(1));
+ assert_eq!(1, b.value(2));
+ assert_eq!(52, b.value(3));
+ }
+
+ #[test]
+ fn test_temporal_array_timestamp_micro_week() {
+ let a: TimestampMicrosecondArray =
Review comment:
could you possibly also comment here about what dates are present?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] Ted-Jiang commented on a change in pull request #1376: Add extract week in temporal.rs
Posted by GitBox <gi...@apache.org>.
Ted-Jiang commented on a change in pull request #1376:
URL: https://github.com/apache/arrow-rs/pull/1376#discussion_r817302024
##########
File path: arrow/src/compute/kernels/temporal.rs
##########
@@ -197,6 +211,24 @@ where
Ok(b.finish())
}
+/// Extracts the week of a given temporal array as an array of integers
+pub fn week<T>(array: &PrimitiveArray<T>) -> Result<Int32Array>
+where
+ T: ArrowTemporalType + ArrowNumericType,
+ i64: std::convert::From<T::Native>,
+{
+ let mut b = Int32Builder::new(array.len());
+
+ match array.data_type() {
+ &DataType::Date32 | &DataType::Date64 | &DataType::Timestamp(_, None) => {
Review comment:
Thanks，I will create an issue follow up the timezone.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] Ted-Jiang commented on a change in pull request #1376: Add extract week in temporal.rs
Posted by GitBox <gi...@apache.org>.
Ted-Jiang commented on a change in pull request #1376:
URL: https://github.com/apache/arrow-rs/pull/1376#discussion_r817305221
##########
File path: arrow/src/compute/kernels/temporal.rs
##########
@@ -334,6 +366,46 @@ mod tests {
assert_eq!(44, b.value(2));
}
+ #[test]
+ fn test_temporal_array_date32_week() {
+ let a: PrimitiveArray<Date32Type> = vec![Some(0), None, Some(7)].into();
+
+ let b = week(&a).unwrap();
+ assert_eq!(1, b.value(0));
+ assert!(!b.is_valid(1));
+ assert_eq!(2, b.value(2));
+ }
+
+ #[test]
+ fn test_temporal_array_date64_week() {
+ // 1646116175000 -> 2022.03.01 , 1641171600000 -> 2022.01.03
+ // 1640998800000 -> 2022.01.01
+ let a: PrimitiveArray<Date64Type> = vec![
+ Some(1646116175000),
+ None,
+ Some(1641171600000),
+ Some(1640998800000),
+ ]
+ .into();
+
+ let b = week(&a).unwrap();
+ assert_eq!(9, b.value(0));
+ assert!(!b.is_valid(1));
+ assert_eq!(1, b.value(2));
+ assert_eq!(52, b.value(3));
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] liukun4515 commented on a change in pull request #1376: Add extract week in temporal.rs
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on a change in pull request #1376:
URL: https://github.com/apache/arrow-rs/pull/1376#discussion_r817371291
##########
File path: arrow/src/compute/kernels/temporal.rs
##########
@@ -334,6 +366,46 @@ mod tests {
assert_eq!(44, b.value(2));
}
+ #[test]
+ fn test_temporal_array_date32_week() {
+ let a: PrimitiveArray<Date32Type> = vec![Some(0), None, Some(7)].into();
+
+ let b = week(&a).unwrap();
+ assert_eq!(1, b.value(0));
+ assert!(!b.is_valid(1));
+ assert_eq!(2, b.value(2));
+ }
+
+ #[test]
+ fn test_temporal_array_date64_week() {
+ // 1646116175000 -> 2022.03.01 , 1641171600000 -> 2022.01.03
+ // 1640998800000 -> 2022.01.01
+ let a: PrimitiveArray<Date64Type> = vec![
+ Some(1646116175000),
+ None,
+ Some(1641171600000),
+ Some(1640998800000),
+ ]
+ .into();
+
+ let b = week(&a).unwrap();
+ assert_eq!(9, b.value(0));
+ assert!(!b.is_valid(1));
+ assert_eq!(1, b.value(2));
+ assert_eq!(52, b.value(3));
Review 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?
the week of `2022.01.01` belongs to the last year.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on pull request #1376: Add extract week in temporal.rs
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #1376:
URL: https://github.com/apache/arrow-rs/pull/1376#issuecomment-1057121942
Thank you @Ted-Jiang and @liukun4515
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb merged pull request #1376: Support extract `week` in temporal.rs
Posted by GitBox <gi...@apache.org>.
alamb merged pull request #1376:
URL: https://github.com/apache/arrow-rs/pull/1376
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] Ted-Jiang commented on pull request #1376: Add extract week in temporal.rs
Posted by GitBox <gi...@apache.org>.
Ted-Jiang commented on pull request #1376:
URL: https://github.com/apache/arrow-rs/pull/1376#issuecomment-1055132052
@alamb @Dandandan @liukun4515 PTAL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] Ted-Jiang commented on a change in pull request #1376: Add extract week in temporal.rs
Posted by GitBox <gi...@apache.org>.
Ted-Jiang commented on a change in pull request #1376:
URL: https://github.com/apache/arrow-rs/pull/1376#discussion_r817305221
##########
File path: arrow/src/compute/kernels/temporal.rs
##########
@@ -334,6 +366,46 @@ mod tests {
assert_eq!(44, b.value(2));
}
+ #[test]
+ fn test_temporal_array_date32_week() {
+ let a: PrimitiveArray<Date32Type> = vec![Some(0), None, Some(7)].into();
+
+ let b = week(&a).unwrap();
+ assert_eq!(1, b.value(0));
+ assert!(!b.is_valid(1));
+ assert_eq!(2, b.value(2));
+ }
+
+ #[test]
+ fn test_temporal_array_date64_week() {
+ // 1646116175000 -> 2022.03.01 , 1641171600000 -> 2022.01.03
+ // 1640998800000 -> 2022.01.01
+ let a: PrimitiveArray<Date64Type> = vec![
+ Some(1646116175000),
+ None,
+ Some(1641171600000),
+ Some(1640998800000),
+ ]
+ .into();
+
+ let b = week(&a).unwrap();
+ assert_eq!(9, b.value(0));
+ assert!(!b.is_valid(1));
+ assert_eq!(1, b.value(2));
+ assert_eq!(52, b.value(3));
Review 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)
```
I may cause by 2019/12/31 is Tuesday, i use `chronno` to calculate the week it followed `ISO` standard.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] liukun4515 commented on a change in pull request #1376: Add extract week in temporal.rs
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on a change in pull request #1376:
URL: https://github.com/apache/arrow-rs/pull/1376#discussion_r817372806
##########
File path: arrow/src/compute/kernels/temporal.rs
##########
@@ -197,6 +211,24 @@ where
Ok(b.finish())
}
+/// Extracts the week of a given temporal array as an array of integers
+pub fn week<T>(array: &PrimitiveArray<T>) -> Result<Int32Array>
+where
+ T: ArrowTemporalType + ArrowNumericType,
+ i64: std::convert::From<T::Native>,
+{
+ let mut b = Int32Builder::new(array.len());
+
+ match array.data_type() {
+ &DataType::Date32 | &DataType::Date64 | &DataType::Timestamp(_, None) => {
Review comment:
timezone issue: https://github.com/apache/arrow-rs/issues/1380
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org