Fix client-v2: serialize SimpleAggregateFunction columns in RowBinary writer - #2937
Merged
chernser merged 5 commits intoJul 29, 2026
Merged
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. |
…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
|
Member
|
@cursor review |
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 159f43a. Configure here.
…mpleaggregatefunction-writer # Conflicts: # CHANGELOG.md
chernser
requested changes
Jul 28, 2026
chernser
left a comment
chernser
left a comment
Contributor
There was a problem hiding this comment.
Build fails
[ERROR] RowBinaryFormatWriterTest.writeSimpleAggregateFunctionTests:862->writeTest:194->initTable:114 » Server Code: 36. DB::Exception: Column(s) tail of the AggregatingMergeTree table are neither part of the sorting key nor aggregate measures (AggregateFunction or SimpleAggregateFunction). During background merges, rows with the same value of the sorting key are collapsed into one, so these columns keep an arbitrary value, which silently produces wrong results for queries that GROUP BY or filter on them. Add them to the sorting key (ORDER BY), or set 'allow_dimensions_outside_sorting_key = 1' if this is intentional (for example, when the columns are functionally dependent on the sorting key). (BAD_ARGUMENTS) (version 26.7.1.1315 (official build)) (queryId= 2c595b4e-4d4f-4ae3-bbfc-20f92ab413bc)
Check both variants. Document them.
Also update features.md
There should be a test in JDBC at minimum
writeSimpleAggregateFunctionTests created its test table with Engine = AggregatingMergeTree and a non-key `tail Int32` guard column. ClickHouse 26.7 tightened AggregatingMergeTree validation to reject non-key columns that are neither part of the sorting key nor aggregate measures (Code: 36 BAD_ARGUMENTS), breaking the build on CH latest while 25.3 / 25.8 / 26.5 still accepted it. Switch the engine to MergeTree: the SimpleAggregateFunction columns and the trailing `tail` misalignment guard remain valid on every server version, and DESCRIBE still reports the SimpleAggregateFunction types so the RowBinary writer path under test is unchanged. Matches the sibling integration tests in this file, which all use MergeTree.
Collaborator
Author
|
Thanks @chernser — fixed in c6e7d06. Root cause: Reproduced on CH 26.7.1.1315: Fix: switched the engine to Verified |
…mpleaggregatefunction-writer # Conflicts: # CHANGELOG.md
|
chernser
approved these changes
Jul 29, 2026
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)