You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "limadog9 (via GitHub)" <gi...@apache.org> on 2026/09/29 03:25:04 UTC
[PR] fix: Preserve column identifiers in DataFrame fill methods [datafusion]
limadog9 opened a new pull request, #25857:
URL: https://github.com/apache/datafusion/pull/25857
## Which issue does this PR close?
Closes #25829.
## Rationale for this change
`DataFrame::fill_null` and `fill_nan` report a schema error when a column is named `Name` or `a.b`, even if that column is not being filled. Rebuilding the projection parses these existing names as SQL identifiers, changing their case or treating a dot as a table qualifier.
## What changes are included in this PR?
Build column expressions directly from each schema entry, preserving the exact name and table qualifier in the replacement, untouched-column, and failed-cast paths.
Two existing Windows lint blockers are also corrected so the required all-target/all-feature Clippy check can run: a redundant semicolon and the intentionally async interface of the unavailable FIFO example. Neither changes runtime behavior.
## What is the testing strategy for this PR?
Ten DataFrame API regression cases cover both methods with uppercase and dotted names, untouched columns, uncastable replacement values, and duplicate qualified column names passed through a join.
All ten new cases fail with the expected schema errors without the fix; the eight existing fill tests pass on that baseline.
- `cargo fmt --all` passes.
- All 18 focused DataFrame fill tests pass (10 new regressions and 8 existing tests).
- `cargo clippy --all-targets --all-features -- -D warnings` passes with zero warnings.
No existing benchmark exercises these DataFrame methods; execution kernels are unchanged.
## Are there any user-facing changes?
Both methods accept existing column names literally and preserve qualifiers when passing columns through. Public API signatures and replacement/casting behavior are unchanged.
--
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: Preserve column identifiers in DataFrame fill methods [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on PR #25857:
URL: https://github.com/apache/datafusion/pull/25857#issuecomment-6288509596
@limadog9
I would love to review this after the merge conflicts are resolved.
--
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: Preserve column identifiers in DataFrame fill methods [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
kosiew commented on code in PR #25857:
URL: https://github.com/apache/datafusion/pull/25857#discussion_r4334087704
##########
datafusion/core/tests/dataframe/mod.rs:
##########
@@ -7029,6 +7029,106 @@ async fn test_fill_nan_uncastable_value() -> Result<()> {
Ok(())
}
+#[rstest]
+#[tokio::test]
+async fn test_fill_special_column_names(
+ #[values(false, true)] fill_nan: bool,
+ #[values("Name", "a.b")] name: &str,
+ #[values(false, true)] uncastable: bool,
+) -> Result<()> {
+ let fill = if fill_nan {
+ DataFrame::fill_nan
+ } else {
+ DataFrame::fill_null
+ };
+ let missing = if fill_nan { Some(f64::NAN) } else { None };
+ let other_name = if name == "Name" { "a.b" } else { "Name" };
+ let schema = Arc::new(Schema::new(vec![
+ Field::new(name, DataType::Float64, true),
+ Field::new(other_name, DataType::Utf8, true),
+ ]));
+ let batch = RecordBatch::try_new(
+ schema,
+ vec![
+ Arc::new(Float64Array::from(vec![Some(1.0), missing, Some(3.0)])),
+ Arc::new(StringArray::from(vec![Some("a"), None, Some("c")])),
+ ],
+ )?;
+ let ctx = SessionContext::new();
+ let df = ctx.read_batch(batch.clone())?;
+ let value = if uncastable {
+ ScalarValue::from("abc")
+ } else {
+ ScalarValue::from(0.0)
+ };
+
+ let results = fill(&df, &value, &[name])?.collect().await?;
+ let expected = if uncastable {
+ // An uncastable replacement leaves the selected column unchanged.
+ batch
+ } else {
+ RecordBatch::try_new(
+ batch.schema(),
+ vec![
+ Arc::new(Float64Array::from(vec![1.0, 0.0, 3.0])),
+ Arc::clone(batch.column(1)),
+ ],
+ )?
+ };
+ // Both selected and untouched names must be preserved literally.
+ assert_eq!(batches_to_string(&results), batches_to_string(&[expected]));
Review Comment:
Consider asserting the collected schema's field names and types directly. The formatted comparison checks headers and values but cannot distinguish numeric types with identical output; no need to require unchanged nullability.
--
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: Preserve column identifiers in DataFrame fill methods [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #25857:
URL: https://github.com/apache/datafusion/pull/25857#issuecomment-6299498076
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/25857?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 82.93%. Comparing base ([`d89a85d`](https://app.codecov.io/gh/apache/datafusion/commit/d89a85d3e340062a9efad3d8c4b32f10a7b139c5?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`b0883e0`](https://app.codecov.io/gh/apache/datafusion/commit/b0883e08c835187b235fefcdae6a4deff876df67?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 7 commits behind head on main.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #25857 +/- ##
==========================================
- Coverage 82.93% 82.93% -0.01%
==========================================
Files 1154 1154
Lines 457933 457932 -1
Branches 457933 457932 -1
==========================================
- Hits 379786 379776 -10
- Misses 55128 55131 +3
- Partials 23019 23025 +6
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/25857?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] fix: Preserve column identifiers in DataFrame fill methods [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #25857:
URL: https://github.com/apache/datafusion/pull/25857
--
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: Preserve column identifiers in DataFrame fill methods [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #25857:
URL: https://github.com/apache/datafusion/pull/25857#issuecomment-6382872949
This PR appears to be waiting on the author to respond to reviewer feedback so marking as draft. Please mark ready to review when it is ready for another look
--
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: Preserve column identifiers in DataFrame fill methods [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #25857:
URL: https://github.com/apache/datafusion/pull/25857#issuecomment-6452949462
Thank you @kosiew and @limadog9
--
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: Preserve column identifiers in DataFrame fill methods [datafusion]
Posted by "limadog9 (via GitHub)" <gi...@apache.org>.
limadog9 commented on PR #25857:
URL: https://github.com/apache/datafusion/pull/25857#issuecomment-6182942471
Hi @timsaucer @alamb, could you take a look at this fix for #25829 when you have a chance? It preserves column names and qualifiers in `fill_null`/`fill_nan`, with regression coverage for uppercase and dotted names.
The Actions runs are still `action_required`; could a maintainer approve CI as well? GitHub is also reporting merge conflicts with `main`, so a rebase is needed before merging. Thanks!
--
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: Preserve column identifiers in DataFrame fill methods [datafusion]
Posted by "limadog9 (via GitHub)" <gi...@apache.org>.
limadog9 commented on PR #25857:
URL: https://github.com/apache/datafusion/pull/25857#issuecomment-6289317466
@kosiew merge conflict resolved please take a look
--
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