ClickHouse driver V2 support #697
Conversation
| query := "INSERT INTO " + ch.config.MigrationsTable + " (version, dirty, sequence) VALUES (?, ?, ?)" | ||
| if _, err := tx.Exec(query, version, bool(dirty), time.Now().UnixNano()); err != nil { | ||
| query := "INSERT INTO " + ch.config.MigrationsTable + " (version, dirty, sequence) VALUES" | ||
| batch, err := tx.Prepare(query) |
There was a problem hiding this comment.
Since V2 supports insert request only in "prepared statements" or in asynchronous mode
There was a problem hiding this comment.
Using numeric placeholders or named placeholder is also supported. Just FI.
| "net/url" | ||
| "testing" | ||
|
|
||
| _ "github.com/ClickHouse/clickhouse-go/v2" |
There was a problem hiding this comment.
Are the tests split to support both v1 and v2? What's involved in supporting both versions and why would we not update to v2 and drop support for v2? I'm not a fan of duplicate code.
There was a problem hiding this comment.
some people use golang-migrate as part of their application. Go driver V1 and V2 are not compatible and it will be impossible to import them in the same application
There was a problem hiding this comment.
@kshvakov I like the idea to support both for a limited time, on the other hand, for the CLI, we can only support one, so we have to decide if that should be v1 or v2.
| @@ -80,6 +80,11 @@ func (ch *ClickHouse) Open(dsn string) (database.Driver, error) { | |||
| return nil, err | |||
| } | |||
|
|
|||
| database := purl.Query().Get("database") | |||
| if db := strings.TrimPrefix(purl.Path, "/"); len(database) == 0 && len(db) != 0 { | |||
There was a problem hiding this comment.
Remove this. It's more confusing if there are 2 ways to specify the same thing. e.g. if the values differ, which one should take precedence?
There was a problem hiding this comment.
This is necessary for compatibility with both DSN formats in V1 and V2
|
I can remove V1 support, maybe it will even be better for new users |
|
hello, it was fixed in #707 |
|
Someone please merge this PR |
|
@sergey-telpuk #707 was not merged so can't say that it was fixed. Maybe you forgot to merge. |
yes, I'm waiting for the approval PR |
|
I did miss this PR and did something similar to you, except I didn't keep support for v1. Your change looks good for me and includes almost all my changes. I have just added some backquotes around table and database names, but I suppose people should not try the devil, so no need to update. I did use named place holders instead of preparing a request, but your solution is good too. Tested on Go 1.18. |
If you remove V1 support and resolve the conflicts, I'm more than happy to give it a look and merge it within days. 👍🏻 |
|
@kshvakov Please remove V1 support and resolve conflicts so that this can be reviewed and merged. |
|
It seems that #723 is a PR without V1. |
|
Any update on getting this merged? |
No description provided.