Repository navigation
Fix JSON overlapping paths: give way in the parent walk when the deeper path holds nothing - #584
Merged
Conversation
polyglotAI-bot
requested review from
alex-clickhouse and
mzitnik
as code owners
August 24, 2026 10:43
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes an order-dependent bug in JsonType.ReadAsJsonObject when decoding overlapping JSON paths (a path declared both as a value and as a parent of another path): when the shallower path holds a value and the deeper path holds “nothing”, the deeper path now correctly gives way during the parent-walk phase, making the outcome independent of wire ordering.
Changes:
- Update the parent-walk collision logic in
JsonType.ReadAsJsonObjectto consult the incoming node and skip (give way) when it holds no value, mirroring the leaf behavior. - Factor the “holds nothing” predicate into a shared helper (
HoldsNoValue) and apply it in both parent-walk and leaf collision handling. - Add codec-level tests that feed shallow-first wire ordering bytes to pin the give-way vs collide rules; update docs and add a changelog fragment.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| ClickHouse.Driver/Types/JsonType.cs | Makes overlapping-path resolution order-independent by giving way in the parent walk when the incoming deeper path holds no value; shares the predicate via HoldsNoValue. |
| ClickHouse.Driver.Tests/Types/JsonOverlappingPathReadTests.cs | Adds synthetic-bytes tests to cover shallow-first ordering and verify both give-way and collide behaviors (including AllowDuplicateJsonKeys interactions). |
| docs/overview.mdx | Clarifies that the give-way rule is order-independent and narrows the AllowDuplicateJsonKeys “still throws” case to value-holding paths. |
| changelog.d/583-json-overlap-parent-give-way.fixes.md | Adds a fixes fragment documenting the corrected overlapping-path behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
alex-clickhouse
force-pushed
the
polyglot/json-overlap-parent-give-way
branch
from
August 24, 2026 11:24
fb754a5 to
51f25da
Compare
…hing An overlapping JSON path — one declared both as a value and as the parent of another path — gives way when it holds nothing, so the side which has the data survives. The leaf applied that rule; the parent walk decided purely from the occupant and threw whenever it held a value, even where the incoming deeper path held nothing and dropping it lost nothing. The rule is meant to hold whichever of the two paths the wire puts first. It did not: the outcome depended on an undocumented server path ordering. The parent walk now tests the incoming node, which is already decoded and in scope, and gives way instead of throwing. A path which holds a value still collides, in either order. Fixes: #583
alex-clickhouse
force-pushed
the
polyglot/json-overlap-parent-give-way
branch
from
August 24, 2026 11:34
51f25da to
e45a1ca
Compare
alex-clickhouse
approved these changes
Aug 24, 2026
1 task done
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.
Description
Fixes #583.
JsonType.ReadAsJsonObjectresolves an overlapping path — one declared both as a value and as the parent of another path — by the rule from #581: a row where both sides hold a value throws, and a side which holds nothing gives way to the side which has the data. The leaf applied that rule to the incoming node; the parent walk did not. It decided purely from the occupant, so a row where the shallower path held a value and the deeper one held nothing threwSerializationExceptionwhenever the wire sent the value first, instead of keeping it. The rule is meant to be order-independent, and this was the one place where the outcome depended on the server's path ordering. The incoming node is already decoded and in scope at that point, so the walk now consults it and gives way, exactly as the leaf does.The defect is latent on a current server: the deeper typed path is sent first, and a
Mapoccupant is always empty once a deeper path is declared. Verified on 26.7 — no insert produces the failing order. It is also not a regression: the same branch raisedInvalidCastExceptionbefore #581.Changes
ClickHouse.Driver/Types/JsonType.cs: the parent walk gives way when the incoming path holds nothing, instead of throwing. The predicate shared with the leaf is nowHoldsNoValue. The walk stops at that point, so nothing is written for the path which gave way — a part which reaches that branch has an occupant, so no container was built for it earlier in the walk.docs/overview.mdx: state that the give-way rule holds in either order, and that theAllowDuplicateJsonKeysthrow applies to a path which holds a value.changelog.d/583-json-overlap-parent-give-way.fixes.md: changelog fragment.Test
ClickHouse.Driver.Tests/Types/JsonOverlappingPathReadTests.csdecodes a row from bytes, because no insert produces the shallow-first order (an integration test cannot reach the branch). It pins:Map) or an array parent, and with the collision one and two levels up;AllowDuplicateJsonKeys: a path which holds nothing needs no licence to give way, and a subtree still has nowhere to go under a scalar the row also carries.7 of the 13 cases fail on
mainwith the overlapSerializationException; all 13 pass with the fix. Full suite green on net10.0 (10932 passed, 0 failed), no existing test changed.Note:
dotnet run scripts/changelog.cs -- --checkalready reports content underUnreleasedinCHANGELOG.mdonmain(from #581). This PR adds a fragment and leavesCHANGELOG.mduntouched; the pre-existing report is not from this branch.Pre-PR validation gate
main)