Make `column_empty` mask buffer creation consistent with libcudf #16715

mroeschke · 2024-08-30T22:32:56Z

Description

Based on offline discussions, this PR makes column_empty consistent with libcudf where

A size 0 "empty" column should not have a mask buffer
A size > 0 "empty" (i.e all null) column should have a mask buffer

Additionally removes column_empty_like which can be subsumed by column_empty (I didn't find any active usage of this method across RAPIDS https://github.com/search?q=org%3Arapidsai%20column_empty_like&type=code)

column_empty will have an unused masked argument, but since there is usage of this method across RAPIDS I'll need to adjust them before removing that keyword here (https://github.com/search?q=org%3Arapidsai%20column_empty&type=code)

Checklist

I am familiar with the Contributing Guidelines.
New or existing tests cover these changes.
The documentation is up to date with these changes.

…empty/consistent

python/cudf/cudf/core/column/column.py

bdice · 2024-09-03T17:06:32Z

python/cudf/cudf/core/column/column.py

@@ -1486,7 +1454,12 @@ def _has_any_nan(arbitrary: pd.Series | np.ndarray) -> bool:
 def column_empty(
    row_count: int, dtype: Dtype = "object", masked: bool = False


Is the masked parameter no longer used?

I also want to raise a concern: the nullability of a column is conveyed by the presence/absence of a mask. While a column with zero rows has no data, the nullability is moreso a part of its dtype which is independent of the row count. We have to track nullability carefully for things like round-tripping Parquet data (where nullability is part of the schema). I would check carefully if this changes any I/O behaviors for zero-row files -- we may not have adequate test coverage today. libcudf/cudf are notoriously weak/imprecise in this area and I want to get stricter, not less strict, over time.

Is the masked parameter no longer used?

Correct.

I would check carefully if this changes any I/O behaviors for zero-row files -- we may not have adequate test coverage today.

I added a small, zero row roundtrip parquet unit test in 34fbc8a to validate this was unaffected. Agreed that it would be good to make libcudf/cudf stricter w.r.t. mask presence, and at least this PR makes it more consistent for zero row vs all null columns generated in cudf

Can we remove the masked parameter now that it is unused?

I want to remove this parameter in a follow up since there are some usages of masked in other RAPIDS libraries. I'll address those first (after this PR) before opening up a PR to remove it here

…empty/consistent

Co-authored-by: Bradley Dice <[email protected]>

…schke/cudf into ref/column_empty/consistent

…empty/consistent

vyasr · 2024-09-26T22:39:18Z

@bdice could you take another look here?

mroeschke · 2024-10-10T19:24:06Z

Just putting this in draft to not merge while I'm out (might, but not intended to, require some related fixed in other RAPIDS projects), but this PR is fully ready to review.

mroeschke · 2024-10-30T23:32:26Z

Opened back up now that I'm back from PTO.

Could you take another look when you have the time @bdice?

bdice

I asked a potentially breaking question about removing the masked parameter. Also this feels like a potentially disruptive change. This should probably target 25.02, to avoid breaking 24.12 just before freeze.

bdice · 2024-11-20T00:10:39Z

python/cudf/cudf/core/column/column.py

@@ -1486,7 +1454,12 @@ def _has_any_nan(arbitrary: pd.Series | np.ndarray) -> bool:
 def column_empty(
    row_count: int, dtype: Dtype = "object", masked: bool = False


Can we remove the masked parameter now that it is unused?

bdice · 2024-11-20T00:13:30Z

python/cudf/cudf/core/udf/groupby_utils.py

@@ -154,7 +154,10 @@ def jit_groupby_apply(offsets, grouped_values, function, *args):
    offsets = cp.asarray(offsets)
    ngroups = len(offsets) - 1

-    output = cudf.core.column.column_empty(ngroups, dtype=return_type)
+    # numba doesn't support masks
+    output = cudf.core.column.column_empty(


This feels like we're losing performance by allocating a null mask with all nulls, and then dropping it. Is there a better way?

Fair point. I added a for_numba argument in this function to avoid creating a mask in these cases

vyasr · 2024-11-20T16:38:49Z

Thanks for taking a look Bradley! Moving to 25.02

…empty/consistent

…schke/cudf into ref/column_empty/consistent

vyasr · 2024-12-02T18:11:17Z

@mroeschke sorry for not getting this merged earlier, unfortunately there are conflicts now. Could you please resolve?

…empty/consistent

…schke/cudf into ref/column_empty/consistent

mroeschke · 2024-12-03T22:58:14Z

/merge

rapidsai/cudf#16715 makes size 0 cudf columns have no mask buffer which I think aligns with the intention in `empty_geometry_column`. Additionally, it makes the `masked` keyword unnecessary and unused so removing it here. Authors: - Matthew Roeschke (https://github.com/mroeschke) Approvers: - Bradley Dice (https://github.com/bdice) - Michael Wang (https://github.com/isVoid) URL: #1496

mroeschke added 4 commits August 30, 2024 11:31

Make empty column masks consistent

a408412

Merge remote-tracking branch 'upstream/branch-24.10' into ref/column_…

cf241a0

…empty/consistent

Set masks to None when using numba

4580187

Merge remote-tracking branch 'upstream/branch-24.10' into ref/column_…

8a36282

…empty/consistent

mroeschke added Python Affects Python cuDF API. improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Aug 30, 2024

mroeschke requested a review from a team as a code owner August 30, 2024 22:32

mroeschke requested review from bdice and galipremsagar August 30, 2024 22:32

bdice reviewed Sep 3, 2024

View reviewed changes

mroeschke and others added 5 commits September 3, 2024 15:58

Merge remote-tracking branch 'upstream/branch-24.10' into ref/column_…

dd31853

…empty/consistent

Update python/cudf/cudf/core/column/column.py

0cf66a1

Co-authored-by: Bradley Dice <[email protected]>

Update python/cudf/cudf/core/column/column.py

e63b8d8

Co-authored-by: Bradley Dice <[email protected]>

Merge branch 'ref/column_empty/consistent' of https://github.com/mroe…

74e0128

…schke/cudf into ref/column_empty/consistent

Add roundtrip parquet test with zero row columns no mask

34fbc8a

mroeschke changed the base branch from branch-24.10 to branch-24.12 September 25, 2024 20:25

Merge remote-tracking branch 'upstream/branch-24.12' into ref/column_…

2f76b0e

…empty/consistent

vyasr requested a review from bdice September 26, 2024 22:39

mroeschke marked this pull request as draft October 10, 2024 19:23

Merge branch 'branch-24.12' into ref/column_empty/consistent

ad5be0d

mroeschke marked this pull request as ready for review October 30, 2024 20:10

github-actions bot assigned mroeschke Oct 30, 2024

Merge branch 'branch-24.12' into ref/column_empty/consistent

600b1e8

github-actions bot assigned galipremsagar Nov 11, 2024

bdice reviewed Nov 20, 2024

View reviewed changes

vyasr changed the base branch from branch-24.12 to branch-25.02 November 20, 2024 16:38

mroeschke added 4 commits November 20, 2024 12:48

Merge remote-tracking branch 'upstream/branch-25.02' into ref/column_…

101b78a

…empty/consistent

Add for_numba to avoid unnecessary mask creation

459f413

Merge branch 'ref/column_empty/consistent' of https://github.com/mroe…

0b50848

…schke/cudf into ref/column_empty/consistent

Merge branch 'branch-25.02' into ref/column_empty/consistent

feb82dd

bdice approved these changes Nov 26, 2024

View reviewed changes

mroeschke added 2 commits December 2, 2024 16:21

Merge remote-tracking branch 'upstream/branch-25.02' into ref/column_…

f00c210

…empty/consistent

Merge branch 'ref/column_empty/consistent' of https://github.com/mroe…

71a2a08

…schke/cudf into ref/column_empty/consistent

rapids-bot bot merged commit 541e7e8 into rapidsai:branch-25.02 Dec 3, 2024
105 checks passed

mroeschke deleted the ref/column_empty/consistent branch December 3, 2024 22:58

mroeschke mentioned this pull request Dec 3, 2024

Remove unused masked keyword in column_empty rapidsai/cuspatial#1496

Merged

3 tasks

mroeschke mentioned this pull request Dec 5, 2024

Remove unused masked keyword in column_empty #17530

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `column_empty` mask buffer creation consistent with libcudf #16715

Make `column_empty` mask buffer creation consistent with libcudf #16715

mroeschke commented Aug 30, 2024

bdice Sep 3, 2024

mroeschke Sep 3, 2024

bdice Nov 20, 2024

mroeschke Nov 20, 2024

vyasr commented Sep 26, 2024

mroeschke commented Oct 10, 2024 •

edited

Loading

mroeschke commented Oct 30, 2024

bdice left a comment

bdice Nov 20, 2024

bdice Nov 20, 2024

mroeschke Nov 20, 2024

vyasr commented Nov 20, 2024

vyasr commented Dec 2, 2024

mroeschke commented Dec 3, 2024

		@@ -1486,7 +1454,12 @@ def _has_any_nan(arbitrary: pd.Series \| np.ndarray) -> bool:
		def column_empty(
		row_count: int, dtype: Dtype = "object", masked: bool = False

Make column_empty mask buffer creation consistent with libcudf #16715

Make column_empty mask buffer creation consistent with libcudf #16715

Conversation

mroeschke commented Aug 30, 2024

Description

Checklist

bdice Sep 3, 2024

Choose a reason for hiding this comment

mroeschke Sep 3, 2024

Choose a reason for hiding this comment

bdice Nov 20, 2024

Choose a reason for hiding this comment

mroeschke Nov 20, 2024

Choose a reason for hiding this comment

vyasr commented Sep 26, 2024

mroeschke commented Oct 10, 2024 • edited Loading

mroeschke commented Oct 30, 2024

bdice left a comment

Choose a reason for hiding this comment

bdice Nov 20, 2024

Choose a reason for hiding this comment

bdice Nov 20, 2024

Choose a reason for hiding this comment

mroeschke Nov 20, 2024

Choose a reason for hiding this comment

vyasr commented Nov 20, 2024

vyasr commented Dec 2, 2024

mroeschke commented Dec 3, 2024

Make `column_empty` mask buffer creation consistent with libcudf #16715

Make `column_empty` mask buffer creation consistent with libcudf #16715

mroeschke commented Oct 10, 2024 •

edited

Loading