Skip to content

fix!: Use an arrow date type for clickhouse dates #18914

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.

Sign up for GitHub

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

Jump to bottom
Merged
kodiakhq merged 3 commits into from
Aug 13, 2024
Merged

fix!: Use an arrow date type for clickhouse dates #18914

kodiakhq merged 3 commits into from
Aug 13, 2024

Conversation

@savme
Copy link
Contributor

@savme savme commented Aug 12, 2024 •
edited
Loading

I'm not sure what the reasoning was behind representing clickhouse Date as a string.

However, for me it resulted in a panic when using the clickhouse source:

5:06PM INF started call grpc.component=server grpc.method=Init grpc.method_type=unary grpc.service=cloudquery.plugin.v3.Plugin grpc.start_time=2024-08-12T17:06:00+02:00 grpc.time_ms=0.016 peer.address=127.0.0.1:54871 protocol=grpc
5:06PM INF finished call grpc.code=OK grpc.component=server grpc.method=Init grpc.method_type=unary grpc.service=cloudquery.plugin.v3.Plugin grpc.start_time=2024-08-12T17:06:00+02:00 grpc.time_ms=190.086 peer.address=127.0.0.1:54871 protocol=grpc
5:06PM INF started call grpc.component=server grpc.method=Sync grpc.method_type=server_stream grpc.service=cloudquery.plugin.v3.Plugin grpc.start_time=2024-08-12T17:06:00+02:00 grpc.time_ms=0.096 peer.address=127.0.0.1:54871 protocol=grpc
panic: unwrapping *time.Time to string isn't supported

goroutine 57 [running]:
github.com/cloudquery/cloudquery/plugins/destination/clickhouse/typeconv/arrow/values.unwrap[...]({0x103a8c820, 0x14000133398})
	/work/cloudquery/plugins/destination/clickhouse/typeconv/arrow/values/unwrap.go:26 +0x3c4
github.com/cloudquery/cloudquery/plugins/destination/clickhouse/typeconv/arrow/values.buildPrimitive[...](0x14000712158, {0x103a8c820, 0x14000133398})
	/work/cloudquery/plugins/destination/clickhouse/typeconv/arrow/values/primitive.go:13 +0x48
github.com/cloudquery/cloudquery/plugins/destination/clickhouse/typeconv/arrow/values.buildValue({0x103acd578, 0x14000712158}, {0x103a8c820, 0x14000133398})
	/work/cloudquery/plugins/destination/clickhouse/typeconv/arrow/values/values.go:46 +0x128
github.com/cloudquery/cloudquery/plugins/destination/clickhouse/typeconv/arrow/values.AppendToRecordBuilder(0x14000397940, {0x14000646e00, 0xe, 0xe})
	/work/cloudquery/plugins/destination/clickhouse/typeconv/arrow/values/record.go:9 +0xd8
github.com/cloudquery/cloudquery/plugins/source/clickhouse/resources/plugin.(*Client).syncTable(0x1400072d000, {0x103ab5e98, 0x14000445c70}, 0x14000593860, 0x14000718d80)
	/work/cloudquery-private/plugins/source/clickhouse/resources/plugin/sync.go:69 +0x2a8
github.com/cloudquery/cloudquery/plugins/source/clickhouse/resources/plugin.(*Client).syncTables.func1()
	/work/cloudquery-private/plugins/source/clickhouse/resources/plugin/sync.go:48 +0x58
golang.org/x/sync/errgroup.(*Group).Go.func1()
	/go/pkg/mod/golang.org/x/sync@v0.7.0/errgroup/errgroup.go:78 +0xa8
created by golang.org/x/sync/errgroup.(*Group).Go in goroutine 40
	/go/pkg/mod/golang.org/x/sync@v0.7.0/errgroup/errgroup.go:75 +0xfc

Seems more appropriate to use the same type that is used to represent Date32.

https://clickhouse.com/docs/en/sql-reference/data-types/date
https://clickhouse.com/docs/en/sql-reference/data-types/date32

An alternative fix that wouldn't require changing arrow types would be to modify

cloudquery/plugins/destination/clickhouse/typeconv/arrow/values/values.go

Line 45 in 3ae9b11

case *array.StringBuilder:
to use buildFromString instead of buildPrimitive.

@savme savme marked this pull request as ready for review August 12, 2024 15:25
@savme savme requested review from a team, erezrokah and murarustefaan and removed request for a team August 12, 2024 15:25
erezrokah
erezrokah reviewed Aug 12, 2024
View reviewed changes
Copy link
Member

@erezrokah erezrokah left a comment

Choose a reason for hiding this comment

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

Not sure why it's like this and if it works great 🚀 I think this should be a breaking change as it changes the schema

@savme savme changed the title fix: Use an arrow date type for clickhouse dates fix!: Use an arrow date type for clickhouse dates Aug 12, 2024
@savme savme added the automerge Automatically merge once required checks pass label Aug 13, 2024
@kodiakhq kodiakhq bot merged commit fcb8170 into main Aug 13, 2024
@kodiakhq kodiakhq bot deleted the feat/2261-clickhouse-date branch August 13, 2024 11:33
@cq-bot cq-bot mentioned this pull request Aug 13, 2024
Merged
kodiakhq bot pushed a commit that referenced this pull request Aug 13, 2024
@cq-bot
chore(main): Release plugins-destination-clickhouse v5.0.0 (#18884)
8c1b094 
🤖 I have created a release *beep* *boop*
---


## [5.0.0](plugins-destination-clickhouse-v4.2.4...plugins-destination-clickhouse-v5.0.0) (2024-08-13)


### ⚠ BREAKING CHANGES

* Use an arrow date type for clickhouse dates ([#18914](#18914))

### Bug Fixes

* **deps:** Update module github.com/cloudquery/plugin-sdk/v4 to v4.59.0 ([#18881](#18881)) ([8f7667f](8f7667f))
* **deps:** Update module github.com/cloudquery/plugin-sdk/v4 to v4.60.0 ([#18922](#18922)) ([7626636](7626636))
* Use an arrow date type for clickhouse dates ([#18914](#18914)) ([fcb8170](fcb8170))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@murarustefaan murarustefaan murarustefaan approved these changes

@erezrokah erezrokah erezrokah approved these changes

Assignees

No one assigned

Labels

area/plugin/destination/clickhouse automerge Automatically merge once required checks pass

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@savme @murarustefaan @erezrokah @cq-bot