Add support to `time` and `time64` datatypes #1669

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

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

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

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.

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 Copilot and SpencerTorres 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

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

Add support to time and time64 datatypes #1669

Are you sure you want to change the base?

Add support to time and time64 datatypes #1669

Uh oh!

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!

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!

Uh oh!

Add support to `time` and `time64` datatypes #1669

Add support to `time` and `time64` datatypes #1669