Decode compound values stored in JSON shared data - #898
Conversation
Shared data stores values as <binary type encoding><serializeBinary value>. The type encoding is recursive and variable-length, so a single-byte lookup cannot resolve it, and serializeBinary differs from the native column format for compound types. Adds a recursive decoder for the single-value format and routes the non-scalar branch of _decode_variant through it.
|
nivash b seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
There was a problem hiding this comment.
Pull request overview
This PR fixes JSON shared-data decoding so compound values (Array, nested objects, maps, etc) come back as decoded Python objects instead of raw bytes, by adding a recursive decoder for ClickHouse’s single-value <encoded type><serializeBinary value> format and wiring it into variant decoding.
Changes:
- Added a recursive binary decoder (
read_encoded_type+read_binary_value) for the shared-data single-valueserializeBinaryencoding. - Routed the non-scalar/unknown-discriminator branch of
_decode_variantthrough the new decoder with a safe fallback to raw bytes. - Added integration tests covering compound JSON values forced into shared data via
max_dynamic_paths=0.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
clickhouse_connect/datatypes/binary_value.py |
New recursive decoder for <encoded type><value> in serializeBinary single-value format (arrays, tuples, nullable, map, dynamic, nested JSON). |
clickhouse_connect/datatypes/dynamic.py |
Uses the new decoder for previously “unknown discriminator” variant payloads (compound/shared-data cases). |
tests/integration_tests/test_dynamic.py |
Adds regression coverage for compound JSON values stored in shared data. |
Comments suppressed due to low confidence (1)
tests/integration_tests/test_dynamic.py:708
- Same as the previous test: using
test_clienthere makes this regression test sync-only, so it will not catch async decoding regressions. Please useparam_clientwith thecallfixture to exercise both sync and async clients, consistent with the rest of this module.
def test_json_shared_data_map(test_client: Client, table_context: Callable):
type_available(test_client, "json")
if not test_client.min_version("24.10"):
pytest.skip("JSON shared data decoding requires 24.10+")
| def test_json_shared_data_compound_values(test_client: Client, table_context: Callable): | ||
| """Compound values stored in JSON shared data must decode to Python objects. | ||
|
|
||
| max_dynamic_paths=0 forces every path into shared data on insert, so this | ||
| does not depend on merge history. | ||
| """ | ||
| type_available(test_client, "json") | ||
| if not test_client.min_version("24.10"): | ||
| pytest.skip("JSON shared data decoding requires 24.10+") | ||
|
|
||
| with table_context("json_shared_compound", ["id Int32", "data JSON(max_dynamic_paths=0)"]): | ||
| test_client.command( | ||
| """INSERT INTO json_shared_compound VALUES | ||
| (1, '{"s": "hello", "arr": [{"k": "v"}]}'), | ||
| (2, '{"obj": {"a": "1"}}'), | ||
| (3, '{"nums": [1, 2, 3]}'), | ||
| (4, '{"nested": [[1, 2], [3]]}'), | ||
| (5, '{"nullable": [1, null, 3]}')""" | ||
| ) | ||
| rows = {r[0]: r[1] for r in test_client.query("SELECT id, data FROM json_shared_compound ORDER BY id").result_rows} |
There was a problem hiding this comment.
Done in 71fe765 — both tests now use param_client with the call fixture, matching the rest of the module, so the async decoding path is covered too.
| type_name = STANDARD_DISCRIMINATOR_TYPES.get(discriminator) | ||
| if type_name is None: | ||
| return binary_data | ||
| try: | ||
| return decode_binary_value(binary_data, ctx) | ||
| except Exception as e: # noqa: BLE001 never raise out of a decode | ||
| logger.debug("Compound variant decode failed: %s", e) | ||
| return binary_data |
There was a problem hiding this comment.
Correct, and thank you — that's a scope leak between the description and the code. decode_shared_variant_value routes discriminators < 0x20 into _decode_variant, so 0x1E was reaching the new decoder and test_dynamic_shared_variant_unsupported_types broke.
Fixed in 8b8ebcf: _decode_variant now takes decode_compound, defaulting to False; only decode_shared_data_value passes True. The SharedVariant path keeps returning raw bytes for non-scalar discriminators, and the existing test now passes unmodified, which pins that boundary as intentional rather than incidental.
Extending SharedVariant is worth doing, but it needs its own reproduction and tests — I'd rather send it as a separate PR than widen this one.
|
Hey @iamnivash10 I've got a new native decoder in the works that may fix this exact issue. I'm not in front of my machine right now so I can't check but when I'm able I'll post a note back here. Thanks! |
|
Thanks @joe-clickhouse , that's good to hear — happy to have this closed by the better fix if the new decoder covers it. One thing that might be worth checking against your work: shared-data values aren't in the native column format. They're stored as , and the two formats diverge for compound types — serializeBinary writes a var_uint element count with self-contained elements, while the native column format writes UInt64 offsets with bulk elements. They're byte-identical for scalars and String, which is why the current scalar-only path works. So a native-format decoder may still return raw bytes for compound values in shared data unless the single-value format is handled separately. Either way, the integration tests here use max_dynamic_paths=0 to force shared data deterministically, so they'd work as regression coverage for your decoder too if that's useful — happy to strip this down to just the tests if that's the more useful contribution. No rush at all, and thanks for taking a look. |
|
@iamnivash10 thanks for the work! I took a look and my yet-to-be-released work does NOT cover this. So this is definitely welcomed. I'm going to be mostly out of the office next week but I'll review as I'm able! Will keeep you posted. Thanks again. |
Shared data stores values as . The type encoding is recursive and variable-length, so a single-byte lookup cannot resolve it, and serializeBinary differs from the native column format for compound types. Adds a recursive decoder for the single-value format and routes the non-scalar branch of _decode_variant through it.
Summary
Fixes #897 .
Compound JSON values (arrays, nested objects, maps) stored in shared data
are returned to callers as raw
bytesinstead of decoded Python objects.Scalars in the same shared data decode correctly, and the same compound value
decodes correctly when the server stores the path as a dynamic subcolumn — so
the Python type a caller sees depends on an internal server storage decision
they cannot observe or control.
Root cause
Shared data stores each value as
<binary type encoding><serializeBinary value>.The current decoder resolves the type with a single-byte lookup in
STANDARD_DISCRIMINATOR_TYPESand returns the bytes untouched on a miss. Twoproperties of the format defeat that:
1. The type encoding is recursive and variable-length.
0x1Edoes not mean"Array" — it means "an array of whatever type encoding follows", and that
encoding can itself be compound. The bytes from the linked issue decode as:
1e 30 00 00 10 00 00 00 | 01 01 01 6b 15 01 76
└──────── type ────────┘ └────── value ──────┘
1e Array
30 JSON (the array's element type)
00 JSON serialization version
00 max_dynamic_paths
10 max_dynamic_types (16)
00 00 00 leb128 counts: typed paths, SKIP, SKIP REGEXP
01 element count (varuint)
01 path count
01 6b path "k"
15 String (dynamic path, so the value is self-describing)
01 76 "v"
matching the server's own report of the type:
arr: Array(JSON(max_dynamic_types=16, max_dynamic_paths=0)).The type occupies eight bytes, not one; resolving it needs a recursive descent.
2.
serializeBinarydiffers from the native column format for compoundtypes. The single-value format prefixes element counts with a varuint and
keeps elements self-contained; the native column format uses UInt64 cumulative
offsets with bulk-serialized elements. For scalars,
StringandBoolthe twoare byte-identical — which is exactly why the existing scalar path is correct in
borrowing the native column reader. For
Arrayand every other compound typethey diverge at the first byte.
So the missing piece isn't more entries in the discriminator table; it's an
implementation of the single-value format.
Changes
clickhouse_connect/datatypes/binary_value.py: a recursive decoder(
read_encoded_type+read_binary_value) for theserializeBinarysingle-value format, covering Array, Nullable, LowCardinality, Tuple,
NamedTuple, Map, Dynamic and nested JSON.
_decode_variantroutes through it.input and raises otherwise, so a desynced parser fails loudly instead of
returning plausible-but-wrong data. On any failure the caller falls back to
raw bytes — today's behaviour — which makes the broad
exceptsafe ratherthan sloppy.
_decode_variantpreviously returnedNonewhen a known scalardiscriminator failed length validation, silently turning an undecodable value
into a null; it now returns the raw bytes. Pre-existing, unrelated to compound
types — happy to split it out.
Scope / limitations
SharedVariantpath(
decode_shared_variant_value) can also carry compound types withdiscriminators ≥ 0x20 (NamedTuple
0x20, Map0x27, Dynamic0x2B, JSON0x30), which still fall through to the string heuristics. I have noreproduction for that path so I left it out rather than add untested code.
typed paths are not self-describing while dynamic and shared paths are, which
matches the format, but I couldn't construct a shared-data value whose type
carries explicit path hints — a typed path in the outer column becomes its own
subcolumn and never reaches this decoder. Pointers welcome.
type encoding, so new type indices or a bumped serialization version need
matching updates here. Inherent to supporting JSON rather than introduced by
this PR, but a real maintenance obligation worth stating.
-JSON shared data only, enforced in code. Compound decoding is gated behind _decode_variant(..., decode_compound=True), which only decode_shared_data_value passes. The Dynamic SharedVariant path can also carry compound types with discriminators ≥ 0x20 (NamedTuple 0x20, Map 0x27, Dynamic 0x2B, JSON 0x30), and continues to return raw bytes for them. I have no reproduction for that path, so extending it belongs in a separate PR.
Tests
Added to
tests/integration_tests/test_dynamic.py, usingmax_dynamic_paths=0to force shared data deterministically. Verified against ClickHouse 25.6.13.41;
each case fails on unpatched code and passes with the fix.
{"s": "hello", "arr": [{"k": "v"}]}'hello',[{'k': 'v'}]{"obj": {"a": "1"}}{'a': '1'}{"nums": [1, 2, 3]}[1, 2, 3]{"nested": [[1, 2], [3]]}[[1, 2], [3]]{"nullable": [1, null, 3]}[1, None, 3]{"m": {"x": "1", "y": "2"}}{'x': '1', 'y': '2'}The scalar case confirms no regression on the existing path.
CHANGELOG entry
Checklist
This is my first open-source contribution — feedback of any kind is very welcome.