feat: Allow testing of more Arrow types#863
Conversation
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## main #863 +/- ##
==========================================
- Coverage 48.29% 45.27% -3.02%
==========================================
Files 29 29
Lines 2429 2591 +162
==========================================
Hits 1173 1173
- Misses 1154 1316 +162
Partials 102 102
☔ View full report in Codecov by Sentry. |
⏱️ Benchmark results
|
| }, | ||
| }, | ||
| // TestSourceOptions controls which types are included by TestSourceColumns. | ||
| type TestSourceOptions struct { |
There was a problem hiding this comment.
What about adding a shorthand to set all options to true?
Something like func AllTestSourceOptions() TestSourceOptions and then every field set to true in the return struct.
There was a problem hiding this comment.
I'd even swap for SkipTypeX semantics, so that the default would be everything and if not, the destination has to be mindful about it (just like of all the skip modes for writing)
There was a problem hiding this comment.
seems that this testing suite is growing I would do two things, one is more important then the other imo:
- I would use
Skip- this way everything is included by default as usually destinations should support everything, unlike modes of operation the SDK doesn't error out (at least for now) if a mode is not supported so if a destinations updates to a newer SDK with new arrow types this would error out in "prod" instead of tests. - More of a preference - I think mostly we used
WithOption()pattern so I would stick to that but that's not a must.
There was a problem hiding this comment.
Yeah, agreed. Let's use Skip so that it needs to be explicitly declared in destinations when something isn't supported. Can use the WithOption pattern too 👍
yevgenypats
left a comment
There was a problem hiding this comment.
Looks good. Had mostly one comment around Skip vs Include
| bldr.Field(i).(*array.ListBuilder).ValueBuilder().(*types.MacBuilder).Append(mac) | ||
| } else { | ||
| panic("unknown type: " + c.Type.String() + " column: " + c.Name) | ||
| example := getExampleJSON(c.Name, c.Type, opts) |
There was a problem hiding this comment.
I think you mentioned it but I can't recall, why you went with json instead of regular append to builder?
There was a problem hiding this comment.
Both can work, and I don't recall the exact reason anymore either 😅 I think it was just easier and cleaner to split it into a separate recursive function that can also handle composite types, and doesn't require any type switches. Using JSON is also a common pattern in the Arrow library testing files, which is where I got the idea.
| }, | ||
| }, | ||
| // TestSourceOptions controls which types are included by TestSourceColumns. | ||
| type TestSourceOptions struct { |
There was a problem hiding this comment.
seems that this testing suite is growing I would do two things, one is more important then the other imo:
- I would use
Skip- this way everything is included by default as usually destinations should support everything, unlike modes of operation the SDK doesn't error out (at least for now) if a mode is not supported so if a destinations updates to a newer SDK with new arrow types this would error out in "prod" instead of tests. - More of a preference - I think mostly we used
WithOption()pattern so I would stick to that but that's not a must.
|
Updated to use |
…sdk into test-more-arrow-types
🤖 I have created a release *beep* *boop* --- ## [3.2.0](v3.1.0...v3.2.0) (2023-05-15) ### Features * Allow testing of more Arrow types ([#863](#863)) ([28642ec](28642ec)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
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.Fielddirectly 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.