akashjainn opened a new pull request, #25628:
URL: https://github.com/apache/datafusion/pull/25628
## Which issue does this PR close?
- Part of #17498. This covers `datafusion.execution.parquet.coerce_int96`
only.
## Rationale for this change
The option is stored as a plain string, so `SET
datafusion.execution.parquet.coerce_int96 = 'nanos'` succeeds, `SHOW` reports
the bad value, and the error only appears on the first Parquet scan. The same
happens through `CREATE EXTERNAL TABLE ... OPTIONS`. #20253 fixed this in
January and was reviewed, but went stale and was closed in June; this picks it
up and applies that review.
## What changes are included in this PR?
- `DFTimeUnit` in `parquet_config.rs` with `ns`, `us`, `ms` and `s`, parsed
case-insensitively and displayed as those strings, so `SHOW` and the proto
encoding are unchanged. `ConfigField` follows `DFParquetStatistics`, including
the `Option` handling that leaves an unset value unset when a bad one is
rejected. `From<DFTimeUnit> for arrow::datatypes::TimeUnit` replaces the string
parser.
- `ParquetOptions.coerce_int96` becomes `Option<DFTimeUnit>`.
`ParquetFormat::coerce_int96()` and `with_coerce_int96()` take and return the
typed value.
- `parse_coerce_int96_string` in `datasource-parquet` is removed.
`ParquetSource` called it with `unwrap()` on the stored string; with a typed
field there is nothing to parse there.
- Proto conversion parses on the way in, so an invalid value is rejected
there too.
- The docstring for the option read as if it were a boolean ("If true,
..."), which was asked for on #20253; it now says what the value is and lists
the valid values. `information_schema.slt` and the generated `configs.md` carry
that text and are updated.
- The `Option` is kept. #20253 discussed dropping it since `None` behaves
like `ns`, but `coerce_int96_tz` was added since and only applies when
`coerce_int96` is set, so `None` and `Some(ns)` now differ.
This changes the type of a public field and two `ParquetFormat` methods, so
it should carry the `api change` label. One FFI test assigned the literal
`"123456789"` to the field; it now assigns a typed value.
## What is the testing strategy for this PR?
- `parquet_config.rs`: parsing, display and three rejected inputs.
- `config.rs`: `test_parquet_coerce_int96_validation`, mirroring the
statistics test, including that a rejected value leaves the previous one in
place and that an unset value stays unset.
- `proto-common`: a rejected value from proto, and the two existing
coerce_int96 round-trip tests now use the typed value.
- `set_variable.slt`: set, show, one rejected value with its full error
text, and that the setting is unchanged afterwards.
- Run locally: `datafusion-common` with all features and without the parquet
feature, `proto-common`, `proto-models`, `datasource-parquet` (271),
`datafusion-ffi`'s session test, the core Parquet unit tests including the
existing coerce_int96 data tests (93) and `parquet_integration` (242), and the
`set_variable` and `information_schema` sqllogictest files. `cargo clippy
--workspace --all-targets --all-features -- -D warnings`, `cargo fmt --check`,
typos, prettier and `dev/update_config_docs.sh` are clean.
## Are there any user-facing changes?
Yes. An invalid `datafusion.execution.parquet.coerce_int96` value is
rejected by `SET` and by table options with a message, instead of being
accepted and failing at scan time. The field type in `ParquetOptions` changes
from `Option<String>` to `Option<DFTimeUnit>`, as do the two `ParquetFormat`
accessors.
--
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]
