-
Notifications
You must be signed in to change notification settings - Fork 58
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Depend on wider arrow versions, add integration test #366
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #366 +/- ##
=======================================
Coverage 78.28% 78.28%
=======================================
Files 50 50
Lines 10292 10292
Branches 10292 10292
=======================================
Hits 8057 8057
Misses 1781 1781
Partials 454 454 ☔ View full report in Codecov by Sentry. |
arrow = "=52.1.0" | ||
delta_kernel = { path = "../kernel", features = ["arrow-conversion"] } | ||
|
||
[patch.'file:///../kernel'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this feels... wrong, but it does seem to work. upstream crates would use the patch.crates-io
way anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM and appreciate the clear docs! let's merge for 0.4.0 and then iterate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a cargo expert, but the idea is laudable and I don't see any obvious problems with the changes
This widens our arrow dependency and adds a basic integration test.
Note that the patching workflow is not 100% ideal as it requires a version specification that reduces to exactly one version. I don't think there's a way around this, so we may want to in the future adjust to a feature flag based approach. That's a lot more work though, so we'll do this for now and unblock people using older arrow versions, and then iterate if needed.
At this point, the test just makes sure we can compile (and run a trivial program) with all the versions of arrow we say we support. It creates an arrow schema and a kernel schema, and then compares them to make sure the versions work together.
The testing script pulls all versions of arrow, checks that it's in the range we support, then
sed
s each one into theintegration-test
'sCargo.toml
as both the required version of arrow and as the versions in the[patch]
section. Then compiles and runs the crate.Reviewers, recommend you also look at the workflow jobs triggered to see what the tests do.