Skip to content

fix(test): Slice rows properly when reading in tests#1632

Merged
kodiakhq[bot] merged 3 commits intomainfrom
fix/test/read-multirow
Apr 12, 2024
Merged

fix(test): Slice rows properly when reading in tests#1632
kodiakhq[bot] merged 3 commits intomainfrom
fix/test/read-multirow

Conversation

@candiduslynx
Copy link
Copy Markdown
Contributor

@candiduslynx candiduslynx commented Apr 12, 2024

We never actually required the read records to be a single-row ones, so we may as well slice them ourselves

@candiduslynx candiduslynx force-pushed the fix/test/read-multirow branch from 6a25f17 to 7173025 Compare April 12, 2024 17:33
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 12, 2024

⏱️ Benchmark results

Comparing with f80c02b

  • Glob-8 ns/op: 91.14 ⬆️ 0.49% increase vs. f80c02b

@candiduslynx candiduslynx changed the title fix(test): Skip rows properly in migrate tests fix(test): Slice rows properly when reading in tests Apr 12, 2024
@github-actions github-actions bot added fix and removed fix labels Apr 12, 2024
@github-actions github-actions bot added fix and removed fix labels Apr 12, 2024
Copy link
Copy Markdown
Member

@erezrokah erezrokah left a comment

Choose a reason for hiding this comment

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

Can you explain what bug this PR fixes? Maybe via a unit test?

@candiduslynx
Copy link
Copy Markdown
Contributor Author

Can you explain what bug this PR fixes? Maybe via a unit test?

During development of the Databricks destination I found the issue with returned records: in Databricks it's easy to have multirow arrow.Record returned directly from the database. If we pass this as is (with transformation) to the sdk, some of the methods we use in tests will fail:

  • sorting records based on id or other fields
  • skipping first rows (via skipping records)

Basically, it means that our SDK expects to receive a single-row record, but it's never documented.

We have a similar issue inDuckDB destination, where we opted to reslice the returned record manually, but I think the slicing should be handled by the SDK itself:
https://github.com/cloudquery/cloudquery/blob/de3297476bcc5e3aa71bc1b14c4fdf6cf5b49ba0/plugins/destination/duckdb/client/read.go#L68

@candiduslynx candiduslynx requested a review from erezrokah April 12, 2024 18:07
@kodiakhq kodiakhq bot merged commit 537b64c into main Apr 12, 2024
@kodiakhq kodiakhq bot deleted the fix/test/read-multirow branch April 12, 2024 18:47
kodiakhq bot pushed a commit that referenced this pull request Apr 12, 2024
🤖 I have created a release *beep* *boop*
---


## [4.38.1](v4.38.0...v4.38.1) (2024-04-12)


### Bug Fixes

* **deps:** Update golang.org/x/exp digest to c0f41cb ([#1615](#1615)) ([0c21bfb](0c21bfb))
* **deps:** Update module github.com/cloudquery/plugin-pb-go to v1.19.9 ([#1631](#1631)) ([5d45003](5d45003))
* **deps:** Update module google.golang.org/grpc to v1.63.2 ([#1617](#1617)) ([02461b1](02461b1))
* **test:** Slice rows properly when reading in tests ([#1632](#1632)) ([537b64c](537b64c))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
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.

2 participants