Skip to content

Conversation

@erezrokah
Copy link
Member

@erezrokah erezrokah commented Jun 23, 2023

Summary

Discovered this while working on the PostgreSQL source v4 migration.
When we set string values to the inet scalar we set the IP field on the resulting net.IPNet:

ipnet.IP = ip

However the inet arrow type loses that information when returning the value:

_, ipnet, err := net.ParseCIDR(cidr)

This means that if you create an arrow inet type from the string 192.168.0.1/24, then call ValueStr you get back 192.168.0.0/24

This PR sets the IP data on the resulting net.IPNet so it is not lost


Use the following steps to ensure your PR is ready to be reviewed

  • Read the contribution guidelines 🧑‍🎓
  • Run go fmt to format your code 🖊
  • Lint your changes via golangci-lint run 🚨 (install golangci-lint here)
  • Update or add tests 🧪
  • Ensure the status checks below are successful ✅

@erezrokah erezrokah requested a review from yevgenypats as a code owner June 23, 2023 08:40
@github-actions github-actions bot added fix and removed fix labels Jun 23, 2023
@github-actions
Copy link

github-actions bot commented Jun 23, 2023

⏱️ Benchmark results

Comparing with 097621f

  • DefaultConcurrencyDFS-2 resources/s: 10,412 ⬇️ 2.59% decrease vs. 097621f
  • DefaultConcurrencyRoundRobin-2 resources/s: 12,149 ⬆️ 7.30% increase vs. 097621f
  • Glob-2 ns/op: 191.1 ⬆️ 1.88% increase vs. 097621f
  • TablesWithChildrenDFS-2 resources/s: 26,073 ⬇️ 2.66% decrease vs. 097621f
  • TablesWithChildrenRoundRobin-2 resources/s: 27,761 ⬇️ 4.72% decrease vs. 097621f
  • TablesWithRateLimitingDFS-2 resources/s: 28.25 ⬆️ 0.14% increase vs. 097621f
  • TablesWithRateLimitingRoundRobin-2 resources/s: 827.5 ⬇️ 0.31% decrease vs. 097621f

@codecov
Copy link

codecov bot commented Jun 23, 2023

Codecov Report

Patch coverage: 71.42% and project coverage change: +0.02 🎉

Comparison is base (38d8c0a) 47.24% compared to head (0ffa345) 47.27%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #982      +/-   ##
==========================================
+ Coverage   47.24%   47.27%   +0.02%     
==========================================
  Files          82       82              
  Lines        7708     7713       +5     
==========================================
+ Hits         3642     3646       +4     
- Misses       3732     3733       +1     
  Partials      334      334              
Impacted Files Coverage Δ
types/inet.go 60.64% <71.42%> (+0.64%) ⬆️

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

Copy link
Contributor

@candiduslynx candiduslynx 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 if that's required

@disq
Copy link
Member

disq commented Jul 11, 2023

I think it's a good time to merge this, before the final v4 release. @yevgenypats

@kodiakhq kodiakhq bot merged commit fa07032 into cloudquery:main Jul 12, 2023
@erezrokah erezrokah deleted the fix/align_inet_type_scalar_logic branch July 12, 2023 12:17
kodiakhq bot pushed a commit that referenced this pull request Jul 12, 2023
🤖 I have created a release *beep* *boop*
---


## [4.0.0](v4.8.1-rc1...v4.0.0) (2023-07-12)


### Bug Fixes

* **deps:** Update github.com/cloudquery/arrow/go/v13 digest to 0a52533 ([#1083](#1083)) ([0370294](0370294))
* **deps:** Update github.com/cloudquery/arrow/go/v13 digest to a2a76eb ([#1084](#1084)) ([26df75f](26df75f))
* **types-inet:** Align logic with scalar package, set `net.IPNet` `IP` field after parsing `ParseCIDR` ([#982](#982)) ([fa07032](fa07032))
* Use background ctx in batchwriter worker ([#1079](#1079)) ([dea8168](dea8168))


### Miscellaneous Chores

* release 4.0.0 ([a80ee69](a80ee69))

---
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 Jul 15, 2023

#### Summary

Similar to #11696.
~~I had to comment change some of the tests due to cloudquery/plugin-sdk#982 and cloudquery/plugin-sdk#915 (comment), so not ready to merge~~

BEGIN_COMMIT_OVERRIDE
feat!: Upgrades the postgresql source plugin to use plugin-sdk v4. This version does not contain any user-facing breaking changes, but because it is now using CloudQuery gRPC protocol v3, it does require use of a destination plugin that also supports protocol v3. All recent destination plugin versions support this.

feat!: To enable CDC in this version you'll need to use the `cdc_id` configuration string property, instead of the `cdc` boolean one. Please refer to the [docs](https://www.cloudquery.io/docs/plugins/sources/postgresql/overview) for more information 
END_COMMIT_OVERRIDE

<!--
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants