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

Define and use new AsAny trait #450

Merged
merged 5 commits into from
Nov 14, 2024
Merged

Conversation

scovich
Copy link
Collaborator

@scovich scovich commented Oct 31, 2024

What changes are proposed in this pull request?

As part of implementing various kernel operations, engines need the ability to downcast kernel engine traits to their true concrete types. The rust Any trait can help with struct downcasting, but it isn't quite strong enough once traits are involved. For example, given trait Foo: Any + Send + Sync, and struct Bar that implements Foo, the following fails to compile:

let f: Arc<dyn Foo> = Arc::new(Bar);
let f: Arc<Bar> = f.downcast().unwrap(); // `Arc::downcast` method not found 

... as does this:

let f: Arc<Foo> = Arc::new(Foo{});
let a: Arc<dyn Any + Send + Sync> = f; // trait upcasting coercion is not stable rust
let f: Arc<Foo> = a.downcast().unwrap();

We solve the trait downcasting problem with a new trait AsAny: Any + Send + Sync -- automatically implemented for all T: Any + Send + Sync -- which defines methods for downcasting itself to dyn Any + Send + Sync (reference, box, or arc).

This PR affects the following public APIs

The various engine traits (ParquetHandler, EngineData, etc.) are now constrained to be AsAny (which implies Any + Send + Sync) rather than merely Send + Sync).

This change should be source compatible in the vast majority of cases, because most types implement Any automatically.

How was this change tested?

Doc tests, and it also replaces similar special-case code in the arrow default engine data implementation.

@github-actions github-actions bot added the breaking-change Change that will require a version bump label Oct 31, 2024
Copy link

codecov bot commented Oct 31, 2024

Codecov Report

Attention: Patch coverage is 53.84615% with 6 lines in your changes missing coverage. Please review.

Project coverage is 79.60%. Comparing base (b3ab778) to head (ec6968f).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
kernel/src/lib.rs 50.00% 6 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #450      +/-   ##
==========================================
- Coverage   79.64%   79.60%   -0.04%     
==========================================
  Files          57       57              
  Lines       12324    12330       +6     
  Branches    12324    12330       +6     
==========================================
  Hits         9815     9815              
- Misses       1981     1987       +6     
  Partials      528      528              

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

@scovich
Copy link
Collaborator Author

scovich commented Oct 31, 2024

Apparently our codecov doesn't consider doc tests?
image

Because the doc test just a few lines above definitely exercises the supposedly not-covered method.

Copy link
Collaborator

@nicklan nicklan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this will be useful

@zachschuermann
Copy link
Collaborator

regarding coverage of doctests I made an issue for us to follow up: #484

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 only question is does AsAny communicate enough information about our traits? Maybe consider calling SendSyncAny or something?

@scovich
Copy link
Collaborator Author

scovich commented Nov 14, 2024

LGTM only question is does AsAny communicate enough information about our traits? Maybe consider calling SendSyncAny or something?

I'm not sure the "fatter" name is really helpful? AsAny requires Send+Sync in order to support Arc, but I suppose we could do AsAny + Send + Sync at all the various sites just for extra clarity?

@scovich scovich merged commit bb1be88 into delta-io:main Nov 14, 2024
17 of 19 checks passed
@zachschuermann
Copy link
Collaborator

yea agree - could just leave and make that change later if we think it's necessary :)

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.

3 participants