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

version 2.6.1 numeric bind conflict with column named like "$360_name" #905

Closed
WillMeng opened this issue Feb 16, 2023 · 9 comments
Closed
Labels
bug feedback required reporter response needed

Comments

@WillMeng
Copy link

Issue description

We have table_schema like this:

Example code

CREATE TABLE default.text_local
(
    `chan_pub_id` Int64,

    `$360_name` Int64,

    `$360_call_num` Int64
)
ENGINE = MergeTree
PARTITION BY toYYYYMMDD(data_date)
PRIMARY KEY chan_pub_id
ORDER BY chan_pub_id;

insert into default.text_local (chan_pub_id, `$360_name`,  `$360_call_num`) value (
1, 1,1)

Error log

have no arg for $360 param

Configuration

*OS: centos 7.9

Interface: clickhouse interface (formally native interface)

*Driver version: 2.6.1

Go version: 1.20

*ClickHouse Server version: 22.3

@jkaflik
Copy link
Contributor

jkaflik commented Feb 16, 2023

That's an interesting case. The binding mechanism has depreciated since we introduced CH native query parameters support.

It gets disabled if you use native query parameters, but in your case, you are not interested in doing that.
We can introduce an additional query/client option that forces disable binding. WDYT?

@jkaflik jkaflik added the bug label Feb 16, 2023
@WillMeng
Copy link
Author

WillMeng commented Feb 17, 2023

Thank you.

  1. What would you mean about "CH native query parameters support"?
  2. "introduce an additional query/client option that forces disable binding", that is a good idea, by the way, you can introdece an enum option, ie "all" "numeric" "named" "Positional" "numeric && named"

That's an interesting case. The binding mechanism has depreciated since we introduced CH native query parameters support.

It gets disabled if you use native query parameters, but in your case, you are not interested in doing that. We can introduce an additional query/client option that forces disable binding. WDYT?

@WillMeng
Copy link
Author

@jkaflik

@jkaflik
Copy link
Contributor

jkaflik commented Feb 18, 2023

@WillMeng, there is a mechanism called query parameters natively supported by CH: https://clickhouse.com/docs/en/interfaces/cli#cli-queries-with-parameters

This doc is explaining how to use it with a CH CLI client. For a clickhouse-go, there is an example in repo: https://github.com/ClickHouse/clickhouse-go/blob/669fda2413a4d2233688a949ec9d5ebbb818c2b2/examples/std/query_parameters.go#L31-L30


I found this issue unreproducable. Can you post a code snippet? Here is my attempt to reproduce it with a test:

package issues

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

func Test905(t *testing.T) {
	var (
		conn, err = clickhouse_tests.GetConnection("issues", clickhouse.Settings{
			"max_execution_time": 60,
			"flatten_nested":     0,
		}, nil, &clickhouse.Compression{
			Method: clickhouse.CompressionLZ4,
		})
	)

	ctx := context.Background()
	require.NoError(t, err)
	env, err := clickhouse_tests.GetTestEnvironment(testSet)
	require.NoError(t, err)

	ddl := fmt.Sprintf("CREATE TABLE `%s`.`test_905` (`$360_name` String, `$360_call_num` Int32) Engine MergeTree() ORDER BY tuple()", env.Database)
	defer func() {
		conn.Exec(ctx, fmt.Sprintf("DROP TABLE IF EXISTS `%s`.`test_905`", env.Database))
	}()
	require.NoError(t, conn.Exec(ctx, ddl))

	err = conn.Exec(ctx, fmt.Sprintf("INSERT INTO `%s`.`test_905` (`$360_name`, `$360_call_num`) VALUES ('foo', 5)", env.Database))
	require.NoError(t, err)
}

@jkaflik jkaflik added the feedback required reporter response needed label Feb 20, 2023
@WillMeng
Copy link
Author

WillMeng commented Feb 21, 2023

err = conn.Exec(ctx, fmt.Sprintf("INSERT INTO `%s`.`test_905` (`$360_name`, `$360_call_num`) VALUES ($1, $2)", env.Database), 'foo', 5)

@WillMeng
Copy link
Author

@jkaflik Thank you

@jkaflik
Copy link
Contributor

jkaflik commented Feb 21, 2023

@WillMeng thanks. I see what's the deal here.

This is only reproducible if you provide an args and have a positional bind. What I can suggest you to do, is to adapt your code to use native query parameters:

	execCtx := clickhouse.Context(ctx, clickhouse.WithParameters(clickhouse.Parameters{
		"name":    "foo",
		"callNum": "5",
	}))

	err = conn.Exec(execCtx, fmt.Sprintf("INSERT INTO `%s`.`test_905` (`$360_name`, `$360_call_num`) VALUES ({name:String}, {callNum:Int32})", env.Database))

or by using NamedArg:

	err = conn.Exec(
		ctx,
		fmt.Sprintf("INSERT INTO `%s`.`test_905` (`$360_name`, `$360_call_num`) VALUES ({name:String}, {callNum:Int32})", env.Database),
		clickhouse.Named("name", "foo"),
		clickhouse.Named("callNum", "5"),
	)

Also, you can consider using conn.PrepareBatch for ingestion. There is no need for binding/query parameters in that case.


Like I said, binding mechanism is no longer supported, so no further improvements are planned.

@WillMeng
Copy link
Author

@WillMeng thanks. I see what's the deal here.

This is only reproducible if you provide an args and have a positional bind. What I can suggest you to do, is to adapt your code to use native query parameters:

	execCtx := clickhouse.Context(ctx, clickhouse.WithParameters(clickhouse.Parameters{
		"name":    "foo",
		"callNum": "5",
	}))

	err = conn.Exec(execCtx, fmt.Sprintf("INSERT INTO `%s`.`test_905` (`$360_name`, `$360_call_num`) VALUES ({name:String}, {callNum:Int32})", env.Database))

or by using NamedArg:

	err = conn.Exec(
		ctx,
		fmt.Sprintf("INSERT INTO `%s`.`test_905` (`$360_name`, `$360_call_num`) VALUES ({name:String}, {callNum:Int32})", env.Database),
		clickhouse.Named("name", "foo"),
		clickhouse.Named("callNum", "5"),
	)

Also, you can consider using conn.PrepareBatch for ingestion. There is no need for binding/query parameters in that case.

Like I said, binding mechanism is no longer supported, so no further improvements are planned.

Thanks very much, I got it

@jkaflik
Copy link
Contributor

jkaflik commented Feb 21, 2023

@WillMeng, you are welcome. Feel free to reach out to me in case of any questions.

@jkaflik jkaflik closed this as completed Feb 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug feedback required reporter response needed
Projects
None yet
Development

No branches or pull requests

2 participants