Add support to `time` and `time64` datatypes by kavirajk · Pull Request #1669 · ClickHouse/clickhouse-go

kavirajk · 2025-09-25T09:11:46Z

Summary

This is built on top of other PR preserving original authors commits.

Checklist

Delete items not relevant to your PR:

Unit and integration tests covering the common scenarios were added
A human-readable description of the changes was provided to include in CHANGELOG
For significant changes, documentation in https://github.com/ClickHouse/clickhouse-docs was updated with further explanations or tutorials

1. Populate column_gen.go 2. Removed unused the interface implementations Signed-off-by: Kaviraj <[email protected]>

Signed-off-by: Kaviraj <[email protected]>

1. Removed the need for timezone information from these types (server explicitly states these types don't have timezone info) 2. Handle prefix correctly with time64(precision) 3. Handle default value for precision if not given 4. Update tests Signed-off-by: Kaviraj <[email protected]>

1. Also fixes the tests for timezone related tweaks Signed-off-by: Kaviraj <[email protected]>

Copilot

Pull Request Overview

This PR adds support for ClickHouse Time and Time64 datatypes to the Go driver. It introduces new column types that can handle time-of-day values with different precision levels, where Time stores seconds since midnight and Time64 supports precision up to nanoseconds.

Implementation of Time and Time64 column types with full encoding/decoding support
Addition of comprehensive test coverage for both types including array handling
Updates to documentation and type support matrix

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
lib/column/time.go	New Time column type implementation with second precision
lib/column/time64.go	New Time64 column type implementation with configurable nanosecond precision
lib/column/column_gen.go	Registration of Time and Time64 types in the column factory
lib/proto/block.go	Allow custom serialization for Time and Time64 columns
tests/time_test.go	Comprehensive test suite for Time and Time64 functionality
TYPES.md	Documentation updates showing Time/Time64 support in type matrix
lib/column/codegen/*.tpl	Template updates for code generation (license header removal)
conn_http_test.go	License header removal

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

tests/time_test.go

Copilot · 2025-09-28T11:14:03Z

tests/time_test.go

+		"enable_time_time64_type": 1,
+	}, nil, nil)
+	require.NoError(t, err)
+	if !CheckMinServerServerVersion(conn, 25, 6, 0) {


Function name has a duplicate 'Server' - should be 'CheckMinServerVersion' instead of 'CheckMinServerServerVersion'.

Suggested change

if !CheckMinServerServerVersion(conn, 25, 6, 0) {

if !CheckMinServerVersion(conn, 25, 6, 0) {

I think I've seen this before but was too deep in a different PR to fix it. Let's correct this typo at some point. Should be a quick and easy refactor

Ah thanks for the context. I will do it in follow up PR :) (this PR is already big enough I think)

lib/column/time64.go

lib/column/time.go

lib/column/time64.go

lib/column/time.go

Clickhouse server returns error if client tries to make any queries with unsupported settings. In HTTP protocol, to find server version (ServerVersion() api) we were using `enable_time_time64_type` as connection level settings and causing the server to return error (any version before 25.6) This PR don't pass any connection level settings to very the server version and only if server version is > 25.6 it uses the settings to do actual testing on time and time64 types Signed-off-by: Kaviraj <[email protected]>

Signed-off-by: Kaviraj <[email protected]>

SpencerTorres

Overall looks good, left some comments for things I had questions on. I understand this PR was copied from a different author. I appreciate your help ✨ enhancing ✨ it.

Let me know if you have any questions 👌

SpencerTorres · 2025-09-30T06:13:59Z

lib/column/codegen/column.tpl

 		return &SharedVariant{name: name}, nil
 	case "Object('json')":
 	    return &JSONObject{name: name, root: true, sc: sc}, nil
+	case "Time":


It looks like the changes here are inconsistent with what was generated. Was column_gen.go manually edited?

Also in column_gen.go it looks like Time has a prefix check, does it need a prefix check? I looked at the docs and it looks like Time is just Time. The only one that needs a prefix is Time64 since it has the precision defined

SpencerTorres · 2025-09-30T06:14:18Z

lib/column/column_gen.go

 		return (&DateTime{name: name}).parse(t, sc.Timezone)
+	case strings.HasPrefix(strType, "Time64"):
+		return (&Time64{name: name}).parse(t)
+	case strings.HasPrefix(strType, "Time"):


See comment on column.tpl

SpencerTorres · 2025-09-30T06:16:17Z

lib/column/time.go

+
+func (col *Time) parse(t Type) (_ Interface, err error) {
+	col.chType = t
+	if string(t) == "Time" {


Not sure if Time needs a parse function at all, you can probably initialize it without parsing. The reason we have parsing is for types like Tuple and Time64 where other syntax is included within the type string.

We don't need to return unsupported type error here since the column initialization function would never call parse if the string isn't already equal to Time

SpencerTorres · 2025-09-30T06:17:33Z

lib/column/time.go

+		*d = new(int64)
+		t := col.row(row)
+		**d = timeToSeconds(t)
+	case *sql.NullTime:


I see NullTime here but it makes me wonder if there's a regular SQL Time type or if that's just the Go type

It's part of sql package. It think it is there to handle cases like

var s sql.NullTime db.QueryRow("SELECT time FROM foo WHERE id=?", id).Scan(&s) if s.Valid() { // non-null // do something }

Similar to how we already handle for string type in clickhouse-go

SpencerTorres · 2025-09-30T06:19:22Z

lib/column/time.go

+	switch v := v.(type) {
+	case int64:
+		seconds := v
+		hours := seconds / 3600


It looks like this math is copied around a bunch. Can we make a function or struct that does it? Maybe a function with multiple return values?

SpencerTorres · 2025-09-30T06:26:05Z

lib/column/time64.go

+		return time.Time{}, nil
+	}
+
+	formats := []string{


Is this list/function duplicated from Time? Perhaps Time64 can borrow some helper functions from Time

SpencerTorres · 2025-09-30T06:28:56Z

lib/proto/block.go

-				return &BlockError{
-					Op:  "Decode",
-					Err: errors.New(fmt.Sprintf("custom serialization for column %s. not supported by clickhouse-go driver", columnName)),
+				// Allow Time and Time64 columns with custom serialization


I am so incredibly suspicious of this line, I saw it in the first PR and was concerned. Is this really the first column type that has this hasCustom flag set?

I don't recall if I wrote it in a comment on that PR but perhaps we can have an interface on the column type to declare whether it hasCustom or not, because I don't like the idea of these types being hardcoded here.

Does this even match on Time64(3) ? Is this necessary? Let's ask the core team to confirm, or maybe try a hexdump of Native format to see if this flag is really being set.

SpencerTorres · 2025-09-30T06:29:42Z

tests/time_test.go

+		"enable_time_time64_type": 1,
+	}, nil, nil)
+	require.NoError(t, err)
+	if !CheckMinServerServerVersion(conn, 25, 6, 0) {


I think I've seen this before but was too deep in a different PR to fix it. Let's correct this typo at some point. Should be a quick and easy refactor

SpencerTorres · 2025-09-30T06:31:11Z

TYPES.md

+colTime64.col.WithPrecision(9)
+
+// Append values
+colTime.AppendRow(time.Date(2024, 7, 11, 12, 34, 56, 0, time.UTC))


Let's make sure the docs are super clear about how the date component + timezone behaves. It's not immediately clear from here that it starts at 1970 or whatever we talked about

The more I think about this, the more I realize, time.Time is not the correct Go type for this time and time64 CH type. Unwanted complexities with Date (year, month, day, timezone) aspect of it.

The ideal Go type would be I think is time.Duration. Which includes only time without date component.

This exactly align with time and time64 docs

Also Rust library uses time::Duration

also Go definition of time.Duration seems to reflect the same purpose.

const ( Nanosecond Duration = 1 Microsecond = 1000 * Nanosecond Millisecond = 1000 * Microsecond Second = 1000 * Millisecond Minute = 60 * Second Hour = 60 * Minute )

We only have to make sure that time has precision max up to only milliseconds. We don't have to fight with default values for year, month, day and timezone aspect of it. Which can leads to some subtle logical errors depends on how users use those apis.

I think sticking to "Make your APIs easy to use correctly, and make it hard to use in-correctly" philosophy will help here :)

wdyt @SpencerTorres ?

Looks like it doesn't need lots of change(may be i'm wrong 😆 ). In fact may lead to removing lots of code from both ch-go and clickhouse-go. I'm going to try with time.Duration type and see what happens.

This sounds much more correct, let's get ch-go updated too then. ch-go technically allows for these breaking changes with its versioning.

I'd say take all the code from here and ch-go and make it use time.Duration, it is much more fitting as you said.

SpencerTorres · 2025-09-30T06:36:37Z

tests/time_test.go

+	return ctx, func() {}, conn
+}
+
+func TestTimeAndTime64(t *testing.T) {


For the sake of test coverage and isolating issues with these types, I would suggest splitting these into separate tests, maybe even separate files since Time is different than Time64. I would use Date/DateTime/DateTime64 as reference (unless those are all in one test too, haven't looked recently)

Also considering the feature is experimental, the setup function is the correct way to go, glad to see it being used.

Also also, and I know it's annoying to add, but we need to add coverage for the standard library database/sql tests. These are in a different folder and are very similar to these tests. Be aware that if you're testing HTTP with the standard library connector it may need you to set the session_id if you're modifying the experimental setting.

I would reference Variant/Dynamic/JSON as well as DateTime/DateTime64 for these. You can copy/paste/rework the stdlib tests from that folder. Make sure we have all the right sanity checks for things like arrays, matching input/output, etc.

Time type doesn't need parse, because it doesn't take any additional params Signed-off-by: Kaviraj <[email protected]>

Signed-off-by: Kaviraj <[email protected]>

Changes: 1. Vendor the un-released version of ch-go to use the changes 2. Remove lots of helpers and parsing code that is unrelated to timezone, day, month and year 3. Update tests to validate the changes Signed-off-by: Kaviraj <[email protected]>

Signed-off-by: Kaviraj <[email protected]>

kavirajk · 2025-10-03T22:15:56Z

go.mod


 require (
-	github.com/ClickHouse/ch-go v0.68.0
+	github.com/ClickHouse/ch-go v0.68.1-0.20251003160956-f6cfd63ac24c


FYI: This points to this PR commit in ch-go. Once it's merged we can point main (or after releasing point to specific newer version)

Signed-off-by: Kaviraj <[email protected]>

SpencerTorres

Looks usable, in a followup PR we should add tests for the stdlib. They're usually the same, but there's some quirks sometimes

shivanshuraj1333 and others added 11 commits July 20, 2025 13:56

feat: time and time64 column support

621a570

feat: add time and time64 support to codegen and column_gen.go

8eba9b9

test: add tests for time and time64 with setup and protocol wrappers

cf78e2a

chore: document time and time64 go type mappings in types.md

aeadbc7

Merge branch 'main' into feat/issues/1579

7c74e19

Update list

8b7ef7c

Couple of additions

3b9562e

1. Populate column_gen.go 2. Removed unused the interface implementations Signed-off-by: Kaviraj <[email protected]>

Merge branch 'main' into kavirajk/time-time64-support

64d22b6

remove the need for license headers even in the template generators

ee17487

Signed-off-by: Kaviraj <[email protected]>

Fix column generate for Time type

dc62229

1. Also fixes the tests for timezone related tweaks Signed-off-by: Kaviraj <[email protected]>

kavirajk marked this pull request as ready for review September 26, 2025 12:03

mshustov requested review from SpencerTorres and Copilot September 28, 2025 11:11

Copilot AI reviewed Sep 28, 2025

View reviewed changes

kavirajk added 2 commits September 28, 2025 22:26

Fix some TYPES.md related docs and remarks

47dc5e9

Signed-off-by: Kaviraj <[email protected]>

SpencerTorres requested changes Sep 30, 2025

View reviewed changes

kavirajk added 9 commits October 1, 2025 15:39

PR remark: Fix column.tpl correctly

ef43736

Time type doesn't need parse, because it doesn't take any additional params Signed-off-by: Kaviraj <[email protected]>

remove time.parse method

f7bc653

Signed-off-by: Kaviraj <[email protected]>

moving some repeated code to time helpers

b3bc01d

Signed-off-by: Kaviraj <[email protected]>

remove the need for custom serialization handling for time and time64

5325af3

Signed-off-by: Kaviraj <[email protected]>

vendoring custom ch-go to have updated time and time64 types

bd2fa17

Signed-off-by: Kaviraj <[email protected]>

Fix precision for time64 and split tests

5d8c820

Signed-off-by: Kaviraj <[email protected]>

Merge branch 'main' into kavirajk/time-time64-support

4b25e30

Signed-off-by: Kaviraj <[email protected]>

update docs in TYPES.md

52eac54

Signed-off-by: Kaviraj <[email protected]>

kavirajk commented Oct 3, 2025

View reviewed changes

kavirajk requested a review from SpencerTorres October 3, 2025 22:23

Merge branch 'main' into kavirajk/time-time64-support

18a497d

Signed-off-by: Kaviraj <[email protected]>

SpencerTorres approved these changes Oct 22, 2025

View reviewed changes

SpencerTorres merged commit 6ff07ce into main Oct 22, 2025
30 of 34 checks passed

kavirajk mentioned this pull request Jan 23, 2026

fix(time/time64): fix the ScanType of both time and time64 #1758

Merged

3 tasks

	if !CheckMinServerServerVersion(conn, 25, 6, 0) {
	if !CheckMinServerVersion(conn, 25, 6, 0) {

Conversation

kavirajk commented Sep 25, 2025

Summary

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Copilot AI Sep 28, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SpencerTorres left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kavirajk Oct 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SpencerTorres left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

kavirajk Oct 2, 2025 •

edited

Loading