kosiew commented on code in PR #26045:
URL: https://github.com/apache/datafusion/pull/26045#discussion_r4346619366
##########
datafusion/common/src/config.rs:
##########
@@ -4146,6 +4202,100 @@ 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)
Review Comment:
Optional follow-up: could we add a CLI regression test in
`datafusion-cli/src/print_format.rs` that calls
`PrintFormat::Table.print_batches` with a `Vec<u8>` writer, `time_format =
Some("%Y".into())`, a nonempty time batch, and `MaxRows::Limited(1)` with
multiple input rows? Assert a configuration error naming
`datafusion.format.time_format` and an empty writer to verify error propagation
before row-limit formatting; an empty temporal batch with a nonempty schema
would also cover the separate `print_empty` path.
##########
datafusion/common/src/config.rs:
##########
@@ -4146,6 +4202,100 @@ 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}"
+ );
+ }
+
+ 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())
Review Comment:
Optional test improvement: could we add a successful custom format case with
`timestamp_tz_format = Some("%+".into())`? Convert the options, format a UTC
timestamp at Unix epoch, and assert `1970-01-01T00:00:00+00:00` to verify that
supported timezone directives are accepted and preserved, not just that invalid
formats are rejected.
--
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]
