akashjainn opened a new pull request, #25626: URL: https://github.com/apache/datafusion/pull/25626
## Which issue does this PR close? - Part of #17498. This covers `datafusion.execution.parquet.compression` only. ## Rationale for this change The option is stored as a plain string, so `SET datafusion.execution.parquet.compression = 'zstdd(3)'` succeeds, `SHOW` reports the bad value, and the error only appears later when a Parquet file is written. The same goes for a codec without its level (`zstd`), a level on a codec that takes none (`snappy(2)`), and a level out of range (`zstd(23)`). The issue is about catching these at `SET` time, and this option was converted in the first attempt (#17697) and planned in #19498 but never landed. ## What changes are included in this PR? - `DFParquetCompression` in `parquet_config.rs`, an enum of the seven supported codecs with the level carried by `Gzip`, `Brotli` and `Zstd`. `FromStr` does the validation, `Display` produces the same strings the option stored before (`zstd(3)`, `lz4_raw`), and the `ConfigField` impls follow `DFParquetStatistics`, including the `Option` handling that leaves an unset value unset when a bad value is rejected. - `ParquetOptions.compression` becomes `Option<DFParquetCompression>`, with the same default of `zstd(3)`. The per-column `ParquetColumnOptions.compression` stays a string, matching the scope of #24642. - `parse_compression_string` is now a two-line wrapper over the new type, so there is one parser. Its error messages are unchanged. - Proto conversion parses on the way in, so an invalid value is rejected there too. - The level ranges are mirrored from the parquet crate as constants, with a test that fails if they ever differ from what `GzipLevel`, `BrotliLevel` and `ZstdLevel` accept. That test caught my first copy of the zstd range, which came from parquet 59; the workspace uses parquet 60, where `zstd(0)` is accepted, and it still is. Behavior notes: - A malformed closing parenthesis such as `zstd(3x` used to be read as `zstd(3)` and is now rejected. - An out-of-range level gets a DataFusion message naming the valid range instead of the parquet crate's message. - The error for a bad value set through `COPY ... OPTIONS` now arrives when the option is set, wrapped in "Error setting config ...", which one existing test in `listing/table.rs` asserted on and now expects. This changes the type of a public field, so it should carry the `api change` label. Two call sites in this repository that assigned a string needed to parse instead. ## What is the testing strategy for this PR? - `parquet_config.rs`: parsing and display for every codec, nine rejected inputs with their messages, and the range-drift test above. - `config.rs`: `test_parquet_compression_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 round trip and a rejected value from proto. - `set_variable.slt`: set, show, three rejected values with their full error text, and that the setting is unchanged afterwards. - Run locally: `datafusion-common` with all features (628 unit tests) and without the parquet feature, `proto-common` and `proto-models`, `datasource-parquet` (271), the `set_variable` and `information_schema` sqllogictest files, the core listing, parquet and dataframe unit tests (114) and `parquet_integration` (242). `cargo clippy --workspace --all-targets --all-features -- -D warnings`, `cargo fmt --check`, typos and `dev/update_config_docs.sh` are clean; the generated docs did not change because the default displays the same. ## Are there any user-facing changes? Yes. An invalid `datafusion.execution.parquet.compression` value is rejected by `SET` and by `COPY ... OPTIONS` with a message, instead of being accepted and failing at write time. The field type in `ParquetOptions` changes from `Option<String>` to `Option<DFParquetCompression>`. -- 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]
