-
Notifications
You must be signed in to change notification settings - Fork 508
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
Add clickhouse-replicated dialect #809
base: master
Are you sure you want to change the base?
Conversation
var _ Querier = (*ClickhouseReplicated)(nil) | ||
|
||
func (c *ClickhouseReplicated) CreateTable(tableName string) string { | ||
q := `CREATE TABLE IF NOT EXISTS %s ON CLUSTER '{cluster}' ( |
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.
It is better to make the macro {cluster}
configurable, since it is not standard.
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.
Hi @funvit! Thanks for the feedback 👍
I understand the concern about the {cluster}
macro not being a standard. While it might not be universally recognized, it is widely used when deploying clusters via the Altinity ClickHouse Operator, and it’s also mentioned in the official ClickHouse documentation.
I believe using the {cluster}
macro is a good practice because it simplifies cluster management and cluster queries.
Regarding making it configurable, the reason I didn’t pursue that route is due to the current limitations in the implementation of dialects, which don’t support additional settings. To avoid significant changes, something I’d prefer to minimize when adding a new dialect, the most viable approach is to pass the cluster name via an environment variable, like CLICKHOUSE_REPLICATED_CLUSTER_NAME
, defaulting to {cluster}
.
What do you think - should we keep using the macro as is, or switch to an environment variable? Or maybe there's a better solution I haven't thought of?
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.
Perhaps this macro is standard or recommended, but in tests it would be easier to replace it with some custom value.
For example, now I had to implement Store for an older version of goose, and there I just made a Cluster field for ClickhouseReplicated.
This simplified the tests (using testcontainers), because it was no longer necessary to support the configuration of macros.
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 for explanation, it makes sense!
I'm added extra env var GOOSE_CLICKHOUSE_CLUSTER_NAME
which allows redefine cluster name and added clickhouse-replicated
dialect tests. PTAL
ee7131e
to
8a0af1a
Compare
8a0af1a
to
cf87405
Compare
This PR introduces a new clickhouse-replicated dialect, and here’s why it’s needed:
ReplicatedMergeTree
engine is required.ON CLUSTER
clause is a must forCREATE TABLE
queries.These differences between a single-node ClickHouse instance and a replicated cluster are why we’re adding this new dialect.
Added the environment variable
GOOSE_CLICKHOUSE_CLUSTER_NAME
(default:{cluster}
) allows to set Clickhouse cluster name