Fix InvalidCastException when summing double/float columns (#46) - #48
Merged
Conversation
`Sum`/`SumAsync` over a `double` or `float` column threw `InvalidCastException`. EF Core wraps a top-level aggregate so the empty case returns 0, supplying that fallback as a boxed `Int32` carrying the `Float64`/`Float32` mapping; `GenerateNonNullSqlLiteral` unboxed it with a hard `(double)`/`(float)` cast and threw. Both float literal generators now convert instead of unboxing. The `Float32` read path also converts (GetValue + Convert.ToSingle), since ClickHouse widens `sum(Float32)` to `Float64` and the driver's `GetFloat()` refuses to downcast the returned Double. Audited the rest of the type system: the Double read path is fine (Float64 is the widest float type) and the integer/decimal mappings already handle boxed fallbacks and aggregate widening. Added regression tests for the fixed mappings plus the adjacent decimal/integer aggregate paths. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
Fixes ClickHouse EF Core type-mapping edge cases that caused Sum/SumAsync over double/float columns to throw InvalidCastException before or during result materialization, aligning literal generation and Float32 reading with EF Core’s boxed fallback behavior and ClickHouse aggregate widening.
Changes:
- Updated
Float64/Float32SQL literal generation to useConvert.ToDouble/ToSingle(avoids unboxing casts when EF supplies a boxedInt32fallback0). - Updated
Float32materialization to read viaDbDataReader.GetValue()andConvert.ToSingle()(handlessum(Float32)returningFloat64). - Added regression coverage across literal generation and end-to-end aggregate queries; documented fix in changelog/release notes.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/EFCore.ClickHouse.Tests/TypeMappingLiteralTests.cs | Adds regression tests ensuring boxed-Int32 fallback literals for Float32/Float64 don’t throw and don’t emit unnecessary casts. |
| test/EFCore.ClickHouse.Tests/FloatSpecialValueTests.cs | Adds end-to-end SumAsync coverage for Float64/Float32, including empty-result (0) cases. |
| test/EFCore.ClickHouse.Tests/AllTypesQueryTests.cs | Extends aggregate regression coverage to adjacent numeric types and widening behaviors (decimal, int32/int64, avg->double). |
| src/EFCore.ClickHouse/Storage/Internal/Mapping/ClickHouseFloatTypeMapping.cs | Fixes Float32 literal generation and materialization by converting instead of unboxing / typed reads. |
| src/EFCore.ClickHouse/Storage/Internal/Mapping/ClickHouseDoubleTypeMapping.cs | Fixes Float64 literal generation by converting instead of unboxing. |
| RELEASENOTES.md | Documents the user-visible bug fix for Sum/SumAsync over float/double. |
| CHANGELOG.md | Records the same fix details for v0.3.0 bug fixes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
v0.3.0 is already released, so the bug-fix entry belongs under the next version rather than the shipped one. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Summary
Fixes #46.
Sum/SumAsyncover adoubleorfloatcolumn threwSystem.InvalidCastExceptionbefore ever reaching ClickHouse:Root cause. EF Core wraps a top-level aggregate so the empty-result case returns
0, and supplies that fallback as a boxedInt32carrying the column'sFloat64/Float32type mapping.GenerateNonNullSqlLiteralunboxed it with a hard(double)/(float)cast, which throws for any runtime type other than the exact target.While adding an end-to-end test I hit a second, related ClickHouse-specific issue:
sum(Float32)widens toFloat64, and the driver'sGetFloat()refuses to downcast the returnedDouble— so summing afloatcolumn failed on the read path too.Changes
ClickHouseDoubleTypeMapping—Convert.ToDouble(...)instead of an unbox cast (NaN/±Infinity handling preserved).ClickHouseFloatTypeMapping—Convert.ToSingle(...)in the literal generator, plus aGetValue()+Convert.ToSingle()read path, mirroringClickHouseIntegerTypeMapping(which already solves the same widening problem forCOUNT→UInt64).Float64is the widest float type ClickHouse aggregates return, soGetDouble()always works.Type-system audit
I checked the whole type system for both bug classes:
DoubleandFloatwere affected.Decimal/BigDecimal/BigIntegeralready useConvert.To*; integer mappings have no literal override; non-numeric mappings aren'tSum/Averagetargets.Sum/Average/Min/Maxover every numeric type (small, boundary, and empty-result cases). With the fix in place all variants pass — integer mappings already handle their widening, andDecimal128reads back fine within .NETdecimalrange.Tests
TypeMappingLiteralTests— boxed-Int32literal for Double & Float (fail on the old cast).FloatSpecialValueTests—SumAsyncover Float64/Float32 with matches and with a no-match predicate.AllTypesQueryTests— extended the Sum over Double gives InvalidCastException #46 guard across the adjacent types:Sumover Decimal (matches + empty),Sumover Int32 (widen-to-Int64 read) and Int64 (empty), andAverageover Int32 (→double).All affected suites pass (85 tests); full solution builds with 0 errors. CHANGELOG.md and RELEASENOTES.md updated.
🤖 Generated with Claude Code