You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "akashjainn (via GitHub)" <gi...@apache.org> on 2026/10/16 16:30:15 UTC
[PR] fix: validate parquet.coerce_int96_tz when it is set. [datafusion]
akashjainn opened a new pull request, #26420:
URL: https://github.com/apache/datafusion/pull/26420
## Which issue does this PR close?
- Closes #26362.
## Rationale for this change
`SET datafusion.execution.parquet.coerce_int96_tz = 'Not/A_Timezone'` succeeds today, and the error only shows up on the next Parquet read. After #25628 the unit option next to it is checked when it is set, so this does the same for the timezone.
## What changes are included in this PR?
`ParquetOptions::coerce_int96_tz` is now `Option<ConfigTimeZone>`, the type `execution.time_zone` already uses, so it accepts exactly what it accepted before. `parse_coerce_int96_tz_string` is removed, and both protobuf decoders parse the value.
An invalid value is rejected even when `coerce_int96` is unset, as the issue asked.
## What is the testing strategy for this PR?
- `set_variable.slt`: SET/SHOW keeps the spelling (`+08`), a bad value is rejected and leaves the old one, RESET goes back to NULL, and bad table options fail at CREATE with an inferred and an explicit schema. These fail on main.
- `test_parquet_coerce_int96_tz_validation` in `config.rs`.
- A rejection test for each protobuf decoder, and the existing round-trip tests use the typed value.
- The existing timezone read tests in `parquet.slt` pass unchanged.
## Are there any user-facing changes?
Yes. Invalid timezones are rejected when set. The public field type changes, so this needs `api change`. I added a note to the 56.0.0 upgrade guide.
--
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 parquet.coerce_int96_tz when it is set. [datafusion]
Posted by "akashjainn (via GitHub)" <gi...@apache.org>.
akashjainn commented on code in PR #26420:
URL: https://github.com/apache/datafusion/pull/26420#discussion_r4315900589
##########
docs/source/library-user-guide/upgrading/56.0.0.md:
##########
@@ -403,6 +403,30 @@ options.execution.time_zone = Some("America/Denver".parse()?);
let tz: Option<&str> = options.execution.time_zone.as_ref().map(|tz| tz.as_str());
```
+### `ParquetOptions::coerce_int96_tz` is now a validated `ConfigTimeZone`
+
+`ParquetOptions::coerce_int96_tz` changed from `Option<String>` to
+`Option<ConfigTimeZone>`, the same type as `ExecutionOptions::time_zone`. An
+invalid time zone is now rejected by `SET` and by table options instead of on
Review Comment:
Added that to the upgrade guide here: daaf6e4534e633009e4e362d7317d84351f6f89c. Thanks!
--
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 parquet.coerce_int96_tz when it is set. [datafusion]
Posted by "akashjainn (via GitHub)" <gi...@apache.org>.
akashjainn commented on code in PR #26420:
URL: https://github.com/apache/datafusion/pull/26420#discussion_r4315900589
##########
docs/source/library-user-guide/upgrading/56.0.0.md:
##########
@@ -403,6 +403,30 @@ options.execution.time_zone = Some("America/Denver".parse()?);
let tz: Option<&str> = options.execution.time_zone.as_ref().map(|tz| tz.as_str());
```
+### `ParquetOptions::coerce_int96_tz` is now a validated `ConfigTimeZone`
+
+`ParquetOptions::coerce_int96_tz` changed from `Option<String>` to
+`Option<ConfigTimeZone>`, the same type as `ExecutionOptions::time_zone`. An
+invalid time zone is now rejected by `SET` and by table options instead of on
Review Comment:
Added that to the docstring here: daaf6e4534e633009e4e362d7317d84351f6f89c. Thanks!
--
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 parquet.coerce_int96_tz when it is set. [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew merged PR #26420:
URL: https://github.com/apache/datafusion/pull/26420
--
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 parquet.coerce_int96_tz when it is set. [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #26420:
URL: https://github.com/apache/datafusion/pull/26420#issuecomment-6221245186
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/26420?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 `97.72727%` with `2 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 82.89%. Comparing base ([`1da3cf0`](https://app.codecov.io/gh/apache/datafusion/commit/1da3cf049fa1a7a708acc2c29eba4f2e9dece8db?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`046e41c`](https://app.codecov.io/gh/apache/datafusion/commit/046e41cf9ba78c2728c066563d3de592e4cad575?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/26420?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/datasource-parquet/src/file\_format.rs](https://app.codecov.io/gh/apache/datafusion/pull/26420?src=pr&el=tree&filepath=datafusion%2Fdatasource-parquet%2Fsrc%2Ffile_format.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9kYXRhc291cmNlLXBhcnF1ZXQvc3JjL2ZpbGVfZm9ybWF0LnJz) | 50.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/26420?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/ffi/src/session/mod.rs](https://app.codecov.io/gh/apache/datafusion/pull/26420?src=pr&el=tree&filepath=datafusion%2Fffi%2Fsrc%2Fsession%2Fmod.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9mZmkvc3JjL3Nlc3Npb24vbW9kLnJz) | 0.00% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/26420?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 #26420 +/- ##
=======================================
Coverage 82.89% 82.89%
=======================================
Files 1153 1153
Lines 457487 457548 +61
Branches 457487 457548 +61
=======================================
+ Hits 379213 379281 +68
+ Misses 55264 55259 -5
+ Partials 23010 23008 -2
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/26420?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 parquet.coerce_int96_tz when it is set. [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on code in PR #26420:
URL: https://github.com/apache/datafusion/pull/26420#discussion_r4310387656
##########
datafusion/sqllogictest/test_files/parquet.slt:
##########
@@ -784,6 +784,120 @@ ERROR: Cast error: Failed to convert -9357363680509551 to datetime for Timestamp
statement ok
drop table int96_from_spark;
+# Without coerce_int96 the timezone has no effect: same type and values as the
+# default read above.
+statement ok
+reset datafusion.execution.parquet.coerce_int96;
+
+statement ok
+CREATE EXTERNAL TABLE int96_from_spark
+STORED AS PARQUET
+LOCATION '../../parquet-testing/data/int96_from_spark.parquet';
+
+query TTT
+describe int96_from_spark;
+----
+a Timestamp(ns) YES
+
+query P
+select * from int96_from_spark
+----
+2024-01-01T20:34:56.123456
+2024-01-01T01:00:00
+1816-03-29T08:56:08.066277376
+2024-12-30T23:00:00
+NULL
+1815-11-08T16:01:01.191053312
+
+statement ok
+drop table int96_from_spark;
+
+# An explicit nanosecond unit applies the timezone.
Review Comment:
Could we add a small INT96 read test using `+08:00` to check the Arrow timezone and an in-range timestamp rendered at that offset? The current read tests only use UTC, so this would help catch accidental reinterpretation of the stored instant. Please reset the setting afterward. This is optional and not a blocker.
##########
docs/source/library-user-guide/upgrading/56.0.0.md:
##########
@@ -403,6 +403,30 @@ options.execution.time_zone = Some("America/Denver".parse()?);
let tz: Option<&str> = options.execution.time_zone.as_ref().map(|tz| tz.as_str());
```
+### `ParquetOptions::coerce_int96_tz` is now a validated `ConfigTimeZone`
+
+`ParquetOptions::coerce_int96_tz` changed from `Option<String>` to
+`Option<ConfigTimeZone>`, the same type as `ExecutionOptions::time_zone`. An
+invalid time zone is now rejected by `SET` and by table options instead of on
Review Comment:
Could we also mention that protobuf decoding now rejects invalid timezones, even when `coerce_int96` is unset? This means malformed legacy options fail during restoration rather than at read time, while valid values and the wire format remain compatible. A short sentence would be enough, and this is not a blocker.
--
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 parquet.coerce_int96_tz when it is set. [datafusion]
Posted by "akashjainn (via GitHub)" <gi...@apache.org>.
akashjainn commented on code in PR #26420:
URL: https://github.com/apache/datafusion/pull/26420#discussion_r4315898297
##########
datafusion/sqllogictest/test_files/parquet.slt:
##########
@@ -784,6 +784,120 @@ ERROR: Cast error: Failed to convert -9357363680509551 to datetime for Timestamp
statement ok
drop table int96_from_spark;
+# Without coerce_int96 the timezone has no effect: same type and values as the
+# default read above.
+statement ok
+reset datafusion.execution.parquet.coerce_int96;
+
+statement ok
+CREATE EXTERNAL TABLE int96_from_spark
+STORED AS PARQUET
+LOCATION '../../parquet-testing/data/int96_from_spark.parquet';
+
+query TTT
+describe int96_from_spark;
+----
+a Timestamp(ns) YES
+
+query P
+select * from int96_from_spark
+----
+2024-01-01T20:34:56.123456
+2024-01-01T01:00:00
+1816-03-29T08:56:08.066277376
+2024-12-30T23:00:00
+NULL
+1815-11-08T16:01:01.191053312
+
+statement ok
+drop table int96_from_spark;
+
+# An explicit nanosecond unit applies the timezone.
Review Comment:
Fixed here: daaf6e4534e633009e4e362d7317d84351f6f89c. 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
Re: [PR] fix: validate parquet.coerce_int96_tz when it is set. [datafusion]
Posted by "akashjainn (via GitHub)" <gi...@apache.org>.
akashjainn commented on PR #26420:
URL: https://github.com/apache/datafusion/pull/26420#issuecomment-6220831993
@kosiew would love a review on this when you get a chance! 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
Re: [PR] fix: validate parquet.coerce_int96_tz when it is set. [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on PR #26420:
URL: https://github.com/apache/datafusion/pull/26420#issuecomment-6285439510
🚀
@akashjainn
Thank you for your contribution.
--
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