-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
chore(deps)!: Bump github.com/ClickHouse/clickhouse-go from v1.5.4 to v2.18.0 #14877
Conversation
Download PR build artifacts for linux_amd64.tar.gz, darwin_arm64.tar.gz, and windows_amd64.zip. 📦 Click here to get additional PR build artifactsArtifact URLs |
// exec sequence on query later. | ||
if s.driverName == "clickhouse" { | ||
return | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did this work wtih the v1 library?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so as the test did not complain. We can work on doing it (we already do have a special handling on the sql output plugin). The biggest issue is that Query()
and Exec()
will return two completely different data-types and we would need to implement this for only ClickHouse... Would you prefer we are doing this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you prefer we are doing this?
I'm not sure. From a let's not break anything that already works standpoint, then yes, but that seems quite involved. Are any of the other clickhouse libraries support this better out of the box?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is in the SQL driver. See upstream bug ClickHouse/clickhouse-go#1203. I suggest we are waiting for this one to get some answers...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks - I agree on holding.
We will close this for now until we get a better idea of how we could migrate to v2 or some other library in the future. |
Summary
Version 1 of the ClickHouse SQL driver is no longer actively maintained. Therefore update to the new version 2.
Note: The DSN of v1 and v2 differs slightly, you might need to adapt your settings!
Checklist
Related issues