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/09/22 18:53:49 UTC
[PR] fix: validate parquet.compression when it is set [datafusion]
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: 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.compression when it is set [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #25626:
URL: https://github.com/apache/datafusion/pull/25626#issuecomment-5783223833
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/25626?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 `95.72954%` with `12 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 82.43%. Comparing base ([`714956b`](https://app.codecov.io/gh/apache/datafusion/commit/714956b3a8aed43e8abd27ed400710417d231982?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`1f4c143`](https://app.codecov.io/gh/apache/datafusion/commit/1f4c14343002067a28e3b0d15b64bc107b5002ef?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 9 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/25626?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/parquet\_config.rs](https://app.codecov.io/gh/apache/datafusion/pull/25626?src=pr&el=tree&filepath=datafusion%2Fcommon%2Fsrc%2Fparquet_config.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9jb21tb24vc3JjL3BhcnF1ZXRfY29uZmlnLnJz) | 95.10% | [6 Missing and 3 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25626?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...tafusion/common/src/file\_options/parquet\_writer.rs](https://app.codecov.io/gh/apache/datafusion/pull/25626?src=pr&el=tree&filepath=datafusion%2Fcommon%2Fsrc%2Ffile_options%2Fparquet_writer.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9jb21tb24vc3JjL2ZpbGVfb3B0aW9ucy9wYXJxdWV0X3dyaXRlci5ycw==) | 87.50% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25626?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/core/src/dataframe/parquet.rs](https://app.codecov.io/gh/apache/datafusion/pull/25626?src=pr&el=tree&filepath=datafusion%2Fcore%2Fsrc%2Fdataframe%2Fparquet.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9jb3JlL3NyYy9kYXRhZnJhbWUvcGFycXVldC5ycw==) | 0.00% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25626?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/proto-models/src/from\_proto.rs](https://app.codecov.io/gh/apache/datafusion/pull/25626?src=pr&el=tree&filepath=datafusion%2Fproto-models%2Fsrc%2Ffrom_proto.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9wcm90by1tb2RlbHMvc3JjL2Zyb21fcHJvdG8ucnM=) | 87.50% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/25626?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 #25626 +/- ##
========================================
Coverage 82.42% 82.43%
========================================
Files 1140 1140
Lines 435586 435789 +203
Branches 435586 435789 +203
========================================
+ Hits 359042 359248 +206
+ Misses 54828 54822 -6
- Partials 21716 21719 +3
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/25626?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.compression when it is set [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #25626:
URL: https://github.com/apache/datafusion/pull/25626
--
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