Change `nullable()` to `has_nulls()` in `cudf::detail::gather` #14363

divyegala · 2023-11-06T18:18:02Z

Description

In #13795, we found out that nullable() causes severe perf degradation for the nested-type case when the input is read from file via cudf::io::read_json. This is because the JSON reader adds a null mask for columns that don't have NULLs. This change is a no-overhead replacement that checks the actual null count instead of checking if a null mask is present.

This PR also solves a bug in quantile/median groupby where NULLs were being set but the null count was not updated.

Checklist

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

bdice · 2023-11-06T18:43:17Z

If I have a column with an allocated null mask but no null values, and call gather, does this change whether the output column has an allocated null mask? If a null mask is allocated for the input, libcudf should preserve that property on the output even if there are no null values in the column. This is problematic for round-tripping data, as nullability of a column is a part of the schema/metadata even if there are no nulls declared.

If a null mask is allocated but there are zero null values, it would be fine to take a shortcut and not gather the bitmasks -- just allocate a nullmask with all valid entries before returning. That could be a special-case compromise between schema integrity and performance here.

divyegala · 2023-11-06T18:44:35Z

@bdice good question, I'll try it out

divyegala · 2023-11-06T23:57:14Z

Closing in favor of #14366

…nulls

bdice

@divyegala I can check the failures you saw in CI after it runs.

cpp/include/cudf/detail/gather.cuh

…nulls

cpp/src/groupby/sort/group_quantiles.cu

bdice

Approving with one name change suggestion.

cpp/include/cudf/table/table_view.hpp

vyasr

Not sure if my approval should count given that I've made significant modifications to this PR, but I did review it substantially and think the logic is sound.

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

cpp/include/cudf/detail/gather.cuh

davidwendt · 2023-11-15T14:02:25Z

cpp/include/cudf/table/table_view.hpp

+ * @param input The table to check for nullable columns
+ * @return True if the table has nullable columns at any level of the column hierarchy, false otherwise
+ */
+inline bool has_nested_nullable_columns(table_view const& input)


Same with this implementation. This could be moved to https://github.com/rapidsai/cudf/blob/branch-23.12/cpp/src/table/table.cpp
This applies to the other non-templated inline functions in this header as well so I would be ok if this was done in a follow up PR -- in 24.02 too.

Let's make an issue for this.

…nulls

bdice

Approving up to some docstrings additions.

cpp/include/cudf/detail/null_mask.hpp

ttnghia · 2023-11-15T21:20:49Z

cpp/src/groupby/sort/group_quantiles.cu

@@ -49,6 +49,7 @@ struct calculate_quantile_fn {
  double const* d_quantiles;
  size_type num_quantiles;
  interpolation interpolation;
+  size_type* null_count;


This seems to be an unrelated change to this PR so ideally it should be in a separate PR. But I'm fine to keep this here but please clarify that in the PR description.

ttnghia · 2023-11-15T21:45:27Z

cpp/include/cudf/detail/gather.cuh

+    auto const has_nulls =
+      bounds_policy == out_of_bounds_policy::NULLIFY || cudf::has_nested_nulls(source_table);


Should this use && instead? Because if we indeed don't have any nulls here then we don't need to call gather_bitmask.

Probably I misunderstood the usage of this variable. So this variable should be called need_new_bitmask or so. It should not be has_nulls.

But we need to call gather_bitmask if out_of_bounds_policy::NULLIFY. gather_bitmask will help nullify any OOB accesses.

Okay, so || is indeed needed, but please rename that variable.

…s-nulls

…nulls

cpp/include/cudf/detail/gather.cuh

bdice · 2023-11-16T20:21:04Z

cpp/include/cudf/detail/gather.cuh

+  auto needs_new_bitmask = bounds_policy == out_of_bounds_policy::NULLIFY ||
+                           cudf::has_nested_nullable_columns(source_table);
+  if (needs_new_bitmask) {
+    needs_new_bitmask = needs_new_bitmask || cudf::has_nested_nulls(source_table);


Non-blocking suggestion:

Suggested change

needs_new_bitmask = needs_new_bitmask || cudf::has_nested_nulls(source_table);

needs_new_bitmask |= cudf::has_nested_nulls(source_table);

We'll have to rerun CI again. Happy to make this change if you feel strongly about it, otherwise let's pass on it

divyegala · 2023-11-16T21:43:25Z

/merge

change nullable to has_nulls

f2971f8

divyegala added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Nov 6, 2023

divyegala requested a review from a team as a code owner November 6, 2023 18:18

divyegala self-assigned this Nov 6, 2023

divyegala requested review from shrshi and PointKernel November 6, 2023 18:18

divyegala changed the title ~~Change nullable() to has_nulls() in cudf::detail::gather`~~ Change nullable() to has_nulls() in cudf::detail::gather Nov 6, 2023

github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Nov 6, 2023

divyegala closed this Nov 6, 2023

bdice mentioned this pull request Nov 7, 2023

JSON reader provides a full null mask for a column with no NULLs #14366

Closed

divyegala reopened this Nov 8, 2023

divyegala added 2 commits November 8, 2023 09:55

only create null masks for thouse source columns that have one

748aaff

Merge remote-tracking branch 'upstream/branch-23.12' into gather-has-…

ba5667b

…nulls

bdice reviewed Nov 8, 2023

View reviewed changes

cpp/include/cudf/detail/gather.cuh Outdated Show resolved Hide resolved

GregoryKimball requested a review from davidwendt November 13, 2023 18:28

vyasr reviewed Nov 14, 2023

View reviewed changes

cpp/include/cudf/detail/gather.cuh Outdated Show resolved Hide resolved

divyegala and others added 6 commits November 14, 2023 08:25

use has_nested_nulls

f62f6a4

Merge remote-tracking branch 'upstream/branch-23.12' into gather-has-…

3dce824

…nulls

Delete bad test

25ebec7

Set the null count in quantiles, not just the mask

2969be7

has nested nulls

9dad666

Merge branch 'branch-23.12' into gather-has-nulls

c72ea58

bdice reviewed Nov 15, 2023

View reviewed changes

cpp/src/groupby/sort/group_quantiles.cu Show resolved Hide resolved

bdice approved these changes Nov 15, 2023

View reviewed changes

cpp/include/cudf/table/table_view.hpp Outdated Show resolved Hide resolved

vyasr approved these changes Nov 15, 2023

View reviewed changes

Update cpp/include/cudf/table/table_view.hpp

c31c1db

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

davidwendt requested changes Nov 15, 2023

View reviewed changes

divyegala added 2 commits November 15, 2023 07:05

address review

8163a4b

Merge remote-tracking branch 'upstream/branch-23.12' into gather-has-…

b37e5a3

…nulls

divyegala requested a review from davidwendt November 15, 2023 15:07

bdice approved these changes Nov 15, 2023

View reviewed changes

cpp/include/cudf/detail/null_mask.hpp Outdated Show resolved Hide resolved

update docs

11b406f

ttnghia reviewed Nov 15, 2023

View reviewed changes

davidwendt approved these changes Nov 15, 2023

View reviewed changes

ttnghia reviewed Nov 15, 2023

View reviewed changes

Merge branch 'branch-23.12' into gather-has-nulls

4cdd582

vyasr requested a review from ttnghia November 16, 2023 16:53

divyegala added 3 commits November 16, 2023 10:35

rename var

87ce541

Merge remote-tracking branch 'origin/gather-has-nulls' into gather-ha…

04990e3

…s-nulls

Merge remote-tracking branch 'upstream/branch-23.12' into gather-has-…

868f227

…nulls

ttnghia approved these changes Nov 16, 2023

View reviewed changes

non-const

f2f392b

ttnghia reviewed Nov 16, 2023

View reviewed changes

cpp/include/cudf/detail/gather.cuh Show resolved Hide resolved

bdice reviewed Nov 16, 2023

View reviewed changes

divyegala mentioned this pull request Nov 16, 2023

Move non-templated inline function definitions from table_view.hpp to source file #14431

Closed

rapids-bot bot merged commit 8e1ef05 into rapidsai:branch-23.12 Nov 16, 2023
65 checks passed

vyasr mentioned this pull request Dec 14, 2023

Add column sanitization checks in CUDF_TEST_EXPECT_COLUMN_* macros #14559

Merged

3 tasks

GregoryKimball mentioned this pull request Jan 5, 2024

[FEA] Make calling to purge_nonempty_nulls optional in various places #12567

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change `nullable()` to `has_nulls()` in `cudf::detail::gather` #14363

Change `nullable()` to `has_nulls()` in `cudf::detail::gather` #14363

divyegala commented Nov 6, 2023 •

edited

Loading

bdice commented Nov 6, 2023 •

edited

Loading

divyegala commented Nov 6, 2023

divyegala commented Nov 6, 2023

bdice left a comment

bdice left a comment

vyasr left a comment

davidwendt Nov 15, 2023

bdice Nov 16, 2023

divyegala Nov 16, 2023

bdice left a comment

ttnghia Nov 15, 2023

divyegala Nov 15, 2023

ttnghia Nov 15, 2023 •

edited

Loading

ttnghia Nov 15, 2023

divyegala Nov 15, 2023

ttnghia Nov 16, 2023

divyegala Nov 16, 2023

bdice Nov 16, 2023

divyegala Nov 16, 2023

divyegala commented Nov 16, 2023

		auto const has_nulls =
		bounds_policy == out_of_bounds_policy::NULLIFY \|\| cudf::has_nested_nulls(source_table);

	needs_new_bitmask = needs_new_bitmask \|\| cudf::has_nested_nulls(source_table);
	needs_new_bitmask \|= cudf::has_nested_nulls(source_table);

Change nullable() to has_nulls() in cudf::detail::gather #14363

Change nullable() to has_nulls() in cudf::detail::gather #14363

Conversation

divyegala commented Nov 6, 2023 • edited Loading

Description

Checklist

bdice commented Nov 6, 2023 • edited Loading

divyegala commented Nov 6, 2023

divyegala commented Nov 6, 2023

bdice left a comment

Choose a reason for hiding this comment

bdice left a comment

Choose a reason for hiding this comment

vyasr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ttnghia Nov 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

divyegala commented Nov 16, 2023

Change `nullable()` to `has_nulls()` in `cudf::detail::gather` #14363

Change `nullable()` to `has_nulls()` in `cudf::detail::gather` #14363

divyegala commented Nov 6, 2023 •

edited

Loading

bdice commented Nov 6, 2023 •

edited

Loading

ttnghia Nov 15, 2023 •

edited

Loading