Stop mutating the source column dtype in equivalent-type numerical casts - #23364
Merged
rapids-bot[bot] merged 1 commit intoJul 21, 2026
Conversation
NumericalColumn.as_numerical_column short-circuits casts between equivalent dtypes (same pylibcudf type, e.g. float64 -> Float64), but did so by assigning the target dtype onto self._dtype in place. The column object is shared with the caller's Series/DataFrame, so the source frame silently changed dtype as a side effect of the cast. Return a fresh column over the same pylibcudf data instead (nans_to_nulls first for float -> masked casts). Fixes 5 pandas-tests (stack on nullable dtypes, loc setitem with Float64, assert_series/frame_equal extension-dtype mismatch); their xfail entries are removed.
github-actions
Bot
added
Python
galipremsagar
added
bug
Contributor
Author
|
/okay to test 20f09d4 |
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesNullable dtype casting
Pandas failure mappings
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/cudf/cudf/tests/series/methods/test_astype.py`:
- Around line 1631-1636: Extend the parameterization for the Float64Dtype case
in test_astype.py to cover empty, all-null, single-element, and mixed numeric/NA
float inputs, preserving the expected source dtype and nullable masked dtype for
each case. Keep the existing integer case and current float case intact while
ensuring these additions exercise both shared-buffer and NaN-to-null conversion
paths.
- Around line 1638-1645: Update
test_astype_masked_equivalent_dtype_no_source_mutation to wrap Series
construction and the astype call in
cudf.option_context("mode.pandas_compatible", True), ensuring the test exercises
the equivalent-dtype short-circuit path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: fbb90d16-858e-45ba-8599-dbc058631c3e
📒 Files selected for processing (3)
python/cudf/cudf/core/column/numerical.pypython/cudf/cudf/pandas/scripts/pandas-testing-plugin.pypython/cudf/cudf/tests/series/methods/test_astype.py
💤 Files with no reviewable changes (1)
- python/cudf/cudf/pandas/scripts/pandas-testing-plugin.py
Comment on lines
+1631
to
+1636
| @pytest.mark.parametrize( | ||
| "data, src_dtype, masked_dtype", | ||
| [ | ||
| ([1.0, 2.0, float("nan")], "float64", pd.Float64Dtype()), | ||
| ([1, 2, 3], "int64", pd.Int64Dtype()), | ||
| ], |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add nullable-cast boundary cases.
Include empty, all-null, single-element, and mixed numeric/NA float inputs for the Float64Dtype case so the shared-buffer and NaN-to-null paths remain covered. As per coding guidelines, test files must provide comprehensive edge case coverage for “empty, all-null, single-element, mixed types.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/cudf/cudf/tests/series/methods/test_astype.py` around lines 1631 -
1636, Extend the parameterization for the Float64Dtype case in test_astype.py to
cover empty, all-null, single-element, and mixed numeric/NA float inputs,
preserving the expected source dtype and nullable masked dtype for each case.
Keep the existing integer case and current float case intact while ensuring
these additions exercise both shared-buffer and NaN-to-null conversion paths.
Source: Coding guidelines
Comment on lines
+1638
to
+1645
| def test_astype_masked_equivalent_dtype_no_source_mutation( | ||
| data, src_dtype, masked_dtype | ||
| ): | ||
| # casting to the equivalent masked dtype takes a short-circuit path; | ||
| # it must not mutate the source column's dtype in place (the column | ||
| # is shared with the source Series/frame) | ||
| ser = cudf.Series(data, dtype=src_dtype) | ||
| result = ser.astype(masked_dtype) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
paths = [
Path("python/cudf/cudf/tests/series/methods/test_astype.py"),
Path("python/cudf/cudf/core/column/numerical.py"),
]
for p in paths:
print(f"\n=== {p} ===")
if p.exists():
text = p.read_text().splitlines()
for i, line in enumerate(text, 1):
if p.name == "test_astype.py" and 1618 <= i <= 1660:
print(f"{i:4d}: {line}")
if p.name == "numerical.py" and 1 <= i <= 260:
# print only around as_numerical_column by search later
pass
else:
print("missing")
PYRepository: rapidsai/cudf
Length of output: 1594
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
import re
# Find as_numerical_column definition and nearby lines.
p = Path("python/cudf/cudf/core/column/numerical.py")
text = p.read_text().splitlines()
for i, line in enumerate(text, 1):
if "def as_numerical_column" in line:
start = max(1, i - 20)
end = min(len(text), i + 120)
print(f"\n=== {p} lines {start}-{end} ===")
for j in range(start, end + 1):
print(f"{j:4d}: {text[j-1]}")
break
PYRepository: rapidsai/cudf
Length of output: 7290
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
p = Path("python/cudf/cudf/tests/series/methods/test_astype.py")
text = p.read_text().splitlines()
for i, line in enumerate(text, 1):
if 1618 <= i <= 1660:
print(f"{i:4d}: {line}")
PYRepository: rapidsai/cudf
Length of output: 1481
🏁 Script executed:
sed -n '1628,1656p' python/cudf/cudf/tests/series/methods/test_astype.pyRepository: rapidsai/cudf
Length of output: 952
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
import re
p = Path("python/cudf/cudf/tests/series/methods/test_astype.py")
text = p.read_text().splitlines()
# Show imports and any option_context / pandas_compatible usage in the file.
for i, line in enumerate(text, 1):
if i <= 80:
print(f"{i:4d}: {line}")
print("\n--- matches ---")
for i, line in enumerate(text, 1):
if "pandas_compatible" in line or "option_context" in line or "pytestmark" in line:
print(f"{i:4d}: {line}")
PYRepository: rapidsai/cudf
Length of output: 4224
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
# Search nearby test infrastructure for automatic pandas-compatible mode setup.
for path in [
Path("python/cudf/cudf/tests/conftest.py"),
Path("python/cudf/cudf/tests/series/methods/conftest.py"),
]:
print(f"\n=== {path} ===")
if path.exists():
text = path.read_text().splitlines()
for i, line in enumerate(text, 1):
if "pandas_compatible" in line or "option_context" in line or "autouse" in line:
start = max(1, i - 5)
end = min(len(text), i + 10)
for j in range(start, end + 1):
print(f"{j:4d}: {text[j-1]}")
print("---")
else:
print("missing")
PYRepository: rapidsai/cudf
Length of output: 3679
🏁 Script executed:
rg -n "pandas_compatible|option_context|autouse" python/cudf/cudf/tests python/cudf/cudf/core -g '!**/__pycache__/**'Repository: rapidsai/cudf
Length of output: 30266
Enable pandas-compatible mode for this case. Wrap the setup and astype call in cudf.option_context("mode.pandas_compatible", True) so the test exercises the equivalent-dtype short-circuit instead of the fallback cast path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@python/cudf/cudf/tests/series/methods/test_astype.py` around lines 1638 -
1645, Update test_astype_masked_equivalent_dtype_no_source_mutation to wrap
Series construction and the astype call in
cudf.option_context("mode.pandas_compatible", True), ensuring the test exercises
the equivalent-dtype short-circuit path.
3 tasks
vyasr
approved these changes
Jul 21, 2026
Contributor
|
/merge |
rapids-bot
Bot
merged commit Jul 21, 2026
edbf8cf
into
NVIDIA:release/26.08
238 of 242 checks passed
3 tasks
davidwendt
pushed a commit
to wjxiz1992/cudf
that referenced
this pull request
Jul 21, 2026
…sts (NVIDIA#23364) Split out of NVIDIA#23255 (1/6). `NumericalColumn.as_numerical_column` short-circuits casts between equivalent dtypes (same pylibcudf type, e.g. `float64` → `Float64`), but implemented the shortcut by assigning the target dtype onto `self._dtype` in place. The column object is shared with the caller's Series/DataFrame, so the *source* object silently changed dtype as a side effect of the cast. This returns a fresh column over the same pylibcudf data instead (`nans_to_nulls` first for float → masked casts), and adds a classic regression test. Fixes 5 pandas-tests (`test_stack_nullable_dtype[*]`, `test_loc_set_nan_in_categorical_series[Float64]`, `test_assert_series_equal_extension_dtype_mismatch`, `test_assert_frame_equal_extension_dtype_mismatch`); their xfail entries are removed. Attribution verified by running the node ids against an isolated build containing only this fix (they pass) and a clean build (they fail). Independent of the other NVIDIA#23255 split PRs; can merge in any order. Authors: - GALI PREM SAGAR (https://github.com/galipremsagar) Approvers: - Vyas Ramasubramani (https://github.com/vyasr) URL: NVIDIA#23364
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
4 participants
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.
Description
Split out of #23255 (1/6).
NumericalColumn.as_numerical_columnshort-circuits casts between equivalent dtypes (same pylibcudf type, e.g.float64→Float64), but implemented the shortcut by assigning the target dtype ontoself._dtypein place. The column object is shared with the caller's Series/DataFrame, so the source object silently changed dtype as a side effect of the cast. This returns a fresh column over the same pylibcudf data instead (nans_to_nullsfirst for float → masked casts), and adds a classic regression test.Fixes 5 pandas-tests (
test_stack_nullable_dtype[*],test_loc_set_nan_in_categorical_series[Float64],test_assert_series_equal_extension_dtype_mismatch,test_assert_frame_equal_extension_dtype_mismatch); their xfail entries are removed. Attribution verified by running the node ids against an isolated build containing only this fix (they pass) and a clean build (they fail).Independent of the other #23255 split PRs; can merge in any order.
Checklist