GH-47417: [C++] Fix JSON parser losing nulls in a null-typed list - #51108
Open
advitrocks9 wants to merge 1 commit into
Open
GH-47417: [C++] Fix JSON parser losing nulls in a null-typed list#51108advitrocks9 wants to merge 1 commit into
advitrocks9 wants to merge 1 commit into
Conversation
|
|
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a long-standing JSON parsing bug in the C++ BlockParser where null elements could be silently dropped (or trigger DCHECKs in debug) when appending nulls to a null-typed list, by ensuring the inline null-count state in BuilderPtr is updated in-place.
Changes:
- Change
RawBuilderSet::AppendNullto take aBuilderPtr*so increments to the inline null-count forKind::kNullpersist to the caller. - Update all
AppendNullcall sites to pass builder pointers accordingly (including nested object-field null propagation). - Add a regression test covering nulls inside lists (null-only and promoted-from-null cases).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| cpp/src/arrow/json/parser.cc | Fixes null appends by mutating the BuilderPtr in-place, preventing lost null counts in null-typed list builders. |
| cpp/src/arrow/json/parser_test.cc | Adds a regression test to ensure nulls inside lists are preserved and don’t break list offsets. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+270
to
+276
| TEST(BlockParser, NullsInList) { | ||
| auto options = ParseOptions::Defaults(); | ||
| options.unexpected_field_behavior = UnexpectedFieldBehavior::InferType; | ||
| AssertParseColumns(options, R"({"a": [null, null], "b": [null, "hi", null]})", | ||
| {field("a", list(null())), field("b", list(utf8()))}, | ||
| {"[[null, null]]", R"([[null, "hi", null]])"}); | ||
| } |
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
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.
Rationale for this change
{"a": [null, 1]}parses to[[1]]. Nothing raises, andvalidate(full=True)is the only thingthat notices:
Debug builds abort on a DCHECK instead, at
parser.cc:540if the second element is null andparser.cc:1151if it is typed.What changes are included in this PR?
RawBuilderSet::AppendNulltakes the child builder by value. Builders ofKind::kNullhave noarena, their null count is packed into
BuilderPtr::index, so the increment updates the parentbut is lost to the caller. The next null then re-increments the same stale value, which leaves an
all-null list as
NullArray(1)and a promoted list with its leading nulls missing.It now takes a
BuilderPtr*, matchingMakeBuilderin the same class. The regression came inwith
b7054c21aaband has shipped since 0.14.0.Are these changes tested?
BlockParser.NullsInListcovers both arms.arrow-json-testpasses 261/261 in Debug andRelease. Reverting
parser.ccwith the test in place aborts in Debug and fails both columns inRelease.
Are there any user-facing changes?
Those inputs parse correctly now. This is not inference-only: an explicit
list(null())schemareaches the same arm under both
ErrorandIgnore.This PR contains a "Critical Fix". Well-formed JSON silently loses list elements in release
builds and aborts on a DCHECK in debug builds.