Repository navigation
[client-v2] Fix reading Array(Nullable(T)) when values are read as another class - #3293
payallenka wants to merge 3 commits into
Conversation
Reading Array(Nullable(T)) failed with "array element type mismatch" when readValue returns a different class than the one declared for T: UInt64 (BigInteger, array was UnsignedLong[]), DateTime and DateTime64 (ZonedDateTime, array was LocalDateTime[]) and IPv6 holding IPv4-mapped addresses (Inet4Address). Allocate the array with the common class of the values read, and use the declared type only for an array of NULLs. Correct that fallback for UInt64, DateTime and Tuple. Fixes ClickHouse#3283 Fixes ClickHouse#3287
9935557 to
2bdba37
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 2bdba37. Configure here.
|
|
Typing nullable arrays by the values read made Array(Nullable(Point)) fail: a Point is read as double[], and ArrayValue allocates Object[][] for an array class, which cannot hold a double[]. Use an Object array for primitive array values, as before.
Already Agreed |
|
Hi @payallenka , it looks like you haven't signed the CLA yet. Clicking on recheck doesn't change the status. Please try again. Once you've signed, it would look like this ClickHouse/clickhouse-go#2112 (comment) |
| Object[] values = new Object[len]; | ||
| Class<?> itemClass = null; | ||
| for (int i = 0; i < len; i++) { | ||
| Object value = readValue(itemTypeColumn); |
There was a problem hiding this comment.
readValue(itemTypeColumn) applies the top-level binary_string_support flag, so with the flag on, each Nullable(String) / Nullable(FixedString) element is a StringValue. The non-nullable branch uses readNestedValue, and the documented rule is that strings nested in containers stay String (constructor Javadoc, testJsonStringPathsStayStringWithBinaryStringSupport).
On main this combination throws (StringValue does not fit in String[]). With this change it returns List<StringValue> from getList for Array(Nullable(String)), while Array(String) returns List<String>. After a release, callers can depend on this, and a later fix becomes a behavior change.
readNestedValue still reads the null marker, so this one-line change keeps the branch correct:
| Object value = readValue(itemTypeColumn); | |
| Object value = readNestedValue(itemTypeColumn); |
Please also add a test that reads Array(Nullable(String)) with binary_string_support set and expects String elements.
A reply here is read on the next review: an explanation that holds up drops the finding.
| Object value = readValue(itemTypeColumn); | ||
| values[i] = value; | ||
| if (value != null) { | ||
| itemClass = itemClass == null ? value.getClass() : commonSuperclass(itemClass, value.getClass()); |
There was a problem hiding this comment.
💡 Nit: Array component type changes and can differ per row
value.getClass() gives a component type that can differ from main and from row to row:
Array(Nullable(IPv6)):Inet4Address[],Inet6Address[]orInetAddress[], depending on the values in the row.- Types that already work:
Ring/LineString/MultiPointgo fromObject[]toObject[][], andJSONfromObject[]toHashMap[]. An empty JSON value isCollections.emptyMap(), so this type can also differ per row.
This is visible through the public ArrayValue.getArray() and readValue(column, X[].class). getList, getObjectArray and JDBC getArray are not affected.
Suggestion: keep the resolveArrayItemClass corrections, start from resolveArrayItemClass(itemTypeColumn), and widen only when a value is not an instance of that class. Then every input that works on main keeps its component type, and only inputs that fail on main change. commonSuperclass must then accept an interface as the start class (its getSuperclass() is null), for example by returning Object.
A reply here is read on the next review: an explanation that holds up drops the finding.
| Class<?> itemClass = resolveArrayItemClass(itemTypeColumn); | ||
| // Type the array by the values actually read: the declared type does not always tell | ||
| // which class readValue returns (UInt64 -> BigInteger, DateTime64 -> ZonedDateTime). | ||
| Object[] values = new Object[len]; |
There was a problem hiding this comment.
💡 Nit: Extra Object[] buffer and copy for each nullable array
Each Array(Nullable(T)) value now allocates a temporary Object[len] in addition to the typed array, and copies it. docs/ai-review.md lists row decoding as performance-sensitive, and a large array doubles its transient memory.
With the approach in the comment on line 812, the reader can allocate the declared type, set each value directly, and copy once only when a value does not fit. To keep that copy rare, correct the remaining resolveArrayItemClass entries that disagree with readValue (for example Interval*, declared Long but read as Period / Duration).
A reply here is read on the next review: an explanation that holds up drops the finding.
| InetAddress.getByName("2001:db8::1"))); | ||
|
|
||
| List<Object> allNullList = reader.getList("all_null_uint64_arr"); | ||
| Assert.assertEquals(allNullList, Arrays.asList(null, null)); |
There was a problem hiding this comment.
💡 Nit: All-NULL assertion also passes on main
On main, an all-NULL Array(Nullable(UInt64)) is an UnsignedLong[] that holds nulls, so getList also returns [null, null] there. No test fails without the new UInt64 / DateTime* / Tuple entries in resolveArrayItemClass. Assert the component type, for example ((BinaryStreamReader.ArrayValue) reader.readValue("all_null_uint64_arr")).getArray().getClass(), or remove the entries.
Also:
Array(Nullable(DateTime))(32-bit) is in the changelog but has no test.AGENTS.mdprefers an integration test forBinaryStreamReaderread paths. Reading Array(Nullable(DateTime64)) throws 'array element type mismatch' #3287 was reported through JDBC, so one integration test with its query would lock in the fix.
A reply here is read on the next review: an explanation that holds up drops the finding.
FindingsHigh
Medium
Low
The fix for the earlier Summary
Verdict
Review by Polyglot AI (Claude Opus 5.5) of |
polyglotAI-bot
left a comment
polyglotAI-bot
left a comment
There was a problem hiding this comment.
Changes requested in the review of f3ac350: see the summary comment and the inline threads.
Summary
Reading an
Array(Nullable(T))column fails withIllegalArgumentException: array element type mismatchwhenreadValuereturns a different class than the oneresolveArrayItemClasspredicts forT. The reported cases:readValuereturnsUInt64(#3283)UnsignedLong[]BigIntegerDateTime,DateTime64(#3287)LocalDateTime[]ZonedDateTimeIPv6holding an IPv4-mapped addressInet6Address[]Inet4AddressOther types disagree with the table too (
Tuple→Object[],Interval*→Duration/Period,Stringwith binary strings →StringValue). So instead of correcting each entry, the nullable branch ofreadArrayItemnow reads the elements first and allocates the array with the most specific common class of the non-null values.resolveArrayItemClassis used only when every element isNULL. There I correctedUInt64(BigInteger),DateTime/DateTime32/DateTime64(ZonedDateTime) andTuple(Object[]), so an all-NULLarray gets the same component type as one with values. For element types that already worked, the result is unchanged.Closes #3283
Closes #3287
Checklist
ClickHouseBinaryFormatReaderNullableArrayTest#testNullableArraysOfTypesReadAsOtherClassescoversArray(Nullable(UInt64))(including the max value),Array(Nullable(DateTime64(3, 'UTC'))),Array(Nullable(IPv6))with an IPv4-mapped and an IPv6 address, and an all-NULLArray(Nullable(UInt64)). It fails onmainwith the error from [client-v2] Reading Array(Nullable(UInt64)) fails: "Failed to set value ... of class java.math.BigInteger when array type is class [Lcom.clickhouse.data.value.UnsignedLong;" #3283.latest, the table and query from Reading Array(Nullable(DateTime64)) throws 'array element type mismatch' #3287 throw the reported error onmainand return both rows with this change.mvn -pl client-v2,jdbc-v2 installpasses: client-v2 825 + 1201, jdbc-v2 1975 + 799 (21 skipped). Note:DataTypeConverterTest.testDateToStringfails onmaintoo when the JVM timezone is east of UTC (I'm in IST), so I ran the suite with-Duser.timezone=UTC.history/latest/3283.md)Note
Medium Risk
Changes core binary deserialization for all nullable array columns; behavior is broader (runtime typing) but scoped to client-v2 read path with new regression tests.
Overview
Fixes
Array(Nullable(T))deserialization inBinaryStreamReaderwhenreadValuereturns a different Java type thanresolveArrayItemClassassumed (e.g. UInt64 →BigInteger, DateTime64 →ZonedDateTime, IPv6 with IPv4-mapped addresses →Inet4Address/Inet6Address).For nullable elements, the reader buffers values first, then picks the component type from the common superclass of non-null elements, falling back to
resolveArrayItemClasswhen every element is NULL. Primitive array-valued types (e.g. Point asdouble[]) use anObject-typed array soArrayValuedoes not mis-allocate.resolveArrayItemClassis updated for all-null nullable UInt64, DateTime variants, and Tuple.Adds nullable-array tests for those cases plus Point, Ring, and Tuple, and a changelog entry for issues #3283 and #3287.
Reviewed by Cursor Bugbot for commit 4bacdc3. Bugbot is set up for automated code reviews on this repo. Configure here.
Update (Payal Lenka, 2026-11-06): the first commit broke
Array(Nullable(Point)), as Cursor Bugbot reported: aPointis read asdouble[], whichArrayValuecannot store in theObject[][]it allocates for array classes.4bacdc3uses anObjectarray for primitive array values, andtestNullableArraysOfArrayBackedTypescoversPoint,RingandTuple. It fails without that commit.This also corrects the sentence above that the result is unchanged for types that already worked. The values are unchanged, but the array's component type can now be more specific where it was declared as
Object(for exampleHashMap[]instead ofObject[]forArray(Nullable(JSON))).mvn -pl client-v2,jdbc-v2 installpasses with-Duser.timezone=UTC: client-v2 826 + 1201, jdbc-v2 1975 + 799 (21 skipped).