martin-g commented on code in PR #18990:
URL: https://github.com/apache/datafusion/pull/18990#discussion_r2573021219
##########
datafusion/functions/src/datetime/date_part.rs:
##########
@@ -193,12 +201,83 @@ impl ScalarUDFImpl for DatePartFunc {
ColumnarValue::Scalar(scalar) => scalar.to_array()?,
};
+ let (is_timezone_aware, tz_str_opt) = match array.data_type() {
+ Timestamp(_, Some(tz_str)) => (true, Some(Arc::clone(tz_str))),
+ _ => (false, None),
+ };
+
let part_trim = part_normalization(&part);
+ let is_epoch = is_epoch(&part);
+
+ // Epoch is timezone-independent - it always returns seconds since
1970-01-01 UTC
+ let array = if is_epoch {
+ array
+ } else if is_timezone_aware {
+ // For timezone-aware timestamps, extract in their own timezone
+ match tz_str_opt.as_ref() {
+ Some(tz_str) => {
+ let tz = match tz_str.parse::<Tz>() {
+ Ok(tz) => tz,
+ Err(_) => return exec_err!("Invalid timezone"),
Review Comment:
It would be nice to add `tz_str` to the error message. It would help in
debugging.
Also, these 4 lines are repeated at line 247. Maybe you should extract them
to a helper function
```suggestion
Err(err) => return exec_err!("Invalid timezone
'{tz_str}': {err}"),
```
##########
datafusion/functions/src/datetime/date_part.rs:
##########
@@ -124,7 +131,7 @@ impl DatePartFunc {
],
Volatility::Immutable,
Review Comment:
```suggestion
Volatility::Stable,
```
The volatility depends on the session timezone.
`current_date` and `now` also use `Stable`
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
