Interpolate: keep ClickHouse whole-number floats and non-finite values typed correctly - #244
Open
youdie006 wants to merge 1 commit into
Open
Interpolate: keep ClickHouse whole-number floats and non-finite values typed correctly#244youdie006 wants to merge 1 commit into
youdie006 wants to merge 1 commit into
Conversation
…s typed correctly In encodeValue the Float32/Float64 cases rendered floats with strconv.AppendFloat(..., 'g', -1, N) and ignored flavor, unlike the adjacent flavor-aware String/Slice cases. For ClickHouse this had two symptoms: a whole-number float such as float64(1) rendered as bare '1' (ClickHouse infers an integer type and silently narrows a Float column), and NaN/+Inf/-Inf rendered in Go's mixed case (ClickHouse only accepts lowercase nan/inf/-inf). Route both cases through a new encodeFloat helper that, for ClickHouse only, emits lowercase non-finite spellings and appends '.0' when strconv produced a bare integer (detected via bytes.ContainsAny(rendered, ".eE") so 1e+21 is left untouched). Every other flavor keeps its exact existing rendering. Fixes huandu#243
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request fixes ClickHouse-specific float literal interpolation so float arguments remain typed as floats (avoiding whole-number narrowing) and non-finite values parse correctly in ClickHouse SQL.
Changes:
- Route float interpolation through a new
encodeFloathelper that is flavor-aware. - For ClickHouse only: emit lowercase
nan/inf/-infand append.0to whole-number float renderings. - Add targeted tests validating ClickHouse float literal behavior and a non-ClickHouse control.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| interpolate.go | Adds encodeFloat and updates float cases in encodeValue to preserve ClickHouse float typing and non-finite parsing. |
| interpolate_ch_float_test.go | Adds regression tests for ClickHouse whole-number floats and non-finite float literals, plus a MySQL control case. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
Fixes #243.
Problem
In
encodeValue(interpolate.go), thereflect.Float32/reflect.Float64cases rendered floats withstrconv.AppendFloat(..., 'g', -1, N)and ignoredflavor, unlike the adjacent flavor-aware String and byte-slice cases. Two symptoms result for ClickHouse:float64(1)renders as the bare literal1. ClickHouse's SQL parser infers a bare1as an integer type (e.g.UInt8), so a value the caller intended asFloat64is sent typed as an integer. The narrowing only surfaces downstream -- a scan-back type mismatch, or changed arithmetic/promotion in the query itself.NaN/+Inf/-Infrender in Go's default mixed case. ClickHouse only accepts the lowercasenan/inf/-infand rejects the mixed-case spelling as an unknown identifier.The same bug was fixed in the official
ClickHouse/clickhouse-godriver in v2.48.0 (issue #1862 / PR #1894).Fix
Both float cases now route through a new
encodeFloat(buf, f, bitSize, flavor)helper. For ClickHouse only it emits lowercasenan/inf/-inf, and appends.0whenstrconvproduced a bare integer -- detected viabytes.ContainsAny(rendered, ".eE"), so a scientific-notation rendering like1e+21is left untouched. Every other flavor keeps its exact existing rendering. Existing ClickHouse interpolate tests use fractional floats and are unaffected.Note on approach: this widens whole numbers to the
2.0form rather than theclickhouse-gov2.48.0cast(N, 'Float64')wrapper.2.0is a lighter, parser-native float literal, but if you'd prefer the explicitcast(...)form for parity with the sibling driver I'm happy to switch -- let me know your preference.Test
Added
TestClickHouseFloatLiteralcoveringfloat64(1)->SELECT 1.0,float32(2)->SELECT 2.0,NaN/+Inf/-Inflowercasing, a fractional control (1.5->SELECT 1.5), and a non-ClickHouse control (MySQLfloat64(1)unchanged =SELECT 1).Red/green (reverting the fix): the whole-number and non-finite cases fail with e.g.
got "SELECT 1", want "SELECT 1.0"; with the fixgo test ./...passes.gofmtandgo vet ./...are clean.This contribution was prepared with AI assistance.