Skip to content
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

Merged
merged 16 commits into from
Oct 21, 2024

Conversation

nicklan
Copy link
Collaborator

@nicklan nicklan commented Oct 1, 2024

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 seds each one into the integration-test's Cargo.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.

Copy link

codecov bot commented Oct 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.28%. Comparing base (284db10) to head (4a3b76d).
Report is 1 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

@nicklan nicklan requested a review from rtyler October 1, 2024 23:14
arrow = "=52.1.0"
delta_kernel = { path = "../kernel", features = ["arrow-conversion"] }

[patch.'file:///../kernel']
Copy link
Collaborator Author

@nicklan nicklan Oct 1, 2024

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

@zachschuermann zachschuermann self-requested a review October 9, 2024 23:22
@nicklan nicklan changed the title Initial pass at integration tests on arrow versions Depend on wider arrow versions, add integration test Oct 18, 2024
@nicklan nicklan marked this pull request as ready for review October 18, 2024 00:28
@nicklan nicklan requested review from scovich and azdavis October 18, 2024 00:28
Copy link
Collaborator

@zachschuermann zachschuermann left a 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?

Copy link
Collaborator

@scovich scovich left a 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

@nicklan nicklan merged commit 2b1c46f into delta-io:main Oct 21, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants