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

sql.Valuer support #1143

Closed
deankarn opened this issue Nov 17, 2023 · 2 comments · Fixed by #1144
Closed

sql.Valuer support #1143

deankarn opened this issue Nov 17, 2023 · 2 comments · Fixed by #1144

Comments

@deankarn
Copy link
Contributor

Describe the bug

It appears that sql.Valuer is only implemented for String and FixedString. String and FixedString also only check for the returned value from sql.Valuer.Value() call is a string instead of rechecking the supported types in the preceding logic.

Steps to reproduce

  1. Wrap any type such as Float64 in valuer supported type such as this Option generic type that implements the sql.Valuer type.
  2. Attempt to insert a record into the database.
  3. Error occurs about unsupported type.

Expected behaviour

The code to detect sql.Valuer, just like the code does for sql.Scanner when reading data out, to put data in.

Code example

type testDateTimeSerializer struct {
	val time.Time
}

func (c testDateTimeSerializer) Value() (driver.Value, error) {
	return c.val, nil
}

func (c *testDateTimeSerializer) Scan(src any) error {
	if t, ok := src.(time.Time); ok {
		*c = testDateTimeSerializer{val: t}
		return nil
	}
	return fmt.Errorf("cannot scan %T into testDateTimeSerializer", src)
}

func TestDateTimeValuer(t *testing.T) {
	conn, err := GetNativeConnection(nil, nil, &clickhouse.Compression{
		Method: clickhouse.CompressionLZ4,
	})
	ctx := context.Background()
	require.NoError(t, err)
	defer func() {
		conn.Exec(ctx, "DROP TABLE datetime_valuer")
	}()
	const ddl = `
		CREATE TABLE datetime_valuer (
			  Col1 DateTime
		) Engine MergeTree() ORDER BY tuple()
		`
	require.NoError(t, conn.Exec(ctx, ddl))
	batch, err := conn.PrepareBatch(ctx, "INSERT INTO datetime_valuer")
	require.NoError(t, err)
	vals := [1000]time.Time{}
	var now = time.Now()
	for i := 0; i < 1000; i++ {
		vals[i] = now.Add(time.Duration(i) * time.Hour).Truncate(time.Second)
		batch.Append(testDateTimeSerializer{val: vals[i]})
		require.Equal(t, 1, batch.Rows())
		batch.Flush()
	}
	require.Equal(t, 0, batch.Rows())
	batch.Send()
	rows, err := conn.Query(ctx, "SELECT * FROM datetime_valuer")
	require.NoError(t, err)
	i := 0
	for rows.Next() {
		var col1 time.Time
		require.NoError(t, rows.Scan(&col1))
		assert.Equal(t, vals[i].In(time.UTC), col1)
		i += 1
	}
}

Error log

return &ColumnConverterError{
	Op:   "AppendRow",
	To:   "DateTime",
	From: fmt.Sprintf("%T", v),
}

Configuration

Environment

  • Client version: v2.15.0
  • Language version: Go v1.21.4
  • OS: All
  • Interface: database/sql compatible driver

ClickHouse server

  • ClickHouse Server version: v23.8
  • ClickHouse Server non-default settings, if any: N/A
@jkaflik
Copy link
Contributor

jkaflik commented Nov 17, 2023

Small note: it's an enhancement, not a bug. Thanks for filing it out and for submitting a PR!

@deankarn
Copy link
Contributor Author

@jkaflik I suppose its a little of both.

An enhancement for those types that did not have it before, but a bug fix for String and FixedString because the returned values after sql.Valuer.Value() were not re-evaluated through the type checks again and assumed return value was either a string or error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants