feat: Support for User Specifying Primary Key Scheme (default or cq-ids)#732
feat: Support for User Specifying Primary Key Scheme (default or cq-ids)#732kodiakhq[bot] merged 27 commits intocloudquery:mainfrom
Conversation
⏱️ Benchmark results
|
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #732 +/- ##
==========================================
- Coverage 48.82% 43.15% -5.68%
==========================================
Files 70 80 +10
Lines 6923 7886 +963
==========================================
+ Hits 3380 3403 +23
- Misses 3077 4015 +938
- Partials 466 468 +2
... and 6 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
erezrokah
left a comment
There was a problem hiding this comment.
I know this is still in draft, but maybe since PKs and columns are defined on sources, this should be a source option?
I mean, what if I want _cq_id PK for AWS, and regular PKs for Azure?
I am actually against putting it on the source because then the user will have to fetch the same data multiple times if they want to put the same data in multiple destinations (with different PK settings). In the case where a user has multiple sources and a single destination but want different behavior for each source, they should duplicate the destination. |
Got it, makes sense |
disq
left a comment
There was a problem hiding this comment.
LGTM, looked for a simpler approach but I don't see it
erezrokah
left a comment
There was a problem hiding this comment.
Added a few comments (non blocking)
Co-authored-by: Erez Rokah <erezrokah@users.noreply.github.com>
yevgenypats
left a comment
There was a problem hiding this comment.
I think setCQIDAsPrimaryKeysForTables might need to be called also on destination Write . We have e2e test suites in the SDK under https://github.com/cloudquery/plugin-sdk/tree/main/internal/memdb we might want to see that this test is covered there as well.
plugins/destination/plugin.go
Outdated
| SetDestinationManagedCqColumns(tables) | ||
| setCqIDColumnOptionsForTables(tables) | ||
| if p.spec.PKMode == specs.PKModeCQID { | ||
| setCQIDAsPrimaryKeysForTables(tables) |
There was a problem hiding this comment.
Should that also be called on Write?
candiduslynx
left a comment
There was a problem hiding this comment.
1 nit, non-blocking
plugins/destination/plugin.go
Outdated
| case schema.CqIDColumn.Name: | ||
| table.Columns[i].CreationOptions.PrimaryKey = true | ||
| case schema.CqParentIDColumn.Name: | ||
| table.Columns[i].CreationOptions.PrimaryKey = table.Parent != nil |
There was a problem hiding this comment.
Why a parent should ever be a primary key ?
There was a problem hiding this comment.
2 different top level resources could resolve the same nested resource. In that case, the cq_id would be the same which means that one of the resources would be dropped
There was a problem hiding this comment.
I have removed this because the table.Parent is always nil
candiduslynx
left a comment
There was a problem hiding this comment.
also a non-blocking nit
Co-authored-by: candiduslynx <candiduslynx@users.noreply.github.com>
🤖 I have created a release *beep* *boop* --- ## [1.44.0](v1.43.0...v1.44.0) (2023-03-17) ### Features * Support for User Specifying Primary Key Scheme (default or cq-ids) ([#732](#732)) ([a41af50](a41af50)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Summary
Initial implementation for cloudquery/cloudquery#8811
Use the following steps to ensure your PR is ready to be reviewed
go fmtto format your code 🖊golangci-lint run🚨 (install golangci-lint here)