Skip to content

[v0.10.0-rc1] Binary String Support #2911

Open
chernser wants to merge 18 commits into
v0.10.0from
07/06/26/binary_string_support
Open

[v0.10.0-rc1] Binary String Support #2911
chernser wants to merge 18 commits into
v0.10.0from
07/06/26/binary_string_support

Conversation

@chernser

@chernser chernser commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Summary

There are a few problems with reading binary strings:

  • they are not utf-8 strings and should not be decoded like that
  • reading big binary blobs may cause OOM because currently strings are created on read.

This PR introduces StringValue that acts as byte[] holder to create String ad-hoc instead of when read. So strings that suppose to be read via byte[] will stay as byte[].

This affects also JDBC where ResultSet has getBinaryStream for such cases. Now it is supported operation.

Part of #2263

Checklist

Delete items not relevant to your PR:

  • Closes #
  • Unit and integration tests covering the common scenarios were added
  • A human-readable description of the changes was provided to include in CHANGELOG
  • For significant changes, documentation in https://github.com/ClickHouse/clickhouse-docs was updated with further explanations or tutorials

@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown

Repository collaborators can run the JMH benchmark suite against this PR by commenting:

/benchmark

Optional regression threshold override (Δ% on Time or Alloc/op; defaults to 10%):

/benchmark threshold=15

Only one benchmark run per PR is active at a time — issuing a new /benchmark comment cancels the previous run. After the run finishes a separate comment will be posted comparing it against the latest scheduled run on main; the PR check fails if any benchmark regresses by more than the threshold.

@chernser chernser changed the title 07/06/26/binary string support [v0.10.0-rc1] Binary String Support Jul 6, 2026
@chernser chernser marked this pull request as ready for review July 6, 2026 23:22
@chernser chernser requested a review from mzitnik as a code owner July 6, 2026 23:22
@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown

Triage

Category: featureRisk: high

Summary
This PR introduces opt-in binary string support for client-v2 and jdbc-v2 to fix two problems: String/FixedString columns were being decoded as UTF-8 at read time (losing non-UTF-8 binary data) and large binary blobs could cause OOM because strings were decoded eagerly. The core change is a new StringValue class (an internal read-time byte holder), a new binary_string_support configuration property (disabled by default, resolvable per-operation), and plumbing through BinaryStreamReader, AbstractBinaryFormatReader, MapBackedRecord, SerializerUtils (including ASM bytecode generation for POJO setters), and RowBinaryFormatWriter. On the JDBC side, ResultSet#getBinaryStream(int/String) moves from unsupported to implemented. The writer side gains setString(String, byte[]) and setString(int, byte[]) on the public ClickHouseBinaryFormatWriter interface.

What this impacts

  • client-v2: BinaryStreamReader.readValue() hot path now branches on binaryStringSupport; AbstractBinaryFormatReader, MapBackedRecord, SerializerUtils all updated to handle StringValue as a possible value type
  • jdbc-v2: ResultSetImpl.getBinaryStream() now functional; new JDBC tests added
  • Public API: ClickHouseBinaryFormatWriter interface gained two new methods — any third-party implementor that does not recompile will get AbstractMethodError at runtime (explicitly documented in CHANGELOG as a binary-incompatible change)
  • POJO binding: ASM bytecode generation in SerializerUtils.compilePOJOSetter extended with new String and byte[] target-type branches

Concerns

  • High — Public API shape changed: ClickHouseBinaryFormatWriter is a public interface; adding setString(String, byte[]) and setString(int, byte[]) breaks any downstream implementation at the binary level. The CHANGELOG acknowledges this but the reviewer should confirm this is acceptable for v0.10.0-rc1.
  • High — Readers and Writers: BinaryStreamReader.readValue() is the core deserialization hot path; the new stringAsBytes branching inside that method and the introduction of readNestedValue() for all container reads (Array, Map, Tuple, Nested, Variant, JSON) need careful review for correctness and performance.
  • High — Large diff: 2,039 additions + 44 deletions = 2,083 total lines; well above the 400-LOC reviewability threshold. Request split or flag if RC timeline requires proceeding as-is.
  • New connection-string setting: binary_string_support is added as an opt-in flag (default false), satisfying the feature-flag requirement, but the per-operation override path (merging client and query settings) should be verified against the existing settings-resolution logic.
  • ASM bytecode generation: two new branches in compilePOJOSetter emit static dispatch to stringValueToString/stringValueToByteArray; these are exercised by unit tests but are non-trivial to audit statically.

Required reviewer action

  • At least one human reviewer.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit cbd500f. Configure here.

featureManager.unsupportedFeatureThrow("getBinaryStream");

return null;
return getBinaryStream(getSchema().nameToColumnIndex(columnLabel));

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JDBC Object getObject returns StringValue

Medium Severity

With binary_string_support enabled, getObject returns internal StringValue for String/FixedString columns without proper conversion. This causes issues when callers expect String (for Object.class) or byte[] (for byte[].class), as JdbcUtils.convert lacks StringValue handling. getString, getBytes, and getBinaryStream are unaffected.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit cbd500f. Configure here.

private Object readNestedValue(ClickHouseColumn column) throws IOException {
return readValue(column, null, false);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

skipValue still loads string blobs

Medium Severity

skipValue still calls full readValue for every column. With binary_string_support enabled, skipped top-level String/FixedString columns still allocate a StringValue and backing bytes, so readToPOJO with unmapped large string fields can OOM despite the feature’s lazy-read goal.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit cbd500f. Configure here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants