-
Notifications
You must be signed in to change notification settings - Fork 89
from_json: null only schema-mismatched rows #4728
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 2 commits
267ce8b
273f2e9
1a79c86
d87d85e
c2f2e46
ddfb4ad
4729276
ac17dbd
ca0366b
bdc9e71
21ebd2b
fc162be
805559f
e97bb27
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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> | ||
|
|
||
| #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) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟢 SUGGESTION Could the new Suggested fix: Add Confidence: 🟣❗ CERTAIN; reviewer scope: compliant.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Suggested fix: Set NVBench's target stream to Confidence: 🟣❗ CERTAIN; reviewer scope: gpu-kernel-optimization.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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()}, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Confidence: 🟣❗ CERTAIN; reviewer scope: compliant.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟢 SUGGESTION Could the five consecutive 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.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ( Suggested fix: Add a two-column schema with Confidence: 🟣❗ CERTAIN; reviewer scope: test-coverage.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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}) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; Suggested fix: Write the two values as Confidence: 🟣❗ CERTAIN; reviewer scope: compliant.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
| .add_int64_axis("mismatch_percent", {0, 1}); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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> | ||
|
|
@@ -48,6 +51,9 @@ | |
| #include <thrust/transform.h> | ||
| #include <thrust/uninitialized_fill.h> | ||
|
|
||
| #include <algorithm> | ||
| #include <map> | ||
|
|
||
| namespace spark_rapids_jni { | ||
|
|
||
| namespace detail { | ||
|
|
@@ -142,6 +148,80 @@ 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(); | ||
| for (auto const row : row_indices) { | ||
| CUDF_EXPECTS(row >= 0 && row < input_view.size(), "Schema mismatch row index out of bounds."); | ||
| } | ||
|
greptile-apps[bot] marked this conversation as resolved.
Outdated
|
||
|
|
||
| auto null_mask = | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Confidence: 🟣❗ CERTAIN; reviewer scope: gpu-kernel-optimization.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
| input_view.nullable() | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Suggested fix: Add an explicit Confidence: 🟣❗ CERTAIN; reviewer scope: test-coverage.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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), | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Confidence: 🟠❗ LIKELY; reviewer scope: gpu-kernel-optimization.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); }); | ||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
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); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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( | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟢 SUGGESTION Could both new nested-column factories be marked Suggested fix: Add Confidence: 🟣❗ CERTAIN; reviewer scope: compliant.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { output = cudf::purge_nonempty_nulls(output->view(), stream, mr); } | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| return output; | ||
|
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, | ||
|
|
@@ -711,14 +791,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) { | ||
|
|
@@ -733,14 +813,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(); | ||
|
|
@@ -761,14 +839,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) { | ||
|
|
@@ -783,14 +861,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); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -838,9 +914,9 @@ 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."); | ||
|
|
@@ -855,6 +931,14 @@ 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 = | ||
| std::find_if(parsed_result.diagnostics.top_level_columns_with_schema_mismatch_rows.begin(), | ||
| parsed_result.diagnostics.top_level_columns_with_schema_mismatch_rows.end(), | ||
| [&col_name](auto const& row_info) { return row_info.column_name == col_name; }); | ||
| if (mismatch_rows != | ||
| parsed_result.diagnostics.top_level_columns_with_schema_mismatch_rows.end()) { | ||
| nullify_rows(*parsed_columns[i], mismatch_rows->row_indices, stream, mr); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this executed on host?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The cuDF row-diagnostics API returns the mismatch row vectors on the host by design, so this lookup remains host-side. In 1a79c86 the diagnostics are indexed once into an unordered_map and each schema column performs an O(1) lookup; only the null-mask update runs on the device. The focused native and Java/JNI validation passed. |
||
| } | ||
|
greptile-apps[bot] marked this conversation as resolved.
Outdated
|
||
| converted_cols.emplace_back(convert_data_type(std::move(parsed_columns[i]), | ||
| col_schema, | ||
| allow_nonnumeric_numbers, | ||
|
|
@@ -867,15 +951,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 | ||
|
|
||
There was a problem hiding this comment.
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::columnheader directly and keep its project-local header in the first include group?src/main/cpp/benchmarks/from_json_to_structs.cu:31namescudf::column, andsrc/main/cpp/benchmarks/from_json_to_structs.cu:81callscudf::column::view(), butsrc/main/cpp/benchmarks/from_json_to_structs.cu:17-23has no<cudf/column/column.hpp>and places the internalsrc/main/cpp/src/json_utils.hpp:17header after other RAPIDS headers using angle brackets. This makes the translation unit depend on another header exposing the completecudf::columndefinition 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, updated.