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

http stdBatch AppendRow assumes table columns and insert field order is the same #741

Closed
wizzard0 opened this issue Sep 6, 2022 · 11 comments · Fixed by #790
Closed

http stdBatch AppendRow assumes table columns and insert field order is the same #741

wizzard0 opened this issue Sep 6, 2022 · 11 comments · Fixed by #790
Assignees
Labels
Milestone

Comments

@wizzard0
Copy link
Contributor

wizzard0 commented Sep 6, 2022

Issue description

When insert order and column order is different, the driver tries to match them by index and fails on the first incompatible column type

So when the prepared statement with INSERT INTO table1 (col2int, col1string) VALUES (? ?) gets optimized into INSERT INTO table1 FORMAT Native the insert fails.

Workaround: specify fields in the order exactly as is currently in the table

Proposed fix: don't add fields in prepareBatch as-is but respect the field order (and what fields are present) from the query. See https://github.com/ClickHouse/clickhouse-go/blob/main/conn_http_batch.go#L65

Steps to reproduce

  1. CREATE TABLE table1 (col1string String, col2int Int64)
  2. prepare the statement INSERT INTO table1 (col2int, col1string) VALUES (? ?)
  3. execute stmt.Exec(5, "abc")
  4. observe the error

Error log

clickhouse [AppendRow]: build clickhouse [AppendRow]: converting int64 to String is unsupported

Configuration

OS: osx monterey

Interface: database/sql

Driver version: clickhouse-go 2.3.0

Go version: 1.18.5

ClickHouse Server version: 22.8.1.1316

@gingerwizard gingerwizard added this to the 2.4.0 milestone Sep 13, 2022
@gingerwizard
Copy link
Collaborator

Nice catch, will fix in 2.4.0.

@wizzard0
Copy link
Contributor Author

...a similar issue happens if you try to insert only a subset of columns, the error is different but the cause is the same

  • INSERT INTO table1 (col1string) VALUES (?)
  • stmt.Exec(5)
clickhouse [Append]:  clickhouse: expected 2 arguments, got 1  

@gingerwizard gingerwizard self-assigned this Sep 29, 2022
@gingerwizard
Copy link
Collaborator

This is quite tricky in native since we effectively send INSERT INTO issue_741 (Col2, Col1) VALUES to ClickHouse and it responds with the block it expects - this has the columns in the order of the table, not the insert statement. Http is easier as indicated above but we should be consistent for protocols. One option is we store the order of the columns with the batch and align on Exec.

I'll PR.

@gingerwizard
Copy link
Collaborator

i think we'll sort the block according to the requested order. PR today.

@wizzard0
Copy link
Contributor Author

responds with the block it expects

and what would happen if there's only a subset of the columns?

@gingerwizard
Copy link
Collaborator

responds with just those - this is native protocol behavior though. HTTP is different and will need a different approach.

@wizzard0
Copy link
Contributor Author

wizzard0 commented Oct 19, 2022

just to be clear: this particular issue is about the HTTP wire protocol.

I don't know what "FORMAT Native" means in the HTTP request clickhouse-go makes but it inserts successfully if all rows are present and ordered the same as the table definition.

I can't reach that particular ClickHouse instance via native TCP

@gingerwizard
Copy link
Collaborator

@wizzard0 issue existed on both protocols - native and HTTP.

#790
fixes

@gingerwizard
Copy link
Collaborator

there is native format and native protocol. We always use native format, but user can choose between native protocol and http.

@wizzard0
Copy link
Contributor Author

Ah I see, thanks for the explanation

@wizzard0
Copy link
Contributor Author

Had an opportunity to test, can confirm it works with 2.4.3

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

Successfully merging a pull request may close this issue.

2 participants