From aeffc7eeb5f08b0e4d878c870c774eb746e33ef8 Mon Sep 17 00:00:00 2001 From: mac Date: Sun, 4 Oct 2026 23:58:16 -0700 Subject: [PATCH 1/2] fix: validate temporal display formats --- datafusion/common/src/config.rs | 78 +++++++++++++++++++++++++++++++++ 1 file changed, 78 insertions(+) diff --git a/datafusion/common/src/config.rs b/datafusion/common/src/config.rs index bcabdcc1a1654..c5daeb3a810b9 100644 --- a/datafusion/common/src/config.rs +++ b/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 { + 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() + .map_err(|error| { + DataFusionError::Configuration(format!( + "Invalid datafusion.format.{name}: {error}" + )) + })?; + } + } + Ok(Self::new() .with_display_error(options.safe) .with_null(&options.null) @@ -4146,6 +4167,63 @@ mod tests { ConfigEntry, ConfigExtension, ConfigField, ConfigFileType, ExtensionOptions, Extensions, TableOptions, }; + + #[test] + fn invalid_temporal_format_is_rejected_before_rendering() { + use crate::config::FormatOptions; + + let invalid_options = [ + ( + "date_format", + FormatOptions { + date_format: Some("%".to_owned()), + ..Default::default() + }, + ), + ( + "datetime_format", + FormatOptions { + datetime_format: Some("%".to_owned()), + ..Default::default() + }, + ), + ( + "timestamp_format", + FormatOptions { + timestamp_format: Some("%".to_owned()), + ..Default::default() + }, + ), + ( + "timestamp_tz_format", + FormatOptions { + timestamp_tz_format: Some("%".to_owned()), + ..Default::default() + }, + ), + ( + "time_format", + FormatOptions { + time_format: Some("%".to_owned()), + ..Default::default() + }, + ), + ]; + + for (name, options) in invalid_options { + let error = arrow::util::display::FormatOptions::try_from(&options) + .expect_err("invalid format strings must be rejected before rendering"); + assert!( + error + .to_string() + .contains(&format!("datafusion.format.{name}")), + "unexpected validation error: {error}" + ); + } + + arrow::util::display::FormatOptions::try_from(&FormatOptions::default()) + .expect("the default temporal formats must remain valid"); + } use std::any::Any; use std::collections::HashMap; From 310128c15fd1558efaf16e94271ff0ac8f2b66c5 Mon Sep 17 00:00:00 2001 From: mac Date: Thu, 8 Oct 2026 07:31:02 -0700 Subject: [PATCH 2/2] fix: validate temporal format compatibility --- datafusion/common/src/config.rs | 80 +++++++++++++++++++++++++++++++-- 1 file changed, 76 insertions(+), 4 deletions(-) diff --git a/datafusion/common/src/config.rs b/datafusion/common/src/config.rs index c5daeb3a810b9..32797e9c8a6d1 100644 --- a/datafusion/common/src/config.rs +++ b/datafusion/common/src/config.rs @@ -2132,13 +2132,48 @@ impl<'a> TryFrom<&'a FormatOptions> for arrow::util::display::FormatOptions<'a> ("time_format", options.time_format.as_deref()), ] { if let Some(format) = format { - chrono::format::StrftimeItems::new(format) - .parse() - .map_err(|error| { + let items = chrono::format::StrftimeItems::new(format).parse().map_err( + |error| { DataFusionError::Configuration(format!( "Invalid datafusion.format.{name}: {error}" )) - })?; + }, + )?; + + // Parsing validates directives, but some directives require fields + // that are absent from a particular temporal type (for example, + // `%H` for a date). Chrono reports those errors while formatting. + let mut rendered = String::new(); + let result = match name { + "date_format" => chrono::NaiveDate::from_ymd_opt(2001, 2, 3) + .expect("valid sample date") + .format_with_items(items.iter()) + .write_to(&mut rendered), + "datetime_format" | "timestamp_format" => { + chrono::NaiveDate::from_ymd_opt(2001, 2, 3) + .expect("valid sample date") + .and_hms_opt(4, 5, 6) + .expect("valid sample time") + .format_with_items(items.iter()) + .write_to(&mut rendered) + } + "timestamp_tz_format" => { + chrono::DateTime::from_timestamp(981_173_106, 0) + .expect("valid sample timestamp") + .format_with_items(items.iter()) + .write_to(&mut rendered) + } + "time_format" => chrono::NaiveTime::from_hms_opt(4, 5, 6) + .expect("valid sample time") + .format_with_items(items.iter()) + .write_to(&mut rendered), + _ => unreachable!("all temporal format names are handled above"), + }; + if result.is_err() { + return Err(DataFusionError::Configuration(format!( + "Invalid datafusion.format.{name}: format is incompatible with its temporal type" + ))); + } } } @@ -4221,6 +4256,43 @@ mod tests { ); } + let incompatible_options = [ + ( + "time_format", + FormatOptions { + time_format: Some("%Y".to_owned()), + ..Default::default() + }, + ), + ( + "date_format", + FormatOptions { + date_format: Some("%H".to_owned()), + ..Default::default() + }, + ), + ( + "timestamp_format", + FormatOptions { + timestamp_format: Some("%+".to_owned()), + ..Default::default() + }, + ), + ]; + + for (name, options) in incompatible_options { + let error = arrow::util::display::FormatOptions::try_from(&options) + .expect_err( + "type-incompatible formats must be rejected before rendering", + ); + assert!( + error + .to_string() + .contains(&format!("datafusion.format.{name}")), + "unexpected validation error: {error}" + ); + } + arrow::util::display::FormatOptions::try_from(&FormatOptions::default()) .expect("the default temporal formats must remain valid"); }