Fix binary type decoding of SimpleAggregateFunction/AggregateFunction (Dynamic, Variant, JSON) - #506
Conversation
Implement the SimpleAggregateFunction (0x2E) and AggregateFunction (0x25) branches of BinaryTypeDecoder.FromByteCode, which were stubs throwing NotImplementedException. Reading a SimpleAggregateFunction value out of a Dynamic, Variant, JSON or nested Dynamic column now returns the value; AggregateFunction reports the usual "use <function>Merge()" error instead. Fixes: #505
There was a problem hiding this comment.
Pull request overview
This PR fixes decoding of SimpleAggregateFunction and AggregateFunction type headers when they appear inside binary-encoded type contexts (Dynamic, Variant, JSON, and nested Dynamic), replacing previous NotImplementedException stubs with spec-compliant header consumption and adding regression tests to prevent RowBinary stream desynchronization.
Changes:
- Implemented binary decoding for
SimpleAggregateFunctionandAggregateFunctionheaders inBinaryTypeDecoder, including skipping aggregate-function parameters to keep the stream aligned. - Added integration tests for reading
SimpleAggregateFunctioninsideDynamicand verifyingAggregateFunctionsurfaces the existing actionable*Merge()guidance. - Added byte-level decoder tests to assert full header consumption (
stream.Position) and error behavior on unknown parameter codes; updated CHANGELOG/RELEASENOTES.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| ClickHouse.Driver/Types/BinaryTypeDecoder.cs | Implements decoding for SimpleAggregateFunction/AggregateFunction and adds parameter-skipping helpers to avoid stream desync. |
| ClickHouse.Driver.Tests/Types/DynamicTests.cs | Adds integration coverage for SAF-in-Dynamic roundtrip and AF-in-Dynamic error behavior. |
| ClickHouse.Driver.Tests/Types/BinaryTypeDecoderTests.cs | Adds byte-level tests asserting exact header consumption and parameter-skipping behavior. |
| CHANGELOG.md | Documents the bug fix in the Unreleased section. |
| RELEASENOTES.md | Mirrors the Unreleased bug-fix note for release visibility. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…explicitly The varint is the aggregate function's own state serialization version, not a header-layout version, so it is discarded rather than stored in an unused local. Documented why it is not validated and pinned a non-zero version (as written for AggregateFunction(1, uniqExact, UInt64)) with a decoder and a Dynamic read test.
The parameter skipper's per-type-code branches, the malformed-varint guard and the missing-argument-type guard had no tests, so codecov/patch flagged the added lines as uncovered. Each new case pins how many bytes one parameter type code occupies -- consuming one byte too few or too many desynchronises the value (and the next column) that follows the type header in the same stream.
…ggregate-function-binary-decode # Conflicts: # CHANGELOG.md # RELEASENOTES.md
…ggregate-function-binary-decode # Conflicts: # CHANGELOG.md # RELEASENOTES.md
…ggregate-function-binary-decode # Conflicts: # CHANGELOG.md # RELEASENOTES.md
|
Rebased onto Conflicts: Verification: No approval existed on this PR, so none was dismissed by the merge. |
…ggregate-function-binary-decode # Conflicts: # CHANGELOG.md # RELEASENOTES.md
|
Rebase note: merged
Resolved by adopting the new convention rather than re-adding the entry to the old place:
No production or test code changed in this push. |
Description
Fixes #505.
Every value inside a
Dynamiccolumn carries its own binary type header, which the driver decodes inBinaryTypeDecoder.FromByteCode(the same entry point is used forVariant,JSONtyped paths and nestedDynamic). Two dispatch branches were unimplemented stubs that threwNotImplementedException:SimpleAggregateFunction(0x2E) andAggregateFunction(0x25). So a query as simple asSELECT CAST(CAST(42, 'SimpleAggregateFunction(sum, UInt64)') AS Dynamic)failed, even though a top-levelSimpleAggregateFunctioncolumn works (that path goes through the textual type-name grammar, not the binary decoder).Both encodings are now decoded per the binary encoding spec: function name, the aggregate-function parameters, then the argument type encodings.
SimpleAggregateFunctionvalues are stored as the function's return type, which the server requires to equal the first argument type, so that type becomes theUnderlyingTypeused to read the value.AggregateFunctionvalues are aggregation states that cannot be read directly, so the header is consumed and the existingAggregateFunctionTypesurfaces its usual actionable error (Use sumMerge() function to query this value) instead ofNotImplementedException.The function parameters (e.g. the
5inSimpleAggregateFunction(groupArrayArray(5), Array(UInt32)), orquantiles(0.5, 0.9)) hold nothing needed to deserialize values, but their bytes must be consumed or the reader desyncs and silently corrupts the rest of the block — hence the parameter skipper, which implements the documented "aggregate function parameter binary encoding" table.Changes
ClickHouse.Driver/Types/BinaryTypeDecoder.cs: implementedDecodeSimpleAggregateFunctionandDecodeAggregateFunction(both now receiveTypeSettings, soReadStringsAsByteArrays/UseBigDecimalpropagate into the argument type), plusSkipAggregateFunctionParameters/SkipAggregateFunctionParameter/SkipBytes/SkipVarInthelpers for the parameter grammar. Unknown parameter type codes raiseNotSupportedExceptionrather than silently desyncing the stream.CHANGELOG.md/RELEASENOTES.md: bug-fix entry.Test
ClickHouse.Driver.Tests/Types/DynamicTests.csRead_SimpleAggregateFunctionInDynamic_ReturnsUnderlyingValue(parametrized, 5 cases):sum, UInt64,anyLast, Decimal(18, 4),groupArrayArray, Array(UInt32),sumMap, Map(UInt32, UInt64)and the parameterizedgroupArrayArray(5), Array(UInt32). Each query selects a trailingInt32column that is asserted too — it only decodes correctly if the type header consumed exactly its own bytes.Read_AggregateFunctionInDynamic_ThrowsAggregateFunctionException(sumState, andquantilesState(0.5, 0.9)for theFloat64parameter path): asserts the<function>Merge()guidance is what surfaces.ClickHouse.Driver.Tests/Types/BinaryTypeDecoderTests.cs: byte-level tests over headers captured from a real server'sRowBinaryoutput, asserting the decoded type and that decoding consumed exactly the header (stream.Position) — the desync failure mode is invisible to theAggregateFunctionintegration test, since its value bytes are never read. Also covers a multi-byte varint parameter (groupArrayArray(300)),TypeSettingspropagation into the storage type, and the unknown-parameter-code error.All 7 new integration cases fail on
mainwithNotImplementedException; the fullClickHouse.Driver.Testssuite passes on this branch (9604 passed / 142 skipped, net10.0, ClickHouse 26.5). No existing test was modified.Pre-PR validation gate
main, passes here)AGENTS.md(integration tests first,TestCaseSourceparametrization, CHANGELOG + RELEASENOTES updated, no public API change)