tripleaceme opened a new pull request, #26501: URL: https://github.com/apache/datafusion/pull/26501
## Which issue does this PR close? - Closes #24904. #24905 fixed this before but was closed without review, so this is a new PR. It takes the reviewer feedback from #24905 into account: `data_page_row_count_limit = 0` is rejected too, and the valid ranges are in the `config.rs` docstrings. ## Rationale for this change Some Parquet writer options accept values that the `parquet` crate cannot use. The `SET` succeeds, and the next `COPY` panics (or hangs, for `write_batch_size = 0`): ```sql SET datafusion.execution.parquet.max_row_group_size = 0; COPY (SELECT 1) TO 'x.parquet'; -- panicked: Cannot have a 0 max row group row count ``` This PR rejects these values when they are set, which is how `batch_size` (#23054) and `max_row_group_bytes` already work. ## What changes are included in this PR? - `write_batch_size`, `max_row_group_size`, `data_page_row_count_limit` and `content_defined_chunking.min_chunk_size` are now `ConfigNonZeroUsize`. - `column_index_truncate_length` and `statistics_truncate_length` are now `Option<ConfigNonZeroUsize>`. They get their own `ConfigField` impl, so a rejected value does not leave the option half-set (same reason as the `Option<MaxRowGroupBytes>` impl). - New `BloomFilterFpp` type for the global and per-column `bloom_filter_fpp`. It accepts values strictly between 0 and 1, and rejects NaN. - `max_chunk_size > min_chunk_size` compares two options, so it cannot be checked at `SET` time. It is checked when the writer properties are built and returns a config error. - Protobuf decoding applies the same checks, so a serialized plan cannot bring back a value `SET` rejects. For the non-optional fields, a wire value of 0 is read as "not set" and replaced by the default. Proto3 cannot tell 0 from absent, and this keeps sparse messages decoding (same approach as the empty `writer_version` in #22467). An explicit 0 in an optional field (`*_opt`) is still an error. - `From<protobuf::ParquetColumnOptions> for ParquetColumnOptions` is now `TryFrom`, because an out-of-range `bloom_filter_fpp` has to be able to fail. ## What is the testing strategy for this PR? - New `parquet_writer_options_validation.slt`: each zero value and each bad `bloom_filter_fpp` (0, 1, 1.5, -0.1, NaN, text) through `SET`, a rejected `SET` keeping the old value, the same checks through `COPY ... OPTIONS` (global and per-column), and the chunk-size check. On `main` this file panics in the writer. - Two proto-common unit tests: a sparse `ParquetOptions` decodes to the defaults, and an explicit zero truncate length is rejected. - Existing tests updated for the new types. ## Are there any user-facing changes? Yes. Invalid values now fail at `SET` (or `COPY ... OPTIONS`) with a config error instead of panicking on write. The config docs list the valid ranges. This is an API change: the field types above changed, and the `ParquetColumnOptions` proto conversion is now `TryFrom`. Both are documented in the 56.0.0 upgrade guide. It needs the `api change` label. -- 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]
