minor: validate config default_null_ordering when setting it - #24400
Open
shinzoxD wants to merge 1 commit into
Open
minor: validate config default_null_ordering when setting it#24400shinzoxD wants to merge 1 commit into
default_null_ordering when setting it#24400shinzoxD wants to merge 1 commit into
Conversation
`datafusion.sql_parser.default_null_ordering` documented four valid values but stored a free-form string. Invalid values were accepted at SET time and later silently fell back to `nulls_max`. Store the option as a typed `NullOrdering` enum so unknown values are rejected immediately, matching other enum-like config options.
github-actions
Bot
added
sql
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
SET datafusion.sql_parser.default_null_orderingaccepted any string. Invalid values such asnulls_mxor an empty string succeeded and later silently fell back tonulls_maxwhen planningORDER BY.That is the same class of bug as
explain.formatin #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?
NullOrderingintodatafusion-commonand use it as the type ofSqlParserOptions::default_null_ordering.nulls_max,nulls_min,nulls_first,nulls_last).datafusion_sql::planner::NullOrderingas a re-export so existing planner call sites keep compiling.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_orderingcargo test -p datafusion-sqllogictest --test sqllogictests -- set_variablecargo check -p datafusion --libcargo fmton the touched Rust filesCoverage includes valid values, case-insensitive input, typo rejection, empty-string rejection, and RESET back to
nulls_max.Are there any user-facing changes?
Yes.
SET datafusion.sql_parser.default_null_orderingvalues now fail immediately instead of being stored and later treated asnulls_max.SqlParserOptions::default_null_orderingis nowNullOrderingrather thanString.NullOrderingremains available fromdatafusion_sql::planner.