kosiew opened a new issue, #26362:
URL: https://github.com/apache/datafusion/issues/26362
# related PR
Related: apache/datafusion#17498. Follow-up to PR #25628; independently
actionable.
## Problem
`datafusion.execution.parquet.coerce_int96_tz` is an unchecked optional
string.
Invalid timezone text can enter configuration, then fail during Parquet
schema
inference or scan setup rather than at the offending SET/table option.
A regression should demonstrate early rejection of:
```sql
SET datafusion.execution.parquet.coerce_int96_tz = 'Not/A_Timezone';
```
Current behavior is established by code inspection, not an executed SQL
repro.
No panic is claimed: the current timezone parser returns a configuration
error.
## Evidence / root cause
- `datafusion/common/src/config.rs:1422`: `Option<String>` field.
- `datafusion/datasource-parquet/src/source.rs:542`:
`parse_coerce_int96_tz_string` parses Arrow's `Tz` at consumption time.
- `datafusion/datasource-parquet/src/file_format.rs`: schema inference calls
the
same helper.
- `datafusion/common/src/config.rs:2074`: existing `ConfigTimeZone` (added
for
`execution.time_zone`, #25788) wraps a private string, validates with the
same
Arrow `Tz` parse, preserves input spelling, and already has a parse-before-
assign `ConfigField for Option<ConfigTimeZone>`.
- Both protobuf decoder families currently transport the timezone as a
string.
The timezone syntax invariant is owned by consumers rather than
configuration.
## Why it matters
Invalid settings can remain latent until a Parquet operation uses them. Once
PR #25628 lands, the nearby INT96-unit option validates at assignment,
leaving inconsistent behavior
between related options and repeated late parsing.
## Invariant / desired behavior
A stored timezone is absent or accepted by the timezone parser used by the
Parquet reader. Failed updates preserve prior state, including None. Reset
restores absence.
Validation is unconditional at assignment, but application stays conditional:
without `coerce_int96`, a valid configured timezone still has no effect.
`coerce_int96 = None` must remain distinct from an explicit nanosecond unit.
Preserve timestamp unit, timezone spelling/metadata, and actual read values.
## Proposed direction
Reuse `ConfigTimeZone`: it and `parse_coerce_int96_tz_string` both accept
exactly what `s.parse::<arrow::array::timezone::Tz>()` accepts, so the
accepted
language is unchanged. Only the error message differs (it mentions UTC
offsets,
which are in fact accepted). Do not add another timezone wrapper.
Change the field to a validated optional timezone. Adapt consumers to obtain
an
`Arc<str>` from the validated text without repeating syntax validation.
Remove
the old parser once no callers remain. Keep runtime checks concerning actual
file/schema compatibility; they are not syntax validation.
## Scope
### In
- Session SET/reset, direct configuration APIs, and table-format options.
- Schema-inference and explicit-schema scan paths.
- Both protobuf decoders/encoders and affected FFI option transport.
- Docs and migration examples for public Rust field assignments.
### Out
- Changes to session timezone coercion, daylight-saving interpretation,
timezone normalization, accepted timezone language, or INT96 conversion
math.
- Applying this timezone when INT96 coercion is disabled.
- Broad timezone framework refactors or changes to unrelated string configs.
## Compatibility / risks
This intentionally rejects invalid text even while coercion is disabled;
today
such text can remain unused. Document the earlier error as a user-visible
change.
A typed public field is a Rust source break: use `api change` and add a
migration
example such as `Some("UTC".parse()?)`. Preserve protobuf string
fields/numbers.
Valid legacy messages must decode; invalid serialized timezones must return
errors, including when no INT96 unit is configured.
Do not assume the helper's IANA wording defines its full accepted language:
verify UTC-offset forms accepted by Arrow. Preserve case sensitivity and
original
spelling rather than introducing normalization.
## Acceptance criteria
- [ ] Invalid timezone SET/table options fail immediately, with or without a
unit.
- [ ] Invalid updates preserve Some/None state; reset restores None.
- [ ] Valid accepted timezone forms retain existing semantics and spelling.
- [ ] Both protobuf decoders reject malformed timezone strings.
- [ ] Valid/absent timezone and unit combinations round-trip.
- [ ] No unit means no timezone application; explicit units still apply it.
- [ ] Inferred and explicit-schema reads retain expected unit, metadata,
values.
- [ ] Consumers no longer parse configuration timezone syntax late.
- [ ] Migration and error-timing changes are documented.
## Tests / verification
- Unit tests: UTC, an IANA zone, offset forms supported by the current
parser,
invalid/empty input, failed replacement/unset update, nested keys, and
reset.
- SQLLogicTests: SET/SHOW/error/unchanged state/reset; invalid table options
with
inferred and explicit schemas using an existing Parquet fixture.
- INT96 read tests: configured timezone with unit absent, explicit
nanoseconds,
and microseconds. Assert output field types/timezone and values, not just
success.
- Protobuf tests for both conversion families: valid/absent round-trips and
invalid input independent of whether a unit is present.
- Check affected FFI config round-trips and builds without Parquet to avoid
accidentally making a general configuration type feature-dependent.
- Run targeted config/protobuf/INT96/SLT tests, then repository-required
formatting, lint, and extended validation. No runtime speedup is claimed.
--
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]
