feat(clickhouse)!: Upgrade to `github.com/cloudquery/plugin-sdk/v2` #10284

candiduslynx · 2023-04-24T16:40:19Z

BEGIN_COMMIT_OVERRIDE
feat: Update to use Apache Arrow type system (#10284)

BREAKING-CHANGE: This release introduces an internal change to our type system to use Apache Arrow. This should not have any visible breaking changes, however due to the size of the change we are introducing it under a major version bump to communicate that it might have some bugs that we weren't able to catch during our internal tests. If you encounter an issue during the upgrade, please submit a bug report.

feat: Allow nullable columns in primary keys (#10284)
BREAKING-CHANGE: This change enables allow_nullable_key for tables (#10284).

END_COMMIT_OVERRIDE

yevgenypats

Just marking so I've a chance to review before it is merged.

candiduslynx · 2023-04-25T05:59:21Z

@yevgenypats @erezrokah
As some code had to be copied/based on ClickHouse Go SDK, I think we might need to include this info per license requirements:
https://github.com/ClickHouse/clickhouse-go/blob/main/LICENSE

I added the boilerplate reference in 7f057c0

plugins/destination/clickhouse/typeconv/ch/values/list.go

yevgenypats

lot's of code :) few initial comments - will take a break and then give it another fresh look tomorrow.

I did an initial research and seems this should be our best bet in clickhouse indeed in terms of transformation (transforming to json or to arrow will be too slow and we might loose some control that we need)

plugins/destination/clickhouse/typeconv/arrow/types/field.go

plugins/destination/clickhouse/typeconv/ch/values/time.go

plugins/destination/clickhouse/typeconv/ch/values/decimal.go

plugins/destination/clickhouse/typeconv/ch/types/data_type.go

candiduslynx · 2023-04-26T08:53:56Z

~~I'd like to get cloudquery/plugin-sdk#823 merged & update the code in this single PR.~~

Done in ded410e.

yevgenypats

I think let's try go easy on generics. maybe we should disable generics use across all our repos.

I don't see an immediate need for generics in this PR. For example all generics code under values can be replaces with one call to .Values available on all arrow arrays (or .Uint16Values for some reason it is called by different name sometimes but nevertheless it is available there so should save lots of code and test code in this PR which will make it hard to maintain)

candiduslynx · 2023-05-01T13:26:19Z

all generics code under values can be replaces with one call to .Values available on all arrow arrays

@yevgenypats are you referring to the ch/values package?
If so, the reason for the generics is that we do need concrete type for the values passed to the ClickHouse SDK, and the Value method for different array types returns different types altogether (not any).

If we take a look at the arrow/values and generics usage there, removing them would mean going for the separate functions per array (Append takes concrete types once again).

Could you give an example on what do you suggest doing and where?

yevgenypats

Going to stamp approve it. Might go back to that if time allows.

… `array.XBuilder.AppendValueFromString` (#35457) ### Rationale for this change I noticed that some values produced by `array.X.ValueStr` can't be parsed back by `array.XBuilder.AppendValueFromString` while debugging cloudquery/cloudquery#10284. Additionally, some arrays didn't implement the corresponding functions at all. ### What changes are included in this PR? * Ensure interface contract * Use `array.NullValueStr` constant * Fix bugs found along the way * Synced `internal/types/UUID` with CQ implementation to account for `valid` param ### Are these changes tested? This code was developed in TDD mode. The changes workflow was: 1. Introduce `XStringRoundTrip` tests to test that the resulting array will be equal in the Arrow sense (using [`array.Equal`](https://pkg.go.dev/github.com/apache/arrow/go/v12/arrow/array#Equal)) 2. Fix any discrepancies ### Are there any user-facing changes? Some fixes to the accepted values & parse logic to correspond to the interface contract. * Closes: #35421 Authored-by: candiduslynx <[email protected]> Signed-off-by: Matt Topol <[email protected]>

This reverts commit 45d004a.

🤖 I have created a release *beep* *boop* --- ## [3.0.0](plugins-destination-clickhouse-v2.0.1...plugins-destination-clickhouse-v3.0.0) (2023-05-09) ### ⚠ BREAKING CHANGES * This change enables [`allow_nullable_key`](https://clickhouse.com/docs/en/operations/settings/settings#allow-nullable-key) for tables ([#10284](#10284)). ### Features * Allow nullable columns in primary keys ([#10284](#10284)) ([26e7d0e](26e7d0e)) * **deps:** Upgrade to Apache Arrow v13 (latest `cqmain`) ([#10605](#10605)) ([a55da3d](a55da3d)) * Update to use [Apache Arrow](https://arrow.apache.org/) type system ([#10284](#10284)) ([26e7d0e](26e7d0e)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

…Str` & `array.XBuilder.AppendValueFromString` (apache#35457) ### Rationale for this change I noticed that some values produced by `array.X.ValueStr` can't be parsed back by `array.XBuilder.AppendValueFromString` while debugging cloudquery/cloudquery#10284. Additionally, some arrays didn't implement the corresponding functions at all. ### What changes are included in this PR? * Ensure interface contract * Use `array.NullValueStr` constant * Fix bugs found along the way * Synced `internal/types/UUID` with CQ implementation to account for `valid` param ### Are these changes tested? This code was developed in TDD mode. The changes workflow was: 1. Introduce `XStringRoundTrip` tests to test that the resulting array will be equal in the Arrow sense (using [`array.Equal`](https://pkg.go.dev/github.com/apache/arrow/go/v12/arrow/array#Equal)) 2. Fix any discrepancies ### Are there any user-facing changes? Some fixes to the accepted values & parse logic to correspond to the interface contract. * Closes: apache#35421 Authored-by: candiduslynx <[email protected]> Signed-off-by: Matt Topol <[email protected]>

… `array.XBuilder.AppendValueFromString` (#35457) ### Rationale for this change I noticed that some values produced by `array.X.ValueStr` can't be parsed back by `array.XBuilder.AppendValueFromString` while debugging cloudquery/cloudquery#10284. Additionally, some arrays didn't implement the corresponding functions at all. ### What changes are included in this PR? * Ensure interface contract * Use `array.NullValueStr` constant * Fix bugs found along the way * Synced `internal/types/UUID` with CQ implementation to account for `valid` param ### Are these changes tested? This code was developed in TDD mode. The changes workflow was: 1. Introduce `XStringRoundTrip` tests to test that the resulting array will be equal in the Arrow sense (using [`array.Equal`](https://pkg.go.dev/github.com/apache/arrow/go/v12/arrow/array#Equal)) 2. Fix any discrepancies ### Are there any user-facing changes? Some fixes to the accepted values & parse logic to correspond to the interface contract. * Closes: #35421 Authored-by: candiduslynx <[email protected]> Signed-off-by: Matt Topol <[email protected]>

candiduslynx added this to the Migrate codebase to plugin-sdk v2 milestone Apr 24, 2023

candiduslynx self-assigned this Apr 24, 2023

candiduslynx requested review from a team and bbernays and removed request for a team April 24, 2023 16:40

cq-bot added the clickhouse label Apr 24, 2023

candiduslynx requested review from hermanschaaf and yevgenypats April 24, 2023 16:41

yevgenypats suggested changes Apr 24, 2023

View reviewed changes

candiduslynx force-pushed the feat/plugin-sdk-v2/clickhouse branch from ceb1600 to 7f057c0 Compare April 25, 2023 06:09

candiduslynx commented Apr 25, 2023

View reviewed changes

plugins/destination/clickhouse/typeconv/ch/values/list.go Outdated Show resolved Hide resolved

yevgenypats suggested changes Apr 25, 2023

View reviewed changes

candiduslynx requested a review from yevgenypats April 26, 2023 07:28

candiduslynx added the no automerge label Apr 26, 2023

candiduslynx removed the request for review from bbernays April 28, 2023 11:09

yevgenypats suggested changes May 1, 2023

View reviewed changes

candiduslynx requested a review from yevgenypats May 1, 2023 13:26

candiduslynx removed the no automerge label May 2, 2023

candiduslynx force-pushed the feat/plugin-sdk-v2/clickhouse branch 4 times, most recently from fd1471a to 7e11de6 Compare May 3, 2023 23:22

This was referenced May 4, 2023

feat: YDB support for CloudQuery ydb-platform/ydb-go-sdk#447

Open

GH-35421: [Go] Ensure interface contract between array.X.ValueStr & array.XBuilder.AppendValueFromString apache/arrow#35457

Merged

yevgenypats approved these changes May 8, 2023

View reviewed changes

candiduslynx force-pushed the feat/plugin-sdk-v2/clickhouse branch from 22e25fc to 1ca365b Compare May 9, 2023 05:38

candiduslynx and others added 17 commits May 9, 2023 13:48

use reader

c799625

don't use underscore-prefixed var names

1c987ae

feat(clickhouse): Map support (#10555)

9a181a4

bump deps

dbe91d4

simplify test

2f89d6c

simplify switch

6647ad6

extract mapItemValue

1810273

stop forcing non-nullable onto PK

2a74042

doc

59d5d02

Revert "stop forcing non-nullable onto PK"

ecc5572

This reverts commit 45d004a.

actually, we need nullable PKs for Array(Nullable(xyz))

ac4c04c

no array.RecordReader for now (x2 speedup)

b2385ff

bump plugin-sdk to 2.5.3

31ec7ae

up github.com/ClickHouse/clickhouse-go/v2

53fc9ae

upgrade to arrow v13

8809b33

bump deps

d11ede0

up sdk

15a3f9b

candiduslynx force-pushed the feat/plugin-sdk-v2/clickhouse branch from 5b93dfa to 2a01f2f Compare May 9, 2023 10:48

candiduslynx mentioned this pull request May 9, 2023

chore(clickhouse): Extensive Apache Arrow types test #10623

Closed

use string instead of json

2af78f2

candiduslynx force-pushed the feat/plugin-sdk-v2/clickhouse branch from 2a01f2f to 2af78f2 Compare May 9, 2023 11:03

candiduslynx added the automerge Automatically merge once required checks pass label May 9, 2023

kodiakhq bot merged commit 26e7d0e into main May 9, 2023

kodiakhq bot deleted the feat/plugin-sdk-v2/clickhouse branch May 9, 2023 12:53

cq-bot mentioned this pull request May 9, 2023

chore(main): Release plugins-destination-clickhouse v3.0.0 #10616

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat(clickhouse)!: Upgrade to `github.com/cloudquery/plugin-sdk/v2` #10284

feat(clickhouse)!: Upgrade to `github.com/cloudquery/plugin-sdk/v2` #10284

Uh oh!

candiduslynx commented Apr 24, 2023 •

edited

Loading

Uh oh!

yevgenypats left a comment

Uh oh!

candiduslynx commented Apr 25, 2023 •

edited

Loading

Uh oh!

Uh oh!

yevgenypats left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

candiduslynx commented Apr 26, 2023 •

edited

Loading

Uh oh!

yevgenypats left a comment

Uh oh!

candiduslynx commented May 1, 2023 •

edited

Loading

Uh oh!

yevgenypats left a comment

Uh oh!

Uh oh!

feat(clickhouse)!: Upgrade to github.com/cloudquery/plugin-sdk/v2 #10284

feat(clickhouse)!: Upgrade to github.com/cloudquery/plugin-sdk/v2 #10284

Uh oh!

Conversation

candiduslynx commented Apr 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yevgenypats left a comment

Choose a reason for hiding this comment

Uh oh!

candiduslynx commented Apr 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

yevgenypats left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

candiduslynx commented Apr 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yevgenypats left a comment

Choose a reason for hiding this comment

Uh oh!

candiduslynx commented May 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yevgenypats left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

feat(clickhouse)!: Upgrade to `github.com/cloudquery/plugin-sdk/v2` #10284

feat(clickhouse)!: Upgrade to `github.com/cloudquery/plugin-sdk/v2` #10284

candiduslynx commented Apr 24, 2023 •

edited

Loading

candiduslynx commented Apr 25, 2023 •

edited

Loading

candiduslynx commented Apr 26, 2023 •

edited

Loading

candiduslynx commented May 1, 2023 •

edited

Loading