Fix client-v2: serialize SimpleAggregateFunction columns in RowBinary writer#2937
Open
polyglotAI-bot wants to merge 2 commits into
Open
Fix client-v2: serialize SimpleAggregateFunction columns in RowBinary writer#2937polyglotAI-bot wants to merge 2 commits into
polyglotAI-bot wants to merge 2 commits into
Conversation
… writer
SerializerUtils.serializeData had no case for SimpleAggregateFunction, so
inserting into such a column threw UnsupportedOperationException ("Unsupported
data type: SimpleAggregateFunction") even though BinaryStreamReader already
reads these columns. Delegate to the underlying type via serializeNestedData
so a Nullable underlying still gets its RowBinary null-marker byte, mirroring
the read path.
Related to: #2477
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
Member
|
@cursor review |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 9209449. Configure here.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes client-v2 RowBinary serialization for SimpleAggregateFunction(func, T) columns by routing them through the underlying type’s serialization, matching the existing reader behavior and unblocking inserts that previously threw UnsupportedOperationException.
Changes:
- Add a
SimpleAggregateFunctionbranch inSerializerUtils.serializeDatato serialize SAF values via their underlying nested column. - Add unit and integration coverage exercising SAF serialization (including nullable-underlying cases and positional misalignment detection).
- Add a changelog entry documenting the fix.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/SerializerUtils.java | Adds SAF handling in the RowBinary serialization switch. |
| client-v2/src/test/java/com/clickhouse/client/api/data_formats/internal/SerializerUtilsTest.java | Adds round-trip unit tests for SAF (top-level and SAF-in-tuple alignment cases). |
| client-v2/src/test/java/com/clickhouse/client/datatypes/RowBinaryFormatWriterTest.java | Re-enables/fixes SAF integration test DDL + insert/read path. |
| CHANGELOG.md | Documents the SAF RowBinary writer bug fix. |
| " saf_sum SimpleAggregateFunction(sum, UInt64), " + | ||
| " saf_str SimpleAggregateFunction(anyLast, Nullable(String)), " + | ||
| " tail Int32 " + | ||
| " ) Engine = AggregatingMergeTree ORDER BY id"; |
Collaborator
Author
There was a problem hiding this comment.
I checked this empirically and AggregatingMergeTree does not require non-key columns to be aggregate types — a regular column is accepted and simply retains an arbitrary value on merge. The exact DDL from this test creates and round-trips fine:
CREATE TABLE test_saf_ddl
(id Int32,
saf_sum SimpleAggregateFunction(sum, UInt64),
saf_str SimpleAggregateFunction(anyLast, Nullable(String)),
tail Int32)
Engine = AggregatingMergeTree ORDER BY id;
-- OK
INSERT INTO test_saf_ddl VALUES (1, 42, 'hello', 7);
SELECT id, saf_sum, saf_str, tail FROM test_saf_ddl;
-- 1 42 hello 7Verified on ClickHouse 26.5, and this integration test already runs green in CI across CH 25.3 / 25.8 / latest.
The trailing tail Int32 is intentionally a non-key column: it sits after the SimpleAggregateFunction columns so that a byte dropped or added while serializing them misaligns tail and is detected on read-back. Moving tail into ORDER BY (or switching to MergeTree) would weaken that misalignment check, so I've left the DDL as-is.
…LOG code spans - SerializerUtils: expand the top-level SimpleAggregateFunction case comment and serializeNestedData's Javadoc to note the deliberate exception to the "nested elements only" contract (the SAF wrapper is non-nullable so writeValuePreamble writes no marker, but a Nullable underlying still needs one). - CHANGELOG: reflow the SimpleAggregateFunction entry so no inline-code span wraps across a line break (renders correctly in Markdown), matching the file's other entries. Both changes are documentation-only; no behavior change.
TriageCategory: Summary What this impacts
Concerns
Required reviewer action
|
|
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
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.
Description
Related to #2477.
SerializerUtils.serializeData(the singleRowBinarywrite funnel inclient-v2) had acase AggregateFunctionbut nocase SimpleAggregateFunction, so aSimpleAggregateFunction(func, T)column fell through toserializePrimitiveData'sdefaultbranch and threwUnsupportedOperationException: Unsupported data type: SimpleAggregateFunction. The reader already supports these columns (BinaryStreamReader.readValue→readValue(column.getNestedColumns().get(0))), so writing was the only broken direction. This is theSimpleAggregateFunctionsibling of theNestedwrite gap tracked in #2477 (fixed forNestedin #2936); it was split out because theNullable-underlying null-marker handling deserves its own change.A
SimpleAggregateFunction(func, T)value is serialized byte-identically to its underlying typeT. The fix delegates through the existingserializeNestedDatahelper (rather thanserializeDatadirectly) so that when the underlying type isNullable(e.g.SimpleAggregateFunction(anyLast, Nullable(String))) the inner null-marker byte (0x00present /0x01null) is written, exactly mirroring the read path.Changes
client-v2SerializerUtils.serializeData: addedcase SimpleAggregateFunctiondelegating toserializeNestedData(stream, value, column.getNestedColumns().get(0)).SerializerUtilsTest: newtestSimpleAggregateFunctionRoundTrip(@DataProvider) covering top-level SAF columns and SAF nested mid-Tuplewith a trailingFloat64for positional misalignment detection — non-nullable underlying (fixed- and variable-width),Nullableunderlying present, andNullableunderlying null.RowBinaryFormatWriterTest.writeSimpleAggregateFunctionTests: re-enabled and fixed (the old version wasenabled = falsewith invalid DDLSimpleAggregateFunction(count, Int8)); now inserts intoSimpleAggregateFunction(sum, UInt64)+SimpleAggregateFunction(anyLast, Nullable(String))onAggregatingMergeTree, with a trailingInt32to catch byte misalignment, and round-trips against the server.CHANGELOG.md: bug-fix entry.Test
testSimpleAggregateFunctionRoundTripfails onmainfor every SAF row withUnsupportedOperationException: Unsupported data type: SimpleAggregateFunction, and passes with the fix. Expected values were derived from a live ClickHouse 26.5 server.Float64if the branch wrote a marker unconditionally.RowBinaryFormatWriterand a live server.SerializerUtilsTest(47) +SerializerUtilsPrimitiveSerializationTests+BinaryStreamReaderTests+RowBinaryFormatReaderTest(108 total) pass — no existing tests weakened.Known limitation (follow-up)
Writing an explicit
nullinto a top-levelSimpleAggregateFunction(<func>, Nullable(T))column viaRowBinaryFormatWriterstill throws inRowBinaryFormatSerializer.writeValuePreamble("attempt to write null into not nullable column"), because the preamble keys off the outer column's nullability (SAF is not nullable). Fixing that requires the preamble to treat a SAF/AggregateFunctioncolumn with aNullableunderlying as nullable — a broader, shared change (AggregateFunctionhas the same limitation), left for a follow-up. Non-null values (the common case) and null values when the SAF is nested inside a container are handled by this PR.changes_checklist.md — "String, SQL, or serialized output changed"
Pre-PR validation gate
main, pass with fix)AGENTS.md/changes_checklist.mdCHANGELOG.mdupdatedRowBinaryFormatWriter+ server)