-
Notifications
You must be signed in to change notification settings - Fork 669
fix(bind): treat []byte as String and time.Duration as Time #1945
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -483,7 +483,9 @@ func formatTime(tz *time.Location, scale TimeUnit, value time.Time) (string, err | |
| return fmt.Sprintf("toDateTime64('%s', %d, '%s')", value.Format(fmt.Sprintf("2006-01-02 15:04:05.%0*d", int(scale*3), 0)), int(scale*3), escapedTimezone), nil | ||
| } | ||
|
|
||
| var stringQuoteReplacer = strings.NewReplacer(`\`, `\\`, `'`, `\'`) | ||
| // Escape order: backslash first so later replacements are not re-escaped. | ||
| // NUL is written as \0 so binary String values survive client-side bind. | ||
| var stringQuoteReplacer = strings.NewReplacer(`\`, `\\`, `'`, `\'`, "\x00", `\0`) | ||
|
|
||
| // formatMode says which syntax formatValue should produce. A value spliced | ||
| // into the query text needs SQL syntax; a server-side query parameter needs | ||
|
|
@@ -512,6 +514,30 @@ func format(tz *time.Location, scale TimeUnit, v any) (string, error) { | |
| return formatValue(tz, scale, v, formatSQL) | ||
| } | ||
|
|
||
| // formatDuration renders a time.Duration as a ClickHouse Time/Time64 text | ||
| // literal body: HH:MM:SS or HH:MM:SS.frac with trailing fractional zeros | ||
| // trimmed. time.Duration is the ScanType for Time/Time64 columns; without a | ||
| // dedicated case it would hit fmt.Stringer and emit Go's "14h30m0s" form. | ||
| func formatDuration(d time.Duration) string { | ||
| sign := "" | ||
| if d < 0 { | ||
| sign = "-" | ||
| d = -d | ||
| } | ||
| hours := d / time.Hour | ||
| d -= hours * time.Hour | ||
| mins := d / time.Minute | ||
| d -= mins * time.Minute | ||
| secs := d / time.Second | ||
| frac := d % time.Second | ||
| if frac == 0 { | ||
| return fmt.Sprintf("%s%02d:%02d:%02d", sign, hours, mins, secs) | ||
| } | ||
| fracStr := fmt.Sprintf("%09d", frac.Nanoseconds()) | ||
| fracStr = strings.TrimRight(fracStr, "0") | ||
| return fmt.Sprintf("%s%02d:%02d:%02d.%s", sign, hours, mins, secs, fracStr) | ||
| } | ||
|
|
||
| // formatValue turns v into a string in the given mode. The mode carries down | ||
| // into nested values, so a bool or map keeps its formatting at any depth. | ||
| // | ||
|
|
@@ -528,6 +554,11 @@ func formatValue(tz *time.Location, scale TimeUnit, v any, mode formatMode) (str | |
| return "NULL", nil | ||
| case string: | ||
| return quote(v), nil | ||
| case []byte: | ||
| // []byte is a valid database/sql driver.Value and the natural Go type | ||
| // for a ClickHouse String holding arbitrary bytes. Without this case | ||
| // it falls through to reflect.Slice and becomes Array(UInt8). | ||
| return quote(string(v)), nil | ||
| case time.Time: | ||
| if mode == formatParamText { | ||
| return quote(formatTimeParam(v)), nil | ||
|
|
@@ -541,6 +572,14 @@ func formatValue(tz *time.Location, scale TimeUnit, v any, mode formatMode) (str | |
| return quote(formatTimeParam(*v)), nil | ||
| } | ||
| return formatTime(tz, scale, *v) | ||
| case time.Duration: | ||
| // Must precede fmt.Stringer: Duration.String() is Go's "14h30m0s". | ||
| return quote(formatDuration(v)), nil | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in 8393cdc, please take another look. |
||
| case *time.Duration: | ||
| if v == nil { | ||
| return "NULL", nil | ||
| } | ||
| return quote(formatDuration(*v)), nil | ||
| case bool: | ||
| if mode == formatParamText { | ||
| if v { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -983,3 +983,68 @@ func BenchmarkBindNamed(b *testing.B) { | |
| } | ||
| } | ||
| } | ||
|
|
||
| // TestBindDuration checks that time.Duration (the ScanType for ClickHouse | ||
| // Time/Time64) binds as a Time-parseable literal, not Go's duration string. | ||
| func TestBindDuration(t *testing.T) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add tests with Time64 with different precision.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in 8393cdc, please take another look. |
||
| q, err := bind(time.UTC, "SELECT toTime(?)", 14*time.Hour+30*time.Minute) | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, "SELECT toTime('14:30:00')", q) | ||
|
|
||
| // *time.Duration and zero / fractional / negative values | ||
| d := 1*time.Second + 250*time.Millisecond | ||
| q, err = bind(time.UTC, "SELECT ?", &d) | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, "SELECT '00:00:01.25'", q) | ||
|
|
||
| q, err = bind(time.UTC, "SELECT ?", time.Duration(0)) | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, "SELECT '00:00:00'", q) | ||
|
|
||
| q, err = bind(time.UTC, "SELECT ?", -90*time.Second) | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, "SELECT '-00:01:30'", q) | ||
|
|
||
| var nilDur *time.Duration | ||
| q, err = bind(time.UTC, "SELECT ?", nilDur) | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, "SELECT NULL", q) | ||
| } | ||
|
|
||
| // TestBindBytes checks that []byte binds as a String literal (with the same | ||
| // escaping as string), not as Array(UInt8). | ||
| func TestBindBytes(t *testing.T) { | ||
| q, err := bind(time.UTC, "SELECT ?", []byte("A\x00B")) | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, "SELECT 'A\\0B'", q) | ||
|
|
||
| q, err = bind(time.UTC, "SELECT ?", []byte(`a'b\c`)) | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, `SELECT 'a\'b\\c'`, q) | ||
|
|
||
| q, err = bind(time.UTC, "SELECT ?", []byte{}) | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, "SELECT ''", q) | ||
|
|
||
| // nested: Array(String) of binary strings, not Array(Array(UInt8)) | ||
| q, err = bind(time.UTC, "SELECT ?", [][]byte{[]byte("x"), []byte("y")}) | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, "SELECT ['x', 'y']", q) | ||
| } | ||
|
|
||
| func TestFormatDuration(t *testing.T) { | ||
| cases := []struct { | ||
| in time.Duration | ||
| want string | ||
| }{ | ||
| {0, "00:00:00"}, | ||
| {14*time.Hour + 30*time.Minute, "14:30:00"}, | ||
| {1*time.Second + 250*time.Millisecond, "00:00:01.25"}, | ||
| {123 * time.Nanosecond, "00:00:00.000000123"}, | ||
| {-90 * time.Second, "-00:01:30"}, | ||
| {25 * time.Hour, "25:00:00"}, | ||
| } | ||
| for _, tc := range cases { | ||
| assert.Equal(t, tc.want, formatDuration(tc.in), "in=%v", tc.in) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add single quotes as
formatTimeputs them - API should be consistent.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 8393cdc, please take another look.