Skip to content
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/main/cpp/benchmarks/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,9 @@ ConfigureBench(BLOOM_FILTER_BENCH
ConfigureBench(GET_JSON_OBJECT_BENCH
get_json_object.cu)

ConfigureBench(FROM_JSON_TO_STRUCTS_BENCH
from_json_to_structs.cu)

ConfigureBench(PARSE_URI_BENCH
parse_uri.cpp)

Expand Down
100 changes: 100 additions & 0 deletions src/main/cpp/benchmarks/from_json_to_structs.cu
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
/*
* Copyright (c) 2026, NVIDIA CORPORATION.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#include <cudf_test/column_wrapper.hpp>

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 SHOULD FIX

Can the new benchmark include the owning cudf::column header directly and keep its project-local header in the first include group? src/main/cpp/benchmarks/from_json_to_structs.cu:31 names cudf::column, and src/main/cpp/benchmarks/from_json_to_structs.cu:81 calls cudf::column::view(), but src/main/cpp/benchmarks/from_json_to_structs.cu:17-23 has no <cudf/column/column.hpp> and places the internal src/main/cpp/src/json_utils.hpp:17 header after other RAPIDS headers using angle brackets. This makes the translation unit depend on another header exposing the complete cudf::column definition and diverges from the project's local-first/internal-header include convention.

Suggested fix: Move the project-local header to the first group using quotes and directly include <cudf/column/column.hpp> in the cuDF group.

Confidence: 🟣❗ CERTAIN; reviewer scope: compliant.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, updated.


#include <cudf/strings/strings_column_view.hpp>
#include <cudf/types.hpp>

#include <json_utils.hpp>
#include <nvbench/nvbench.cuh>

#include <memory>
#include <string>
#include <vector>

namespace {

std::unique_ptr<cudf::column> make_input(cudf::size_type num_rows, cudf::size_type mismatch_percent)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 SUGGESTION

Could the new make_input factory be marked [[nodiscard]]? Its std::unique_ptr<cudf::column> owns the generated benchmark input, so a diagnostic on accidental result disposal would make ownership mistakes visible.

Suggested fix: Add [[nodiscard]] to the factory declaration.

Confidence: 🟣❗ CERTAIN; reviewer scope: compliant.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

{
std::string const valid = R"({"data":{"c2":[{"c3":19,"c4":"x"}],"c1":1},"id":10})";
std::string const mismatched = R"({"data":{"c2":[19],"c1":2},"id":20})";

std::vector<std::string> rows;
rows.reserve(num_rows);
for (cudf::size_type row = 0; row < num_rows; ++row) {
rows.push_back(mismatch_percent > 0 && row % 100 < mismatch_percent ? mismatched : valid);
}
return cudf::test::strings_column_wrapper(rows.begin(), rows.end()).release();
}

std::vector<std::string> nested_schema_names()
{
return {"data", "c1", "c2", "element", "c3", "c4", "id"};
}

std::vector<int> nested_schema_num_children() { return {2, 0, 1, 2, 0, 0, 0}; }

std::vector<int> nested_schema_types()
{
return {static_cast<int>(cudf::type_id::STRUCT),
static_cast<int>(cudf::type_id::INT32),
static_cast<int>(cudf::type_id::LIST),
static_cast<int>(cudf::type_id::STRUCT),
static_cast<int>(cudf::type_id::INT32),
static_cast<int>(cudf::type_id::STRING),
static_cast<int>(cudf::type_id::INT32)};
}

std::vector<int> nested_schema_scales() { return {0, 0, 0, 0, 0, 0, 0}; }

std::vector<int> nested_schema_precisions() { return {-1, -1, -1, -1, -1, -1, -1}; }

} // namespace

void BM_from_json_to_structs(nvbench::state& state)
{
auto const num_rows = static_cast<cudf::size_type>(state.get_int64("num_rows"));
auto const mismatch_percent = static_cast<cudf::size_type>(state.get_int64("mismatch_percent"));
auto const input = make_input(num_rows, mismatch_percent);

auto const col_names = nested_schema_names();
auto const num_children = nested_schema_num_children();
auto const types = nested_schema_types();
auto const scales = nested_schema_scales();
auto const precisions = nested_schema_precisions();

state.exec(nvbench::exec_tag::sync, [&](nvbench::launch& launch) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 SHOULD FIX

Can the benchmark bind NVBench's target stream to the stream used by the API? The timed region at src/main/cpp/benchmarks/from_json_to_structs.cu:83 does not call state.set_cuda_stream, while the call at src/main/cpp/benchmarks/from_json_to_structs.cu:84 omits the stream argument and therefore uses cudf::get_default_stream() from src/main/cpp/src/json_utils.hpp:88. NVBench creates a separate explicit stream when none is configured at /tmp/nvbench-pr4728-reference/nvbench/state.cuh:79 and records its CUDA timer events on that stream at /tmp/nvbench-pr4728-reference/nvbench/detail/measure_cold.cuh:226. Those events have no CUDA stream-ordering relationship with the JSON work, so the reported GPU interval can mis-bound the operation and make this new benchmark unreliable for comparing the row-nulling paths.

Suggested fix: Set NVBench's target stream to cudf::get_default_stream() before src/main/cpp/benchmarks/from_json_to_structs.cu:83. This is the same pattern used by the existing JSON benchmark at src/main/cpp/benchmarks/from_json.cu:357 and orders NVBench's events with every stream-ordered operation in from_json_to_structs.

Confidence: 🟣❗ CERTAIN; reviewer scope: gpu-kernel-optimization.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, updated in ca0366b: NVBench now records on the API default stream. The benchmark target built and all 10 smoke cases completed.

auto output = spark_rapids_jni::from_json_to_structs(cudf::strings_column_view{input->view()},

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 SUGGESTION

Could the ignored benchmark result at src/main/cpp/benchmarks/from_json_to_structs.cu:84 be explicitly immutable and intentionally unused? It is never mutated or otherwise read, and the launch parameter at src/main/cpp/benchmarks/from_json_to_structs.cu:83 is also unused. The neighboring JSON benchmark uses the established form at src/main/cpp/benchmarks/from_json.cu:359.

Suggested fix: Omit the unused launch parameter name and declare the owning result [[maybe_unused]] auto const, making both intentions explicit and following the project's auto const preference.

Confidence: 🟣❗ CERTAIN; reviewer scope: compliant.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in ca0366b: the launch parameter is unnamed and the result is [[maybe_unused]] auto const. The benchmark target built and all 10 smoke cases completed.

col_names,
num_children,
types,
scales,
precisions,
true,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 SUGGESTION

Could the five consecutive true arguments at src/main/cpp/benchmarks/from_json_to_structs.cu:87-91 be labeled with their parameter names? Their meanings are only recoverable by cross-referencing the signature at src/main/cpp/src/from_json_to_structs.cu:900-910, and each value controls a different parser behavior.

Suggested fix: Add inline argument-name comments at src/main/cpp/benchmarks/from_json_to_structs.cu:87-91. This makes the benchmark configuration readable at the call site and matches the established boolean-argument labeling at src/main/cpp/src/from_json_to_structs.cu:348-349 and src/main/cpp/src/from_json_to_structs.cu:763.

Confidence: 🟣❗ CERTAIN; reviewer scope: refactor.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

true,
true,
true,
true);
});

state.add_buffer_size(num_rows, "rows", "Rows");
}

NVBENCH_BENCH(BM_from_json_to_structs)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 SHOULD FIX

Could the regression include two mismatched top-level columns with different row-index vectors? The new lookup at src/main/cpp/src/from_json_to_structs.cu:974 builds one map entry per diagnostic column and selects rows by each schema name at src/main/cpp/src/from_json_to_structs.cu:990. The only current regression schema has one mismatch-capable top-level STRUCT (data) and a valid scalar sibling (id) at src/test/java/com/nvidia/spark/rapids/jni/FromJsonToStructsTest.java:45, so it supplies one diagnostic entry. A mutation that reuses the first row vector for every diagnostic key, associates rows by vector order instead of name, or nullifies only the first mismatched column would leave every current assertion unchanged.

Suggested fix: Add a two-column schema with a mismatched only in row 0 and b mismatched only in row 1, then assert the complete output. That verifies independent name-to-row association rather than only the single-entry case.

Confidence: 🟣❗ CERTAIN; reviewer scope: test-coverage.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in ca0366b: added a two-column regression with independent mismatch rows for a and b. FromJsonToStructsTest passed 5/5.

.set_name("from_json_to_structs")
.add_int64_axis("num_rows", {10000, 100000})

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 SHOULD FIX

Could the row-count axis use thousands separators? The project adopts cuDF's C++ guide, which requires decimal separators at each thousands place; 10000 and 100000 are the only benchmark axis literals in src/main/cpp/benchmarks/from_json_to_structs.cu:99 that omit them, reducing scanability compared with adjacent benchmark registrations.

Suggested fix: Write the two values as 10'000 and 100'000.

Confidence: 🟣❗ CERTAIN; reviewer scope: compliant.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

.add_int64_axis("mismatch_percent", {0, 1});

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finding 5 — 🟢 SUGGESTION

Could the benchmark include a dense-mismatch axis? The only nonzero case at src/main/cpp/benchmarks/from_json_to_structs.cu:100 is 1%, and the generator at src/main/cpp/benchmarks/from_json_to_structs.cu:39 spaces those mismatches 100 rows apart. Because each validity word contains 32 rows at thirdparty/cudf/cpp/include/cudf/types.hpp:86, the benchmark never places two mismatch updates in one word and cannot measure the dense atomic contention at src/main/cpp/src/from_json_to_structs.cu:171. A 100% case also exercises the maximum row-level null-sanitization workload introduced by this change.

Suggested fix: Add 100% mismatches to cover the contiguous worst case without expanding the matrix more than one additional point per row-count axis.

Confidence: 🟣❗ CERTAIN; reviewer scope: gpu-kernel-optimization.


Finding 6 — 🟢 SUGGESTION

Could the mismatch-density axis include at least one dense workload? The new benchmark's {0, 1} values measure the baseline and path activation, but no dense point measures the scaling of the per-diagnostic host-to-device copy at src/main/cpp/src/from_json_to_structs.cu:163 or the per-row atomic updates at src/main/cpp/src/from_json_to_structs.cu:166. This leaves the performance-sensitive part of the change uncharacterized as the mismatch vector grows.

Suggested fix: Add 10% and 50% cells so benchmark results show the slope from sparse to dense diagnostics while preserving the existing 0% and 1% measurements.

Confidence: 🟣❗ CERTAIN; reviewer scope: test-coverage.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

169 changes: 127 additions & 42 deletions src/main/cpp/src/from_json_to_structs.cu
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,17 @@

#include <cudf/column/column_device_view.cuh>
#include <cudf/column/column_factories.hpp>
#include <cudf/copying.hpp>
#include <cudf/detail/iterator.cuh>
#include <cudf/detail/utilities/cuda.cuh>
#include <cudf/detail/utilities/vector_factories.hpp>
#include <cudf/detail/valid_if.cuh>
#include <cudf/io/json.hpp>
#include <cudf/lists/lists_column_view.hpp>
#include <cudf/null_mask.hpp>
#include <cudf/strings/detail/strings_children.cuh>
#include <cudf/strings/strings_column_view.hpp>
#include <cudf/utilities/bit.hpp>
#include <cudf/utilities/traits.hpp>

#include <rmm/cuda_stream_view.hpp>
Expand All @@ -48,6 +51,10 @@
#include <thrust/transform.h>
#include <thrust/uninitialized_fill.h>

#include <algorithm>
#include <map>
#include <unordered_map>
Comment on lines +55 to +59

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::back_inserter is declared in <iterator>. Could we include it directly rather than rely on a transitive include?

Suggested change
#include <algorithm>
#include <map>
#include <span>
#include <unordered_map>
#include <algorithm>
#include <iterator>
#include <map>
#include <span>
#include <unordered_map>

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, updated.


namespace spark_rapids_jni {

namespace detail {
Expand Down Expand Up @@ -142,6 +149,78 @@ std::pair<cudf::io::schema_element, schema_element_with_precision> generate_stru
cudf::data_type{cudf::type_id::STRUCT}, -1, std::move(schema_cols_with_precisions)}};
}

void nullify_rows(cudf::column& input,
std::vector<cudf::size_type> const& row_indices,
rmm::cuda_stream_view stream,
rmm::device_async_resource_ref mr)
{
if (row_indices.empty()) { return; }

auto const input_view = input.view();
auto null_mask =

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 SHOULD FIX

Can the nullable branch update the owned mask in place? src/main/cpp/src/from_json_to_structs.cu:159 allocates and copies the complete existing mask, then src/main/cpp/src/from_json_to_structs.cu:179 immediately replaces the original mask. The only caller at src/main/cpp/src/from_json_to_structs.cu:969 passes an owned column that is moved at src/main/cpp/src/from_json_to_structs.cu:971, so retaining the old mask has no consumer. Every nullable column with mismatches therefore pays an avoidable device allocation and full-mask device copy before the null-count pass at src/main/cpp/src/from_json_to_structs.cu:174.

Suggested fix: For nullable input, obtain the writable pointer from input.mutable_view().null_mask() at thirdparty/cudf/cpp/include/cudf/column/column.hpp:306 and call input.set_null_count(null_count) at thirdparty/cudf/cpp/include/cudf/column/column.hpp:193 after the kernel. Allocate and install a new mask only for non-nullable input.

Confidence: 🟣❗ CERTAIN; reviewer scope: gpu-kernel-optimization.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

input_view.nullable()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 SHOULD FIX

Could you extend the regression fixture to combine a pre-existing null data parent with two adjacent schema-mismatch rows? The new branch at src/main/cpp/src/from_json_to_structs.cu:160 copies an existing validity mask, src/main/cpp/src/from_json_to_structs.cu:166 clears every diagnostic row, and src/main/cpp/src/from_json_to_structs.cu:174 recounts the merged mask. The current fixture at src/test/java/com/nvidia/spark/rapids/jni/FromJsonToStructsTest.java:87 contains no pre-existing null data row and only one mismatch row. A regression that discards an existing parent null or applies only one of multiple row updates would therefore not be detected.

Suggested fix: Add an explicit {"data":null,...} row and a second consecutive mismatched row to src/test/java/com/nvidia/spark/rapids/jni/FromJsonToStructsTest.java:86, then assert the complete output. This exercises preservation of an existing mask, multiple updates within one mask word, the recount, sibling-field preservation, and nested LIST sanitation in one regression.

Confidence: 🟣❗ CERTAIN; reviewer scope: test-coverage.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

? cudf::copy_bitmask(input_view, stream, mr)
: cudf::create_null_mask(input_view.size(), cudf::mask_state::ALL_VALID, stream, mr);
auto d_row_indices = cudf::detail::make_device_uvector_async(row_indices, stream, mr);

auto mask_ptr = static_cast<cudf::bitmask_type*>(null_mask.data());
thrust::for_each(rmm::exec_policy_nosync(stream),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finding 9 — 🟡 SHOULD FIX

Could the device-only row-index scratch vector use the current device resource? The allocation at src/main/cpp/src/from_json_to_structs.cu:163 is consumed only by the validity-mask update at src/main/cpp/src/from_json_to_structs.cu:166 and is not part of the returned column, but it is charged to the caller-provided output resource. That breaks the output-versus-temporary allocator contract documented at /tmp/cudf-pr4728-reference/cpp/doxygen/developer_guide/DEVELOPER_GUIDE.md:703 and can make transient diagnostic storage consume capacity or accounting reserved for returned buffers.

Suggested fix: Allocate the transient row-index vector with cudf::get_current_device_resource_ref(), while retaining mr for the null mask because that mask becomes part of the returned column.

Confidence: 🟣❗ CERTAIN; reviewer scope: memory-stream.


Finding 10 — 🟡 SHOULD FIX

Can dense mismatch rows be coalesced into one update per mask word? The loop at src/main/cpp/src/from_json_to_structs.cu:166 launches one global atomic read-modify-write per mismatched row. The mask word is 32 bits at thirdparty/cudf/cpp/include/cudf/types.hpp:86, so a run of 32 mismatched rows sends 32 threads to the same address and serializes their updates. The diagnostics guarantee sorted, unique indices at thirdparty/cudf/cpp/include/cudf/io/json.hpp:987, which makes host-side grouping deterministic and permits one non-atomic update per distinct word. This reduces a fully mismatched column from 32 global atomic operations per mask word to one ordinary update per word.

Suggested fix: Group the already-host-resident sorted indices into {word_index, bits_to_clear} records before upload and use one thread per distinct word when grouping reduces transfer size. Retain the current row-index path for sparse sets so the 1% workload at src/main/cpp/benchmarks/from_json_to_structs.cu:100 keeps its compact transfer.

Confidence: 🟠❗ LIKELY; reviewer scope: gpu-kernel-optimization.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

d_row_indices.begin(),
d_row_indices.end(),
[mask_ptr] __device__(auto const row) { cudf::clear_bit(mask_ptr, row); });

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No atomicity change is needed here: in the pinned cuDF API, cudf::clear_bit is implemented with atomicAnd and documented as thread-safe; the non-atomic variant is clear_bit_unsafe. Commit c2f2e46 documents that distinction directly at the call site. The native target linked and the focused Java/JNI tests passed.

Comment thread
greptile-apps[bot] marked this conversation as resolved.
Outdated

auto const null_count = cudf::null_count(
static_cast<cudf::bitmask_type const*>(null_mask.data()), 0, input_view.size(), stream);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optimization: if the input doen't have null then output null count will be size of row_indices.

null_count =  input_view.nullable() ? cudf::null_count(...) : static_cast<cudf::size_type>(row_indices.size());

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, updated.

input.set_null_mask(std::move(null_mask), null_count);
}

std::unique_ptr<cudf::column> make_lists_column_with_null_sanitization(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 SUGGESTION

Could both new nested-column factories be marked [[nodiscard]]? src/main/cpp/src/from_json_to_structs.cu:182 and src/main/cpp/src/from_json_to_structs.cu:208 return owning std::unique_ptr<cudf::column> results whose accidental disposal would silently discard all conversion work.

Suggested fix: Add [[nodiscard]] to both factory declarations.

Confidence: 🟣❗ CERTAIN; reviewer scope: compliant.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

cudf::size_type num_rows,
std::unique_ptr<cudf::column> offsets_column,
std::unique_ptr<cudf::column> child_column,
cudf::size_type null_count,
rmm::device_buffer&& null_mask,
rmm::cuda_stream_view stream,
rmm::device_async_resource_ref mr)
{
std::vector<std::unique_ptr<cudf::column>> children;
children.emplace_back(std::move(offsets_column));
children.emplace_back(std::move(child_column));
auto output = std::make_unique<cudf::column>(cudf::data_type{cudf::type_id::LIST},
num_rows,
rmm::device_buffer{},
std::move(null_mask),
null_count,
std::move(children));
// Row-level schema mismatch nulls can leave child data under null parents; sanitize it here.
if (null_count > 0 && cudf::has_nonempty_nulls(output->view(), stream)) {
output = cudf::purge_nonempty_nulls(output->view(), stream, mr);
}
Comment on lines +280 to +284

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in d87d85e by requiring null_count > 0 before calling cudf::has_nonempty_nulls. This avoids the recursive check on the all-valid LIST path while preserving sanitation when null rows exist. The focused native and Java/JNI validation passed.

return output;
Comment thread
greptile-apps[bot] marked this conversation as resolved.
}

std::unique_ptr<cudf::column> make_structs_column_with_null_consistency(
cudf::size_type num_rows,
std::vector<std::unique_ptr<cudf::column>>&& children,
cudf::size_type null_count,
rmm::device_buffer&& null_mask,
rmm::cuda_stream_view stream,
rmm::device_async_resource_ref mr)
{
if (null_count > 0) {
// make_structs_column superimposes parent nulls onto children for a consistent nested column.
return cudf::make_structs_column(
num_rows, std::move(children), null_count, std::move(null_mask), stream, mr);
}

return std::make_unique<cudf::column>(cudf::data_type{cudf::type_id::STRUCT},
num_rows,
rmm::device_buffer{},
std::move(null_mask),
null_count,
std::move(children));
}

using string_index_pair = cuda::std::pair<char const*, cudf::size_type>;

std::unique_ptr<cudf::column> cast_strings_to_booleans(cudf::column_view const& input,
Expand Down Expand Up @@ -711,14 +790,14 @@ std::unique_ptr<cudf::column> convert_data_type(InputType&& input,
new_children.emplace_back(convert_data_type(
std::move(child), child_schema, allow_nonnumeric_numbers, is_us_locale, stream, mr));

// 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.
return std::make_unique<cudf::column>(cudf::data_type{cudf::type_id::LIST},
num_rows,
rmm::device_buffer{},
std::move(*input_content.null_mask),
null_count,
std::move(new_children));
return make_lists_column_with_null_sanitization(
num_rows,
std::move(new_children[cudf::lists_column_view::offsets_column_index]),
std::move(new_children[cudf::lists_column_view::child_column_index]),
null_count,
std::move(*input_content.null_mask),
stream,
mr);
}

if (schema.type.id() == cudf::type_id::STRUCT) {
Expand All @@ -733,14 +812,12 @@ std::unique_ptr<cudf::column> convert_data_type(InputType&& input,
mr));
}

// Do not use `cudf::make_structs_column` since we do not need to call `superimpose_nulls`
// on the children columns.
return std::make_unique<cudf::column>(cudf::data_type{cudf::type_id::STRUCT},
num_rows,
rmm::device_buffer{},
std::move(*input_content.null_mask),
null_count,
std::move(new_children));
return make_structs_column_with_null_consistency(num_rows,
std::move(new_children),
null_count,
std::move(*input_content.null_mask),
stream,
mr);
}
} else { // input_is_const_cv
auto const null_count = input.null_count();
Expand All @@ -761,14 +838,14 @@ std::unique_ptr<cudf::column> convert_data_type(InputType&& input,
new_children.emplace_back(
convert_data_type(child, child_schema, allow_nonnumeric_numbers, is_us_locale, stream, mr));

// 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.
return std::make_unique<cudf::column>(cudf::data_type{cudf::type_id::LIST},
num_rows,
rmm::device_buffer{},
cudf::copy_bitmask(input, stream, mr),
null_count,
std::move(new_children));
return make_lists_column_with_null_sanitization(
num_rows,
std::move(new_children[cudf::lists_column_view::offsets_column_index]),
std::move(new_children[cudf::lists_column_view::child_column_index]),
null_count,
cudf::copy_bitmask(input, stream, mr),
stream,
mr);
}

if (schema.type.id() == cudf::type_id::STRUCT) {
Expand All @@ -783,14 +860,12 @@ std::unique_ptr<cudf::column> convert_data_type(InputType&& input,
mr));
}

// Do not use `cudf::make_structs_column` since we do not need to call `superimpose_nulls`
// on the children columns.
return std::make_unique<cudf::column>(cudf::data_type{cudf::type_id::STRUCT},
num_rows,
rmm::device_buffer{},
cudf::copy_bitmask(input, stream, mr),
null_count,
std::move(new_children));
return make_structs_column_with_null_consistency(num_rows,
std::move(new_children),
null_count,
cudf::copy_bitmask(input, stream, mr),
stream,
mr);
}
}

Expand Down Expand Up @@ -838,13 +913,21 @@ std::unique_ptr<cudf::column> from_json_to_structs(cudf::strings_column_view con
.dtypes(schema)
.prune_columns(schema.child_types.size() != 0);

auto const parsed_table_with_meta = cudf::io::read_json(opts_builder.build());
auto const& parsed_meta = parsed_table_with_meta.metadata;
auto parsed_columns = parsed_table_with_meta.tbl->release();
auto parsed_result = cudf::io::read_json_with_row_diagnostics(opts_builder.build(), stream, mr);
auto const& parsed_meta = parsed_result.data.metadata;
auto parsed_columns = parsed_result.data.tbl->release();

CUDF_EXPECTS(parsed_columns.size() == schema.child_types.size(),
"Numbers of output columns is different from schema size.");

auto const& mismatch_diagnostics =
parsed_result.diagnostics.top_level_columns_with_schema_mismatch_rows;
std::unordered_map<std::string, std::vector<cudf::size_type> const*> mismatch_rows_by_column;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 SUGGESTION

Can the row-diagnostics lookup at src/main/cpp/src/from_json_to_structs.cu:974 store non-owning spans instead of nullable pointers to whole vectors? Every value inserted at src/main/cpp/src/from_json_to_structs.cu:977 is non-null, and the consumer at src/main/cpp/src/from_json_to_structs.cu:156 only iterates and queries the size. A std::span<cudf::size_type const> expresses that borrowed range directly, removes the pointer dereferences at src/main/cpp/src/from_json_to_structs.cu:992 and src/main/cpp/src/from_json_to_structs.cu:994, and keeps the lifetime constraint visible. The spans remain valid because parsed_result at src/main/cpp/src/from_json_to_structs.cu:965 owns the diagnostics until after the conversion loop ends at src/main/cpp/src/from_json_to_structs.cu:1003, with no intervening mutation.

Suggested fix: Use std::span<cudf::size_type const> as the input type at src/main/cpp/src/from_json_to_structs.cu:157 and as the map value at src/main/cpp/src/from_json_to_structs.cu:974. At the existing host-to-device copy at src/main/cpp/src/from_json_to_structs.cu:196, adapt the standard span to the cudf::host_span overload verified at thirdparty/cudf/cpp/include/cudf/detail/utilities/vector_factories.hpp:89. This preserves the current asynchronous copy and allocation behavior.

Confidence: 🟣❗ CERTAIN; reviewer scope: refactor.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in ca0366b: mismatch lookup and nullify_rows now use read-only std::span values. The upload staging also uses stream-ordered pinned host storage. The native target built and FromJsonToStructsTest passed 5/5.

mismatch_rows_by_column.reserve(mismatch_diagnostics.size());
for (auto const& mismatch : mismatch_diagnostics) {
mismatch_rows_by_column.emplace(mismatch.column_name, &mismatch.row_indices);
}

std::vector<std::unique_ptr<cudf::column>> converted_cols;
converted_cols.reserve(parsed_columns.size());
for (std::size_t i = 0; i < parsed_columns.size(); ++i) {
Expand All @@ -855,6 +938,10 @@ std::unique_ptr<cudf::column> from_json_to_structs(cudf::strings_column_view con

auto const& [col_name, col_schema] = schema_with_precision.child_types[i];
CUDF_EXPECTS(parsed_meta.schema_info[i].name == col_name, "Mismatched column name.");
auto const mismatch_rows = mismatch_rows_by_column.find(col_name);
if (mismatch_rows != mismatch_rows_by_column.end()) {
nullify_rows(*parsed_columns[i], *mismatch_rows->second, stream, mr);
}

@ttnghia ttnghia Jul 8, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may or may not set nulls for the output. As such, we should call has_nonempty_nulls or make_structs_column only if there was call to nullify_rows. This can be achieved by passing a boolean flag has_rows_nullified to convert_data_type all the way down to creating the final output.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

converted_cols.emplace_back(convert_data_type(std::move(parsed_columns[i]),
col_schema,
allow_nonnumeric_numbers,
Expand All @@ -867,15 +954,13 @@ std::unique_ptr<cudf::column> from_json_to_structs(cudf::strings_column_view con
auto [null_mask, null_count] = cudf::detail::valid_if(
valid_it, valid_it + should_be_nullified->size(), thrust::logical_not<bool>{}, stream, mr);

// Do not use `cudf::make_structs_column` since we do not need to call `superimpose_nulls`
// on the children columns.
return std::make_unique<cudf::column>(
cudf::data_type{cudf::type_id::STRUCT},
return make_structs_column_with_null_consistency(
input.size(),
rmm::device_buffer{},
null_count > 0 ? std::move(null_mask) : rmm::device_buffer{0, stream, mr},
std::move(converted_cols),
null_count,
std::move(converted_cols));
null_count > 0 ? std::move(null_mask) : rmm::device_buffer{0, stream, mr},
stream,
mr);
}

} // namespace
Expand Down
Loading