Fix client-v2: quote identifiers when rebuilding a type name from a binary type encoding - #3002
Open
polyglotAI-bot wants to merge 4 commits into
Open
Fix client-v2: quote identifiers when rebuilding a type name from a binary type encoding#3002polyglotAI-bot wants to merge 4 commits into
polyglotAI-bot wants to merge 4 commits into
Conversation
…inary type encoding readDynamicData() reconstructs a ClickHouse type name as a string from the binary type encoding of a Dynamic column and re-parses it with ClickHouseColumn.of(). JSON typed paths, JSON skip paths and named-Tuple element names were appended unquoted, so a name containing a space, a comma or a bracket produced a malformed type name and the whole query failed with "Unknown data type". Such names are now back-quoted with inner back-quotes escaped (names that need no quoting are unchanged), and JSON skip paths and path regexps are emitted with their SKIP / SKIP REGEXP markers. Fixes: #3001
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
This was referenced Aug 3, 2026
TriageCategory: Summary What this impacts
Concerns
Required reviewer action
|
|
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
1 participant
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 #3001.
BinaryStreamReader.readDynamicData()decodes the binary type encoding of aDynamiccolumn byreconstructing a ClickHouse type name as a string and re-parsing it with
ClickHouseColumn.of(...).Identifiers read off the wire were appended to that string verbatim, so a name that cannot be read back
as a bare identifier — one containing a space, a comma or a bracket — split the type name at the wrong
place and the whole query failed while decoding the column type, e.g.
SELECT '{"a b": 1}'::JSON(a bInt64)::DynamicthrewIllegalArgumentException: Unknown data type: b Int64. Such a name is now back-quoted with innerback-quotes and backslashes escaped, exactly as the server renders it, which
ClickHouseUtils.readNameOrQuotedStringalready reads back correctly; names that need no quoting are rendered as before.
Top-level
JSONcolumns andJSONnested inMap/Tuple/Arraywere never affected — there the typecomes from the
RowBinaryWithNamesAndTypesheader, which the server already quotes.Two related notes on the same defect in the same decoder:
Tuplein aDynamiccolumn takes the samecode path, and
SELECT (1,2)::Tuple(x yInt32, b Int32)::Dynamicfailed identically(
Unknown data type: y Int32). It is fixed by the same helper and covered by a test.SKIP/SKIP REGEXPmarkers. That only appeared to work because a bare trailing path is silently ignored by
ClickHouseColumn.parseJSONColumn; with more than one skip path, or a skip path followed by anythingelse, it mis-parsed. They are now emitted in the server's form, which
parseJSONColumnhandles throughits existing
SKIPbranch.Scope: only the binary type-encoding reader is touched. Other reconstruction defects noticed in
readDynamicData()while working on this (theVariantbranch wrapping its type name twice,Decimal/Enumlosing precision/constants when nested one level deeper, and
Nestedreading element names without elementtypes) are not part of this PR — they are separate bugs and will be reported and fixed separately.
Changes
client-v2/.../data_formats/internal/BinaryStreamReader.javaappendIdentifier/identifierRequiresQuoting: append a wire-read identifier to atype name, back-quoting it (with
ClickHouseUtils.escape(name, '')`) only when it cannot be read back asa bare identifier. A dot stays bare, since JSON paths are dot-separated.
readDynamicData(): use it for JSON typed paths, JSON skip paths and named-Tupleelement names.readDynamicData(): emitSKIP <path>andSKIP REGEXP '<regexp>'(regexp escaped as a stringliteral) for the JSON skip paths and path regexps, reusing
ClickHouseColumn.JSON_SKIP_MARKER.CHANGELOG.md: bug-fix entry under0.11.0-rc1.Test
client-v2integration tests inDataTypeTests:testDynamicWithJSON@DataProvider, one per distinct mechanism: a typedpath with a space, a typed path with a comma, a dotted path whose components contain spaces
(
JSON(`a b`.`c d` Int64)), a path containing a back-quote (exercises the escaping), a quotedSKIPpath, a
SKIP REGEXPwhose pattern contains a space, and a type combining a typed path with a skip path(exercises more than one section in one type name).
testDynamicWithNestedTypesWithQuotedNames: a namedTuplein aDynamiccolumn with element namescontaining a space and a comma, with a trailing
Int32column asserted so a byte shift cannot pass silently.All six new cases fail on
mainwith the exception from the issue (Unknown data type: b Int64,Unknown data type: c,Unknown data type: y Int32) and pass with the fix. The five pre-existing rows of thatprovider — plain paths,
JSON(SKIP a.b),JSON(SKIP REGEXP 'a\.b')— act as contrast cases and keep passingunchanged, which is what pins the "no quoting when none is needed" behavior. No existing test was modified.
Verified against ClickHouse
26.5.1.882:mvn -pl client-v2 test→ 529 tests, 0 failuresmvn -pl client-v2 -Dit.test=DataTypeTests verify→ 102 tests, 0 failuresPre-PR validation gate
SELECT '{"a b": 1}'::JSON(a bInt64)::Dynamic)toTypeNamerendersthese names back-quoted) and from the equivalent non-
Dynamicqueryclient.queryAll(...), i.e. the actualBinaryStreamReaderread path, perAGENTS.md("Fix read-path bugs where the value is actually read")CHANGELOG.mdupdateddocs/features.mdchange (no feature added, removed, or intentionally changed)changes_checklist.md: no logging, enum constant, config property, or default value change. The two newmethods are
private static(smallest possible visibility), do not duplicate an existing helper, andcarry no nullability ambiguity — they are only reachable from
readDynamicData().