Support reinterpreting an Array as a String in reinterpret#109383
Merged
Conversation
`reinterpret(<string>, 'Array(T)')` was already supported, but the inverse
`reinterpret(<array of fixed size elements>, 'String')` threw
`ILLEGAL_TYPE_OF_ARGUMENT` ("Cannot reinterpret Array(...) as String"),
even though `getReturnTypeImpl` already allowed it: an `Array` whose element
type is fixed size is unambiguously represented in a contiguous memory region.
The mismatch was in `executeImpl`, which dispatches source/destination type
pairs through `callOnTwoTypeIndexes`. That helper has no case for the `Array`
type index, so an `Array` source fell through to the two-argument error path.
`ColumnArray::getDataAt` already exposes each row's elements as a single
contiguous byte range, so handle this case explicitly by feeding it to the
existing `executeToString`.
Closes: #109379
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
Workflow [PR], commit [f89350c] Summary: ✅
AI ReviewSummaryThis PR extends Final Verdict
|
clickhouse-gh
Bot
added
the
pr-improvement
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
`reinterpret(<empty array>, 'String')` triggered `UndefinedBehaviorSanitizer` because `ColumnArray::getDataAt` returns an empty range with a null data pointer for an empty array, and `executeToString` then called `memcpy(dst, nullptr, 0)`. Passing a null pointer to `memcpy` is undefined behavior even when the size is zero (its arguments are declared `nonnull`), so the `arm_asan_ubsan` and `amd_asan_ubsan` builds aborted the server with `undefined-behavior src/Functions/reinterpretAs.cpp`, which cascaded into sibling reinterpret tests failing with `Connection reset by peer`. Skip the copy when the source range is empty. CI report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=109383&sha=ed61b8ad23efe55fc81a3575b05b31c63e871676&name_0=PR&name_1=Stateless%20tests%20%28arm_asan_ubsan%2C%20targeted%29 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The `Array -> String` branch reused the scalar `executeToString`, which trims trailing zero bytes. That trimming is correct for scalar `reinterpretAsString` (the `String -> scalar` path pads the value back with zeros), but wrong for arrays: the `String -> Array` path requires the byte length to be an exact multiple of the element size and does not pad, so trimming broke the round-trip that this PR advertises as the inverse of `reinterpret(<string>, 'Array(T)')`. For example `[toInt32(1)]` produced a 1-byte string that can no longer be reinterpreted as `Array(Int32)` (it throws `BAD_ARGUMENTS`), and `[toUInt8(1), toUInt8(0)]` silently round-tripped as `[1]`. Add `executeContiguousToString`, which copies each row's contiguous byte range verbatim (keeping the null-pointer guard for empty `Array` sources), and route the `Array -> String` branch through it. Extend the test with trailing-zero `hex` checks and round-trip regressions for `Array(UInt8)` and `Array(Int32)`. Addresses the AI review "Request changes" verdict on #109383 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address the AI-review nit on #109383: the structured `FunctionDocumentation` for `reinterpretAsString` still advertised only scalar inputs and stated that trailing zero bytes are always ignored. Since this PR added the `Array` of fixed-size elements -> `String` path, the docs are now inaccurate. Update the description to note that for an `Array` the element bytes are copied verbatim (trailing zero bytes are NOT trimmed, so the result round-trips back to the original `Array` type), add `Array` to the accepted argument types, and add an `Array` example mirroring the existing `reinterpret` one. The example output `02010403` matches the verified reference of the identical query in `04503_reinterpret_array_to_string`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…et-array-to-string
The `Array -> String` reinterpret helper `executeContiguousToString` reversed the whole row buffer on big-endian via `reverseMemcpy`. That broke the round-trip with `String -> Array`: the `String -> Array` path (`ColumnArray::insertData`) splits the string into element-sized chunks and stores each chunk into the array element memory verbatim, with no byte swapping on any endianness. A whole-row reverse therefore does not invert it — for example `[toUInt16(0x0102), toUInt16(0x0304)]` has in-memory bytes `01 02 03 04` on big-endian, a whole-row reverse yields `04 03 02 01`, and reading that back as `Array(UInt16)` gives `[0x0403, 0x0201]` instead of the original array. Copy the bytes verbatim on every architecture, matching the `String -> Array` semantics so the two directions are exact inverses. Unlike the scalar `executeToString`, this path must not apply the little-endian String byte-order convention, because `String -> Array` does not apply it either. No change on little-endian (the copy was already a plain `memcpy` there). The existing multi-element round-trip regressions in 04503_reinterpret_array_to_string guard element order and would catch this on a big-endian machine. Addresses review comment: #109383 (comment) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…et-array-to-string
…docs to scalar inputs The `reinterpretAsString` `FunctionDocumentation` description said the function reinterprets its input "assuming little endian order" as a blanket statement, but that only holds for scalar inputs: `executeToString` normalizes to little endian by reversing bytes on big-endian builds. The `Array` path (`executeContiguousToString`) deliberately copies each element's bytes verbatim in native in-memory order with no byte-order normalization, since it must stay the exact inverse of `reinterpret(<string>, 'Array(T)')` on every architecture. Scoped the little-endian sentence to scalars and spelled out the `Array` exception.
…dent The AI review (`clickhouse-gh`) noted that the `hex` checks in `04503_reinterpret_array_to_string.sql` and the two new `reinterpretAsString` / `reinterpret` `FunctionDocumentation` examples hard-code the little-endian byte layout. The `Array -> String` path (`executeContiguousToString`) copies each element's bytes verbatim in native in-memory order (to match the verbatim `String -> Array` path on every architecture), so `hex` of a multi-byte element type differs between little- and big-endian builds. On a big-endian build (e.g. s390x) `[toUInt16(0x0102), toUInt16(0x0304)]` yields `01020304`, not `02010403`, and the `Int32` / `LowCardinality(Int32)` expectations change for the same reason, so the test and the documented examples would be wrong there. Keep byte-exact `hex` assertions only for the architecture-independent cases (single-byte `Array(UInt8)` and `Array(FixedString(N))`, whose bytes are the same on every architecture) and the table read. For multi-byte element types, assert the byte count via `length` (element size times element count, independent of byte order) and rely on the existing round-trip assertions to verify the element values on every architecture. Replace the `LowCardinality(Int32)` `hex` check with a round-trip. Change both documentation examples to an `Array(UInt8)` input so the shown `hex` output (`0102FF`) is correct on every architecture. No runtime logic changes; verified locally that the updated test matches the reference.
…et-array-to-string
…einterpret Address review on `src/Functions/reinterpretAs.cpp`: the review suggested the `Array -> String` path is unreachable for non-numeric fixed-size `LowCardinality` element types such as `Array(LowCardinality(FixedString(2)))` and `Array(LowCardinality(UUID))`, because `DataTypeLowCardinality` inherits the default `isValueUnambiguouslyRepresentedInFixedSizeContiguousMemoryRegion` (which is true only for numeric-like dictionaries), so `getReturnTypeImpl` would reject them with `ILLEGAL_TYPE_OF_ARGUMENT`. Verified empirically that these queries already work. The framework recursively strips nested `LowCardinality` (`recursiveRemoveLowCardinality`, via `convertLowCardinalityColumnsToFull`) in `IFunctionOverloadResolver::getReturnType` before `getReturnTypeImpl` runs, so the type gate already sees the plain `Array(FixedString(2))` / `Array(UUID)` and the contiguous-memory check passes; execution strips it the same way before `executeImpl`. No runtime change is needed and the suggested extra `recursiveRemoveLowCardinality` in `getReturnTypeImpl` would be dead code. Extend `04503_reinterpret_array_to_string` with non-numeric fixed-size `LowCardinality` cases (`FixedString(2)` hex + round-trip, `UUID` round-trip, using architecture-independent assertions) to lock the behavior in. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Member
Author
|
@groeneai, could you investigate this
It is unrelated to this PR (which only adds |
…tring The `Array -> String` example added `reinterpret(<array>, 'String')` to the public `reinterpret` contract, but the generic `reinterpret` `FunctionDocumentation` still described only the old array-*destination* restriction. Extend `description_reinterpret` and the `x` argument documentation to state that a source `Array` must have fixed-size, contiguous elements when the destination is `String`, so that `Array(String)` and `Array(Nullable(Int32))` are documented as unsupported (they throw `ILLEGAL_TYPE_OF_ARGUMENT` during type checking). This is a documentation-only change; no runtime behavior is affected. Addresses the AI review finding on PR #109383. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
Confirmed unrelated to this PR. STID 4811-6023 is the JSON_EXISTS / JSON_VALUE / JSON_QUERY over Reproducer from the same runs: SELECT JSON_EXISTS(CAST('{"a": 1, "b": 2}', 'Dynamic'), tuple('$.b'));Spread (CIDB, 30d): the Owner: fix PR #109944 (open, MERGEABLE), |
Contributor
LLVM Coverage Report
Changed lines: Changed C/C++ lines covered: 66/67 (98.51%) · Uncovered code |
robot-clickhouse-ci-1
added
the
pr-synced-to-cloud
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
3 participants
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.
reinterpret(<string>, 'Array(T)')was already supported, but the inversereinterpret(<array of fixed size elements>, 'String')threwILLEGAL_TYPE_OF_ARGUMENT("Cannot reinterpret Array(...) as String") — even thoughgetReturnTypeImplalready permitted it, since anArraywhose element type is fixed size is unambiguously represented in a contiguous memory region.The mismatch was in
executeImpl, which dispatches source/destination type pairs throughcallOnTwoTypeIndexes. That helper has no case for theArraytype index, so anArraysource fell through to the two-argument error path — whileArraydestinations were already handled by a dedicated fallback block.ColumnArray::getDataAtalready exposes each row's elements as a single contiguous byte range, so this case is now handled explicitly by feeding it to the existingexecuteToString, makingreinterpretbetweenArrayandStringsymmetric.Note: no
LowCardinality-nested guard is needed for the source (unlike theArraydestination), because source columns pass throughrecursiveRemoveLowCardinalitybeforereinterpretruns, soArray(LowCardinality(Int32))arrives asArray(Int32).Closes: #109379
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Support
reinterpretof anArrayof fixed-size elements as aString, the inverse of the existingreinterpretof aString/FixedStringas anArray.Documentation entry for user-facing changes
Version info
26.7.1.1143(included in26.7and later)