Repository navigation
fix: reject NULL partition keys during partitioned writes - #24951
Merged
kosiew merged 2 commits intoSep 14, 2026
Merged
Conversation
github-actions
Bot
added
sqllogictest
bvolpato
force-pushed
the
bvolpato/reject-null-partition-writes
branch
from
September 10, 2026 04:54
ddf6bdb to
2527ec7
Compare
bvolpato
marked this pull request as ready for review
September 10, 2026 04:56
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #24951 +/- ##
==========================================
+ Coverage 81.83% 81.95% +0.11%
==========================================
Files 1130 1133 +3
Lines 418634 423855 +5221
Branches 418634 423855 +5221
==========================================
+ Hits 342577 347354 +4777
- Misses 55850 55907 +57
- Partials 20207 20594 +387 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
kosiew
approved these changes
Sep 10, 2026
kosiew
left a comment
kosiew
left a comment
Contributor
There was a problem hiding this comment.
Thanks for working on this. The NULL validation looks good overall, and the added coverage for primitive, string, sliced, dictionary, and COPY cases gives me confidence in the change.
I just have one non-blocking suggestion around the dictionary test coverage.
kosiew
approved these changes
Sep 14, 2026
kosiew
left a comment
kosiew
left a comment
Contributor
There was a problem hiding this comment.
@bvolpato, thanks for the follow-up!
The new fixture in b81280ef99 addresses the earlier feedback nicely. It now specifically covers a dictionary array with a NULL key and non-NULL dictionary values, while dictionary_with_null_value() continues to cover the separate case where a valid key resolves to a NULL dictionary value.
The added test makes the distinction between these cases clear, and I don't see any new issues with the follow-up change.
Looks good to me!
Contributor
|
🚀 |
16 tasks
Omega359
pushed a commit
to Omega359/arrow-datafusion
that referenced
this pull request
Oct 11, 2026
## Which issue does this PR close? Related to apache#18083. Full support for NULL partition values remains covered by that issue. ## Rationale for this change Partitioned writes read partition values without checking their validity. A NULL integer partition key can be written under `p=0`, so reading the output turns NULL into zero and silently changes the data. For example: ```sql CREATE TABLE src(p INT, v INT) AS VALUES (NULL, 1), (0, 2); COPY src TO '/tmp/null_partition/' STORED AS PARQUET PARTITIONED BY (p); ``` Both rows can end up in the zero partition. This PR rejects the write with `NULL values are not supported for partition column 'p'`. ## What changes are included in this PR? - Check logical nulls before extracting partition keys, including NULL dictionary values referenced by valid dictionary indices. - Continue accepting nullable columns when the selected rows contain no NULL values. - Add focused unit tests and a `copy.slt` regression covering rejection and a filtered successful write. ## What is the testing strategy for this PR? - The rejection regressions fail without the production fix and pass with it. - Focused partition-key tests: 5 passed with `RUST_BACKTRACE=1`. - `cargo clippy --all-targets --all-features -- -D warnings` passed. - The extended workspace Rust suites passed after excluding four host memory-measurement runners that also fail on current main in this environment. - The SQL logic harness passed all 513 files, including encrypted Parquet coverage. - `cargo fmt --all -- --check` and `git diff --check` passed. ## Are there any user-facing changes? Partitioned writes containing NULL partition keys now return an execution error. Callers can filter or replace these values before writing. Validation happens per batch, so files written by earlier batches can remain after an error.
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?
Related to #18083. Full support for NULL partition values remains covered by that issue.
Rationale for this change
Partitioned writes read partition values without checking their validity. A NULL integer partition key can be written under
p=0, so reading the output turns NULL into zero and silently changes the data.For example:
Both rows can end up in the zero partition. This PR rejects the write with
NULL values are not supported for partition column 'p'.What changes are included in this PR?
copy.sltregression covering rejection and a filtered successful write.What is the testing strategy for this PR?
RUST_BACKTRACE=1.cargo clippy --all-targets --all-features -- -D warningspassed.cargo fmt --all -- --checkandgit diff --checkpassed.Are there any user-facing changes?
Partitioned writes containing NULL partition keys now return an execution error. Callers can filter or replace these values before writing. Validation happens per batch, so files written by earlier batches can remain after an error.