You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "bvolpato (via GitHub)" <gi...@apache.org> on 2026/09/05 03:12:27 UTC
[PR] fix: reject NULL partition keys during partitioned writes [datafusion]
bvolpato opened a new pull request, #24951:
URL: https://github.com/apache/datafusion/pull/24951
## 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:
```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?
- Both new rejection tests fail against upstream production code at `35f58f53cde2f634c21f6370e385cf1d7e9bc55c` and pass with the fix.
- `cargo test --locked -p datafusion-datasource --lib`: 180 passed.
- `cargo fmt --all -- --check` and `git diff --check` passed.
- Draft pending all-target/all-feature Clippy, the extended workspace suite, and execution of `copy.slt`.
## 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 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] fix: reject NULL partition keys during partitioned writes [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on PR #24951:
URL: https://github.com/apache/datafusion/pull/24951#issuecomment-5662384609
🚀
@bvolpato
Thank you for your contribution.
--
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] fix: reject NULL partition keys during partitioned writes [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on code in PR #24951:
URL: https://github.com/apache/datafusion/pull/24951#discussion_r3977245241
##########
datafusion/datasource/src/write/demux.rs:
##########
@@ -595,3 +602,69 @@ fn compute_hive_style_file_path(
file_path.join(format!("{write_id}.{file_extension}"))
}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+ use arrow::array::{Array, ArrayRef, DictionaryArray, Int32Array};
+ use arrow::datatypes::Int32Type;
+
+ fn partition_batch(column: ArrayRef) -> RecordBatch {
+ RecordBatch::try_from_iter_with_nullable([("p", column, true)]).unwrap()
+ }
+
+ #[test]
+ fn partition_keys_reject_null_values() {
+ let columns: Vec<ArrayRef> = vec![
+ Arc::new(Int32Array::from(vec![Some(0), None])),
+ Arc::new(StringArray::from(vec![Some(""), None])),
+ ];
+ for column in columns {
+ let partition_by = vec![("p".to_string(), column.data_type().clone())];
+ let batch = partition_batch(column);
+ let err = compute_partition_keys_by_row(&batch, &partition_by).unwrap_err();
+ assert_eq!(
+ err.strip_backtrace(),
+ "Execution error: NULL values are not supported for partition column 'p'"
+ );
+ }
+ }
+
+ #[test]
+ fn partition_keys_accept_slice_without_nulls() {
+ let column = Int32Array::from(vec![None, Some(0), Some(1)]).slice(1, 2);
+ let batch = partition_batch(Arc::new(column));
+ let partition_by = vec![("p".to_string(), DataType::Int32)];
+ let keys = compute_partition_keys_by_row(&batch, &partition_by).unwrap();
+ assert_eq!(keys, vec![vec![Cow::Borrowed("0"), Cow::Borrowed("1")]]);
+ }
+
+ fn dictionary_with_null_value() -> DictionaryArray<Int32Type> {
Review Comment:
Could we also add a dictionary test where the dictionary key itself is NULL, rather than only testing a key that points to a NULL dictionary value? `logical_null_count()` intentionally accounts for both kinds of logical NULLs, so covering both cases would help keep this validation robust if Arrow's dictionary handling or these fixtures change in the future.
--
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] fix: reject NULL partition keys during partitioned writes [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew merged PR #24951:
URL: https://github.com/apache/datafusion/pull/24951
--
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] fix: reject NULL partition keys during partitioned writes [datafusion]
Posted by "bvolpato (via GitHub)" <gi...@apache.org>.
bvolpato commented on code in PR #24951:
URL: https://github.com/apache/datafusion/pull/24951#discussion_r3998888265
##########
datafusion/datasource/src/write/demux.rs:
##########
@@ -595,3 +602,69 @@ fn compute_hive_style_file_path(
file_path.join(format!("{write_id}.{file_extension}"))
}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+ use arrow::array::{Array, ArrayRef, DictionaryArray, Int32Array};
+ use arrow::datatypes::Int32Type;
+
+ fn partition_batch(column: ArrayRef) -> RecordBatch {
+ RecordBatch::try_from_iter_with_nullable([("p", column, true)]).unwrap()
+ }
+
+ #[test]
+ fn partition_keys_reject_null_values() {
+ let columns: Vec<ArrayRef> = vec![
+ Arc::new(Int32Array::from(vec![Some(0), None])),
+ Arc::new(StringArray::from(vec![Some(""), None])),
+ ];
+ for column in columns {
+ let partition_by = vec![("p".to_string(), column.data_type().clone())];
+ let batch = partition_batch(column);
+ let err = compute_partition_keys_by_row(&batch, &partition_by).unwrap_err();
+ assert_eq!(
+ err.strip_backtrace(),
+ "Execution error: NULL values are not supported for partition column 'p'"
+ );
+ }
+ }
+
+ #[test]
+ fn partition_keys_accept_slice_without_nulls() {
+ let column = Int32Array::from(vec![None, Some(0), Some(1)]).slice(1, 2);
+ let batch = partition_batch(Arc::new(column));
+ let partition_by = vec![("p".to_string(), DataType::Int32)];
+ let keys = compute_partition_keys_by_row(&batch, &partition_by).unwrap();
+ assert_eq!(keys, vec![vec![Cow::Borrowed("0"), Cow::Borrowed("1")]]);
+ }
+
+ fn dictionary_with_null_value() -> DictionaryArray<Int32Type> {
Review Comment:
added a separate test with a NULL dictionary key and entirely non-NULL dictionary values. this now covers both forms of logical NULL.
--
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] fix: reject NULL partition keys during partitioned writes [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #24951:
URL: https://github.com/apache/datafusion/pull/24951#issuecomment-5613676929
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/24951?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 81.82%. Comparing base ([`4c031a2`](https://app.codecov.io/gh/apache/datafusion/commit/4c031a267e8621cb0ec8c5c5a9853c4c0e1c97c0?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`2527ec7`](https://app.codecov.io/gh/apache/datafusion/commit/2527ec70bccdcb5fb3c19db07a0b2e66e58a7a30?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on main.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #24951 +/- ##
==========================================
- Coverage 81.83% 81.82% -0.01%
==========================================
Files 1130 1130
Lines 418634 418679 +45
Branches 418634 418679 +45
==========================================
+ Hits 342577 342597 +20
- Misses 55850 55873 +23
- Partials 20207 20209 +2
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/24951?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