Omega359 commented on code in PR #19078: URL: https://github.com/apache/datafusion/pull/19078#discussion_r2636534791
########## datafusion/functions/src/datetime/to_timestamp.rs: ########## @@ -199,80 +270,161 @@ Additional examples can be found [here](https://github.com/apache/datafusion/blo ), argument( name = "format_n", - description = "Optional [Chrono format](https://docs.rs/chrono/latest/chrono/format/strftime/index.html) strings to use to parse the expression. Formats will be tried in the order they appear with the first successful one being returned. If none of the formats successfully parse the expression an error will be returned." + description = r#" +Optional [Chrono format](https://docs.rs/chrono/latest/chrono/format/strftime/index.html) strings to use to parse the expression. +Formats will be tried in the order they appear with the first successful one being returned. If none of the formats successfully +parse the expression an error will be returned. Note: parsing of named timezones (e.g. 'America/New_York') using %Z is +only supported at the end of the string preceded by a space. +"# ) )] #[derive(Debug, PartialEq, Eq, Hash)] pub struct ToTimestampNanosFunc { signature: Signature, + timezone: Option<Arc<str>>, } impl Default for ToTimestampFunc { fn default() -> Self { - Self::new() + Self::new_with_config(&ConfigOptions::default()) } } impl ToTimestampFunc { + #[deprecated(since = "52.0.0", note = "use `new_with_config` instead")] + /// Deprecated constructor retained for backwards compatibility. + /// + /// Prefer [`ToTimestampFunc::new_with_config`] which allows specifying the + /// timezone via [`ConfigOptions`]. This helper now mirrors the + /// canonical default offset (None) provided by `ConfigOptions::default()`. pub fn new() -> Self { + Self::new_with_config(&ConfigOptions::default()) + } + + pub fn new_with_config(config: &ConfigOptions) -> Self { Review Comment: I'll be honest, I'm not much of a fan or macros in general but I could see it being useful here. I however am horrible at writing them .. would you be able to do a PR to my repo/branch with this? ########## datafusion/functions/src/datetime/to_timestamp.rs: ########## @@ -199,80 +270,161 @@ Additional examples can be found [here](https://github.com/apache/datafusion/blo ), argument( name = "format_n", - description = "Optional [Chrono format](https://docs.rs/chrono/latest/chrono/format/strftime/index.html) strings to use to parse the expression. Formats will be tried in the order they appear with the first successful one being returned. If none of the formats successfully parse the expression an error will be returned." + description = r#" +Optional [Chrono format](https://docs.rs/chrono/latest/chrono/format/strftime/index.html) strings to use to parse the expression. +Formats will be tried in the order they appear with the first successful one being returned. If none of the formats successfully +parse the expression an error will be returned. Note: parsing of named timezones (e.g. 'America/New_York') using %Z is +only supported at the end of the string preceded by a space. +"# ) )] #[derive(Debug, PartialEq, Eq, Hash)] pub struct ToTimestampNanosFunc { signature: Signature, + timezone: Option<Arc<str>>, } impl Default for ToTimestampFunc { fn default() -> Self { - Self::new() + Self::new_with_config(&ConfigOptions::default()) } } impl ToTimestampFunc { + #[deprecated(since = "52.0.0", note = "use `new_with_config` instead")] + /// Deprecated constructor retained for backwards compatibility. + /// + /// Prefer [`ToTimestampFunc::new_with_config`] which allows specifying the + /// timezone via [`ConfigOptions`]. This helper now mirrors the + /// canonical default offset (None) provided by `ConfigOptions::default()`. pub fn new() -> Self { + Self::new_with_config(&ConfigOptions::default()) + } + + pub fn new_with_config(config: &ConfigOptions) -> Self { Review Comment: I'll be honest, I'm not much of a fan of macros in general but I could see it being useful here. I however am horrible at writing them .. would you be able to do a PR to my repo/branch with this? -- 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]
