Skip to content

minor: validate config default_null_ordering when setting it - #24401

Closed
shinzoxD wants to merge 1 commit into
apache:mainfrom
shinzoxD:validate-default-null-ordering
Closed

minor: validate config default_null_ordering when setting it#24401
shinzoxD wants to merge 1 commit into
apache:mainfrom
shinzoxD:validate-default-null-ordering

Conversation

@shinzoxD

Copy link
Copy Markdown

Which issue does this PR close?

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:

> 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.

`datafusion.sql_parser.default_null_ordering` was stored as a raw
string. Invalid values such as `nuls_max` or an empty string were
accepted at SET time and later silently treated as `nulls_max`.

Store the option as a typed `NullOrdering` enum so only the documented
values (`nulls_max`, `nulls_min`, `nulls_first`, `nulls_last`) can be
set, matching other enum config options such as `explain.format`.
Copilot AI lite review requested due to automatic review settings August 15, 2026 21:46

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@github-actions github-actions Bot added sql SQL Planner core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) common Related to common crate labels Aug 15, 2026
@shinzoxD

Copy link
Copy Markdown
Author

Closing as a duplicate of #24400 — same default_null_ordering validation, opened first. Please review #24400.

@shinzoxD shinzoxD closed this Aug 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

common Related to common crate core Core DataFusion crate sql SQL Planner sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants