Skip to content

fix(types): Extensions conversion with storage #948

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 5 commits into from
Jun 5, 2023
Merged

fix(types): Extensions conversion with storage #948

kodiakhq merged 5 commits into from
Jun 5, 2023

Conversation

@candiduslynx
Copy link
Contributor

@candiduslynx candiduslynx commented Jun 4, 2023 •
edited
Loading

This change ensures that we can construct a working extension array from the underlying storage.
Specifically, there should be no difference between the array constructed from storage & the one constructed from the extension builder.
Additionally, the constructed array should be intact (so that it son't panic if the underlying storage doesn't).

@candiduslynx candiduslynx requested a review from yevgenypats as a code owner June 4, 2023 10:58
@github-actions github-actions bot added the fix label Jun 4, 2023
@codecov
Copy link

codecov bot commented Jun 4, 2023 •
edited
Loading

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.38 🎉

Comparison is base (32a0c05) 52.30% compared to head (131cc37) 52.68%.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #948      +/-   ##
==========================================
+ Coverage   52.30%   52.68%   +0.38%     
==========================================
  Files          62       62              
  Lines        6325     6332       +7     
==========================================
+ Hits         3308     3336      +28     
+ Misses       2712     2691      -21     
  Partials      305      305
Impacted Files Coverage Δ
types/inet.go 60.00% <100.00%> (+5.89%) ⬆️
types/mac.go 62.85% <100.00%> (+5.19%) ⬆️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@github-actions
Copy link

github-actions bot commented Jun 4, 2023 •
edited
Loading

⏱️ Benchmark results

Comparing with 32a0c05

  • DefaultConcurrencyDFS-2 resources/s: 10,568 ⬆️ 2.44% increase vs. 32a0c05
  • DefaultConcurrencyRoundRobin-2 resources/s: 12,500 ⬆️ 7.47% increase vs. 32a0c05
  • Glob-2 ns/op: 253.5 ⬇️ 1.38% decrease vs. 32a0c05
  • TablesWithChildrenDFS-2 resources/s: 23,258 ⬇️ 6.59% decrease vs. 32a0c05
  • TablesWithChildrenRoundRobin-2 resources/s: 25,335 ⬆️ 6.53% increase vs. 32a0c05
  • TablesWithRateLimitingDFS-2 resources/s: 28.43 ⬆️ 0.60% increase vs. 32a0c05
  • TablesWithRateLimitingRoundRobin-2 resources/s: 840.9 ⬆️ 0.23% increase vs. 32a0c05

@candiduslynx candiduslynx mentioned this pull request Jun 4, 2023
Merged
yevgenypats
yevgenypats suggested changes Jun 5, 2023
View reviewed changes
Copy link
Contributor

@yevgenypats yevgenypats left a comment

Choose a reason for hiding this comment

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

Looks good but can you please add a short description on what does it do / solve ?

@candiduslynx candiduslynx requested a review from yevgenypats June 5, 2023 09:48
@github-actions github-actions bot added fix and removed fix labels Jun 5, 2023
yevgenypats
yevgenypats suggested changes Jun 5, 2023
View reviewed changes
Copy link
Contributor

@yevgenypats yevgenypats left a comment

Choose a reason for hiding this comment

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

Few minor comments/questions.

@candiduslynx candiduslynx requested a review from yevgenypats June 5, 2023 10:07
@kodiakhq kodiakhq bot merged commit 1132c02 into main Jun 5, 2023
@kodiakhq kodiakhq bot deleted the fix/extensions-storage branch June 5, 2023 10:15
@cq-bot cq-bot mentioned this pull request Jun 5, 2023
Merged
kodiakhq bot pushed a commit that referenced this pull request Jun 5, 2023
@cq-bot
chore(main): Release v3.10.3 (#945)
6945cbd 
🤖 I have created a release *beep* *boop*
---


## [3.10.3](v3.10.2...v3.10.3) (2023-06-05)


### Bug Fixes

* **deps:** Update github.com/cloudquery/arrow/go/v13 digest to 20b0de9 ([#947](#947)) ([32a0c05](32a0c05))
* **deps:** Update github.com/cloudquery/arrow/go/v13 digest to 6d34568 ([#944](#944)) ([f92fd66](f92fd66))
* **deps:** Update github.com/cloudquery/arrow/go/v13 digest to c655015 ([#946](#946)) ([4b6e3a3](4b6e3a3))
* **types:** Extensions conversion with storage ([#948](#948)) ([1132c02](1132c02))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
kodiakhq bot pushed a commit to cloudquery/cloudquery that referenced this pull request Jun 5, 2023
@candiduslynx
feat(nested): Simplify constructing nested ClickHouse value (#11205)
dd6b275 
Requires cloudquery/plugin-sdk#948

#### Benchmarks
Performed by the following command in `typeconv/ch/values` dir:
```sh
go test \
  -test.run=BenchmarkFromArray \
  -test.bench=BenchmarkFromArray \
  -test.count 10 -test.benchmem -test.benchtime 10000x
```

<details><summary>Before this update</summary>

```
goos: darwin
goarch: arm64
pkg: github.com/cloudquery/cloudquery/plugins/destination/clickhouse/typeconv/ch/values
BenchmarkFromArray-10    	   10000	    465316 ns/op	  508005 B/op	    7044 allocs/op
BenchmarkFromArray-10    	   10000	    442365 ns/op	  508002 B/op	    7044 allocs/op
BenchmarkFromArray-10    	   10000	    434197 ns/op	  508006 B/op	    7044 allocs/op
BenchmarkFromArray-10    	   10000	    430641 ns/op	  508006 B/op	    7044 allocs/op
BenchmarkFromArray-10    	   10000	    406410 ns/op	  507999 B/op	    7044 allocs/op
BenchmarkFromArray-10    	   10000	    429302 ns/op	  508002 B/op	    7044 allocs/op
BenchmarkFromArray-10    	   10000	    399611 ns/op	  508005 B/op	    7044 allocs/op
BenchmarkFromArray-10    	   10000	    425171 ns/op	  508004 B/op	    7044 allocs/op
BenchmarkFromArray-10    	   10000	    423713 ns/op	  508007 B/op	    7044 allocs/op
BenchmarkFromArray-10    	   10000	    413656 ns/op	  508006 B/op	    7044 allocs/op
PASS
ok  	github.com/cloudquery/cloudquery/plugins/destination/clickhouse/typeconv/ch/values	108.016s
```

</details> 

<details><summary>After this update</summary>

```
goos: darwin
goarch: arm64
pkg: github.com/cloudquery/cloudquery/plugins/destination/clickhouse/typeconv/ch/values
BenchmarkFromArray-10    	   10000	    326041 ns/op	  235184 B/op	    4512 allocs/op
BenchmarkFromArray-10    	   10000	    310762 ns/op	  235187 B/op	    4512 allocs/op
BenchmarkFromArray-10    	   10000	    324254 ns/op	  235182 B/op	    4512 allocs/op
BenchmarkFromArray-10    	   10000	    320649 ns/op	  235188 B/op	    4512 allocs/op
BenchmarkFromArray-10    	   10000	    326467 ns/op	  235183 B/op	    4512 allocs/op
BenchmarkFromArray-10    	   10000	    310552 ns/op	  235189 B/op	    4512 allocs/op
BenchmarkFromArray-10    	   10000	    310553 ns/op	  235188 B/op	    4512 allocs/op
BenchmarkFromArray-10    	   10000	    313457 ns/op	  235187 B/op	    4512 allocs/op
BenchmarkFromArray-10    	   10000	    327252 ns/op	  235185 B/op	    4512 allocs/op
BenchmarkFromArray-10    	   10000	    312083 ns/op	  235188 B/op	    4512 allocs/op
PASS
ok  	github.com/cloudquery/cloudquery/plugins/destination/clickhouse/typeconv/ch/values	97.273s
```

</details>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@yevgenypats yevgenypats yevgenypats approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

automerge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@candiduslynx @yevgenypats