Skip to content

Fix JSON: a path holding a composite value, and Dynamic(max_types=N) - #628

Open
polyglotAI-bot wants to merge 2 commits into
mainfrom
polyglot/json-composite-path-values
Open

polyglotAI-bot wants to merge 2 commits into
mainfrom
polyglot/json-composite-path-values

Conversation

@polyglotAI-bot

Copy link
Copy Markdown
Collaborator

Description

Fixes #626.

JsonType.ReadJsonNode decided how to materialize a path from its ClickHouse type, but only
Array, Map and FixedString had an arm. Every other type was read as one value and handed to
ReadJsonValue, whose default arm calls JsonValue.Create(JsonSerializer.SerializeToElement(...))
— and JsonValue.Create throws on an element which is an object or an array. So a path holding a
Tuple threw InvalidOperationException: The element cannot be an object or array and the whole
row became unreadable, with no way for the caller to get the value. The reported shape reaches this
through a heterogeneous JSON array: '{"a": ["template", "x", ["macro", 20], "y"]}'::JSON types
path a as Array(Dynamic), whose third element is a Dynamic holding Tuple(String, Int64).

The dispatch is now complete for every type through which a composite value can arrive:

  • a Tuple is read element by element, and rendered as the server renders it — a named tuple as an
    object, an unnamed one as an array;
  • Nested is a repeated tuple (a length, then that many tuples), so it renders as an array of
    objects. It derives from TupleType, so it is matched before the tuple arm — reading it as a
    single tuple would consume the wrong bytes and desync the reader;
  • Dynamic and Variant carry the type of the value itself, so the value's own type is read and
    dispatched again, instead of reading the value whole and losing its structure;
  • SimpleAggregateFunction reads exactly as the type it wraps, so the wrapper is resolved before
    the value is read.

As a defence in depth the default arm now uses JsonSerializer.SerializeToNode, which renders a
composite instead of throwing.

The second defect in the issue is separate: Dynamic(max_types = N) failed in the column-header
parser before a single value was read (ArgumentException: Unknown type: Dynamic(max_types=0)).
max_types bounds only the set of types the server tracks; it does not change the wire layout,
which is self-describing per value. DynamicType is now a ParameterizedType that accepts the
argument, keeps it in the type name and otherwise ignores it — the same thing
DynamicColumnCodec.TryParseMaxTypes already does on the TCP side. Any other argument is rejected
with a message naming it.

Changes

  • Types/JsonType.cs: Nested, Tuple, SimpleAggregateFunction, Dynamic and Variant arms in
    ReadJsonNode, with ReadJsonNested / ReadJsonTuple / ReadJsonVariant; the default arm of
    ReadJsonValue renders composites instead of throwing.
  • Types/TupleType.cs, Types/NestedType.cs: an ElementNames property, read off the name Type
    children. A multi-word type alias (BIGINT UNSIGNED) also holds a space, so an alias is ruled out
    before the name is taken; an element without a name makes the whole tuple unnamed.
  • Types/BinaryTypeDecoder.cs: the named-tuple and Nested decoders keep the field names they
    already read instead of discarding them.
  • Types/DynamicType.cs, Types/TypeConverter.cs: Dynamic is registered as a parameterized type
    and parses max_types=N; TypeConverter.IsTypeAlias exposes the alias check, and
    RegisteredTypes is de-duplicated now that Dynamic is in both registries.
  • Types/VariantType.cs: the 0xFF null discriminator is a named constant, now that the JSON read
    path needs it too.
  • changelog.d/626-json-composite-values.fixes.md.

Test

  • Tests/Types/JsonCompositeValueReadTests.cs (new, 14 cases): each case selects the value and
    the server's own toJSONString of the same expression, and asserts both against the expected
    document, so the driver cannot drift from the server. It covers the reported shape (with and
    without max_dynamic_types = 0), a hinted named and unnamed tuple, a tuple inside Array and
    Map, Nested, SimpleAggregateFunction(anyLast, Tuple(...)), a Dynamic path holding a tuple,
    and a Variant path holding an array and a tuple. Three contrast cases pin the modes the fix does
    not cover — a scalar string under Dynamic, a null Dynamic and a null Variant — and pass
    unchanged before and after.
  • Tests/Types/DynamicTests.cs: Dynamic, Dynamic(max_types=0) and Dynamic(max_types=3) parse
    and keep their declared name; an unsupported argument throws; and the value reads back over a live
    server for each spelling.
  • Tests/Types/BinaryTypeDecoderTests.cs: a named-tuple header keeps its element names, an unnamed
    one has none.
  • Eleven of the new JSON cases and five of the new Dynamic cases fail on main; the whole
    ClickHouse.Driver.Tests suite passes on this branch (11023 passed / 142 skipped, net10.0,
    ClickHouse 26.8).

Existing tests changed, and why

JsonStringAsByteArrayTests.ReadJson_WithNonTextByteArrayPath_IsNotDecodedAsText asserted that
Array(UInt8) under a Variant or SimpleAggregateFunction path renders as base64 ("AQI=").
That was the output of reading the array whole as a byte[]; the server renders [1,2] for the
same value, as does this driver for the same array without the wrapper. The four cases now assert
the array, through ToJsonString() so the exact JSON is pinned rather than a string value. A
Dynamic-hinted string was added to StringBearingJsonShapes: under ReadStringsAsByteArrays it
used to come back base64, because a Dynamic never reached the text-decoding arm.

Related

Reading a Dynamic/Variant-hinted path that holds an array, and a string under a Dynamic hint
coming back base64 — #530 — are the same dispatch
defect and are fixed by the same change.

Pre-PR validation gate

  • Deterministic repro confirmed (fails on main, passes here)
  • Root cause documented above
  • Fix targets the root cause
  • Test fails without fix, passes with fix
  • No existing test weakened; the two edits above are documented
  • Convention compliance verified per AGENTS.md (integration tests against a real server,
    TestCaseSource parametrization, CreateTableName not needed — no test creates a table,
    changelog fragment instead of CHANGELOG.md, no public API change)

…s=N)

JsonType.ReadJsonNode dispatched only on Array, Map and FixedString, so a
Tuple, Nested, Dynamic, Variant or SimpleAggregateFunction path was read as
one value and handed to JsonValue.Create, which throws on an object or an
array element: the whole row was unreadable. Each of those types is now
dispatched on what it actually holds — a tuple element by element, a Nested
as a repeated tuple, and a Dynamic or Variant on the type its value carries —
so the document matches the one the server renders.

Dynamic also accepts a max_types argument now. It bounds only the set of
types the server tracks, not the wire layout, so it is kept in the type name
and otherwise ignored, as the TCP codec already does.

Fixes: #626
Copilot AI lite review requested due to automatic review settings September 25, 2026 15:28
@codecov

codecov Bot commented Sep 25, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.64706% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
ClickHouse.Driver/Types/DynamicType.cs 95.00% 0 Missing and 1 partial ⚠️
ClickHouse.Driver/Types/JsonType.cs 97.50% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Feature gating gaps and nullable composite JSON rendering remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 Medium severity

Open (1)
What changed in this PR

Fixes JSON deserialization for composite values and adds support for Dynamic(max_types=N).

Changes:

  • Adds recursive JSON handling for tuples, nested values, dynamics, variants, and aggregate wrappers.
  • Preserves tuple and nested field names.
  • Adds parsing, regression tests, and changelog documentation.
File Description
ClickHouse.Driver/​Types/​VariantType.cs Names the null discriminator
ClickHouse.Driver/​Types/​TypeConverter.cs Registers parameterized Dynamic
ClickHouse.Driver/​Types/​TupleType.cs Tracks tuple element names
ClickHouse.Driver/​Types/​NestedType.cs Tracks nested element names
ClickHouse.Driver/​Types/​JsonType.cs Implements composite JSON decoding
ClickHouse.Driver/​Types/​DynamicType.cs Parses max_types
ClickHouse.Driver/​Types/​BinaryTypeDecoder.cs Preserves decoded names
ClickHouse.Driver.Tests/​Types/​JsonStringAsByteArrayTests.cs Updates wrapper rendering tests
ClickHouse.Driver.Tests/​Types/​JsonCompositeValueReadTests.cs Adds composite JSON coverage
ClickHouse.Driver.Tests/​Types/​DynamicTests.cs Tests Dynamic(max_types=N)
ClickHouse.Driver.Tests/​Types/​BinaryTypeDecoderTests.cs Tests decoded tuple names
changelog.d/​626-json-composite-values.fixes.md Documents the fixes

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread ClickHouse.Driver/Types/JsonType.cs
ReadJsonNode dispatched Tuple, Nested, Dynamic, Variant and
SimpleAggregateFunction, but a Nullable path still read its value whole, so a
Nullable(Tuple(...)) was serialized from the CLR System.Tuple and a named tuple
came out with Item1/Item2 properties instead of the server's object. Nullable
only decides whether a value is present: consume its marker and dispatch again
on the type it wraps.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

JSON: a path holding a Tuple (heterogeneous JSON array) throws "The element cannot be an object or array"; Dynamic(max_types=N) is an unknown type

2 participants