fix: bind Int128/UInt128/Int256/UInt256 (big.Int) params as numeric values - #1922
fix: bind Int128/UInt128/Int256/UInt256 (big.Int) params as numeric values#1922polyglotAI-bot wants to merge 4 commits into
Conversation
…alues
A *big.Int (the Go type behind Int128/UInt128/Int256/UInt256) implements
fmt.Stringer, so client-side binding matched the `case fmt.Stringer:` arm in
formatValue and emitted the value as a quoted string literal. The server then
saw a String -- `toTypeName(?)` returned `String`, and server-side
`{name:Int128}` parameters failed to parse the quoted value.
Emitting the bare decimal instead is not enough: ClickHouse reads a bare decimal
literal wider than 64 bits as Float64, losing precision (a WHERE on an Int128
column would match nothing). So in SQL mode the value is wrapped in the narrowest
wide-integer conversion that holds it exactly -- toInt128/toUInt128/toInt256/
toUInt256('<decimal>') -- mirroring how times bind as toDateTime(...) and floats
as cast(..., 'Float64'). In query-parameter text mode the {name:Type} placeholder
already declares the type, so the bare decimal is sent as-is.
Also fixes a nil *big.Int binding as '<nil>' instead of NULL.
Fixes: #1917
Add a tests/std regression test that drives a big.Int bind parameter through the database/sql surface (rebind -> bind) on both Native and HTTP, and add tuple and map nesting cases to the unit test, so every entry point and container type that reaches the big.Int bind path in issue #1917 is asserted.
There was a problem hiding this comment.
Pull request overview
Fixes client-side SQL placeholder binding for big.Int/*big.Int (ClickHouse Int128/UInt128/Int256/UInt256) so values are emitted as correctly-typed wide integers instead of quoted strings (or fmt.Sprint struct output), preserving both type inference and precision.
Changes:
- Extend
formatValue()to detectbig.Int/*big.Intbefore thefmt.Stringerpath and format them via the narrowestto(Int|UInt)(128|256)('...')conversion in SQL mode. - Add unit tests covering formatting/binding behavior across placeholder styles and query-parameter mode.
- Add integration tests for native + HTTP and
database/sqlsurfaces to validate server-side typing and round-trips.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
bind.go |
Adds big.Int formatting/conversion selection logic for SQL-rewrite binding and query-parameter text mode. |
bind_test.go |
Adds unit tests for wide-int formatting/binding behavior, including contrast cases for other fmt.Stringer types. |
tests/issues/1917_test.go |
New integration test validating server-side inferred type, exact round-trip, and column match behavior. |
tests/std/1917_test.go |
New database/sql integration test validating server-side inferred type and exact round-trip. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| } | ||
| return "", fmt.Errorf("big.Int value %s is out of range for Int128, UInt128, Int256 and UInt256", v.String()) | ||
| } |
There was a problem hiding this comment.
Thanks — I considered truncating but kept the full value. This branch only fires for values outside [Int256 min, UInt256 max] (>78 digits); it's a single one-shot allocation of a string the caller already holds as a big.Int (no loop, no repeated growth), and AGENTS.md asks that errors be "as actionable as possible" — the exact offending value is the most actionable thing to print. A caller who builds a pathologically huge big.Int has already allocated it, so truncating the echo wouldn't meaningfully bound memory. I'd rather not introduce a magic truncation threshold for that edge, but happy to reconsider if you feel strongly.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
🤖 Claude reviewFixes #1917 by giving The follow-up push addressing the earlier array review is sound. Coverage looks right: unit tables over both format modes and both pointer/value forms, One remaining issue, inline: the single-conversion unification is one level deep, so a mixed-magnitude Blind spots:
Verdict: ✅ Approve Inline comments are attached to the relevant lines. This summary updates in place on re-review. |
A []*big.Int / big.Int slice (and clickhouse.ArraySet) was formatted with a
per-element wide-integer conversion, so a mixed-width array such as
[]*big.Int{1, uint256Max} became [toInt128('1'), toUInt256('...')]. Those
element types have no common ClickHouse type unless use_variant_as_common_type
is on (off by default before 26.x), so WHERE ... IN ? failed with NO_COMMON_TYPE
on 25.8 (the CI floor) where the old quoted-string form used to coerce cleanly.
Render such arrays with one conversion — the narrowest wide type that holds
every element — via a shared bigIntArray helper, falling back to per-element
formatting when no single type fits (unchanged behavior). Tuples and scalars
are untouched.
Also version-gate the wide-integer integration tests at 21.12 (native and
database/sql), matching tests/bigint_test.go, and merge a split stdlib import
group, per review feedback.
…eview) commonBigIntConvFunc unifies only a flat []*big.Int / ArraySet. Mixed-magnitude big.Int values nested in a map, a nested array, or a tuple used as an IN set still get per-element toInt128/toUInt256 conversions and can hit NO_COMMON_TYPE. Document that boundary so a user hitting it from a map/nested bind can find the reason; the deeper container unification is tracked as a separate follow-up.
Description
Fixes #1917.
*big.Int/big.Intare the Go types behind ClickHouseInt128,UInt128,Int256andUInt256. In the client-side SQL bind rewrite (?,$1,@name),formatValue()matched*big.Inton thecase fmt.Stringer:arm and emitted it as a quoted string literal ('170141183460469231731687303715884105727'). The server then saw aString—toTypeName(?)returnedStringinstead of a number — so type-inference contexts (arithmetic, function dispatch,toTypeName) broke. Abig.Intpassed by value was worse still: it fell through tofmt.Sprintand produced garbage struct text.Emitting a bare decimal is not enough: ClickHouse infers an integer literal wider than 64 bits as
Float64, losing precision (aWHEREon anInt128column then matches nothing). The fix instead wraps the exact decimal in the narrowest wide-integer conversion that holds it —toInt128('..'),toUInt128('..'),toInt256('..')ortoUInt256('..')— mirroring how the driver already binds times astoDateTime(...)and floats ascast(..., 'Float64'). This keeps both the exact value and an integer type. Server-side{name:Type}query parameters already declare the type, so there the bare decimal is sent unchanged.Changes
bind.go: handlebig.Intand*big.IntinformatValue()before thefmt.Stringerarm. New helpersformatBigInt/bigIntConvFuncpick the narrowest ofInt128/UInt128/Int256/UInt256that holds the value, or return an actionable error if it fits none.nil *big.Int→NULL.bind_test.go:TestFormatBigInt(magnitudes, spill boundaries, out-of-range, both format modes, value + pointer),TestBindBigInt(all three placeholder styles, arrays, tuples, maps, nil, and contrast cases that must stay quoted:string, anotherfmt.Stringer,decimal.Decimal),TestBindBigIntQueryParameter(server-side{name:Type}path).tests/issues/1917_test.go: integration test over Native + HTTP — parameter binds as a wide integer, round-trips exactly, matches anInt128column, and works via server-side query parameters.tests/std/1917_test.go: the same round-trip through thedatabase/sqlsurface on both protocols.Test
Verified against ClickHouse 26.5 that the new tests fail on
main(*big.Int→'…'→String;big.Intvalue →fmt.Sprintgarbage) and pass with the fix.toInt128('…')etc. confirmed to yield the exact value and the expected wide-integer type on the server.gofmt/go vetclean on changed files; full root-package unit suite green.Pre-PR validation gate
bind.go, pass with fix)CHANGELOG.mdis auto-generated, not hand-edited; regression test undertests/issues/;t.Cleanup+ version-gated server-side subtest)