-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Respect execution timezone in to_timestamp and related functions #19078
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
Open
Omega359
wants to merge
6
commits into
apache:main
Choose a base branch
from
Omega359:timestamp-17998
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
eed97cf
Implement timezone-aware handling for to_timestamp functions.
kosiew bb579d5
Merge branch 'apache:main' into timestamp-17998
Omega359 7c00e07
Implement timezone-aware handling for to_timestamp functions.
Omega359 5c4eaf7
Updates from code review.
Omega359 71fe6d3
Merge remote-tracking branch 'origin/timestamp-17998' into timestamp-…
Omega359 15096a6
Extract parsing of UTC to a static.
Omega359 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,33 +15,54 @@ | |
| // specific language governing permissions and limitations | ||
| // under the License. | ||
|
|
||
| use std::sync::Arc; | ||
| use std::marker::PhantomData; | ||
| use std::sync::{Arc, LazyLock}; | ||
|
|
||
| use arrow::array::timezone::Tz; | ||
| use arrow::array::{ | ||
| Array, ArrowPrimitiveType, AsArray, GenericStringArray, PrimitiveArray, | ||
| StringArrayType, StringViewArray, | ||
| }; | ||
| use arrow::compute::kernels::cast_utils::string_to_timestamp_nanos; | ||
| use arrow::datatypes::DataType; | ||
| use arrow::compute::kernels::cast_utils::{ | ||
| string_to_datetime, string_to_timestamp_nanos, | ||
| }; | ||
| use arrow::datatypes::{DataType, TimeUnit}; | ||
| use arrow_buffer::ArrowNativeType; | ||
| use chrono::format::{parse, Parsed, StrftimeItems}; | ||
| use chrono::LocalResult::Single; | ||
| use chrono::{DateTime, TimeZone, Utc}; | ||
|
|
||
| use datafusion_common::cast::as_generic_string_array; | ||
| use datafusion_common::{ | ||
| exec_datafusion_err, exec_err, unwrap_or_internal_err, DataFusionError, Result, | ||
| ScalarType, ScalarValue, | ||
| exec_datafusion_err, exec_err, internal_datafusion_err, unwrap_or_internal_err, | ||
| DataFusionError, Result, ScalarValue, | ||
| }; | ||
| use datafusion_expr::ColumnarValue; | ||
| use num_traits::{PrimInt, ToPrimitive}; | ||
|
|
||
| /// Error message if nanosecond conversion request beyond supported interval | ||
| const ERR_NANOSECONDS_NOT_SUPPORTED: &str = "The dates that can be represented as nanoseconds have to be between 1677-09-21T00:12:44.0 and 2262-04-11T23:47:16.854775804"; | ||
|
|
||
| static UTC: LazyLock<Tz> = LazyLock::new(|| "UTC".parse().expect("UTC is always valid")); | ||
|
|
||
| #[expect(unused)] | ||
| /// Calls string_to_timestamp_nanos and converts the error type | ||
| pub(crate) fn string_to_timestamp_nanos_shim(s: &str) -> Result<i64> { | ||
| string_to_timestamp_nanos(s).map_err(|e| e.into()) | ||
| } | ||
|
|
||
| pub(crate) fn string_to_timestamp_nanos_with_timezone( | ||
| timezone: &Option<Tz>, | ||
| s: &str, | ||
| ) -> Result<i64> { | ||
| let tz = timezone.unwrap_or(*UTC); | ||
| let dt = string_to_datetime(&tz, s)?; | ||
| let parsed = dt | ||
| .timestamp_nanos_opt() | ||
| .ok_or_else(|| exec_datafusion_err!("{ERR_NANOSECONDS_NOT_SUPPORTED}"))?; | ||
|
|
||
| Ok(parsed) | ||
| } | ||
|
|
||
| /// Checks that all the arguments from the second are of type [Utf8], [LargeUtf8] or [Utf8View] | ||
| /// | ||
| /// [Utf8]: DataType::Utf8 | ||
|
|
@@ -69,13 +90,12 @@ pub(crate) fn validate_data_types(args: &[ColumnarValue], name: &str) -> Result< | |
| /// Accepts a string and parses it using the [`chrono::format::strftime`] specifiers | ||
| /// relative to the provided `timezone` | ||
| /// | ||
| /// [IANA timezones] are only supported if the `arrow-array/chrono-tz` feature is enabled | ||
| /// | ||
| /// * `2023-01-01 040506 America/Los_Angeles` | ||
| /// | ||
| /// If a timestamp is ambiguous, for example as a result of daylight-savings time, an error | ||
| /// will be returned | ||
| /// | ||
| /// Note that parsing [IANA timezones] is not supported yet in chrono - <https://github.com/chronotope/chrono/issues/38> | ||
| /// and this implementation only supports named timezones at the end of the string preceded by a space. | ||
| /// | ||
| /// [`chrono::format::strftime`]: https://docs.rs/chrono/latest/chrono/format/strftime/index.html | ||
| /// [IANA timezones]: https://www.iana.org/time-zones | ||
| pub(crate) fn string_to_datetime_formatted<T: TimeZone>( | ||
|
|
@@ -89,11 +109,55 @@ pub(crate) fn string_to_datetime_formatted<T: TimeZone>( | |
| ) | ||
| }; | ||
|
|
||
| let mut datetime_str = s; | ||
| let mut format = format; | ||
|
|
||
| // Manually handle the most common case of a named timezone at the end of the timestamp. | ||
| // Note that %+ handles 'Z' at the end of the string without a space. This code doesn't | ||
| // handle named timezones with no preceding space since that would require writing a | ||
| // custom parser (or switching to Jiff) | ||
| let tz: Option<chrono_tz::Tz> = if format.ends_with(" %Z") { | ||
| // grab the string after the last space as the named timezone | ||
| if let Some((dt_str, timezone_name)) = datetime_str.rsplit_once(' ') { | ||
| datetime_str = dt_str; | ||
|
|
||
| // attempt to parse the timezone name | ||
| let result: Result<chrono_tz::Tz, chrono_tz::ParseError> = | ||
| timezone_name.parse(); | ||
| let Ok(tz) = result else { | ||
| return Err(err(&result.unwrap_err().to_string())); | ||
| }; | ||
|
|
||
| // successfully parsed the timezone name, remove the ' %Z' from the format | ||
| format = &format[..format.len() - 3]; | ||
|
|
||
| Some(tz) | ||
| } else { | ||
| None | ||
| } | ||
| } else if format.contains("%Z") { | ||
| return Err(err( | ||
| "'%Z' is only supported at the end of the format string preceded by a space", | ||
| )); | ||
| } else { | ||
| None | ||
| }; | ||
|
|
||
| let mut parsed = Parsed::new(); | ||
| parse(&mut parsed, s, StrftimeItems::new(format)).map_err(|e| err(&e.to_string()))?; | ||
| parse(&mut parsed, datetime_str, StrftimeItems::new(format)) | ||
| .map_err(|e| err(&e.to_string()))?; | ||
|
|
||
| // attempt to parse the string assuming it has a timezone | ||
| let dt = parsed.to_datetime(); | ||
| let dt = match tz { | ||
| Some(tz) => { | ||
| // A timezone was manually parsed out, convert it to a fixed offset | ||
| match parsed.to_datetime_with_timezone(&tz) { | ||
| Ok(dt) => Ok(dt.fixed_offset()), | ||
| Err(e) => Err(e), | ||
| } | ||
| } | ||
| // default to parse the string assuming it has a timezone | ||
| None => parsed.to_datetime(), | ||
| }; | ||
|
|
||
| if let Err(e) = &dt { | ||
| // no timezone or other failure, try without a timezone | ||
|
|
@@ -141,6 +205,7 @@ pub(crate) fn string_to_datetime_formatted<T: TimeZone>( | |
| /// | ||
| /// [`chrono::format::strftime`]: https://docs.rs/chrono/latest/chrono/format/strftime/index.html | ||
| #[inline] | ||
| #[expect(unused)] | ||
|
Member
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. Since it is crate-private why not remove it ? |
||
| pub(crate) fn string_to_timestamp_nanos_formatted( | ||
| s: &str, | ||
| format: &str, | ||
|
|
@@ -152,6 +217,19 @@ pub(crate) fn string_to_timestamp_nanos_formatted( | |
| .ok_or_else(|| exec_datafusion_err!("{ERR_NANOSECONDS_NOT_SUPPORTED}")) | ||
| } | ||
|
|
||
| pub(crate) fn string_to_timestamp_nanos_formatted_with_timezone( | ||
| timezone: &Option<Tz>, | ||
| s: &str, | ||
| format: &str, | ||
| ) -> Result<i64, DataFusionError> { | ||
| let dt = string_to_datetime_formatted(&timezone.unwrap_or(*UTC), s, format)?; | ||
| let parsed = dt | ||
| .timestamp_nanos_opt() | ||
| .ok_or_else(|| exec_datafusion_err!("{ERR_NANOSECONDS_NOT_SUPPORTED}"))?; | ||
|
|
||
| Ok(parsed) | ||
| } | ||
|
|
||
| /// Accepts a string with a `chrono` format and converts it to a | ||
| /// millisecond precision timestamp. | ||
| /// | ||
|
|
@@ -176,14 +254,50 @@ pub(crate) fn string_to_timestamp_millis_formatted(s: &str, format: &str) -> Res | |
| .timestamp_millis()) | ||
| } | ||
|
|
||
| pub(crate) fn handle<O, F, S>( | ||
| pub(crate) struct ScalarDataType<T: PrimInt> { | ||
| data_type: DataType, | ||
| _marker: PhantomData<T>, | ||
| } | ||
|
|
||
| impl<T: PrimInt> ScalarDataType<T> { | ||
| pub(crate) fn new(dt: DataType) -> Self { | ||
| Self { | ||
| data_type: dt, | ||
| _marker: PhantomData, | ||
| } | ||
| } | ||
|
|
||
| fn scalar(&self, r: Option<i64>) -> Result<ScalarValue> { | ||
| match &self.data_type { | ||
| DataType::Date32 => Ok(ScalarValue::Date32(r.and_then(|v| v.to_i32()))), | ||
| DataType::Timestamp(u, tz) => match u { | ||
| TimeUnit::Second => Ok(ScalarValue::TimestampSecond(r, tz.clone())), | ||
| TimeUnit::Millisecond => { | ||
| Ok(ScalarValue::TimestampMillisecond(r, tz.clone())) | ||
| } | ||
| TimeUnit::Microsecond => { | ||
| Ok(ScalarValue::TimestampMicrosecond(r, tz.clone())) | ||
| } | ||
| TimeUnit::Nanosecond => { | ||
| Ok(ScalarValue::TimestampNanosecond(r, tz.clone())) | ||
| } | ||
| }, | ||
| t => Err(internal_datafusion_err!( | ||
| "Unsupported data type for ScalarDataType<T>: {t:?}" | ||
| )), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| pub(crate) fn handle<O, F, T>( | ||
| args: &[ColumnarValue], | ||
| op: F, | ||
| name: &str, | ||
| sdt: &ScalarDataType<T>, | ||
| ) -> Result<ColumnarValue> | ||
| where | ||
| O: ArrowPrimitiveType, | ||
| S: ScalarType<O::Native>, | ||
| T: PrimInt, | ||
| F: Fn(&str) -> Result<O::Native>, | ||
| { | ||
| match &args[0] { | ||
|
|
@@ -210,8 +324,13 @@ where | |
| }, | ||
| ColumnarValue::Scalar(scalar) => match scalar.try_as_str() { | ||
| Some(a) => { | ||
| let result = a.as_ref().map(|x| op(x)).transpose()?; | ||
| Ok(ColumnarValue::Scalar(S::scalar(result))) | ||
| let result = a | ||
| .as_ref() | ||
| .map(|x| op(x)) | ||
| .transpose()? | ||
| .and_then(|v| v.to_i64()); | ||
| let s = sdt.scalar(result)?; | ||
| Ok(ColumnarValue::Scalar(s)) | ||
| } | ||
| _ => exec_err!("Unsupported data type {scalar:?} for function {name}"), | ||
| }, | ||
|
|
@@ -221,17 +340,18 @@ where | |
| // Given a function that maps a `&str`, `&str` to an arrow native type, | ||
| // returns a `ColumnarValue` where the function is applied to either a `ArrayRef` or `ScalarValue` | ||
| // depending on the `args`'s variant. | ||
| pub(crate) fn handle_multiple<O, F, S, M>( | ||
| pub(crate) fn handle_multiple<O, F, M, T>( | ||
| args: &[ColumnarValue], | ||
| op: F, | ||
| op2: M, | ||
| name: &str, | ||
| sdt: &ScalarDataType<T>, | ||
| ) -> Result<ColumnarValue> | ||
| where | ||
| O: ArrowPrimitiveType, | ||
| S: ScalarType<O::Native>, | ||
| F: Fn(&str, &str) -> Result<O::Native>, | ||
| M: Fn(O::Native) -> O::Native, | ||
| T: PrimInt, | ||
| { | ||
| match &args[0] { | ||
| ColumnarValue::Array(a) => match a.data_type() { | ||
|
|
@@ -286,9 +406,9 @@ where | |
| if let Some(s) = x { | ||
| match op(a, s.as_str()) { | ||
| Ok(r) => { | ||
| ret = Some(Ok(ColumnarValue::Scalar(S::scalar(Some( | ||
| op2(r), | ||
| ))))); | ||
| let result = op2(r).to_i64(); | ||
| let s = sdt.scalar(result)?; | ||
| ret = Some(Ok(ColumnarValue::Scalar(s))); | ||
| break; | ||
| } | ||
| Err(e) => ret = Some(Err(e)), | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Since it is crate-private why not remove it ?
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.
Honestly I wasn't sure if it still could be used by a custom UDF that used the same package name. I was just being super cautious since I hate breaking other people's code needlessly.