feat: Support sources in SDK V3 #864
Conversation
yevgenypats
force-pushed
the
feat/sdkv3_sources
branch
from
350eda1 to
8196a48
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #864 +/- ##
==========================================
- Coverage 52.33% 46.86% -5.47%
==========================================
Files 29 51 +22
Lines 2595 4720 +2125
==========================================
+ Hits 1358 2212 +854
- Misses 1133 2278 +1145
- Partials 104 230 +126
☔ View full report in Codecov by Sentry. |
⏱️ Benchmark results
|
Related but not blocked as SDK and proto-pb-go are now decoupled cloudquery/plugin-sdk#864 . This PR should be merged and released first
yevgenypats
force-pushed
the
feat/sdkv3_sources
branch
from
8196a48 to
1ed24b1
Compare
hermanschaaf
left a comment
hermanschaaf
left a comment
There was a problem hiding this comment.
Awesome, generally looks good to me from a read-through 👍 I haven't been through it all in great detail because I think it's mostly been copied from Arrow and our old SDK version
| schemas, err := schema.NewSchemasFromBytes(r.Tables) | ||
| if err != nil { | ||
| return status.Errorf(codes.InvalidArgument, "failed to create schemas: %v", err) | ||
| } | ||
| tables, err := schema.NewTablesFromArrowSchemas(schemas) | ||
| if err != nil { | ||
| return status.Errorf(codes.InvalidArgument, "failed to create tables: %v", err) | ||
| } |
There was a problem hiding this comment.
This could be using schema.NewTablesFromBytes(r.Tables) (which is a little bit shorter)
scalar/float.go
Outdated
| case int8: | ||
| s.Value = float32(value) | ||
| case int16: | ||
| if value > math.MaxInt8 { |
There was a problem hiding this comment.
I don't really understand why this check is here - what's wrong with int16 being given to float32? Seems like it should work regardless of being bigger than math.MaxInt8? (Same for the cases below)
|
|
||
| import "testing" | ||
|
|
||
| func TestFloat64Set(t *testing.T) { |
There was a problem hiding this comment.
Seems like we're missing a Float32 test
yevgenypats
force-pushed
the
feat/sdkv3_sources
branch
from
3f9d16f to
9e1a409
Compare
|
Going to put an auto-merge here so it will be easier to test other plugins |
|
I did some small changes to the testing code and commented some stuff out so we can more easily follow-up with fixes and review them given the large code change here (due to the copy from a previous PR). |
🤖 I have created a release *beep* *boop* --- ## [3.3.0](v3.2.1...v3.3.0) (2023-05-15) ### Features * Support sources in SDK V3 ([#864](#864)) ([a49abcb](a49abcb)) ### Bug Fixes * **deps:** Update module github.com/cloudquery/plugin-pb-go to v1.0.8 ([#874](#874)) ([56c0e84](56c0e84)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
| // if !opts.SkipMaps { | ||
| // compositeColumns = append(compositeColumns, mapOfColumns(basicColumnsWithExclusions)...) | ||
| // } |
There was a problem hiding this comment.
Why not just use SkipMaps to skip this?
There was a problem hiding this comment.
It was failing the plugin tests themselves. seems there is an error in arrow compare function.
There was a problem hiding this comment.
Oh weird, it was passing for me yesterday (but I did find it a bit suspicious 😄 )
Few notes:
schema.Tablewhich means use ofarrow.DataTypeinstead of our oldValueTypescalarpackage which is mostly used by our "managed" sources and basically our old cqtypes just now using arrow types (which also support nesting). This will hopefully go upstream one day but I don't think we should block on that as the upstream package is not a fit right now and will take time for such refactor to make it upstream.Follow-up PRs with example sources and dest will follow soon.
IMPORTANT: all destinations should be released first as we decided new sources will work only with new destination to avoid un-necessary backward compatibility maintenance.
Example PRs: