You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "mikamikasuki (via GitHub)" <gi...@apache.org> on 2026/10/05 07:00:47 UTC
[PR] fix: validate temporal display formats [datafusion]
mikamikasuki opened a new pull request, #26045:
URL: https://github.com/apache/datafusion/pull/26045
## Which issue does this PR close?
- Closes #24909
## Rationale for this change
An invalid temporal display format such as '%' can reach Arrow's table formatter and panic when the DataFusion CLI renders a time value. Invalid formats should return a configuration error before rendering.
## What changes are included in this PR?
Validate configured date, datetime, timestamp, timestamp-with-timezone, and time formats when converting DataFusion format options to Arrow format options. Invalid values now return a configuration error that identifies the datafusion.format.* setting.
## What is the testing strategy for this PR?
Added invalid_temporal_format_is_rejected_before_rendering, covering all five settings and confirming defaults remain valid. The CLI reproducer now returns a configuration error instead of panicking. The datafusion-common test suite, extended workspace tests, full workspace Clippy with -D warnings, formatting, and git diff --check passed.
## Are there any user-facing changes?
Invalid temporal format settings now return a clear configuration error instead of panicking while query results are rendered.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: validate temporal display formats [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #26045:
URL: https://github.com/apache/datafusion/pull/26045#issuecomment-6503953466
I had to mark https://github.com/apache/datafusion/pull/26049 as a draft to so you were upder the limit.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: validate temporal display formats [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #26045:
URL: https://github.com/apache/datafusion/pull/26045#issuecomment-6503959299
Thank you @mikamikasuki and @kosiew
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: validate temporal display formats [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #26045:
URL: https://github.com/apache/datafusion/pull/26045#issuecomment-6344974966
<!-- semver-check-comment -->
Thank you for opening this pull request!
Reviewer note: [cargo-semver-checks](https://github.com/obi1kenobi/cargo-semver-checks) reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch).
<details>
<summary>Details</summary>
```
Cloning apache/main
Building datafusion-common v55.1.0 (current)
Built [ 40.518s] (current)
Parsing datafusion-common v55.1.0 (current)
Parsed [ 0.073s] (current)
Building datafusion-common v55.1.0 (baseline)
Built [ 39.600s] (baseline)
Parsing datafusion-common v55.1.0 (baseline)
Parsed [ 0.075s] (baseline)
Checking datafusion-common v55.1.0 -> v55.1.0 (no change; assume patch)
Checked [ 0.722s] 223 checks: 217 pass, 6 fail, 0 warn, 31 skip
--- failure enum_missing: pub enum removed or renamed ---
Description:
A publicly-visible enum cannot be imported by its prior path. A `pub use` may have been removed, or the enum itself may have been renamed or removed entirely.
ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/enum_missing.ron
Failed in:
enum datafusion_common::config::ArrowIpcCompression, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/31ee0145cb027b1c55ea56bd1cead4d5f420c10e/datafusion/common/src/config.rs:3899
enum datafusion_common::parquet_config::DFParquetEncoding, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/31ee0145cb027b1c55ea56bd1cead4d5f420c10e/datafusion/common/src/parquet_config.rs:282
enum datafusion_common::parquet_config::DFTimeUnit, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/31ee0145cb027b1c55ea56bd1cead4d5f420c10e/datafusion/common/src/parquet_config.rs:606
--- failure enum_tuple_variant_changed_kind: An enum tuple variant changed kind ---
Description:
A public enum's exhaustive tuple variant has changed to a different kind of enum variant, breaking possible instantiations and patterns.
ref: https://doc.rust-lang.org/reference/items/enumerations.html
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/enum_tuple_variant_changed_kind.ron
Failed in:
variant OutputFormat::ARROW in /home/runner/work/datafusion/datafusion/datafusion/common/src/config.rs:4193
--- failure enum_variant_missing: pub enum variant removed or renamed ---
Description:
A publicly-visible enum has at least one variant that is no longer available under its prior name. It may have been renamed or removed entirely.
ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/enum_variant_missing.ron
Failed in:
variant ConfigFileType::ARROW, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/31ee0145cb027b1c55ea56bd1cead4d5f420c10e/datafusion/common/src/config.rs:2950
--- failure inherent_method_missing: pub method removed or renamed ---
Description:
A publicly-visible method or associated fn is no longer available under its prior name. It may have been renamed or removed entirely.
ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/inherent_method_missing.ron
Failed in:
ScalarValue::try_from_date, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/31ee0145cb027b1c55ea56bd1cead4d5f420c10e/datafusion/common/src/scalar/mod.rs:1541
ScalarValue::try_as_timestamp, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/31ee0145cb027b1c55ea56bd1cead4d5f420c10e/datafusion/common/src/scalar/mod.rs:1615
ScalarValue::try_from_date, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/31ee0145cb027b1c55ea56bd1cead4d5f420c10e/datafusion/common/src/scalar/mod.rs:1541
ScalarValue::try_as_timestamp, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/31ee0145cb027b1c55ea56bd1cead4d5f420c10e/datafusion/common/src/scalar/mod.rs:1615
--- failure struct_missing: pub struct removed or renamed ---
Description:
A publicly-visible struct cannot be imported by its prior path. A `pub use` may have been removed, or the struct itself may have been renamed or removed entirely.
ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/struct_missing.ron
Failed in:
struct datafusion_common::config::ArrowOptions, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/31ee0145cb027b1c55ea56bd1cead4d5f420c10e/datafusion/common/src/config.rs:3956
--- failure struct_pub_field_missing: pub struct's pub field removed or renamed ---
Description:
A publicly-visible struct has at least one public field that is no longer available under its prior name. It may have been renamed or removed entirely.
ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/struct_pub_field_missing.ron
Failed in:
field arrow of struct TableOptions, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/31ee0145cb027b1c55ea56bd1cead4d5f420c10e/datafusion/common/src/config.rs:2971
field write_row_group_number_distinct_values of struct ParquetOptions, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/31ee0145cb027b1c55ea56bd1cead4d5f420c10e/datafusion/common/src/config.rs:1315
Summary semver requires new major version: 6 major and 0 minor checks failed
Finished [ 82.432s] datafusion-common
```
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: validate temporal display formats [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #26045:
URL: https://github.com/apache/datafusion/pull/26045#issuecomment-6382874536
This PR appears to be waiting on the author to respond to reviewer feedback so marking as draft. Please mark ready to review when it is ready for another look
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: validate temporal display formats [datafusion]
Posted by "quwin (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: validate temporal display formats [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #26045:
URL: https://github.com/apache/datafusion/pull/26045
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: validate temporal display formats [datafusion]
Posted by "mikamikasuki (via GitHub)" <gi...@apache.org>.
mikamikasuki commented on PR #26045:
URL: https://github.com/apache/datafusion/pull/26045#issuecomment-6431706765
Added the requested positive `%+` UTC timestamp formatting case and CLI regressions for invalid `time_format` with populated and empty temporal batches. Both focused tests pass. I also ran `cargo fmt --all`, the required extended workspace test suite, and `cargo clippy --all-targets --all-features -- -D warnings`; all passed.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: validate temporal display formats [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #26045:
URL: https://github.com/apache/datafusion/pull/26045#issuecomment-6349816522
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/26045?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `94.59459%` with `6 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 82.75%. Comparing base ([`c3ef346`](https://app.codecov.io/gh/apache/datafusion/commit/c3ef346887412eeaeef644809c8fcfaf782f94e4?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`310128c`](https://app.codecov.io/gh/apache/datafusion/commit/310128c15fd1558efaf16e94271ff0ac8f2b66c5?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 247 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/26045?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [datafusion/common/src/config.rs](https://app.codecov.io/gh/apache/datafusion/pull/26045?src=pr&el=tree&filepath=datafusion%2Fcommon%2Fsrc%2Fconfig.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9jb21tb24vc3JjL2NvbmZpZy5ycw==) | 94.59% | [6 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/26045?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #26045 +/- ##
==========================================
+ Coverage 82.66% 82.75% +0.08%
==========================================
Files 1147 1147
Lines 446357 449881 +3524
Branches 446357 449881 +3524
==========================================
+ Hits 368971 372280 +3309
+ Misses 54997 54928 -69
- Partials 22389 22673 +284
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/26045?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: validate temporal display formats [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #26045:
URL: https://github.com/apache/datafusion/pull/26045#discussion_r4415814168
##########
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:
And end to end test please (as in do with a slt / test that runs the actual cli)
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: validate temporal display formats [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: validate temporal display formats [datafusion]
Posted by "zuccamia (via GitHub)" <gi...@apache.org>.
zuccamia commented on PR #26045:
URL: https://github.com/apache/datafusion/pull/26045#issuecomment-6488116570
Hi @mikamikasuki 👋 Nice work on this! Sorry for jumping in, I came across your PR while looking into the same temporal `datafusion.format.*` options for #17498 as my first contribution. I'm wondering if you might have a plan to reuse your check here at config `SET` time as well? If not, I'd love to try it for validating `date_format` at `SET` once this lands, building on your work here. That's if the maintainers think a SET-time check is reasonable, of course.
Thank you!
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org