Fix client-v2: keep type parameters when rebuilding a type from a binary type encoding - #3004
Open
polyglotAI-bot wants to merge 4 commits into
Open
Fix client-v2: keep type parameters when rebuilding a type from a binary type encoding#3004polyglotAI-bot wants to merge 4 commits into
polyglotAI-bot wants to merge 4 commits into
Conversation
… binary type encoding readDynamicData() rebuilds the concrete type of a value stored in a Dynamic column as a type name that is parsed back, and parent types append the child's original type name. Four branches produced a type that did not match what the server encoded: Variant wrapped its own type name twice, Nested read only the element names and left the element type encodings in the stream, Decimal and Enum returned a bare type name so their precision/scale and their constants were dropped, and the enum constant width was taken from the number of constants instead of from the type tag. Fixes: #3003
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
8 tasks
…-encoding-parameters
TriageCategory: Summary What this impacts
Concerns
Required reviewer action
|
…-encoding-parameters
…-encoding-parameters
|
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 #3003.
BinaryStreamReader.readDynamicData()decodes the binary type encoding of the concrete type of a value stored in aDynamiccolumn by rebuilding a type name and parsing it back withClickHouseColumn.of(name, typeString); every container branch (array, tuple, map, variant, JSON, low cardinality, nullable) appends the child'sgetOriginalTypeName(). Four branches produced a name that does not represent what the server encoded, so the value was either unreadable or silently wrong:case Variant:builtVariant(...)in a local builder and then wrapped it a second time —Variant(Variant(Int64, String))parses to one nested column instead of N, so the discriminator byte indexes the wrong element list and the read runs off the end of the stream (EOFException).case Nested:read only the element names. Per the data types binary encoding specNestedis encoded like a namedTuple: every name is followed by the type encoding of that element. Those encodings were neither consumed nor kept, so the stream desynchronized and the rebuilt name (Nested(a,b)) failed to parse (Unknown data type: b).case Decimal*:usedClickHouseColumn.of(name, dataType, nullable, precision, scale), which leavesoriginalTypeNameunset — it then defaults to the bare"Decimal64". A parent appended that, so the precision and the scale read from the wire were dropped and a decimal came back unscaled (1.2500as12500).case Enum8/Enum16:had the same problem ("Enum8"carries no constants, so every value read back as<unknown>), and additionally picked the constant width from the number of constants (constants > 127) rather than from the type tag. The tag is what defines it —Enum8(0x17) encodes the constants asInt8,Enum16(0x18) asInt16— so anEnum16with fewer than 128 constants desynchronized the stream, and anEnum8with 128 or more constants was read two bytes at a time. The values were also read unsigned, so negative constants never matched.Only values inside a
Dynamiccolumn go through this decoder; top-level columns take their type from theRowBinaryWithNamesAndTypesheader, which is unaffected.Changes
client-v2/.../data_formats/internal/BinaryStreamReader.java,readDynamicData():Variantreturns the type name it built instead of wrapping it again.Nestedconsumes the type encoding of every element and renders it asNested(name Type, ...), so both the stream position and the element types are correct.Decimal*rendersDecimal(P, S)— the form the server itself renders for every decimal width — so a parent keeps the precision and the scale. The physical width is derived from the precision exactly as before (readDecimal), and theDecimaldata type with an explicit precision/scale is already what the header path produces for these columns.Enum8/Enum16render the constants (Enum8('a' = 1, 'b' = 2), names escaped withClickHouseUtils.escape) as the original type name, take the constant width from the type tag, and read the values signed. TheClickHouseEnumis still built from the decoded names and values directly, not from the round-tripped name.Test
DataTypeTests(integration), all new tests, none changed or removed. Each asserts a trailing fixed-widthnumcolumn as a desync guard:testDynamicWithParametrizedElementType— a@DataProviderover parametrized element types inside anArray(...)::Dynamic:Decimal32(2),Decimal64(4)(incl. a negative value),Decimal(10, 2)(a precision that is not the width maximum),Decimal128(6),Decimal256(20),Enum8with positive, negative and quoting-sensitive constant names ('a,b','c\'d'), andEnum16with positive and negative constants.testDynamicWithEnum8WithMoreThan127Constants— anEnum8with 130 constants, the case the old constant-count heuristic read as anEnum16.testDynamicWithParametrizedTypesInsideMapAndTuple— the same element types under aMapand a namedTupleparent, since every parent appends the child's original type name through the same path.testDynamicWithVariantElement—Array(Variant(String, Int32))::Dynamic, both discriminators exercised, elements declared in an order that differs from the order the server encodes them in.testDynamicWithNestedElement—Nested(a Int32, b String, c Decimal64(4))::Dynamicover two rows, so the element types (and theDecimalfix inside aNested) are pinned too.All 14 of these fail on
main(EOFException/Unknown data type: b/12500/<unknown>) and pass with the fix. The existingtestDynamicColumnFractionalDecimalRepresentationUnchangedandtestDynamicColumnPreservesBigDecimalValueare the contrast cases for theDecimalchange (they pin the representation and the scale of a top-level decimal in aDynamiccolumn) and still pass unchanged, as does the rest ofDataTypeTests(108),QueryTests/MetadataTests(109) and theclient-v2unit tests (529).Pre-PR validation gate
AGENTS.md/docs/changes_checklist.md— no public API, method signature, enum constant, config property or logging change; behavior change is confined to reading aDynamiccolumn that previously failed or returned a wrong valueNotes
Tuplebranch does onmain; back-quoting them is [client-v2] JSON typed paths containing a space or comma break when the JSON value is nested in a Dynamic column #3001 / Fix client-v2: quote identifiers when rebuilding a type name from a binary type encoding #3002, and theNestedbranch will pick that up once it merges.readDynamicData()has further, separate gaps found while working on this (noSimpleAggregateFunctionbranch, theTime/Time64-with-timezone tags are unregistered, unquoted timezones, zero-element containers, andVariantelement ordering when an element isLowCardinality). They are deliberately out of this PR — happy to file them separately if useful.