-
Notifications
You must be signed in to change notification settings - Fork 163
Support TIME64 data type for ClickHouse replication #3691
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
Conversation
flow/e2e/clickhouse_mysql_test.go
Outdated
|
|
||
| // If ClickHouse >= 25.6 and using latest internal version, should use Time64(3) | ||
| if major > 25 || (major == 25 && minor >= 6) { | ||
| if flowConnConfig.Version >= shared.InternalVersion_ClickHouseTime64 { |
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.
nit: we currently don't run CI against different versions of PeerDB/ClickHouse, could you add an explicit test case for an older version to verify backwards compatibility by setting something like flowConnConfig.Version = shared.InternalVersion_First
jgao54
left a comment
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.
small nit, lgtm 🚀
|
Heads up: had to revert this PR, tests were failing on main but for some reason were not triggered with this PR (maybe due to stale merge base?) Haven't gotten a chance to look at it in details yet. |
Adds support for ClickHouse
TIME64(3)data type when replicating TIME columns from PostgreSQL and MySQL sources.Changes:
InternalVersion_ClickHouseTime64for backward compatibilityTIMEtype toQValueKindTime(instead ofQValueKindTimestamp) for new mirrorsTime64(3)for ClickHouse 25.6+ when internal version >=InternalVersion_ClickHouseTime64DateTime64(6)for older ClickHouse versions or existing mirrorsTime64(3)columnsBackward Compatibility:
DateTime64(6)Time64(3)DateTime64(6)Fixes #3468