You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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.
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.
Summary
This PR introduces opt-in binary string support (new binary_string_support config property, disabled by default) to prevent lossy UTF-8 decoding of binary data stored in String/FixedString columns. It adds a new internal StringValue holder class that preserves raw bytes and decodes to String lazily. The core BinaryStreamReader and AbstractBinaryFormatReader are updated so that, when the flag is enabled, top-level string columns are returned as StringValue rather than String; strings nested inside containers (Array, Map, Tuple, Nested, Variant, JSON) continue to decode as String. The public ClickHouseBinaryFormatWriter interface gains two new setString(…, byte[]) overloads, making this a binary- and source-incompatible change for any third-party implementations. JDBC ResultSet#getBinaryStream is implemented (previously always threw "unsupported"). The diff is large (~1,994 additions) but a substantial portion is unit and integration tests. Linked to issue #2263.
What this impacts
client-v2/: Core read path — BinaryStreamReader, AbstractBinaryFormatReader, RowBinaryFormatSerializer, RowBinaryFormatWriter; public writer interface ClickHouseBinaryFormatWriter; new StringValue class; POJO binding bytecode generation in SerializerUtils; new ClientConfigProperties.BINARY_STRING_SUPPORT config key; Client.Builder#binaryStringSupport(boolean) API
jdbc-v2/: ResultSetImpl#getBinaryStream(int) and #getBinaryStream(String) — previously unsupported, now implemented
docs/features.md: compatibility contract updated for both modules
Concerns
Interface break (HIGH — public API shape):ClickHouseBinaryFormatWriter is a public interface and the two new setString(…, byte[]) overloads have no default implementation. Any third-party that implements the interface will fail with AbstractMethodError at runtime after upgrading; the CHANGELOG calls this out, but a reviewer should consider whether adding default stubs (throwing UnsupportedOperationException) would be a safer migration path.
Reader/writer changes (HIGH): Core read path in BinaryStreamReader and AbstractBinaryFormatReader is modified; this is a high-risk area per repo rules. Reviewer should verify the feature-off code path is truly unchanged.
Large diff (HIGH): ~1,994 additions exceed the 400-line threshold; reviewer should confirm the hot-path delta is minimal and the bulk is tests.
StringValue mutability hazard:toByteArray() returns the live backing array when it spans the full buffer — callers can inadvertently mutate the holder's value. The Javadoc documents this, but it is a subtle aliasing risk across the call sites in SerializerUtils, MapBackedRecord, and DataTypeConverter.
String.format in exception path (ResultSetImpl.java): The last SQL statement (getLastStatementSql()) is embedded into the exception message via String.format. If the statement contains sensitive query parameters they will appear in logs/stack traces.
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
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.
Summary
There are a few problems with reading binary strings:
This PR introduces
StringValuethat acts asbyte[]holder to createStringad-hoc instead of when read. So strings that suppose to be read viabyte[]will stay asbyte[].This affects also JDBC where
ResultSethasgetBinaryStreamfor such cases. Now it is supported operation.Part of #2263
Checklist
Delete items not relevant to your PR: