Skip to content

Conversation

@NehharShah
Copy link
Contributor

Adds support for ClickHouse TIME64(3) data type when replicating TIME columns from PostgreSQL and MySQL sources.

Changes:

  • Bump internal version to InternalVersion_ClickHouseTime64 for backward compatibility
  • Map MySQL TIME type to QValueKindTime (instead of QValueKindTimestamp) for new mirrors
  • Use Time64(3) for ClickHouse 25.6+ when internal version >= InternalVersion_ClickHouseTime64
  • Fall back to DateTime64(6) for older ClickHouse versions or existing mirrors
  • Update normalization queries to handle Time64(3) columns

Backward Compatibility:

  • Existing mirrors continue using DateTime64(6)
  • New mirrors with ClickHouse >= 25.6 use Time64(3)
  • Older ClickHouse versions automatically fall back to DateTime64(6)

Fixes #3468

@NehharShah NehharShah requested a deployment to external-contributor November 12, 2025 21:30 — with GitHub Actions Waiting
@CLAassistant
Copy link

CLAassistant commented Nov 12, 2025

CLA assistant check
All committers have signed the CLA.

@NehharShah NehharShah temporarily deployed to external-contributor November 12, 2025 21:30 — with GitHub Actions Inactive
@NehharShah NehharShah temporarily deployed to external-contributor November 12, 2025 21:30 — with GitHub Actions Inactive

// 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 {
Copy link
Contributor

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

Copy link
Contributor

@jgao54 jgao54 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small nit, lgtm 🚀

@NehharShah NehharShah temporarily deployed to external-contributor November 13, 2025 13:26 — with GitHub Actions Inactive
@NehharShah NehharShah temporarily deployed to external-contributor November 13, 2025 13:26 — with GitHub Actions Inactive
@NehharShah NehharShah temporarily deployed to external-contributor November 13, 2025 13:26 — with GitHub Actions Inactive
@NehharShah NehharShah temporarily deployed to external-contributor November 13, 2025 13:26 — with GitHub Actions Inactive
@jgao54 jgao54 merged commit cc39d16 into PeerDB-io:main Nov 13, 2025
21 checks passed
jgao54 added a commit that referenced this pull request Nov 14, 2025
@jgao54
Copy link
Contributor

jgao54 commented Nov 14, 2025

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.

jgao54 added a commit that referenced this pull request Nov 14, 2025
…3696)

This reverts commit 1c865e9

This broke CI :( Need to see why it wasn't caught in
#3691 (it may be related to the
PR having a different merge base?)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support replicating TIME64 data type in ClickHouse

4 participants