Fix all null list column with missing child column in JSON reader

[karthikeyann]

Fixes #17341 and fixes #17349.

• expose make_empty_column
• fix all null list with empty nested child on missing list child column when prune columns is enabled
• add unit test

Description

Checklist

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

[davidwendt]

Why is this change needed?
Could this utility be used instead?

cudf/cpp/include/cudf/lists/detail/lists_column_factories.hpp

Lines 50 to 52 in 9cc9071

	std::unique_ptr<column> make_empty_lists_column(data_type child_type,
	rmm::cuda_stream_view stream,
	rmm::device_async_resource_ref mr);

[karthikeyann]

It works only for fixed width type child.

[karthikeyann]

This change is to avoid any purge/has_nonempty_nulls calls.

[davidwendt]

What does this do different than the factory call?

[ttnghia]

We discovered through profiling that make_lists_column and make_structs_column introduce significant overhead by unnecessarily calling to purge_non_empty_nulls and superimpose_nulls_no_sanitize. These sanitization may be needed in general, however, here we know that the child(ren) of JSON output column cannot have non-null rows if the parent is null (due to invalid input) thus we want to avoid such overhead by calling to the column constructor directly.

[ttnghia]

@karthikeyann Please add comments to clarify the reason why we have this change:

// Do not use `cudf::make_lists_column` since we do not need to call `purge_nonempty_nulls`
// on the child column as it does not have non-empty nulls.

and

// Do not use `cudf::make_structs_column` since we do not need to call `superimpose_nulls`
// on the children columns.

[ttnghia]

I've filed a related issue for this: #17356

[davidwendt]

#12567

[davidwendt]

Seems we could fix the make_structs_column and make_lists_column to not call these utilities for empty columns.
Or the utilities could be fixed to no-op for empty columns perhaps?

[karthikeyann]

Either fix is good.

[davidwendt]

Sorry, I'm still not seeing how make_structs_column is calling superimpose_nulls when the creating an empty column. The logic in the factory function only calls superimpose_nulls if a non-empty null-mask is passed:

cudf/cpp/src/structs/structs_column_factories.cu

Lines 47 to 55 in be9ba6c

	if (!null_mask.is_empty()) {
	for (auto& child : child_columns) {
	child = structs::detail::superimpose_nulls(static_cast<bitmask_type const*>(null_mask.data()),
	null_count,
	std::move(child),
	stream,
	mr);
	}
	}

[ttnghia]

Ah yes, I was wrong on this: it is unnecessary here (for constructing empty column) since the input column has empty null mask.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (branch-25.02@c791f80). Learn more about missing BASE report.

Additional details and impacted files

@@               Coverage Diff               @@
##             branch-25.02   #17348   +/-   ##
===============================================
  Coverage                ?   84.15%           
===============================================
  Files                   ?      231           
  Lines                   ?    32928           
  Branches                ?        0           
===============================================
  Hits                    ?    27712           
  Misses                  ?     5216           
  Partials                ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[lamarrr]

Looks good to me

[davidwendt]

Should this use thrust::distance ?

Suggested change

	list_children_end - thrust::make_zip_iterator(node_ids.begin(), parent_col_ids.begin()),
	thrust::distance(thrust::make_zip_iterator(node_ids.begin(), parent_col_ids.begin(), list_children_end),

[ttnghia]

Seem to cause CI to fail:

 /home/coder/cudf/cpp/src/io/json/host_tree_algorithms.cu(953): error: no instance of function template "thrust::distance" matches the argument list
              argument types are: (thrust::zip_iterator<thrust::tuple<cudf::size_type *, cudf::size_type *, thrust::zip_iterator<thrust::tuple<cudf::io::json::NodeIndexT *, cudf::io::json::NodeIndexT *>>>>, lambda [](cudf::size_type)->void)
        thrust::distance(thrust::make_zip_iterator(node_ids.begin(), parent_col_ids.begin(), list_children_end),
        ^
  /home/coder/cudf/cpp/build/conda/cuda-12.5/release/_deps/cccl-src/thrust/thrust/cmake/../../thrust/detail/distance.inl(38): note #3327-D: candidate function template "thrust::distance" failed deduction
    distance(InputIterator first, InputIterator last)
    ^
  
  /home/coder/cudf/cpp/src/io/json/host_tree_algorithms.cu(969): error: expected a ")"
        });

Essentially there is a missing ) for make_zip_iterator:

--thrust::distance(thrust::make_zip_iterator(..., parent_col_ids.begin(), list_children_end),
++thrust::distance(thrust::make_zip_iterator(..., parent_col_ids.begin()), list_children_end),

[ttnghia]

I realize that the same expression is used to compute num_list_children below this line.

[ttnghia]

So we can reduce code duplicate by something like this:

auto const list_children_begin = thrust::make_zip_iterator(node_ids.begin(), parent_col_ids.begin());
// replace all `thrust::make_zip_iterator` by `list_children_begin`.
auto const num_list_children = thrust::distance(list_children_begin, list_children_end);
// use num_list_children

[karthikeyann]

/ok to test

[karthikeyann]

/ok to test

[karthikeyann]

/ok to test

[karthikeyann]

/merge