You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "shinzoxD (via GitHub)" <gi...@apache.org> on 2026/08/15 21:36:55 UTC
[PR] minor: validate config `default_null_ordering` when setting it [datafusion]
shinzoxD opened a new pull request, #24400:
URL: https://github.com/apache/datafusion/pull/24400
## Which issue does this PR close?
- Part of #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`.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] minor: validate config `default_null_ordering` when setting it [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #24400:
URL: https://github.com/apache/datafusion/pull/24400#issuecomment-5341950411
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/24400?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `93.18182%` with `6 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 81.18%. Comparing base ([`e7e037d`](https://app.codecov.io/gh/apache/datafusion/commit/e7e037dbeb6e578f13a6ba16d528366e888c253a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`788f2d1`](https://app.codecov.io/gh/apache/datafusion/commit/788f2d1d1b3128dc3c2b97e2af0e3e8bb58ef232?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 37 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/24400?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [datafusion/common/src/config.rs](https://app.codecov.io/gh/apache/datafusion/pull/24400?src=pr&el=tree&filepath=datafusion%2Fcommon%2Fsrc%2Fconfig.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9jb21tb24vc3JjL2NvbmZpZy5ycw==) | 93.02% | [6 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24400?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ 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
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/24400?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] minor: validate config `default_null_ordering` when setting it [datafusion]
Posted by "Jefffrey (via GitHub)" <gi...@apache.org>.
Jefffrey commented on PR #24400:
URL: https://github.com/apache/datafusion/pull/24400#issuecomment-5350201238
for anyone looking at this PR please see my comment here:
- https://github.com/apache/datafusion/pull/24407#issuecomment-5350186733
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] minor: validate config `default_null_ordering` when setting it [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #24400:
URL: https://github.com/apache/datafusion/pull/24400#issuecomment-5591784856
What shall we do with this PR? Is it mergeable? Should we close it?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] minor: validate config `default_null_ordering` when setting it [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew closed pull request #24400: minor: validate config `default_null_ordering` when setting it
URL: https://github.com/apache/datafusion/pull/24400
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] minor: validate config `default_null_ordering` when setting it [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #24400:
URL: https://github.com/apache/datafusion/pull/24400#issuecomment-5341657943
<!-- semver-check-comment -->
Thank you for opening this pull request!
Reviewer note: [cargo-semver-checks](https://github.com/obi1kenobi/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>
<summary>Details</summary>
```
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
```
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org