Skip to content
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

Handle double pointers for Nullable columns when batch inserting #774

Merged
merged 3 commits into from
Oct 18, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion lib/column/nullable.go
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,15 @@ func (col *Nullable) Append(v interface{}) ([]uint8, error) {
}

func (col *Nullable) AppendRow(v interface{}) error {
if v == nil || (reflect.ValueOf(v).Kind() == reflect.Ptr && reflect.ValueOf(v).IsNil()) {
// Might receive double pointers like **String, because of how Nullable columns are read
// Unpack because we can't write double pointers
rv := reflect.ValueOf(v)
if v != nil && rv.Kind() == reflect.Pointer && !rv.IsNil() && rv.Elem().Kind() == reflect.Pointer {
v = rv.Elem().Interface()
rv = reflect.ValueOf(v)
}

if v == nil || (rv.Kind() == reflect.Pointer && rv.IsNil()) {
col.nulls.Append(1)
// used to detect sql.Null* types
} else if val, ok := v.(driver.Valuer); ok {
Expand Down
68 changes: 68 additions & 0 deletions tests/issues/777_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
package issues

import (
"context"
"github.com/ClickHouse/clickhouse-go/v2"
"github.com/ClickHouse/clickhouse-go/v2/lib/driver"
clickhouse_tests "github.com/ClickHouse/clickhouse-go/v2/tests"
"github.com/stretchr/testify/require"
"reflect"
"testing"
)

func TestInsertNullableString(t *testing.T) {
var (
conn, err = clickhouse_tests.GetConnection("issues", clickhouse.Settings{
"max_execution_time": 60,
}, nil, &clickhouse.Compression{
Method: clickhouse.CompressionLZ4,
})
)
ctx := context.Background()
require.NoError(t, err)
const ddl = `
CREATE TABLE test_nullable_string_insert (
Col1 String
, Col2 Nullable(String)
) Engine Memory
`
defer func() {
conn.Exec(ctx, "DROP TABLE IF EXISTS test_nullable_string_insert")
}()
require.NoError(t, conn.Exec(ctx, ddl))
const baseValues = `
INSERT INTO test_nullable_string_insert (Col1, Col2) VALUES ('Val1', 'Val2'), ('Val11', NULL)
`
require.NoError(t, conn.Exec(ctx, baseValues))

rows, err := conn.Query(ctx, "SELECT * FROM test_nullable_string_insert")
require.NoError(t, err)
defer func(rows driver.Rows) {
_ = rows.Close()
}(rows)

records := make([][]any, 0)
for rows.Next() {
record := make([]any, 0, len(rows.ColumnTypes()))
for _, ct := range rows.ColumnTypes() {
record = append(record, reflect.New(ct.ScanType()).Interface())
}
err = rows.Scan(record...)
require.NoError(t, err)

records = append(records, record)
}
require.Greater(t, len(records), 0)

// Try to insert records in the same format as queried above
batch, err := conn.PrepareBatch(ctx, "INSERT INTO test_nullable_string_insert")
require.NoError(t, err)

for _, r := range records {
err = batch.Append(r...)
require.NoError(t, err)
}

err = batch.Send()
require.NoError(t, err)
}
6 changes: 2 additions & 4 deletions tests/resources/custom.xml
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
<?xml version="1.0" ?>
<clickhouse>
<listen_host>::</listen_host>
<listen_host>0.0.0.0</listen_host>
Comment on lines 3 to -4
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://clickhouse.com/docs/en/operations/server-configuration-parameters/settings/#server_configuration_parameters-listen_host

<listen_host>::</listen_host> already means to listen on all hosts. No need to specify 0.0.0.0 as well (and is double specified).
Moved port definitions closer to listen host for easier spotting.

<listen_try>1</listen_try>
<https_port>8443</https_port>
<tcp_port_secure>9440</tcp_port_secure>
<logger>
<console>1</console>
</logger>
Expand Down Expand Up @@ -30,7 +31,4 @@
</client>
</openSSL>
<display_name>clickhouse</display_name>
<listen_host>0.0.0.0</listen_host>
<https_port>8443</https_port>
<tcp_port_secure>9440</tcp_port_secure>
</clickhouse>
11 changes: 7 additions & 4 deletions tests/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -155,9 +155,12 @@ func CreateClickHouseTestEnvironment(testSet string) (ClickHouseTestEnvironment,
Image: fmt.Sprintf("clickhouse/clickhouse-server:%s", GetClickHouseTestVersion()),
Name: fmt.Sprintf("clickhouse-go-%s-%d", strings.ToLower(testSet), time.Now().UnixNano()),
ExposedPorts: []string{"9000/tcp", "8123/tcp", "9440/tcp", "8443/tcp"},
WaitingFor: wait.ForAll(wait.ForLog("Ready for connections").WithStartupTimeout(time.Second*time.Duration(120)), wait.ForSQL("9000/tcp", "clickhouse", func(port nat.Port) string {
return fmt.Sprintf("clickhouse://default:ClickHouse@localhost:%s", port.Port())
})).WithStartupTimeout(time.Second * time.Duration(120)),
WaitingFor: wait.ForAll(
wait.ForLog("Ready for connections").WithStartupTimeout(time.Second*time.Duration(120)),
wait.ForSQL("9000/tcp", "clickhouse", func(port nat.Port) string {
return fmt.Sprintf("clickhouse://default:[email protected]:%s", port.Port())
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed localhost to 127.0.0.1 explicitly, because on IPv6 enabled systems this might be pointing to the IPv6 loopback address ::1:

$ cat /etc/hosts
127.0.0.1        localhost
127.0.1.1        housepaper
::1              localhost ip6-localhost ip6-loopback
ff02::1          ip6-allnodes
ff02::2          ip6-allrouters

In that scenario, startup of the container might timeout and ClickHouse only shows very strange error messages.

When debugging, it can be seen that the golang net.Dialer prefers the IPv6 address above the IPv4 one, though with the incorrect port. Using 127.0.0.1 makes it explicit IPv4.

image

image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And I think it's fine for now to use IPv4 explicitly.

I'll see with the Testcontainers project if its possible to create an issue / PR to solve this, specifying whether you want the IPv4 or IPv6 mapped port.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}),
).WithStartupTimeout(time.Second * time.Duration(120)),
Mounts: []testcontainers.ContainerMount{
testcontainers.BindMount(path.Join(basePath, "./resources/custom.xml"), "/etc/clickhouse-server/config.d/custom.xml"),
testcontainers.BindMount(path.Join(basePath, "./resources/admin.xml"), "/etc/clickhouse-server/users.d/admin.xml"),
Expand Down Expand Up @@ -185,7 +188,7 @@ func CreateClickHouseTestEnvironment(testSet string) (ClickHouseTestEnvironment,
HttpPort: hp.Int(),
SslPort: sslPort.Int(),
HttpsPort: hps.Int(),
Host: "localhost",
Host: "127.0.0.1",
// we set this explicitly - note its also set in the /etc/clickhouse-server/users.d/admin.xml
Username: "default",
Password: "ClickHouse",
Expand Down