feat: add support for time/time64 #1596

shivanshuraj1333 · 2025-07-11T17:16:40Z

fixes: #1579

related to ClickHouse/ClickHouse#81217

depends on ClickHouse/ch-go#1074

Summary

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

CLAassistant · 2025-07-11T17:16:47Z

All committers have signed the CLA.

go.mod

shivanshuraj1333 · 2025-07-15T15:05:38Z

Hey @SpencerTorres when you get some time, can you please take a look at this one?

(also tagging you as some other PRs which can get some help from your reviews)

Thanks!

go.mod

shivanshuraj1333 · 2025-07-16T08:08:21Z

ClickHouse/ch-go#1074 is now, merged, this is now ready for review/merge.

SpencerTorres · 2025-07-17T01:21:25Z

Released ch-go v0.67.0 https://github.com/ClickHouse/ch-go/releases/tag/v0.67.0

I'll review once you get the mod file updated. Thanks!

shivanshuraj1333 · 2025-07-17T05:13:38Z

Done, bumped ch-go to v0.67.0

SpencerTorres

Hey! Thanks for submitting this.

The implementation itself looks good, but there's definitely some changes needed.

Apologies for the testing issues as well, I'm not sure if you have docker available but we have a really easy way of running integration tests to where you don't need to run your own instance. Let me know if you have any questions about this or any other notes I've left.

Overall I'd recommend you take a look at the other column types and integration tests.

Also it would be good to get some tests for the standard library database/sql interface. You can find those in tests/std. These are important for ensuring our driver works with other SQL wrappers.

SpencerTorres · 2025-07-17T21:50:08Z

lib/column/time.go

+	return col.name
+}
+
+func (col *Time) parse(t Type, tz *time.Location) (_ Interface, err error) {


Update function signature to the latest. I recently changed this on main, the parse function is now expected to accept a sc *ServerContext. You could then use sc.Timezone.

You could keep the param as timezone though, see my other comment about column.tpl.

Same for Time64

SpencerTorres · 2025-07-17T21:50:41Z

lib/column/time.go

+	return time.Time{}, fmt.Errorf("cannot parse time value: %s", value)
+}
+
+func (col *Time) WriteStatePrefix(buffer *proto.Buffer) error {


I believe this is an optional interface, along with ReadStatePrefix. You should be able to remove these.

Same for Time64

SpencerTorres · 2025-07-17T21:57:03Z

lib/proto/block.go

-					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
+				if columnType != "Time" && columnType != "Time64" {


What is this? How is Time and Time64 encoded where this is something that needs to be changed? I never saw what hasCustom is in the protocol, is this really the first column that has the bool set to true?

If we need to support some columns with custom encoding let's make an interface similar to CustomSerialization with WriteStatePrefix and ReadStatePrefix. Perhaps an interface that has a single AllowCustom() bool on it. If it returns false then we throw this error.

Also note that columnType could be complex such as Time64(precision), so Time and Time64 would never match if they had any parameters.

SpencerTorres · 2025-07-17T21:59:45Z

tests/time_test.go

+)
+
+func TestTimeBasic(t *testing.T) {
+	// Skip if no ClickHouse server is available


ClickHouse server is always available in this testing folder, these are integration tests. There are other places we could put this logic, let's not duplicate these skips everywhere.

If you're testing logic related to the column type itself, you can put it in the column package next to the source file. time_test.go and such.

SpencerTorres · 2025-07-17T22:03:41Z

tests/time_test.go

+	}
+
+	ctx := context.Background()
+	conn, err := clickhouse.Open(&clickhouse.Options{


You should reference variant_test or dynamic_test or json_test, these are also experimental/new similar to the time type.

This connection will not run on CI due to the server being unavailable. Our local machines and CI machines usually have docker so we can run multiple versions of ClickHouse through the same tests.

Some considerations/notes:

Use a setup function. This will be useful for skipping versions that don't have this column type.

The setup function can also be used to include the enable_time_timet64_type setting

Use the GetNativeConnection function as seen in the other tests

We must test TCP and HTTP connections, use TestProtocols wapper in each test as seen in the other tests

SpencerTorres · 2025-07-17T22:05:20Z

tests/time_test.go

+	require.NoError(t, conn.Exec(ctx, "DROP TABLE IF EXISTS test_time_registration"))
+
+	const ddl = `
+		CREATE TABLE test_time_registration (


This is good that the table names are different. This prevents session/table locking issues for HTTP tests. It's best that each test has its own table name

SpencerTorres · 2025-07-17T22:11:44Z

lib/column/time.go

+
+const defaultTimeFormat = "15:04:05"
+
+type Time struct {


You will need to add Time and Time64 to lib/column/codegen/column.tpl and run the codegen to re-generate column_gen.go. This will allow our column parser to automatically initialize the Time and Time64 types.

This is critical for actually adding the type, otherwise the column won't be recognized in the driver. It also helps other wrapper types like Array(Time) init the type.

shivanshuraj1333 changed the title ~~feat: time/time64 timezone test and example build errors~~ feat: add support for time/time64 Jul 11, 2025

shivanshuraj1333 marked this pull request as draft July 11, 2025 18:04

shivanshuraj1333 force-pushed the feat/issues/1579 branch 2 times, most recently from 303b18b to 58d8921 Compare July 12, 2025 08:42

shivanshuraj1333 marked this pull request as ready for review July 12, 2025 08:42

shivanshuraj1333 commented Jul 12, 2025

View reviewed changes

go.mod Outdated Show resolved Hide resolved

This was referenced Jul 12, 2025

feat: support Time/Time64 data types ClickHouse/ch-go#1070

Closed

Add support for Time and Time64 types #1579

Open

mshustov requested a review from SpencerTorres July 13, 2025 09:12

shivanshuraj1333 force-pushed the feat/issues/1579 branch from ca6efd1 to 73d50d0 Compare July 16, 2025 08:02

shivanshuraj1333 commented Jul 16, 2025

View reviewed changes

go.mod Outdated Show resolved Hide resolved

shivanshuraj1333 force-pushed the feat/issues/1579 branch from 25c62ce to 15a81a7 Compare July 17, 2025 04:56

SpencerTorres requested changes Jul 17, 2025

View reviewed changes

SpencerTorres mentioned this pull request Jul 17, 2025

feat: add ssh key authentication support #1602

Open

3 tasks

shivanshuraj1333 added 4 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

shivanshuraj1333 force-pushed the feat/issues/1579 branch from 99e5511 to aeadbc7 Compare July 20, 2025 08:40

feat: add support for time/time64 #1596

Are you sure you want to change the base?

feat: add support for time/time64 #1596

Uh oh!

Conversation

shivanshuraj1333 commented Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

CLAassistant commented Jul 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

shivanshuraj1333 commented Jul 15, 2025

Uh oh!

Uh oh!

shivanshuraj1333 commented Jul 16, 2025

Uh oh!

SpencerTorres commented Jul 17, 2025

Uh oh!

shivanshuraj1333 commented Jul 17, 2025

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!

Uh oh!

shivanshuraj1333 commented Jul 11, 2025 •

edited

Loading

CLAassistant commented Jul 11, 2025 •

edited

Loading