-
Notifications
You must be signed in to change notification settings - Fork 548
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
Support custom types (eg type customString string) #850
Conversation
Hi @DarkDrim |
sure, do I need to remove unreachable code in array.go? |
@DarkDrim that's not necessary, but your help is much appreciated. Thanks for letting us know |
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) |
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.
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
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.
yes. would you mind adding a custom array string test case?
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.
yes. would you mind adding a custom array string test case?
Sure, added
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)) |
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.
Example # 2
Enum can be used with custom string types
I want to add one more example with arrays |
if s, ok := elem.(fmt.Stringer); ok { | ||
return col.AppendRow(s.String()) | ||
} else { |
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.
Additional check if element implement Stringer - we can use it too
if s, ok := elem.(fmt.Stringer); ok { | ||
return col.AppendRow(s.String()) | ||
} else { |
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.
Additional check if element implement Stringer - we can use it too
if s, ok := v.(fmt.Stringer); ok { | ||
return col.AppendRow(s.String()) | ||
} else { |
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.
Additional check if element implement Stringer - we can use it too
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 | ||
} |
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.
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).
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.
Will prepare go/play in a moment to show this issue
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.
Additional explanation: https://go.dev/play/p/UrtEjsnZEG0
type customArr []customStr | ||
|
||
func TestCustomArray(t *testing.T) { | ||
conn, err := GetNativeConnection(nil, nil, &clickhouse.Compression{ |
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.
example of usage # 3 - for column Array(Enum) we can use custom string type
It could be useful
return reflect.Value{}, &Error{ | ||
ColumnType: fmt.Sprint(sliceType.Kind()), | ||
Err: fmt.Errorf("column %s - needs a slice or interface{}", col.Name()), | ||
} |
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.
Removed unreachable code
@@ -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 |
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.
Removed unreachable code
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.
I like the changes. If possible, I'd add one or two test cases for generated columns.
Added support for base types Also added a few tests, will comment them in a few moments |
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), | ||
} |
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.
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.
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.
nice
Col1 Array(Enum ('hello' = 1, 'world' = 2)), | ||
Col2 Array(String) | ||
) Engine MergeTree() ORDER BY tuple() |
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.
Updated test: added Array(Enum) and Array(String)
to show simple []customStrArray not only enum
@@ -40,6 +50,7 @@ func TestUInt8(t *testing.T) { | |||
, Col2 Nullable(UInt8) | |||
, Col3 Array(UInt8) | |||
, Col4 Array(Nullable(UInt8)) | |||
, Col5 UInt8 |
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.
Added new test with custom uint8 type
CREATE TABLE test_float ( | ||
Col1 Float32, | ||
Col2 Float64 | ||
) Engine MergeTree() ORDER BY tuple() |
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.
Added new test with custom float32 and float64
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.
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
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 | ||
`) |
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.
Besides tests I added example how to works with custom types (in this example String and Enum)
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.
Thank you. This is something I wanted to ask for. Will review this today. :)
@DarkDrim can you have a look at the failed test? don't care about |
Sure will do today |
This solves #753 |
Proposal: In each ScanRow() check if destination type is implementing sql.Scanner
This PR will add support of custom types, eg:
One requirement: implement sql.Scanner interface.
Resolves #845
Also I found unreachable code:
lib/column/array
Contains unreachable code