Support custom types (eg type customString string) #850

DarkDrim · 2022-12-13T20:22:39Z

Proposal: In each ScanRow() check if destination type is implementing sql.Scanner
This PR will add support of custom types, eg:

type myStr string
type myInt int32
type myFloat float64

One requirement: implement sql.Scanner interface.

Resolves #845

Also I found unreachable code:
lib/column/array
Contains unreachable code

CLAassistant · 2022-12-13T20:22:44Z

All committers have signed the CLA.

jkaflik · 2022-12-13T20:26:13Z

Hi @DarkDrim
Thanks for your contribution. Can you provide a set of tests? It's also helps to understand the use case better.

DarkDrim · 2022-12-13T20:28:41Z

Hi @DarkDrim Thanks for your contribution. Can you provide a set of tests? It's also helps to understand the use case better.

sure, do I need to remove unreachable code in array.go?

jkaflik · 2022-12-13T20:30:56Z

@DarkDrim that's not necessary, but your help is much appreciated. Thanks for letting us know

DarkDrim · 2022-12-13T20:58:38Z

tests/string_test.go

+	type data struct {
+		Col1 string    `ch:"Col1"`
+		Col2 customStr `ch:"Col2"`
+	}
+	require.NoError(t, batch.AppendStruct(&data{
+		Col1: "A",
+		Col2: "B",
+	}))
+	require.NoError(t, batch.Send())
+
+	var dest data
+	require.NoError(t, conn.QueryRow(ctx, "SELECT * FROM test_string").ScanStruct(&dest))
+	assert.Equal(t, "A", dest.Col1)
+	assert.Equal(t, customStr("B"), dest.Col2)


Added simple example: now it's possible to use custom string types (as in this example - customStr)
It could be useful when you have some struct with custom string/or other types
To not add some kind of converter that will do cast types.

It should works with array-type too.
For example when you have:
Array(String)

type CustomStrArr []string

yes. would you mind adding a custom array string test case?

yes. would you mind adding a custom array string test case?

Sure, added

DarkDrim · 2022-12-13T21:16:50Z

tests/enum_test.go

+			CREATE TABLE test_enum (
+				  Col1 Enum ('hello'   = 1,  'world' = 2),
+				  Col2 Enum8 ('hello'   = 1,  'world' = 2),
+				  Col3 Enum16 ('hello'   = 1,  'world' = 2)
+			) Engine MergeTree() ORDER BY tuple()
+		`
+	defer func() {
+		conn.Exec(ctx, "DROP TABLE IF EXISTS test_enum")
+	}()
+	require.NoError(t, conn.Exec(ctx, ddl))
+	batch, err := conn.PrepareBatch(ctx, "INSERT INTO test_enum")
+	require.NoError(t, err)
+	var (
+		col1Data = customStr("hello")
+		col2Data = customStr("world")
+		col3Data = customStr("hello")
+	)
+	require.NoError(t, batch.Append(
+		col1Data,
+		col2Data,
+		col3Data,
+	))
+	require.NoError(t, batch.Send())
+	var (
+		col1 customStr
+		col2 customStr
+		col3 customStr
+	)
+	require.NoError(t, conn.QueryRow(ctx, "SELECT * FROM test_enum").Scan(&col1, &col2, &col3))


Example # 2
Enum can be used with custom string types

DarkDrim · 2022-12-13T21:41:13Z

I want to add one more example with arrays
Will try to do it soon

DarkDrim · 2022-12-13T23:47:17Z

lib/column/enum16.go

+		if s, ok := elem.(fmt.Stringer); ok {
+			return col.AppendRow(s.String())
+		} else {


Additional check if element implement Stringer - we can use it too

DarkDrim · 2022-12-13T23:47:31Z

lib/column/enum8.go

+		if s, ok := elem.(fmt.Stringer); ok {
+			return col.AppendRow(s.String())
+		} else {


Additional check if element implement Stringer - we can use it too

DarkDrim · 2022-12-13T23:47:39Z

lib/column/fixed_string.go

+		if s, ok := v.(fmt.Stringer); ok {
+			return col.AppendRow(s.String())
+		} else {


Additional check if element implement Stringer - we can use it too

DarkDrim · 2022-12-13T23:49:56Z

lib/column/tuple.go

 	if field.Kind() == reflect.String {
-		field.Set(reflect.ValueOf(fmt.Sprint(value.Interface())))
-		return nil
+		if v := reflect.ValueOf(fmt.Sprint(value.Interface())); v.Type().AssignableTo(field.Type()) {
+			field.Set(v)
+			return nil
+		}


Super important:
see test TestCustomArray
in case we have custom type instead of string -> field.Set(v) , where field is a custom string type and v is string type - reflect package will panic
So to fix this possible issue firstly we're checking if value can be assignable to field (if not - just go to next condition and trying to convert val to field).

Will prepare go/play in a moment to show this issue

Additional explanation: https://go.dev/play/p/UrtEjsnZEG0

DarkDrim · 2022-12-13T23:50:59Z

tests/array_test.go

+type customArr []customStr
+
+func TestCustomArray(t *testing.T) {
+	conn, err := GetNativeConnection(nil, nil, &clickhouse.Compression{


example of usage # 3 - for column Array(Enum) we can use custom string type
It could be useful

DarkDrim · 2022-12-14T00:07:32Z

lib/column/array.go

-	return reflect.Value{}, &Error{
-		ColumnType: fmt.Sprint(sliceType.Kind()),
-		Err:        fmt.Errorf("column %s - needs a slice or interface{}", col.Name()),
-	}


Removed unreachable code

DarkDrim · 2022-12-14T00:07:35Z

lib/column/array.go

@@ -352,7 +348,6 @@ func (col *Array) scanSliceOfObjects(sliceType reflect.Type, row int) (reflect.V
 				Err:        fmt.Errorf("column %s - needs a slice of objects or an interface{}", col.Name()),
 			}
 		}
-		return reflect.Value{}, nil


Removed unreachable code

jkaflik

I like the changes. If possible, I'd add one or two test cases for generated columns.

DarkDrim · 2022-12-14T22:19:42Z

I like the changes. If possible, I'd add one or two test cases for generated columns.

Added support for base types
It took a little fiddling, but everything works well
Due to the fact that, for example, int to int64 conversion should fail - I had to add one more extra check

Also added a few tests, will comment them in a few moments

DarkDrim · 2022-12-14T22:23:06Z

lib/column/codegen/column.tpl

+		if rv := reflect.ValueOf(v); rv.Kind() == col.ScanType().Kind() && rv.CanConvert(col.ScanType()) {
+			col.col.Append(rv.Convert(col.ScanType()).Interface().({{ .GoType }}))
+		} else {
+			return &ColumnConverterError{
+				Op:   "AppendRow",
+				To:   "{{ .ChType }}",
+				From: fmt.Sprintf("%T", v),
+			}


Supporting base types:
I had to add additional check

rv.Kind() == col.ScanType().Kind()

because you have one test (SimpleInt) that should fail (int should not be converted into int64)

so how it works:

when we have type int64 and custom int64 type:

type customInt64 int64

var c customInt64
reflect.TypeOf(c).Kind() == reflect.Int64

that's why it works well

from documentation:

The second property is that the Kind of a reflection object describes the underlying type, not the static type. If a reflection object contains a value of a user-defined integer type, as in type MyInt int var x MyInt = 7 v := reflect.ValueOf(x) the Kind of v is still reflect.Int, even though the static type of x is MyInt, not int. In other words, the Kind cannot discriminate an int from a MyInt even though the Type can.

https://go.dev/blog/laws-of-reflection

DarkDrim · 2022-12-14T22:23:59Z

tests/array_test.go

+			  Col1 Array(Enum ('hello'   = 1,  'world' = 2)),
+			  Col2 Array(String)
+		) Engine MergeTree() ORDER BY tuple()


Updated test: added Array(Enum) and Array(String)
to show simple []customStrArray not only enum

DarkDrim · 2022-12-14T22:24:22Z

tests/base_types_test.go

@@ -40,6 +50,7 @@ func TestUInt8(t *testing.T) {
 				, Col2 Nullable(UInt8)
 				, Col3 Array(UInt8)
 				, Col4 Array(Nullable(UInt8))
+				, Col5 UInt8


Added new test with custom uint8 type

DarkDrim · 2022-12-14T22:24:48Z

tests/float_test.go

+		CREATE TABLE test_float (
+			  Col1 Float32,
+			  Col2 Float64
+		) Engine MergeTree() ORDER BY tuple()


Added new test with custom float32 and float64

DarkDrim

With strings, it turned out much easier, because we can simply check for the fmt.Stringer interface and, if it is defined, use it
For other types, this did not work out, so reflection is used, but this is not the only place with reflection in this CH-client))

so should not be a problem

Plus, reflection is used at the very end, when the client would simply return an error
Therefore, there is no problem with performance degradation of base types

DarkDrim · 2022-12-14T22:55:52Z

examples/clickhouse_api/custom_types.go

+func CustomTypes() error {
+	conn, err := GetNativeConnection(nil, nil, nil)
+	if err != nil {
+		return err
+	}
+	ctx := context.Background()
+	defer func() {
+		conn.Exec(context.Background(), "DROP TABLE example")
+	}()
+	if err := conn.Exec(ctx, `DROP TABLE IF EXISTS example`); err != nil {
+		return err
+	}
+	err = conn.Exec(ctx, `
+		CREATE TABLE IF NOT EXISTS example (
+			  Col1 String,
+			  Col2 Enum ('hello'   = 1,  'world' = 2),
+		) Engine = Memory
+	`)


Besides tests I added example how to works with custom types (in this example String and Enum)

Thank you. This is something I wanted to ask for. Will review this today. :)

jkaflik · 2022-12-16T12:24:21Z

@DarkDrim can you have a look at the failed test? don't care about integration-tests-cloud. it fails as run on your fork (you don't have access to secrets)

DarkDrim · 2022-12-16T12:51:02Z

@DarkDrim can you have a look at the failed test? don't care about integration-tests-cloud. it fails as run on your fork (you don't have access to secrets)

Sure will do today

DarkDrim · 2022-12-16T16:04:34Z

@DarkDrim can you have a look at the failed test? don't care about integration-tests-cloud. it fails as run on your fork (you don't have access to secrets)

@jkaflik Can you re-run CI please

jkaflik · 2022-12-16T21:10:01Z

This solves #753

jkaflik self-requested a review December 13, 2022 20:31

DarkDrim commented Dec 13, 2022

View reviewed changes

Add sql.Scanner for all column types

aede918

DarkDrim force-pushed the issue_845 branch from 2ef483e to aede918 Compare December 13, 2022 21:15

DarkDrim commented Dec 13, 2022

View reviewed changes

Add test with custom array

30a90c5

DarkDrim commented Dec 13, 2022

View reviewed changes

Removed unreachable code

b78defb

DarkDrim commented Dec 14, 2022

View reviewed changes

jkaflik approved these changes Dec 14, 2022

View reviewed changes

Support base types, add tests

b126d19

DarkDrim commented Dec 14, 2022

View reviewed changes

Add example with custom types

08fb79c

DarkDrim commented Dec 14, 2022

View reviewed changes

DarkDrim changed the title ~~Add sql.Scanner for all column types~~ Support custom types (eg type customString string) Dec 14, 2022

Fix test

aa017b6

jkaflik merged commit f62eefe into ClickHouse:main Dec 16, 2022

jkaflik mentioned this pull request Dec 16, 2022

Support for custom base types #753

Closed

DarkDrim deleted the issue_845 branch December 16, 2022 22:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support custom types (eg type customString string) #850

Support custom types (eg type customString string) #850

DarkDrim commented Dec 13, 2022 •

edited

Loading

CLAassistant commented Dec 13, 2022 •

edited

Loading

jkaflik commented Dec 13, 2022

DarkDrim commented Dec 13, 2022

jkaflik commented Dec 13, 2022 •

edited

Loading

DarkDrim Dec 13, 2022 •

edited

Loading

jkaflik Dec 14, 2022

DarkDrim Dec 14, 2022

DarkDrim Dec 13, 2022 •

edited

Loading

DarkDrim commented Dec 13, 2022

DarkDrim Dec 13, 2022

DarkDrim Dec 13, 2022

DarkDrim Dec 13, 2022

DarkDrim Dec 13, 2022

DarkDrim Dec 13, 2022

DarkDrim Dec 14, 2022

DarkDrim Dec 13, 2022

DarkDrim Dec 14, 2022

DarkDrim Dec 14, 2022

jkaflik left a comment

DarkDrim commented Dec 14, 2022

DarkDrim Dec 14, 2022

gingerwizard Dec 16, 2022

DarkDrim Dec 14, 2022

DarkDrim Dec 14, 2022

DarkDrim Dec 14, 2022

DarkDrim left a comment

DarkDrim Dec 14, 2022

jkaflik Dec 15, 2022

jkaflik commented Dec 16, 2022

DarkDrim commented Dec 16, 2022

DarkDrim commented Dec 16, 2022 •

edited

Loading

jkaflik commented Dec 16, 2022

Support custom types (eg type customString string) #850

Support custom types (eg type customString string) #850

Conversation

DarkDrim commented Dec 13, 2022 • edited Loading

CLAassistant commented Dec 13, 2022 • edited Loading

jkaflik commented Dec 13, 2022

DarkDrim commented Dec 13, 2022

jkaflik commented Dec 13, 2022 • edited Loading

DarkDrim Dec 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DarkDrim Dec 13, 2022 • edited Loading

Choose a reason for hiding this comment

DarkDrim commented Dec 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkaflik left a comment

Choose a reason for hiding this comment

DarkDrim commented Dec 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DarkDrim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkaflik commented Dec 16, 2022

DarkDrim commented Dec 16, 2022

DarkDrim commented Dec 16, 2022 • edited Loading

jkaflik commented Dec 16, 2022

DarkDrim commented Dec 13, 2022 •

edited

Loading

CLAassistant commented Dec 13, 2022 •

edited

Loading

jkaflik commented Dec 13, 2022 •

edited

Loading

DarkDrim Dec 13, 2022 •

edited

Loading

DarkDrim Dec 13, 2022 •

edited

Loading

DarkDrim commented Dec 16, 2022 •

edited

Loading