-
Notifications
You must be signed in to change notification settings - Fork 875
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
Support extract week
in temporal.rs
#1376
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -40,6 +40,20 @@ macro_rules! extract_component_from_array { | |
} | ||
} | ||
}; | ||
($array:ident, $builder:ident, $extract_fn1:ident, $extract_fn2:ident, $using:ident) => { | ||
for i in 0..$array.len() { | ||
if $array.is_null(i) { | ||
$builder.append_null()?; | ||
} else { | ||
match $array.$using(i) { | ||
Some(dt) => { | ||
$builder.append_value(dt.$extract_fn1().$extract_fn2() as i32)? | ||
} | ||
None => $builder.append_null()?, | ||
} | ||
} | ||
} | ||
}; | ||
($array:ident, $builder:ident, $extract_fn:ident, $using:ident, $tz:ident, $parsed:ident) => { | ||
if ($tz.starts_with('+') || $tz.starts_with('-')) && !$tz.contains(':') { | ||
return_compute_error_with!( | ||
|
@@ -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) => { | ||
extract_component_from_array!(array, b, iso_week, week, value_as_datetime) | ||
} | ||
dt => return_compute_error_with!("week does not support", dt), | ||
} | ||
|
||
Ok(b.finish()) | ||
} | ||
|
||
/// Extracts the seconds of a given temporal array as an array of integers | ||
pub fn second<T>(array: &PrimitiveArray<T>) -> Result<Int32Array> | ||
where | ||
|
@@ -334,6 +366,48 @@ 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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand how a date of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry for forgetting provide enough info.
It may cause by 2019/12/31 is Tuesday, i use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@alamb There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thank you for the clarification. Sorry for my confusion |
||
} | ||
|
||
#[test] | ||
fn test_temporal_array_timestamp_micro_week() { | ||
//1612025847000000 -> 2021.1.30 | ||
//1722015847000000 -> 2024.7.27 | ||
let a: TimestampMicrosecondArray = | ||
Ted-Jiang marked this conversation as resolved.
Show resolved
Hide resolved
|
||
vec![Some(1612025847000000), None, Some(1722015847000000)].into(); | ||
|
||
let b = week(&a).unwrap(); | ||
assert_eq!(4, b.value(0)); | ||
assert!(!b.is_valid(1)); | ||
assert_eq!(30, b.value(2)); | ||
} | ||
|
||
#[test] | ||
fn test_temporal_array_date64_second() { | ||
let a: PrimitiveArray<Date64Type> = | ||
|
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.
we need to consider the
timezone
when support theTimestamp
type.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.
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 handletimezone
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks,I will create an issue follow up the timezone.
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.
timezone issue: #1380
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.
we can add the enhancement pull request to resolve the time zone issue. @alamb