Skip to content

fix shared variant ordering bug#718

Merged
joe-clickhouse merged 2 commits intomainfrom
joe/712-parsing-json-column-with-integers-fails-if-it-was-parsed-with-rapidjson-on-server-side
Apr 20, 2026
Merged

fix shared variant ordering bug#718
joe-clickhouse merged 2 commits intomainfrom
joe/712-parsing-json-column-with-integers-fails-if-it-was-parsed-with-rapidjson-on-server-side

Conversation

@joe-clickhouse
Copy link
Copy Markdown
Contributor

Summary

ClickHouse's DataTypeVariant constructor sorts its members alphabetically by name, and the wire-format discriminator byte indexes into that sorted order. The client was appending SharedVariant to the end of the variant list without sorting, so any Dynamic/JSON path whose type name sorted after "SharedVariant" (e.g. UInt*, Tuple(...), UUID) was decoded through the wrong variant reader.

The fix is one line in read_dynamic_prefix where we sort variant_types by name after appending SharedVariant.

Closes #712.

Checklist

Delete items not relevant to your PR:

  • Unit and integration tests covering the common scenarios were added
  • A human-readable description of the changes was provided to include in CHANGELOG

…ils-if-it-was-parsed-with-rapidjson-on-server-side
@joe-clickhouse
Copy link
Copy Markdown
Contributor Author

@codex[agent] can you review this PR according to the guide in .agents/review.md

@Codex
Copy link
Copy Markdown

Codex AI commented Apr 20, 2026

@codex[agent] can you review this PR according to the guide in .agents/review.md

Findings: no issues spotted. Sorting SharedVariant into the variant list before reading discriminators matches the server’s sorted ordering, and the new JSON/Dynamic cases exercise both clients via param_client. Open: I did not rerun the suite locally; relying on the added coverage and existing CI.

@joe-clickhouse
Copy link
Copy Markdown
Contributor Author

@claude[agent] can you review this PR according to the guide in .agents/review.md

@Claude
Copy link
Copy Markdown

Claude AI commented Apr 20, 2026

@claude[agent] can you review this PR according to the guide in .agents/review.md

Review of PR #[Fix shared variant ordering bug]

I've reviewed this PR according to .agents/review.md. The fix is correct and well-tested.

Findings

No material issues found. The PR correctly addresses the root cause.

Code Analysis

Core fix (dynamic.py:224): The one-line sort is correct. I verified against the ClickHouse server source:

  • Server code in src/DataTypes/DataTypeVariant.cpp:26-47 uses std::map<String, DataTypePtr> to sort variants alphabetically by name before pushing them to the variants list
  • Server code in src/DataTypes/Serializations/SerializationDynamic.cpp:398-399 appends SharedVariant and passes to DataTypeVariant constructor, which sorts
  • Client now replicates this exact behavior

Sync/async parity: ✓ No issue. The fix is in read_dynamic_prefix, which is shared serialization logic called by both sync and async read paths through _read_column_binary.

Tests: ✓ Well-covered

  • test_json_path_sorts_after_shared_variant and test_dynamic_uint64_single_variant use param_client and call fixtures from conftest.py:221-240,134-139, which run tests in both sync and async modes
  • test_dynamic_prefix_sorts_shared_variant is a focused unit test that validates the sorting directly on wire-format bytes

CHANGELOG: ✓ Entry is clear, accurate, and follows repo style. Explains the problem and the fix at the right level of detail.

Public API: ✓ No changes to public surface. This is pure read-path correctness.

Compatibility: ✓ No compatibility concerns. This is a bug fix that restores correct behavior for types sorting after "SharedVariant" alphabetically (UInt*, Tuple, UUID, etc.). There is no backward compatibility to preserve here, the old behavior was wrong.

Minor Observations

  • The fix is extremely surgical and focused
  • Test coverage is appropriate for the bug class
  • No performance impact
  • No optional dependency or packaging changes

The PR is ready to merge.

@joe-clickhouse joe-clickhouse merged commit 904c9c7 into main Apr 20, 2026
39 checks passed
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.

Parsing JSON column with integers fails if it was parsed with rapidjson on server side

3 participants