shinzoxD opened a new pull request, #24401: URL: https://github.com/apache/datafusion/pull/24401
## Which issue does this PR close? - Part of #17498 ## Rationale for this change `datafusion.sql_parser.default_null_ordering` is documented to accept only four values (`nulls_max`, `nulls_min`, `nulls_first`, `nulls_last`), but it was stored as a raw `String`. Invalid values were accepted at SET time and later converted with `NullOrdering`'s `From<&str>` impl, which silently fell back to `nulls_max`. For example: ```sql > SET datafusion.sql_parser.default_null_ordering = nuls_max; 0 row(s) fetched. ``` That typo was stored and then treated as `nulls_max`, with no hint that the value was invalid. This is the same class of problem as `explain.format` (fixed in #17549) and the later per-option validation PRs on #17498. ## What changes are included in this PR? - Add a typed `NullOrdering` config enum in `datafusion-common` that validates values when the setting is applied. - Change `SqlParserOptions::default_null_ordering` from `String` to `NullOrdering`. - Re-export that enum from `datafusion-sql` so existing `datafusion_sql::planner::NullOrdering` users keep compiling. - Reject invalid values immediately, including the previously accepted empty string. - Accept the four documented values case-insensitively, matching other enum config options. ## Are these changes tested? Yes. - Unit test for valid values, case-insensitive parsing, invalid values, and leaving the previous setting unchanged on error - `set_variable.slt` coverage for an invalid SET - `order.slt` now expects empty / typo values to error instead of silently using `nulls_max` - `cargo test -p datafusion-common test_default_null_ordering_validation` - `cargo test -p datafusion-sql --lib` - `cargo test -p datafusion-sql --test sql_integration` - `cargo test -p datafusion-sqllogictest --test sqllogictests -- -- set_variable.slt` - `cargo clippy -p datafusion-common -p datafusion-sql -p datafusion --all-targets --all-features -- -D warnings` - `cargo fmt --all -- --check` ## Are there any user-facing changes? Yes. - `SET datafusion.sql_parser.default_null_ordering` now errors on invalid values instead of silently using `nulls_max`. - `SqlParserOptions::default_null_ordering` is now `NullOrdering` rather than `String` (API change). - `datafusion_sql::planner::NullOrdering` is a re-export of the common type. `From<&str>` (the silent fallback) is removed; parse with `FromStr` instead. -- 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]
