Omega359 commented on code in PR #18990:
URL: https://github.com/apache/datafusion/pull/18990#discussion_r2582273520
##########
datafusion/functions/src/datetime/date_part.rs:
##########
@@ -240,23 +314,43 @@ impl ScalarUDFImpl for DatePartFunc {
}
}
+fn adjust_timestamp_array<T: ArrowTimestampType>(
+ array: &ArrayRef,
+ tz: Tz,
+) -> Result<ArrayRef> {
+ let mut builder = PrimitiveBuilder::<T>::new();
+ let primitive_array = as_primitive_array::<T>(array)?;
+ for ts_opt in primitive_array.iter() {
+ match ts_opt {
+ None => builder.append_null(),
+ Some(ts) => {
+ let adjusted_ts = adjust_to_local_time::<T>(ts, tz)?;
+ builder.append_value(adjusted_ts);
+ }
+ }
+ }
+ Ok(Arc::new(builder.finish()))
+}
+
fn is_epoch(part: &str) -> bool {
let part = part_normalization(part);
Review Comment:
This work seems to be done twice - once before this function is called and
once in the function. Perhaps we should just remove this part_normalization()
call and pass the result of the previous call on line 209.
##########
datafusion/functions/src/datetime/mod.rs:
##########
@@ -37,6 +44,55 @@ pub mod to_local_time;
pub mod to_timestamp;
pub mod to_unixtime;
+// Adjusts a timestamp to local time by applying the timezone offset.
Review Comment:
Might want to put in common.rs instead.
##########
datafusion/functions/src/datetime/date_part.rs:
##########
@@ -240,23 +314,43 @@ impl ScalarUDFImpl for DatePartFunc {
}
}
+fn adjust_timestamp_array<T: ArrowTimestampType>(
+ array: &ArrayRef,
+ tz: Tz,
+) -> Result<ArrayRef> {
+ let mut builder = PrimitiveBuilder::<T>::new();
+ let primitive_array = as_primitive_array::<T>(array)?;
+ for ts_opt in primitive_array.iter() {
+ match ts_opt {
+ None => builder.append_null(),
+ Some(ts) => {
+ let adjusted_ts = adjust_to_local_time::<T>(ts, tz)?;
+ builder.append_value(adjusted_ts);
+ }
+ }
+ }
+ Ok(Arc::new(builder.finish()))
+}
+
fn is_epoch(part: &str) -> bool {
let part = part_normalization(part);
matches!(part.to_lowercase().as_str(), "epoch")
}
-// Try to remove quote if exist, if the quote is invalid, return original
string and let the downstream function handle the error
fn part_normalization(part: &str) -> &str {
part.strip_prefix(|c| c == '\'' || c == '\"')
.and_then(|s| s.strip_suffix(|c| c == '\'' || c == '\"'))
.unwrap_or(part)
}
-/// Invoke [`date_part`] on an `array` (e.g. Timestamp) and convert the
-/// result to a total number of seconds, milliseconds, microseconds or
-/// nanoseconds
+fn interpret_session_timezone(tz_str: &str) -> Result<Tz> {
+ match tz_str.parse::<Tz>() {
+ Ok(tz) => Ok(tz),
+ Err(err) => exec_err!("Invalid timezone '{tz_str}': {err}"),
+ }
+}
+
fn seconds_as_i32(array: &dyn Array, unit: TimeUnit) -> Result<ArrayRef> {
- // Nanosecond is neither supported in Postgres nor DuckDB, to avoid dealing
Review Comment:
Why remove this comment?
##########
datafusion/sqllogictest/test_files/extract_tz.slt:
##########
@@ -0,0 +1,158 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+
+# http://www.apache.org/licenses/LICENSE-2.0
+
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# Tests for timezone-aware extract SQL statement support.
+# Test with different timezone
+statement ok
+SET datafusion.execution.time_zone = '-03:00';
+
+query I
+SELECT EXTRACT(HOUR FROM TIMESTAMP '2025-11-18 10:00:00');
+----
+7
+
+query II
+SELECT EXTRACT(MINUTE FROM TIMESTAMP '2023-10-30 10:45:30'),
+ EXTRACT(SECOND FROM TIMESTAMP '2023-10-30 10:45:30');
Review Comment:
It would be good to test these with timezone offset like -3:30 (newfoundland)
##########
datafusion/functions/src/datetime/date_part.rs:
##########
@@ -240,23 +314,43 @@ impl ScalarUDFImpl for DatePartFunc {
}
}
+fn adjust_timestamp_array<T: ArrowTimestampType>(
+ array: &ArrayRef,
+ tz: Tz,
+) -> Result<ArrayRef> {
+ let mut builder = PrimitiveBuilder::<T>::new();
+ let primitive_array = as_primitive_array::<T>(array)?;
+ for ts_opt in primitive_array.iter() {
+ match ts_opt {
+ None => builder.append_null(),
+ Some(ts) => {
+ let adjusted_ts = adjust_to_local_time::<T>(ts, tz)?;
+ builder.append_value(adjusted_ts);
+ }
+ }
+ }
+ Ok(Arc::new(builder.finish()))
+}
+
fn is_epoch(part: &str) -> bool {
let part = part_normalization(part);
matches!(part.to_lowercase().as_str(), "epoch")
}
-// Try to remove quote if exist, if the quote is invalid, return original
string and let the downstream function handle the error
Review Comment:
Why remove this comment?
--
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]
