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

Adopt types for table features, fixed a few clippy warnings #684

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

hntd187
Copy link
Collaborator

@hntd187 hntd187 commented Feb 7, 2025

What changes are proposed in this pull request?

This PR just moves the string typed table features to adopt the numbers we already have specified for this. This should do the string conversion parsing only once instead of every time we check if table features are supported.

I also fixed 2 bug clippy made me aware of. One was using .next_back() vs. .last() since it was a double ended iterator. And the other was an error message on a deletion vector's length not being long enough where it would emit an error message that has an unformatted capture variable in it.

This PR affects the following public APIs

This changes the underlying type signature Protocol exposes from Option<Vec<String>> to it's associated table feature enum. ReaderFeatures and WriterFeatures respectively.

How was this change tested?

Test cases were altered to suit the new usage, but for the most part a lot of general parsing was remove albeit the cost is some code duplication.

Copy link

codecov bot commented Feb 7, 2025

Codecov Report

Attention: Patch coverage is 92.42424% with 10 lines in your changes missing coverage. Please review.

Project coverage is 84.10%. Comparing base (eedfd47) to head (5e5fd69).

Files with missing lines Patch % Lines
kernel/src/actions/mod.rs 93.25% 6 Missing ⚠️
kernel/src/table_features/mod.rs 90.90% 2 Missing ⚠️
kernel/src/actions/deletion_vector.rs 0.00% 1 Missing ⚠️
kernel/src/engine/arrow_data.rs 87.50% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #684   +/-   ##
=======================================
  Coverage   84.09%   84.10%           
=======================================
  Files          77       77           
  Lines       17805    17875   +70     
  Branches    17805    17875   +70     
=======================================
+ Hits        14973    15033   +60     
- Misses       2117     2126    +9     
- Partials      715      716    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@github-actions github-actions bot added the breaking-change Change that will require a version bump label Feb 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Change that will require a version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant