minor: validate config default_null_ordering when setting it - #24401
Closed
shinzoxD wants to merge 1 commit into
Closed
minor: validate config default_null_ordering when setting it#24401shinzoxD wants to merge 1 commit into
default_null_ordering when setting it#24401shinzoxD wants to merge 1 commit into
Conversation
`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`.
Author
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
datafusion.sql_parser.default_null_orderingis documented to accept only four values (nulls_max,nulls_min,nulls_first,nulls_last), but it was stored as a rawString. Invalid values were accepted at SET time and later converted withNullOrdering'sFrom<&str>impl, which silently fell back tonulls_max.For example:
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 asexplain.format(fixed in #17549) and the later per-option validation PRs on #17498.What changes are included in this PR?
NullOrderingconfig enum indatafusion-commonthat validates values when the setting is applied.SqlParserOptions::default_null_orderingfromStringtoNullOrdering.datafusion-sqlso existingdatafusion_sql::planner::NullOrderingusers keep compiling.Are these changes tested?
Yes.
set_variable.sltcoverage for an invalid SETorder.sltnow expects empty / typo values to error instead of silently usingnulls_maxcargo test -p datafusion-common test_default_null_ordering_validationcargo test -p datafusion-sql --libcargo test -p datafusion-sql --test sql_integrationcargo test -p datafusion-sqllogictest --test sqllogictests -- -- set_variable.sltcargo clippy -p datafusion-common -p datafusion-sql -p datafusion --all-targets --all-features -- -D warningscargo fmt --all -- --checkAre there any user-facing changes?
Yes.
SET datafusion.sql_parser.default_null_orderingnow errors on invalid values instead of silently usingnulls_max.SqlParserOptions::default_null_orderingis nowNullOrderingrather thanString(API change).datafusion_sql::planner::NullOrderingis a re-export of the common type.From<&str>(the silent fallback) is removed; parse withFromStrinstead.