Fix JSON reader guards for scatter validity, validation, and max nesting depth - #22452
Merged
rapids-bot[bot] merged 8 commits intoMay 18, 2026
Merged
Conversation
karthikeyann
added
bug
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR hardens JSON parsing by guarding validity-mask writes, fixing non-ASCII and numeric-token handling, adding device-checked nesting-depth casting with an atomic out-of-range flag, exporting token-stream validation, and expanding tests for malformed inputs and depth limits. ChangesJSON Parsing Robustness: Validity Masks, Character Encoding, Depth Bounds, and Token Validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
karthikeyann
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/src/io/json/process_tokens.cu (1)
115-169:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject exponents directly after
.in strict numeric validation.
number_state::SAW_RADIXstill transitions toSTART_EXPONENT, so inputs like1.e2and0.e1are accepted even though JSON requires at least one fractional digit after the decimal point. That leaves strict validation too permissive in the same path this PR is tightening.Suggested fix
case number_state::SAW_RADIX: if (c >= '0' && c <= '9') { num_state = number_state::FRACTION; - } else if ('e' == c || 'E' == c) { - num_state = number_state::START_EXPONENT; } else { return false; } break;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/io/json/process_tokens.cu` around lines 115 - 169, The SAW_RADIX state currently allows an exponent transition (to number_state::START_EXPONENT), which accepts forms like "1.e2"; change the number_state::SAW_RADIX branch so it only accepts a digit (transition to number_state::FRACTION) and otherwise returns false—remove the 'e'/'E' -> START_EXPONENT branch so an exponent is only allowed after at least one fractional digit; update the switch case in process_tokens.cu handling number_state::SAW_RADIX accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/tests/io/json/json_test.cpp`:
- Around line 1605-1611: The loop that verifies root column names uses
a_child_col_names.size() as its bound, so the final root column ("d") is never
checked; change the loop bound to iterate over root_col_names.size() (or
new_reader_table.metadata.schema_info.size()) instead so every entry in
new_reader_table.metadata.schema_info is compared against root_col_names[i] in
that for loop.
---
Outside diff comments:
In `@cpp/src/io/json/process_tokens.cu`:
- Around line 115-169: The SAW_RADIX state currently allows an exponent
transition (to number_state::START_EXPONENT), which accepts forms like "1.e2";
change the number_state::SAW_RADIX branch so it only accepts a digit (transition
to number_state::FRACTION) and otherwise returns false—remove the 'e'/'E' ->
START_EXPONENT branch so an exponent is only allowed after at least one
fractional digit; update the switch case in process_tokens.cu handling
number_state::SAW_RADIX accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: cebd270f-24a7-449c-b56c-8dfba460e276
📒 Files selected for processing (7)
cpp/src/io/json/host_tree_algorithms.cucpp/src/io/json/json_tree.cucpp/src/io/json/nested_json.hppcpp/src/io/json/nested_json_gpu.cucpp/src/io/json/process_tokens.cucpp/tests/io/json/json_test.cppcpp/tests/io/json/nested_json_test.cpp
vuule
reviewed
May 11, 2026
vuule
left a comment
vuule
left a comment
Contributor
There was a problem hiding this comment.
bunch of questions mostly
looks good in general
| ? static_cast<int32_t>(newline) | ||
| : (symbol == newline ? static_cast<int32_t>(whitespace) : static_cast<int32_t>(symbol)); | ||
| : (symbol == newline ? static_cast<int32_t>(whitespace) | ||
| : static_cast<int32_t>(static_cast<unsigned char>(symbol))); |
Contributor
There was a problem hiding this comment.
Why a chain of casts? is this clamping to unsigned char range?
Contributor
There was a problem hiding this comment.
Can we add a brief comment here explaining that we need this chain of casts?
karthikeyann
force-pushed
the
fix-json_memory_error
branch
from
May 12, 2026 18:04
f9784d5 to
c9e4a06
Compare
shrshi
reviewed
May 12, 2026
vuule
requested changes
May 13, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cpp/src/io/json/json_tree.cu (1)
320-325: ⚡ Quick winKeep the
CUDF_EXPECTSpredicate side-effect free.Pull
depth_out_of_range.value(stream)into a local first, then pass that local intoCUDF_EXPECTS. That keeps the sync/read explicit instead of hiding it inside the macro condition.♻️ Proposed fix
- CUDF_EXPECTS( - !depth_out_of_range.value(stream), + auto const is_depth_out_of_range = depth_out_of_range.value(stream); + CUDF_EXPECTS( + !is_depth_out_of_range, "JSON token nesting depth is outside the supported range for TreeDepthT [" + std::to_string(static_cast<size_type>(cuda::std::numeric_limits<TreeDepthT>::min())) + ", " + std::to_string(static_cast<size_type>(cuda::std::numeric_limits<TreeDepthT>::max())) + "]");As per coding guidelines,
CUDF_EXPECTS condition must be a pure predicate with no side effects.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/io/json/json_tree.cu` around lines 320 - 325, The CUDF_EXPECTS predicate currently calls depth_out_of_range.value(stream) which causes a hidden read; instead call depth_out_of_range.value(stream) once into a local bool (e.g., bool depth_bad = depth_out_of_range.value(stream)) just before the CUDF_EXPECTS line and then pass that local (depth_bad) into CUDF_EXPECTS so the macro predicate is side-effect free; update the surrounding message to use TreeDepthT and the same variables as before but do not perform any stream reads inside CUDF_EXPECTS.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/src/io/json/json_tree.cu`:
- Around line 132-145: The depth-out-of-range check in
checked_token_level_output is being applied to all tokens (including
non-materialized closing tokens) and must be moved so only materialized nodes
are validated: remove the narrowing check from checked_token_level_output and
perform it when writing node levels in the copy_if path (the code that uses
is_node and writes into node_levels), or filter non-node tokens before running
the scan so the scan’s output never exceeds TreeDepthT; also remove the
device-to-host side-effect from the CUDF_EXPECTS predicate by not calling
value(stream) inside the predicate—pass a pure boolean or precomputed host-side
value instead so the predicate remains side-effect-free.
---
Nitpick comments:
In `@cpp/src/io/json/json_tree.cu`:
- Around line 320-325: The CUDF_EXPECTS predicate currently calls
depth_out_of_range.value(stream) which causes a hidden read; instead call
depth_out_of_range.value(stream) once into a local bool (e.g., bool depth_bad =
depth_out_of_range.value(stream)) just before the CUDF_EXPECTS line and then
pass that local (depth_bad) into CUDF_EXPECTS so the macro predicate is
side-effect free; update the surrounding message to use TreeDepthT and the same
variables as before but do not perform any stream reads inside CUDF_EXPECTS.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 42295f5b-5598-4dda-9ad0-225e386edd0c
📒 Files selected for processing (4)
cpp/src/io/json/json_tree.cucpp/src/io/json/nested_json_gpu.cucpp/tests/io/json/json_test.cppcpp/tests/io/json/nested_json_test.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- cpp/tests/io/json/nested_json_test.cpp
- cpp/tests/io/json/json_test.cpp
shrshi
approved these changes
May 15, 2026
vuule
force-pushed
the
fix-json_memory_error
branch
from
May 15, 2026 22:17
db58b1d to
758714a
Compare
vuule
approved these changes
May 15, 2026
vuule
added
5 - Ready to Merge
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cpp/src/io/json/host_tree_algorithms.cu (1)
911-911: 💤 Low valueRedundant check: already verified at lambda entry.
The condition
d_ignore_vals[col_ids[i]]is already checked at Line 898 with an early return. If execution reaches this switch case, the condition must be false, making this check dead code.♻️ Suggested cleanup
case NC_STR: [[fallthrough]]; case NC_VAL: - if (d_ignore_vals[col_ids[i]]) break; if (d_columns_data[col_ids[i]].validity) set_bit(d_columns_data[col_ids[i]].validity, row_offsets[i]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/io/json/host_tree_algorithms.cu` at line 911, Remove the redundant check for d_ignore_vals[col_ids[i]] inside the switch case: since the lambda containing this switch already returns early when d_ignore_vals[col_ids[i]] is true (the prior guard at the lambda entry), delete the "if (d_ignore_vals[col_ids[i]]) break;" line to avoid dead code and rely on the existing early-return guard that uses d_ignore_vals, keeping the logic in the switch case focused on the active paths for col_ids[i].
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cpp/src/io/json/host_tree_algorithms.cu`:
- Line 911: Remove the redundant check for d_ignore_vals[col_ids[i]] inside the
switch case: since the lambda containing this switch already returns early when
d_ignore_vals[col_ids[i]] is true (the prior guard at the lambda entry), delete
the "if (d_ignore_vals[col_ids[i]]) break;" line to avoid dead code and rely on
the existing early-return guard that uses d_ignore_vals, keeping the logic in
the switch case focused on the active paths for col_ids[i].
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b409b1d3-0232-428f-9bde-0c3f57c24e9c
📒 Files selected for processing (7)
cpp/src/io/json/host_tree_algorithms.cucpp/src/io/json/json_tree.cucpp/src/io/json/nested_json.hppcpp/src/io/json/nested_json_gpu.cucpp/src/io/json/process_tokens.cucpp/tests/io/json/json_test.cppcpp/tests/io/json/nested_json_test.cpp
🚧 Files skipped from review as they are similar to previous changes (5)
- cpp/src/io/json/process_tokens.cu
- cpp/src/io/json/nested_json_gpu.cu
- cpp/src/io/json/nested_json.hpp
- cpp/src/io/json/json_tree.cu
- cpp/tests/io/json/nested_json_test.cpp
Contributor
Author
|
/merge |
rapids-bot
Bot
merged commit May 18, 2026
9a9c726
into
NVIDIA:release/26.06
120 of 121 checks passed
This was referenced May 19, 2026
madsbk
pushed a commit
to madsbk/cudf
that referenced
this pull request
May 19, 2026
…ing depth (NVIDIA#22452) Tightens JSON reader handling for malformed and deeply nested inputs by avoiding null validity-mask writes, fixing strict value validation, handling high-bit input bytes safely, and reporting an error when nesting exceeds the supported `TreeDepthT` range. Adds unit coverage for recovery-mode malformed records, invalid unquoted values, and nesting-depth boundaries. Authors: - Karthikeyan (https://github.com/karthikeyann) Approvers: - Shruti Shivakumar (https://github.com/shrshi) - Vukasin Milovanovic (https://github.com/vuule) URL: NVIDIA#22452
rapids-bot Bot
pushed a commit
that referenced
this pull request
May 19, 2026
Fixes a memcheck error introduced by #22452 where an atomic operation on a bool variable is reported by compute-sanitizer as an out-of-bounds access. Changing the variable to an `int32_t` resolves the error. Closes #22570 Authors: - David Wendt (https://github.com/davidwendt) Approvers: - Bradley Dice (https://github.com/bdice) - Yunsong Wang (https://github.com/PointKernel) URL: #22571
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Tightens JSON reader handling for malformed and deeply nested inputs by avoiding null validity-mask writes, fixing strict value validation, handling high-bit input bytes safely, and reporting an error when nesting exceeds the supported
TreeDepthTrange.Adds unit coverage for recovery-mode malformed records, invalid unquoted values, and nesting-depth boundaries.
Checklist