feat: Improve destination Read API #542
Conversation
|
Here is a draft PR for the cloudquery repo to show what usage would look like: cloudquery/cloudquery#6107 |
|
|
⏱️ Benchmark resultsComparing with dcd8834
|
|
I like the interface but have a few questions/thought: Do we really need the |
|
@yevgenypats Indeed, the meaning of A single However, when we get to cursor-based syncing, we don't know the ID we're looking for up-front. Let's take a few examples:
The important thing is that we have |
erezrokah
left a comment
There was a problem hiding this comment.
Added a few comments, but I think I'm missing something regarding the reason this is required for testing purposes.
Could we update the equality check in the tests to ignore the order of the results instead?
I get that we might need this for a future feature (curser based fetching), but since the read is only used in tests, maybe we could find a solution that doesn't require adding functionality to non test related code?
As for cursor based fetching, maybe we could add the order by functionality once we implement that feature, as we might discover more requirements once we try to implement cursor based fetching.
Approving the code change regardless of ⬆️
| .PHONY: lint | ||
| lint: | ||
| golangci-lint run | ||
| golangci-lint run --config .golangci.yml |
There was a problem hiding this comment.
[non blocking] I don't think we need to specify the configuration here as golangci-lint looks for it by default https://golangci-lint.run/usage/configuration/#config-file
| } | ||
| sortedRes = append(sortedRes, row) | ||
| } | ||
| sort.Slice(sortedRes, func(i, j int) bool { |
There was a problem hiding this comment.
| sort.Slice(sortedRes, func(i, j int) bool { | |
| sort.SliceStable(sortedRes, func(i, j int) bool { |
|
|
||
| resourcesRead, err := p.readAll(ctx, tables[0], sourceName) | ||
| cqIDIndex := table.Columns.Index(schema.CqIDColumn.Name) | ||
| sort.Slice(resources, func(i, j int) bool { |
There was a problem hiding this comment.
| sort.Slice(resources, func(i, j int) bool { | |
| sort.SliceStable(resources, func(i, j int) bool { |
|
@erezrokah Yeah, I was working on this with the eye on it being the first step towards cursor-based syncing (rather than doing it all in one or two massive PRs). However after seeing how it works out, I think we'll probably not go with this Read API approach and use a different interface to achieve cursor-based syncing. I'll simplify this PR to focus on fixing the tests and mostly try and leave the Read API untouched. |
Extracted from #542 to allow us to fix cloudquery/cloudquery#6069 Before this change, running cockroachDB tests: ``` Jan 2 11:03:08.760000 INF Query args=["testAppendSource474bca19-69ba-4a5a-8af0-3d890198a243"] commandTag="SELECT 2" module=pgx pid=1570743 sql="SELECT * FROM \"cq_test_write_append\" WHERE _cq_source_name = $1 order by _cq_sync_time asc" --- FAIL: TestPgPlugin (0.36s) --- FAIL: TestPgPlugin/TestWriteOverwrite (0.22s) client_test.go:20: expected first resource diff: value at 2 is(d1ebd2a374f14556852c4d5b23608201) TypeUUID while other is(3da5b2eafc814ecb973bcee770bb70e1) TypeUUID value at 3 is(d03dd7f8781a4dbb93aaebefb6aaba87) TypeUUID while other is(f16b7d3a0f85441daf30da5d2d8afb26) TypeUUID value at 7 is(758c419b5a1b4a46ba45499b402c6aaf) TypeUUID while other is(0f252e92137b4f55a856cbf97d43d098) TypeUUID FAIL FAIL github.com/cloudquery/cloudquery/plugins/destination/postgresql/client 0.640s ``` After this change: ``` ok github.com/cloudquery/cloudquery/plugins/destination/postgresql/client 0.662s ```
This adds new options to the Read API to:
I have chosen not to support filtering using operators (like greater than, less than) as an option for now, as it's a complicated and we don't have a strong need for it. We may want to add it later to make it feature-complete.
This update allows us to fix cloudquery/cloudquery#6069 (will be done in another PR to the CloudQuery repo) and should also allow for cursor-based syncing (#413). This is not implemented as part of this PR though.