Skip to content

fix(jdbc-v2): render array contents for named tuples - #3046

Open
014-code wants to merge 1 commit into
ClickHouse:mainfrom
014-code:fix/3045-jdbc-array-tuple-display
Open

fix(jdbc-v2): render array contents for named tuples#3046
014-code wants to merge 1 commit into
ClickHouse:mainfrom
014-code:fix/3045-jdbc-array-tuple-display

Conversation

@014-code

@014-code 014-code commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Make JDBC Array values display their nested contents.
  • Support readable rendering of Array(Tuple(...)) values with named tuple elements.
  • Preserve the existing java.sql.Array return type and getArray() behavior.
  • Add unit and integration regression tests.
  • Update CHANGELOG.md.

Problem

When JDBC v2 returned an array containing a named tuple, clients such as IntelliJ IDEA could display the value as:

com.clickhouse.jdbc.types.Array@54c3b772

For example:

SELECT [('550e8400-e29b-41d4-a716-446655440000')::Tuple(id UUID)];

The actual contents were available through java.sql.Array#getArray(), but the JDBC wrapper inherited Object#toString(), which produced an unreadable class name and identity hash.

Fixes #3045.

Changes

com.clickhouse.jdbc.types.Array#toString() now uses deep array formatting via Arrays.deepToString(...).

The value is now rendered as:

[[550e8400-e29b-41d4-a716-446655440000]]

Nested arrays and tuple values are also rendered correctly.

Compatibility

No public API signatures, JDBC return types, or array contents were changed.

This change only improves the textual representation returned by com.clickhouse.jdbc.types.Array#toString().

Tests

Added:

  • ArrayTest#testToStringForArrayOfNamedTuples

    • Verifies deep formatting of Array(Tuple(id UUID)).
    • Passed: 1 test, 0 failures.
  • ArrayTupleIntegrationTest#testArrayOfNamedAndUnnamedTuplesToString

    • Runs against ClickHouse using Docker.
    • Covers both named and unnamed tuple arrays.
    • Verifies ResultSet#getObject() returns java.sql.Array.
    • Verifies toString() matches the contents returned by Array#getArray().
    • Passed: 1 test, 0 failures.

Fixes ClickHouse#3045. Render nested JDBC Array values with deep formatting so array-of-tuple results are readable to clients that use ResultSet#getObject().
@mshustov

Copy link
Copy Markdown
Member

@claude review

@claude

claude Bot commented Aug 10, 2026

Copy link
Copy Markdown

Claude finished @mshustov's task in 4m 7s —— View job


Review: fix(jdbc-v2): render array contents for named tuples

  • Read repo review guidelines (docs/ai-review.md, AGENTS.md, docs/features.md)
  • Examine the diff and surrounding code
  • Analyze changes for correctness, compatibility, and test quality
  • Post review feedback

Findings (ordered by severity)

1. The output format is a one-way compatibility decision — confirm deepToString is the format you want to ship. (design/compatibility)

Arrays.deepToString produces Java-style rendering, not ClickHouse literal syntax: Array(Tuple(id UUID)) renders as [[550e8400-…]] rather than [('550e8400-…')]. Consequences worth a deliberate maintainer decision before this is released, since per docs/ai-review.md toString() output is compatibility-sensitive and changing it later would be a breaking change of formatted output:

  • Tuples and nested arrays are indistinguishable — Array(Tuple(UUID)) and Array(Array(UUID)) both render as [[…]].
  • Strings are unquoted and unescaped, so the output is ambiguous: ['a, b'] and ['a', 'b'] both render as [a, b].
  • Common JDBC practice (e.g. pgjdbc's PgArray#toString) is to render the database's own literal form. If you ever want ClickHouse-literal output (which would round-trip and match getString()-style server rendering), it's cheaper to do it now than after clients start depending on this format.

If readable-but-approximate output is the intended contract (which is a defensible reading of #3045 — the goal is fixing Array@54c3b772 in IDE viewers), this is fine as-is; the new tests correctly lock the format in. Just flagging that this PR is the moment the format gets frozen. Fix this →

2. Struct (top-level Tuple(...) columns) still has the same display problem. (consistency)

com.clickhouse.jdbc.types.Struct (Struct.java) does not override toString(), so SELECT ('…')::Tuple(id UUID) (without the array) still displays as com.clickhouse.jdbc.types.Struct@… in the same IDE viewers. Fine as a follow-up to keep this PR focused, but worth an issue so array and tuple rendering don't diverge — and it's another reason to settle finding 1 first, since Struct#toString should use the same format.

3. toString() after free() returns the string "null". (minor)

free() nulls the array field (Array.java:120-123), and Arrays.deepToString(null) returns "null". No NPE, and toString() shouldn't throw SQLException, so this behavior is acceptable — just noting it's a deliberate trade-off, and a one-line unit-test case would lock it in.

4. New single-test integration class instead of an existing home. (style, minor)

ArrayTupleIntegrationTest introduces a new class (and a naming pattern — other integration tests here are *Test/*Tests, e.g. JdbcDataTypeTests, with groups = {"integration"}) for one test method. Folding it into JdbcDataTypeTests or ResultSetImplTest would match existing conventions. The unit ArrayTest is fine — it's the natural new home for types.Array unit tests.

What's good

  • The fix is the smallest safe change: only toString() is added; getArray(), return types, and all JDBC semantics are untouched. I verified getString() on an array column goes through the client-v2 reader (ResultSetImpl.getStringreader.getString), and getObject(col, String.class) converts the raw reader value — neither routes through this wrapper's toString(), so the PR's "display-only" compatibility claim holds.
  • Arrays.deepToString correctly handles nested Object[] tuples, nested primitive arrays, and self-referential arrays.
  • Both a unit test and an integration test through the real read path (named and unnamed tuples), matching the repo's preference for integration coverage; tests are clean of issue-number narration.
  • CHANGELOG entry is in the right section with the issue link.

Docs

docs/features.md line 90 documents the jdbc-v2 arrays/tuples feature; per docs/ai-review.md, an intentional user-visible behavior change should be reflected there. Suggest adding a short clause that Array#toString() renders nested contents (deep formatting).

Tests run

I could not run Maven locally (the mvn command is not in this run's allowed tools); PR CI (Build, Coverage) was still in progress at review time. The tests are simple and read as correct against the verified real-path behavior.

Summary

  • What changed: jdbc-v2 Array gains a toString() override using Arrays.deepToString, plus unit/integration tests and a CHANGELOG entry.
  • Compatibility risk: Low. No API or return-type changes; the only externally visible change is toString() output, which previously was the unusable Object#toString default. The chosen format itself becomes a compatibility commitment (finding 1).
  • Docs: recommend a one-line docs/features.md update.

Verdict: ready for human review

--- · Branch

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.

Regression in JDBC 0.9.8 when reading Array(Tuple(...)) with named tuple elements.

2 participants