from_json: null only schema-mismatched rows [databricks] - #4728
from_json: null only schema-mismatched rows [databricks]#4728wjxiz1992 wants to merge 13 commits into
Conversation
5b64481 to
1f9b3c9
Compare
Signed-off-by: Allen Xu <allxu@nvidia.com>
1f9b3c9 to
267ce8b
Compare
|
Requesting early JNI-side review while this remains draft. Context: depends on rapidsai/cudf#22915. Could you please review the JNI integration and row-level nulling approach before the cuDF dependency lands? The main things I would like feedback on are:
Thanks @ttnghia @jihoonson @thirtiseven. |
Greptile SummaryThis PR replaces the previously-reverted whole-column schema-mismatch nulling with targeted, row-level nulling of only the affected depth-1 parent columns, matching Spark's
Confidence Score: 5/5Safe to merge; the change is well-scoped, all previous review concerns have been addressed, and the new regression suite exercises the affected code paths including word-boundary grouping and LIST nonempty-null sanitization. All three previously-flagged issues (O(N×M) linear scan, non-atomic same-word race, missing nonempty-null guard on LIST) are resolved in this revision. The new nullify_rows correctly merges bits per 32-bit word before dispatching non-atomic GPU writes, and falls back to per-row atomic_ref when rows span more words than the merge saves. The make_lists_column_with_null_sanitization function gates the expensive purge behind has_nonempty_nulls. The unordered_map replaces the per-column linear search. Tests specifically exercise word-boundary grouping (rows 0, 1, 2, 31, 32, 63, 64), multi-column mismatch isolation, LIST null sanitization, and empty/null inputs. No new logic gaps were found. Files Needing Attention: No files require special attention; from_json_to_structs.cu is the most complex file but is thoroughly covered by the new test suite. Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller
participant from_json_to_structs
participant read_json_with_row_diagnostics
participant nullify_rows
participant convert_data_type
participant make_lists_col as make_lists_column_with_null_sanitization
participant make_structs_col as make_structs_column_with_null_consistency
Caller->>from_json_to_structs: input strings, schema
from_json_to_structs->>read_json_with_row_diagnostics: opts with strict_validation
read_json_with_row_diagnostics-->>from_json_to_structs: parsed data plus diagnostics
note over from_json_to_structs: build unordered_map of col_name to mismatch row_indices
loop each top-level schema column
from_json_to_structs->>nullify_rows: column plus mismatch row_indices
note over nullify_rows: merge bits per word then use non-atomic write or atomic_ref per row
nullify_rows-->>from_json_to_structs: null mask updated in place
from_json_to_structs->>convert_data_type: column with did_nullify flag
alt LIST column
convert_data_type->>make_lists_col: offsets, child, null_count, did_nullify
note over make_lists_col: has_nonempty_nulls guard before purge_nonempty_nulls
make_lists_col-->>convert_data_type: sanitized LIST column
else STRUCT column
convert_data_type->>make_structs_col: children, null_mask, did_nullify
note over make_structs_col: if did_nullify use make_structs_column to superimpose parent nulls
make_structs_col-->>convert_data_type: consistent STRUCT column
end
convert_data_type-->>from_json_to_structs: converted column
end
note over from_json_to_structs: assemble top-level STRUCT with should_be_nullified mask
from_json_to_structs-->>Caller: output STRUCT column
Reviews (12): Last reviewed commit: "Address from_json follow-up review feedb..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Commit 267ce8b accidentally swept a thirdparty/cudf gitlink bump (18e8ccd8d7 -> c9cb6c288f) into the from_json row-mask fix. The row-mask approach is JNI-side and needs no cudf change, so reset the gitlink to the merge-base. This drops thirdparty/cudf from the PR diff; since only main's side then differs from the merge-base, the submodule no longer conflicts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Allen Xu <allxu@nvidia.com>
| 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); } |
There was a problem hiding this comment.
Use cudf::has_nonempty_nulls() to reduce overhead.
| 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); |
There was a problem hiding this comment.
Why is this executed on host?
There was a problem hiding this comment.
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.
…w-mask Signed-off-by: Allen Xu <allxu@nvidia.com> # Conflicts: # src/test/java/com/nvidia/spark/rapids/jni/FromJsonToStructsTest.java
There was a problem hiding this comment.
Pull request overview
This PR updates from_json_to_structs to match Spark’s from_json behavior more closely by nulling only the depth-1 parent rows affected by JSON schema mismatches (instead of nulling entire columns). It consumes cuDF’s new row-level schema-mismatch diagnostics, adds a JNI regression test to lock in Spark parity, and introduces an nvbench target to measure the affected path.
Changes:
- Switch
from_json_to_structsto use cuDF row-level schema-mismatch diagnostics and null only the mismatched rows for each top-level column. - Ensure nested LIST/STRUCT outputs remain null-consistent by superimposing parent nulls onto children and sanitizing non-empty null LIST rows when needed.
- Add a Java regression test and a focused nvbench benchmark target for
from_json_to_structs.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/main/cpp/src/from_json_to_structs.cu | Applies row-level mismatch nulling and enforces nested null consistency (STRUCT propagation, LIST sanitization). |
| src/test/java/com/nvidia/spark/rapids/jni/FromJsonToStructsTest.java | Adds regression coverage verifying only mismatched rows are nulled and siblings remain intact. |
| src/main/cpp/benchmarks/from_json_to_structs.cu | Adds a benchmark for from_json_to_structs with configurable mismatch rate. |
| src/main/cpp/benchmarks/CMakeLists.txt | Wires the new from_json_to_structs nvbench target into the build. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| auto mask_ptr = static_cast<cudf::bitmask_type*>(null_mask.data()); | ||
| thrust::for_each(rmm::exec_policy_nosync(stream), | ||
| d_row_indices.begin(), | ||
| d_row_indices.end(), | ||
| [mask_ptr] __device__(auto const row) { cudf::clear_bit(mask_ptr, row); }); |
There was a problem hiding this comment.
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.
| // Row-level schema mismatch nulls can leave child data under null parents; sanitize it here. | ||
| if (cudf::has_nonempty_nulls(output->view(), stream)) { | ||
| output = cudf::purge_nonempty_nulls(output->view(), stream, mr); | ||
| } |
There was a problem hiding this comment.
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.
Signed-off-by: Allen Xu <allxu@nvidia.com>
Signed-off-by: Allen Xu <allxu@nvidia.com>
| auto const null_count = cudf::null_count( | ||
| static_cast<cudf::bitmask_type const*>(null_mask.data()), 0, input_view.size(), stream); |
There was a problem hiding this comment.
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());
There was a problem hiding this comment.
Thanks, updated.
| if (mismatch_rows != mismatch_rows_by_column.end()) { | ||
| nullify_rows(*parsed_columns[i], *mismatch_rows->second, stream, mr); | ||
| } |
There was a problem hiding this comment.
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.
Signed-off-by: Allen Xu <allxu@nvidia.com>
Signed-off-by: Allen Xu <allxu@nvidia.com>
firestarman
left a comment
firestarman
left a comment
There was a problem hiding this comment.
Quick multi-agent review (validation skipped): 0 must-fix, 7 should-fix, and 6 suggestions. Related findings on the same code line are grouped into one inline comment.
| * limitations under the License. | ||
| */ | ||
|
|
||
| #include <cudf_test/column_wrapper.hpp> |
There was a problem hiding this comment.
🟡 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.
There was a problem hiding this comment.
Thanks, updated.
|
|
||
| namespace { | ||
|
|
||
| std::unique_ptr<cudf::column> make_input(cudf::size_type num_rows, cudf::size_type mismatch_percent) |
There was a problem hiding this comment.
🟢 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.
| types, | ||
| scales, | ||
| precisions, | ||
| true, |
There was a problem hiding this comment.
🟢 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.
|
|
||
| NVBENCH_BENCH(BM_from_json_to_structs) | ||
| .set_name("from_json_to_structs") | ||
| .add_int64_axis("num_rows", {10000, 100000}) |
There was a problem hiding this comment.
🟡 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.
| NVBENCH_BENCH(BM_from_json_to_structs) | ||
| .set_name("from_json_to_structs") | ||
| .add_int64_axis("num_rows", {10000, 100000}) | ||
| .add_int64_axis("mismatch_percent", {0, 1}); |
There was a problem hiding this comment.
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.
|
|
||
| auto const input_view = input.view(); | ||
| auto null_mask = | ||
| input_view.nullable() |
There was a problem hiding this comment.
🟡 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.
| 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), |
There was a problem hiding this comment.
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.
| input.set_null_mask(std::move(null_mask), null_count); | ||
| } | ||
|
|
||
| std::unique_ptr<cudf::column> make_lists_column_with_null_sanitization( |
There was a problem hiding this comment.
🟢 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.
| child_schema, | ||
| allow_nonnumeric_numbers, | ||
| is_us_locale, | ||
| has_rows_nullified, |
There was a problem hiding this comment.
🟡 SHOULD FIX
Can the recursive child conversions at src/main/cpp/src/from_json_to_structs.cu:798 and src/main/cpp/src/from_json_to_structs.cu:821 pass false for has_rows_nullified? The only mask changed by src/main/cpp/src/from_json_to_structs.cu:969 is the current top-level parsed column. Propagating true into every descendant makes nullable nested LIST nodes run the exact recursive scan at src/main/cpp/src/from_json_to_structs.cu:202 and makes nullable nested STRUCT nodes invoke the subtree-wide mask superimposition at src/main/cpp/src/from_json_to_structs.cu:217 before the current node repeats that work at src/main/cpp/src/from_json_to_structs.cu:812 or src/main/cpp/src/from_json_to_structs.cu:834. On deep schemas with pre-existing nested nulls, this produces repeated full-column mask kernels, exact non-empty-null scans, and possible identity gathers. The current node's helper already handles the entire reconstructed subtree after its children return: the LIST helper scans/purges the complete output at src/main/cpp/src/from_json_to_structs.cu:202, while the STRUCT helper delegates to the recursive cuDF implementation at thirdparty/cudf/cpp/src/structs/structs_column_factories.cu:34.
Suggested fix: Pass false to recursive child conversions at src/main/cpp/src/from_json_to_structs.cu:802, src/main/cpp/src/from_json_to_structs.cu:825, src/main/cpp/src/from_json_to_structs.cu:858, and src/main/cpp/src/from_json_to_structs.cu:881. Preserve has_rows_nullified only for the helper that reconstructs the current node at src/main/cpp/src/from_json_to_structs.cu:812, src/main/cpp/src/from_json_to_structs.cu:834, src/main/cpp/src/from_json_to_structs.cu:868, and src/main/cpp/src/from_json_to_structs.cu:890.
Confidence: 🟣❗ CERTAIN; reviewer scope: gpu-kernel-optimization.
| cudf::copy_bitmask(input, stream, mr), | ||
| null_count, | ||
| std::move(new_children)); | ||
| new_children.emplace_back(convert_data_type(child, |
There was a problem hiding this comment.
🟢 SUGGESTION
Could the nested cudf::column_view arm at src/main/cpp/src/from_json_to_structs.cu:838-893 delegate to the owning arm at src/main/cpp/src/from_json_to_structs.cu:781-837? The two arms repeat the LIST/STRUCT recursion and reconstruction, including the new has_rows_nullified propagation at src/main/cpp/src/from_json_to_structs.cu:854-892. The only nonrecursive view entry at src/main/cpp/src/from_json_to_structs.cu:1050-1055 receives a cudf::strings_column_view; thirdparty/cudf/cpp/src/strings/strings_column_view.cpp:14-17 enforces that this view has STRING type, so no current call enters the duplicated nested-view arm.
Suggested fix: At src/main/cpp/src/from_json_to_structs.cu:781, can you normalize a nested view to an owning column and then reuse the existing owning branch? The deep-copy constructor required by this delegation is defined at thirdparty/cudf/cpp/include/cudf/column/column.hpp:122-134 and accepts the same stream and memory resource. This keeps one implementation of the recursive null-consistency logic while preserving support for a future nested-view caller.
Confidence: 🟣❗ CERTAIN; reviewer scope: refactor.
There was a problem hiding this comment.
I kept the view branch separate. Delegating it to the owning branch would first deep-copy the entire nested subtree, including children that conversion immediately replaces, while the current view path copies only retained buffers and converted children. The only current view caller is STRING, so that extra allocation would not simplify an exercised nested path.
Signed-off-by: Allen Xu <allxu@nvidia.com>
|
build |
firestarman
left a comment
firestarman
left a comment
There was a problem hiding this comment.
Follow-up quick review of the updated PR head. Detailed findings are attached inline.
| auto const scales = nested_schema_scales(); | ||
| auto const precisions = nested_schema_precisions(); | ||
|
|
||
| state.exec(nvbench::exec_tag::sync, [&](nvbench::launch& launch) { |
There was a problem hiding this comment.
🟡 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.
There was a problem hiding this comment.
Thanks, updated in ca0366b: NVBench now records on the API default stream. The benchmark target built and all 10 smoke cases completed.
| auto const precisions = nested_schema_precisions(); | ||
|
|
||
| state.exec(nvbench::exec_tag::sync, [&](nvbench::launch& launch) { | ||
| auto output = spark_rapids_jni::from_json_to_structs(cudf::strings_column_view{input->view()}, |
There was a problem hiding this comment.
🟢 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.
There was a problem hiding this comment.
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.
| state.add_buffer_size(num_rows, "rows", "Rows"); | ||
| } | ||
|
|
||
| NVBENCH_BENCH(BM_from_json_to_structs) |
There was a problem hiding this comment.
🟡 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.
There was a problem hiding this comment.
Done in ca0366b: added a two-column regression with independent mismatch rows for a and b. FromJsonToStructsTest passed 5/5.
|
|
||
| // Diagnostic row indices are sorted and unique, so updates to one mask word are adjacent. | ||
| std::vector<mask_word_update> word_updates; | ||
| word_updates.reserve(row_indices.size()); |
There was a problem hiding this comment.
🟡 SHOULD FIX
Can the host reservation be bounded by the number of mask words? src/main/cpp/src/from_json_to_structs.cu:173 reserves one mask_word_update slot per diagnostic row before src/main/cpp/src/from_json_to_structs.cu:174 compacts all rows sharing a word into one element. Because cudf::bitmask_type is 32 bits at thirdparty/cudf/cpp/include/cudf/types.hpp:86, the 100% mismatch workload registered at src/main/cpp/benchmarks/from_json_to_structs.cu:103 reserves 32 times the element capacity that the compacted vector can use. On large dense diagnostics this unnecessary host allocation can dominate the diagnostic payload and fail before the GPU update is launched.
Suggested fix: Reserve the smaller of the diagnostic count and cudf::num_bitmask_words(input_view.size()). The latter is the exact upper bound on distinct word_index values, while the diagnostic count preserves the tight reservation for sparse inputs.
Confidence: 🟣❗ CERTAIN; reviewer scope: gpu-kernel-optimization.
There was a problem hiding this comment.
Done in ca0366b: the pinned host reservation is bounded by min(diagnostic rows, mask words). The native target built successfully.
| } | ||
| } | ||
|
|
||
| auto const use_word_updates = |
There was a problem hiding this comment.
🟡 SHOULD FIX
Could the regression exercise the grouped mask-word update and bitmask word boundaries? The only mismatch regression at src/test/java/com/nvidia/spark/rapids/jni/FromJsonToStructsTest.java:95 supplies two mismatch indices in one 32-bit word. On this ABI, src/main/cpp/src/from_json_to_structs.cu:73 contains two 32-bit fields, so one 8-byte mask_word_update is not smaller than two 4-byte row indices; the strict comparison at src/main/cpp/src/from_json_to_structs.cu:184 selects the atomic fallback. No assertion currently executes the performance optimization at src/main/cpp/src/from_json_to_structs.cu:187 or clears rows across the 31/32 and 63/64 word boundaries. A mutation that drops bits_to_clear, writes the wrong word_index, or shifts the boundary calculation would pass the current test suite.
Suggested fix: Add an assertion-based input with more than 64 rows and sorted mismatches such as {0, 1, 2, 31, 32, 63, 64}. Those seven indices produce three 8-byte word updates versus seven 4-byte row indices, force the grouped branch, cross two mask-word boundaries, and can verify the exact null pattern plus preservation of the sibling id field. Keep the existing two-adjacent-row case because it covers the atomic fallback and preservation of a pre-existing nullable mask.
Confidence: 🟣❗ CERTAIN; reviewer scope: test-coverage.
There was a problem hiding this comment.
Done in ca0366b: added the 65-row regression with mismatches at 0, 1, 2, 31, 32, 63, and 64, asserting the full result and sibling ids. FromJsonToStructsTest passed 5/5.
| auto const use_word_updates = | ||
| word_updates.size() * sizeof(mask_word_update) < row_indices.size() * sizeof(cudf::size_type); | ||
| if (use_word_updates) { | ||
| auto d_word_updates = cudf::detail::make_device_uvector_async( |
There was a problem hiding this comment.
🔴 MUST FIX
Can the grouped mask updates use stream-ordered pinned host storage? The local word_updates std::vector at src/main/cpp/src/from_json_to_structs.cu:172 is passed to the non-synchronizing upload at src/main/cpp/src/from_json_to_structs.cu:187. For initially non-nullable input, src/main/cpp/src/from_json_to_structs.cu:207-213 computes the count from the host vector and returns without synchronizing. On the supported CUDA 13 build at ci/Jenkinsfile.premerge:33-34, the helper reaches cudaMemcpyBatchAsync with cudaMemcpySrcAccessOrderStream at thirdparty/cudf/cpp/src/utilities/cuda_memcpy.cu:62-96, so source access is deferred until the stream reaches the copy. The local vector can be destroyed first, leaving the upload to read freed host memory and allowing schema-mismatched rows to remain valid or receive invalid word updates.
Suggested fix: Allocate word_updates with the existing pinned host-vector factory and the same stream. Its allocator passes deallocation to the host async resource on that stream at thirdparty/cudf/cpp/include/cudf/detail/utilities/host_vector.hpp:150-163, keeping the staging allocation valid without adding a stream synchronization.
Confidence: 🟣❗ CERTAIN; reviewer scope: memory-stream.
There was a problem hiding this comment.
Done in ca0366b: both grouped updates and atomic-fallback row indices now use stream-ordered pinned host staging. The native target built and FromJsonToStructsTest passed 5/5.
| } | ||
| } | ||
|
|
||
| [[nodiscard]] std::unique_ptr<cudf::column> make_lists_column_with_null_sanitization( |
There was a problem hiding this comment.
🟢 SUGGESTION
Could the repeated nested-column state be grouped into a named parameter object? The LIST factory at src/main/cpp/src/from_json_to_structs.cu:217 has eight positional parameters and the STRUCT factory at src/main/cpp/src/from_json_to_structs.cu:243 has seven; both carry the same num_rows, null_count, null_mask, and has_rows_nullified state. That four-value clump is repeated in the owning LIST call at src/main/cpp/src/from_json_to_structs.cu:836, the owning STRUCT call at src/main/cpp/src/from_json_to_structs.cu:860, both view calls at src/main/cpp/src/from_json_to_structs.cu:887 and src/main/cpp/src/from_json_to_structs.cu:911, and the outer STRUCT call at src/main/cpp/src/from_json_to_structs.cu:1009. A named aggregate makes the row-count/null-count boundary state explicit at each call and gives future null-consistency changes one shared abstraction.
Suggested fix: Define a nested_column_state aggregate beside mask_word_update at src/main/cpp/src/from_json_to_structs.cu:73, pass it by value to both factories, and use designated initializers at every call. Keep stream and mr as the final standalone parameters so the project stream/resource convention remains unchanged.
Confidence: 🟣❗ CERTAIN; reviewer scope: refactor.
There was a problem hiding this comment.
I left this refactor out of ca0366b. The proposed aggregate would own and move the null mask, expanding a correctness-focused change across every recursive factory call without changing behavior; the existing explicit parameters keep ownership visible. I can follow up separately if you still prefer the API consolidation.
| null_count, | ||
| std::move(children)); | ||
| // Row-level schema mismatch nulls can leave child data under null parents; sanitize it here. | ||
| if (has_rows_nullified && null_count > 0 && cudf::has_nonempty_nulls(output->view(), stream)) { |
There was a problem hiding this comment.
🟡 SHOULD FIX
Would it make sense to add a top-level LIST mismatch regression? The changed helper at src/main/cpp/src/from_json_to_structs.cu:237 only scans and purges a LIST when has_rows_nullified is true. The current schema at src/test/java/com/nvidia/spark/rapids/jni/FromJsonToStructsTest.java:47 makes the diagnostic parent a top-level STRUCT named data; its nested c2 LIST is deliberately converted with has_rows_nullified=false at src/main/cpp/src/from_json_to_structs.cu:833. The current hasNonEmptyNulls assertion therefore validates cuDF's STRUCT superimposition path, but it cannot detect deleting or inverting the top-level LIST sanitation condition at src/main/cpp/src/from_json_to_structs.cu:237.
Suggested fix: Add a schema whose root child is LIST<STRUCT<...>>, mix valid rows with a row containing a scalar element where a STRUCT element is required, assert the full output, and assert that the top-level LIST child has no non-empty nulls. That sends has_rows_nullified=true through the owning LIST arm at src/main/cpp/src/from_json_to_structs.cu:821.
Confidence: 🟣❗ CERTAIN; reviewer scope: test-coverage.
There was a problem hiding this comment.
Done in ca0366b: added a top-level LIST mismatch regression and asserted both the full output and no non-empty nulls. FromJsonToStructsTest passed 5/5.
|
|
||
| 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; |
There was a problem hiding this comment.
🟢 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.
There was a problem hiding this comment.
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.
| auto const mismatch_rows = mismatch_rows_by_column.find(col_name); | ||
| auto const has_rows_nullified = | ||
| mismatch_rows != mismatch_rows_by_column.end() && !mismatch_rows->second->empty(); | ||
| if (has_rows_nullified) { |
There was a problem hiding this comment.
🟡 SHOULD FIX
Could the row-level schema-mismatch contract introduced at src/main/cpp/src/from_json_to_structs.cu:965-1000 be documented at the public Java API boundary at src/main/java/com/nvidia/spark/rapids/jni/JSONUtils.java:238-258? The current Javadoc says only that each JSON row is parsed according to the schema; it does not state that a descendant category mismatch nulls only the affected depth-1 parent for that row while preserving sibling top-level fields. That behavior is externally visible and is explicitly asserted at src/test/java/com/nvidia/spark/rapids/jni/FromJsonToStructsTest.java:85-115, so leaving it implicit makes it difficult for callers and future maintainers to distinguish the intended Spark-compatible contract from an implementation detail.
Suggested fix: Add the exact mismatch/nulling semantics to the Javadoc for the public Java entry point at src/main/java/com/nvidia/spark/rapids/jni/JSONUtils.java:238-258 so callers know which part of a row becomes null and that unaffected siblings remain available.
Confidence: 🟣❗ CERTAIN; reviewer scope: architecture.
There was a problem hiding this comment.
Done in ca0366b: the public Java Javadoc now documents depth-one parent nulling and preservation of unaffected top-level siblings. Java compilation and FromJsonToStructsTest passed 5/5.
Signed-off-by: Allen Xu <allxu@nvidia.com>
|
NOTE: release/26.08 has been created from main. Please retarget your PR to release/26.08 if it should be included in the release. |
|
All current review feedback has been addressed on head ca0366b, and all visible checks are green. @firestarman, could you please confirm the follow-up changes and resolve the remaining review threads when you have a chance? @ttnghia, could you please take a final maintainer look? This unblocks downstream NVIDIA/cudf-spark#14773. |
thirtiseven
left a comment
thirtiseven
left a comment
There was a problem hiding this comment.
Two really optional comments (i'm playing with a new review skill, feel free to ignore).
Please update the PR description and making sure it won't break plugin tests before merging. Otherwise LGTM.
| : static_cast<cudf::bitmask_type*>(null_mask.data()); | ||
|
|
||
| // Diagnostic row indices are sorted and unique, so updates to one mask word are adjacent. | ||
| auto word_updates = cudf::detail::make_empty_pinned_vector<mask_word_update>( |
There was a problem hiding this comment.
Could this select the compact representation before allocating pinned staging storage? The sparse path first allocates and fills pinned word_updates, then discards it and allocates pinned h_row_indices. Each make_empty_pinned_vector reserve reaches a stream.synchronize(), so sparse diagnostics pay two pinned allocations and two synchronizations. Please build or count the grouping in ordinary host storage first, choose the representation, and pin only the range actually uploaded.
There was a problem hiding this comment.
Thanks, updated in 805559f. The compact representation is selected in ordinary host storage before pinning only the uploaded range. Native build and FROM_JSON 52/52 passed; Java/JNI 6/6 passed.
|
|
||
| // Diagnostic row indices are sorted and unique, so updates to one mask word are adjacent. | ||
| auto word_updates = cudf::detail::make_empty_pinned_vector<mask_word_update>( | ||
| std::min(row_indices.size(), |
There was a problem hiding this comment.
Could you include <algorithm> directly? This code adds std::min, but the standard-header group does not include its owning header.
There was a problem hiding this comment.
Done in 805559f; <algorithm> is now included directly.
| thrust::for_each(rmm::exec_policy_nosync(stream), | ||
| d_word_updates.begin(), | ||
| d_word_updates.end(), | ||
| [mask_ptr] __device__(auto const update) { |
There was a problem hiding this comment.
Could both changed extended device lambdas declare -> void? The cuDF developer guide requires explicit return types for device lambdas; please add it to the update lambda here and the row lambda below.
There was a problem hiding this comment.
Done in 805559f; both device lambdas now declare -> void.
| d_row_indices.end(), | ||
| [mask_ptr] __device__(auto const row) { | ||
| // clear_bit uses atomicAnd, so concurrent updates to one mask word are safe. | ||
| cudf::clear_bit(mask_ptr, row); |
There was a problem hiding this comment.
Could this sparse mask update use a device-scoped cuda::atomic_ref directly? cudf::clear_bit is thread-safe but currently calls legacy atomicAnd; the project's atomics convention requires new device atomics to use cuda::atomic_ref. A relaxed fetch_and on the containing mask word preserves the required atomicity.
There was a problem hiding this comment.
Done in 805559f; the sparse path now uses a device-scoped cuda::atomic_ref with relaxed fetch_and.
| } | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
Could the regression matrix cover both a zero-row input and an actual null input string? The current preExistingNull is a valid JSON object with a null child, so it does not exercise the empty-input path or whole-row null-string path that feed the changed diagnostics and root-mask reconstruction.
There was a problem hiding this comment.
Done in 805559f. Added zero-row and actual null-string coverage; Java/JNI FromJsonToStructsTest passed 6/6.
Signed-off-by: Allen Xu <allxu@nvidia.com>
|
Updated the PR description with the current native/Java validation and added downstream cudf-spark #14773 testing as an explicit merge gate. |
Signed-off-by: Allen Xu <allxu@nvidia.com>
|
build |
|
build |
|
pre-commit.ci run |
| try (ColumnVector emptyInput = ColumnVector.fromStrings(); | ||
| ColumnVector emptyOutput = | ||
| JSONUtils.fromJSONToStructs(emptyInput, schema, getOptions(), true)) { | ||
| assertEquals(0, emptyOutput.getRowCount()); | ||
| assertEquals(0, emptyOutput.getNullCount()); | ||
| } |
There was a problem hiding this comment.
The zero-row path reconstructs the full nested schema, but these assertions only validate cardinality. A result with the wrong child count/order or LIST element type would still pass. Could we compare against a typed empty STRUCT so the recursive column assertion checks the schema as well?
| try (ColumnVector emptyInput = ColumnVector.fromStrings(); | |
| ColumnVector emptyOutput = | |
| JSONUtils.fromJSONToStructs(emptyInput, schema, getOptions(), true)) { | |
| assertEquals(0, emptyOutput.getRowCount()); | |
| assertEquals(0, emptyOutput.getNullCount()); | |
| } | |
| try (ColumnVector emptyInput = ColumnVector.fromStrings(); | |
| ColumnVector emptyOutput = | |
| JSONUtils.fromJSONToStructs(emptyInput, schema, getOptions(), true); | |
| ColumnVector expectedEmpty = ColumnVector.fromStructs(schema.asHostDataType())) { | |
| assertColumnsAreEqual(expectedEmpty, emptyOutput); | |
| } |
| #include <algorithm> | ||
| #include <map> | ||
| #include <span> | ||
| #include <unordered_map> |
There was a problem hiding this comment.
std::back_inserter is declared in <iterator>. Could we include it directly rather than rely on a transitive include?
| #include <algorithm> | |
| #include <map> | |
| #include <span> | |
| #include <unordered_map> | |
| #include <algorithm> | |
| #include <iterator> | |
| #include <map> | |
| #include <span> | |
| #include <unordered_map> |
wjxiz1992
changed the title
|
@wjxiz1992 FYI we don't have databricks CI in this repo. |
Summary
from_jsonbehaviorfrom_json_to_structsnvbench targetContributes to #4645. This is the follow-up to the reverted #4536 / #4706 path.
Dependency
2e4198895930d330d7a6dfa15a8e984348c612cathirdparty/cudfat that upstream merge commitcudf-spark-jnimainReview follow-up
cudf::has_nonempty_nulls()before purging LIST null rows, and skip that scan entirely when the LIST has no nullscudf::clear_bit()for validity updates; cuDF implements it withatomicAnd, so same-word updates are thread-safeValidation (2026-07-08)
ninja -C target/jni/cmake-build spark_rapids_jni FROM_JSON -j12from_json_to_structs.cucompiled,libcudf.solinked, andFROM_JSONlinkedmvn -o surefire:test@default-test -Dtest=FromJsonToStructsTest: Tests run: 2, Failures: 0, Errors: 0, Skipped: 0mvn -o surefire:test@non-empty-null-test: Tests run: 4, Failures: 0, Errors: 0, Skipped: 0mvn -o surefire:test@fatal-cuda-test: Tests run: 1, Failures: 0, Errors: 0, Skipped: 0mvn -o jar:jar:BUILD SUCCESSsrc/main/cpp/src/from_json_to_structs.cuReview validation (2026-07-30)
spark_rapids_jniandFROM_JSONlinked successfullyFROM_JSON: 52 tests passedFromJsonToStructsTest: Tests run: 6, Failures: 0, Errors: 0, Skipped: 0 (includes zero-row and null-string inputs)src/main/cpp/src/from_json_to_structs.cuDownstream plugin validation
Performance
The plain
read_jsonpath remains unchanged. Row diagnostics and null-mask updates are used only byfrom_json_to_structs. The review updates replace repeated host diagnostic scans with constant-time column lookups and avoid a recursive non-empty-null scan for all-valid LIST columns. The existing nvbench target remains in this PR; it was not rerun during the 2026-07-08 dependency refresh.Documentation