minor: validate config default_null_ordering when setting it

[shinzoxD]

Which issue does this PR close?

• Part of The config values aren't checked #17498

Rationale for this change

SET datafusion.sql_parser.default_null_ordering accepted any string. Invalid values such as nulls_mx or an empty string succeeded and later silently fell back to nulls_max when planning ORDER BY.

That is the same class of bug as explain.format in #17498: the option documents a fixed set of values, but the config store is a free-form string.

What changes are included in this PR?

• Move NullOrdering into datafusion-common and use it as the type of SqlParserOptions::default_null_ordering.
• Reject unknown values at SET time, with an error that lists the valid options (nulls_max, nulls_min, nulls_first, nulls_last).
• Keep datafusion_sql::planner::NullOrdering as a re-export so existing planner call sites keep compiling.
• Leave the previous valid value in place when a SET is rejected.

This is intentionally scoped to one option, following the review guidance on #17498 to land these as smaller PRs.

Are these changes tested?

Yes.

• cargo test -p datafusion-common default_null_ordering
• cargo test -p datafusion-sqllogictest --test sqllogictests -- set_variable
• cargo check -p datafusion --lib
• cargo fmt on the touched Rust files

Coverage includes valid values, case-insensitive input, typo rejection, empty-string rejection, and RESET back to nulls_max.

Are there any user-facing changes?

Yes.

• Invalid SET datafusion.sql_parser.default_null_ordering values now fail immediately instead of being stored and later treated as nulls_max.
• SqlParserOptions::default_null_ordering is now NullOrdering rather than String. NullOrdering remains available from datafusion_sql::planner.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[kosiew]

@shinzoxD,

Thanks for working on this. The move from a free-form default_null_ordering string to a typed NullOrdering looks good. I also like that the parsing, formatting, and validation now live in datafusion-common, so invalid values are caught when the configuration is set rather than silently falling back later.

The regression coverage looks good too, including valid and invalid values, preserving the configured value after a rejected SET, and resetting to the default.

Looks good to me. Thanks!

[github-actions]

Thank you for opening this pull request!

Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch).

Details

 Cloning apache/main
Building datafusion v54.1.0 (current)
Built [ 57.610s] (current)
Parsing datafusion v54.1.0 (current)
Parsed [ 0.036s] (current)
Building datafusion v54.1.0 (baseline)
Built [ 58.242s] (baseline)
Parsing datafusion v54.1.0 (baseline)
Parsed [ 0.038s] (baseline)
Checking datafusion v54.1.0 -> v54.1.0 (no change; assume patch)
Checked [ 0.991s] 223 checks: 223 pass, 31 skip
Summary no semver update required
Finished [ 118.865s] datafusion
Building datafusion-common v54.1.0 (current)
Built [ 34.094s] (current)
Parsing datafusion-common v54.1.0 (current)
Parsed [ 0.067s] (current)
Building datafusion-common v54.1.0 (baseline)
Built [ 34.411s] (baseline)
Parsing datafusion-common v54.1.0 (baseline)
Parsed [ 0.066s] (baseline)
Checking datafusion-common v54.1.0 -> v54.1.0 (no change; assume patch)
Checked [ 1.025s] 223 checks: 223 pass, 31 skip
Summary no semver update required
Finished [ 70.776s] datafusion-common
Building datafusion-sql v54.1.0 (current)
Built [ 42.366s] (current)
Parsing datafusion-sql v54.1.0 (current)
Parsed [ 0.033s] (current)
Building datafusion-sql v54.1.0 (baseline)
Built [ 42.382s] (baseline)
Parsing datafusion-sql v54.1.0 (baseline)
Parsed [ 0.035s] (baseline)
Checking datafusion-sql v54.1.0 -> v54.1.0 (no change; assume patch)
Checked [ 0.335s] 223 checks: 222 pass, 1 fail, 0 warn, 31 skip
--- failure enum_missing: pub enum removed or renamed ---
Description:
A publicly-visible enum cannot be imported by its prior path. A `pub use` may have been removed, or the enum itself may have been renamed or removed entirely.
ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.50.0/src/lints/enum_missing.ron
Failed in:
enum datafusion_sql::planner::NullOrdering, previously in file /home/runner/work/datafusion/datafusion/target/semver-checks/git-apache_main/6eb2dd7e4b70d2305875b6754fa28edc91ff189d/datafusion/sql/src/planner.rs:169
Summary semver requires new major version: 1 major and 0 minor checks failed
Finished [ 86.197s] datafusion-sql
Building datafusion-sqllogictest v54.1.0 (current)
Built [ 98.912s] (current)
Parsing datafusion-sqllogictest v54.1.0 (current)
Parsed [ 0.023s] (current)
Building datafusion-sqllogictest v54.1.0 (baseline)
Built [ 99.519s] (baseline)
Parsing datafusion-sqllogictest v54.1.0 (baseline)
Parsed [ 0.025s] (baseline)
Checking datafusion-sqllogictest v54.1.0 -> v54.1.0 (no change; assume patch)
Checked [ 0.112s] 223 checks: 223 pass, 31 skip
Summary no semver update required
Finished [ 201.586s] datafusion-sqllogictest

[codecov-commenter]

Codecov Report

❌ Patch coverage is 93.18182% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.18%. Comparing base (e7e037d) to head (788f2d1).
⚠️ Report is 136 commits behind head on main.

Files with missing lines	Patch %	Lines
datafusion/common/src/config.rs	93.02%	6 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #24400 +/- ##
=======================================
Coverage 81.18% 81.18% =======================================
Files 1110 1110 Lines 388906 388971 +65 Branches 388906 388971 +65 =======================================
+ Hits 315733 315786 +53 - Misses 54576 54586 +10 - Partials 18597 18599 +2 

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Jefffrey]

for anyone looking at this PR please see my comment here:

• fix: roll back DiskManager usage when spill write I/O fails #24407 (comment)

[alamb]

What shall we do with this PR? Is it mergeable? Should we close it?