Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #818 +/- ##
=======================================
Coverage 47.46% 47.46%
=======================================
Files 59 59
Lines 6611 6611
=======================================
Hits 3138 3138
Misses 3065 3065
Partials 408 408
☔ View full report in Codecov by Sentry. |
⏱️ Benchmark resultsComparing with ea90e4e
|
| basicFields = append(basicFields, FixedWidthFields()...) | ||
|
|
||
| // add extensions | ||
| basicFields = append(basicFields, arrow.Field{Name: "uuid", Type: types.NewUUIDType(), Nullable: true}) |
There was a problem hiding this comment.
Need to add both nullable and not nullable fields of the same type to the schema
There was a problem hiding this comment.
Maybe; but what is your reasoning?
There was a problem hiding this comment.
CH: Array may contain nullable elements, but the array itself shouldn't be marked nullable.
We need to pass these types to verify the correct destination logic:
- nullable array of nullable
- nullable array of non-nullable
- non-nullable array of nullable
- non-nullable array of non-nullable
There was a problem hiding this comment.
I see, thanks; maybe we can add 4 columns similar to what you suggested just to test nullable behavior specifically, rather than adding a nullable column for every type (doubling the number of columns)? There are already around 200 columns if all the options are enabled, and doubling that will make it harder to figure out what really went wrong in a test.
There was a problem hiding this comment.
we can use testing.T.Run + utilize the fieldPrettify logic to construct the name to be able to deconstruct easily: https://github.com/cloudquery/plugin-sdk/blob/main/schema/arrow.go#L60
yevgenypats
left a comment
There was a problem hiding this comment.
Looks great! I worked on #832, I think if we can review/merge 832 first and then rebase on top it should also simplify part of this PR as well due to the new abstraction
…tamp (#845) Discovered while performing testing for #818 in cloudquery/cloudquery#10557
|
Replaced by #863 |
|
@hermanschaaf is it OK to remove the branch as well? |
This adds support for testing (almost) the complete range of Arrow types in destinations. By default, only the types previously supported by CQTypes are enabled (with the important exception of more uint and int types than before, plus float32). The TestSourceOptions struct allows destinations to declare that they want to test with more column types, including maps, structs, times, etc. I have not included all the nullable column variations. These are a bit more difficult to do now that we are not using `arrow.Field` directly anymore, plus we can rather do that as a follow-up if and when it becomes necessary to test this. This PR is a rebased version for v3 that replaces #818.
This adds support for testing (almost) the complete range of Arrow types in destinations.
By default, only the types previously supported by CQTypes are enabled (with the important exception of more uint and int types than before, plus float32). The
TestSourceOptionsstruct allows destinations to declare that they want to test with more column types, including maps, structs, times, etc.Closes #805