quwin commented on code in PR #26045:
URL: https://github.com/apache/datafusion/pull/26045#discussion_r4198195740
##########
datafusion/common/src/config.rs:
##########
@@ -2121,6 +2121,27 @@ config_namespace! {
impl<'a> TryFrom<&'a FormatOptions> for
arrow::util::display::FormatOptions<'a> {
type Error = DataFusionError;
fn try_from(options: &'a FormatOptions) -> Result<Self> {
+ for (name, format) in [
+ ("date_format", options.date_format.as_deref()),
+ ("datetime_format", options.datetime_format.as_deref()),
+ ("timestamp_format", options.timestamp_format.as_deref()),
+ (
+ "timestamp_tz_format",
+ options.timestamp_tz_format.as_deref(),
+ ),
+ ("time_format", options.time_format.as_deref()),
+ ] {
+ if let Some(format) = format {
+ chrono::format::StrftimeItems::new(format)
+ .parse()
Review Comment:
Thanks for fixing the formatting panic. Could we also validate that each
format is compatible with the temporal type before accepting it?
`StrftimeItems::parse()` checks syntax only, so this still panics:
```sql
SET datafusion.format.time_format = '%Y';
SELECT TIME '12:00:00';
```
this results in `a Display implementation returned an error unexpectedly`.
`date_format='%H'` with a DATE and `timestamp_format='%+'` with a
timezone-free TIMESTAMP also panic.
##########
datafusion/common/src/config.rs:
##########
@@ -4146,6 +4167,63 @@ mod tests {
ConfigEntry, ConfigExtension, ConfigField, ConfigFileType,
ExtensionOptions,
Extensions, TableOptions,
};
+
+ #[test]
Review Comment:
A formatting check against the corresponding Chrono temporal type (e.g.
`DelayedFormat::write_to`) would catch these cases.
--
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]
