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.

Sign up for GitHub

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

Jump to bottom

refactor!: make snapshot.schema() return a SchemaRef #751

Merged
merged 2 commits into from
Mar 21, 2025
Merged

refactor!: make snapshot.schema() return a SchemaRef #751

merged 2 commits into from
Mar 21, 2025

Conversation

maruschin
Copy link
Contributor

@maruschin maruschin commented Mar 20, 2025 •
edited by zachschuermann
Loading

What changes are proposed in this pull request?

Refactor snapshot.schema() to return a SchemaRef
resolves #710

This PR affects the following public APIs

Breaking: changes snapshot.schema() API to return a SchemaRef (Arc<Schema>) instead of an &Schema

How was this change tested?

existing

@maruschin maruschin mentioned this pull request Mar 20, 2025
Closed
@zachschuermann zachschuermann changed the title Made snapshot.schema() return a SchemaRef refactor!: make snapshot.schema() return a SchemaRef Mar 20, 2025
zachschuermann
zachschuermann reviewed Mar 20, 2025
View reviewed changes
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.

Awesome, thanks so much @maruschin! I left a few comments and would you mind updating the PR description to clearly state the breaking change? (snapshot.schema() -> SchemaRef instead of -> &Schema)

@zachschuermann
Copy link
Collaborator

Ah also @maruschin looks like we need to fix ffi: Arc::new(snapshot.schema().clone()).into() should probably a snapshot.schema() now

You should be able to run a top-level cargo test --workspace or cargo nextest run --workspace

I'm actually going to open a PR right quick to make sure that we default to building all crates instead of skipping FFI :)

@zachschuermann
Copy link
Collaborator

I'm actually going to open a PR right quick to make sure that we default to building all crates instead of skipping FFI :)

#752

scovich
scovich reviewed Mar 20, 2025
View reviewed changes
kernel/src/transaction.rs
Comment on lines +154 to +156
let schema = self.read_snapshot.schema();
let fields = schema
.fields()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change will break compilation because L159 was using the fields we no longer define.

Copy link
Contributor Author

@maruschin maruschin Mar 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I removed the redefinition of fields. The fields passed to Expression::struct_from remain the same.

@maruschin maruschin force-pushed the shapshot_schema_return_schemaref branch 2 times, most recently from 485849b to 4c26b67 Compare March 21, 2025 02:40
zachschuermann
zachschuermann approved these changes Mar 21, 2025
View reviewed changes
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 thank you!!

@codecov Codecov
Copy link

codecov bot commented Mar 21, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 87.50000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 84.35%. Comparing base (2573424) to head (3890980).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
ffi/src/lib.rs 0.00% 1 Missing ⚠️
kernel/src/table_configuration.rs 75.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #751      +/-   ##
==========================================
- Coverage   84.36%   84.35%   -0.01%     
==========================================
  Files          81       81              
  Lines       19246    19242       -4     
  Branches    19246    19242       -4     
==========================================
- Hits        16237    16232       -5     
- Misses       2203     2206       +3     
+ Partials      806      804       -2

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@zachschuermann zachschuermann requested a review from roeap March 21, 2025 18:07
roeap
roeap reviewed Mar 21, 2025
View reviewed changes
Copy link
Collaborator

@roeap roeap left a comment

Choose a reason for hiding this comment

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

One comment to maybe consider, otherwise LGTM!

kernel/src/schema/mod.rs Outdated
Comment on lines 334 to 337
pub(crate) fn has_invariants(schema: SchemaRef) -> bool {
let mut checker = InvariantChecker::default();
let _ = checker.transform_struct(schema);
let _ = checker.transform_struct(schema.as_ref());
checker.has_invariants
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we are already borrowing at the callsite, would it make sense to just keep &Schema as argument type here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah +1, callers can just do as_ref() if they have a SchemaRef.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Returned back

@maruschin maruschin force-pushed the shapshot_schema_return_schemaref branch from 730358c to 3bd8902 Compare March 21, 2025 18:46
@github-actions github-actions bot added the breaking-change Change that will require a version bump label Mar 21, 2025
sebastiantia
sebastiantia approved these changes Mar 21, 2025
View reviewed changes
Copy link
Collaborator

@sebastiantia sebastiantia left a comment

Choose a reason for hiding this comment

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

looks good!

@zachschuermann zachschuermann merged commit 3dcad08 into delta-io:main Mar 21, 2025
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zachschuermann zachschuermann zachschuermann approved these changes

@roeap roeap roeap approved these changes

@sebastiantia sebastiantia sebastiantia approved these changes

@nicklan nicklan Awaiting requested review from nicklan

@scovich scovich Awaiting requested review from scovich

Assignees

@maruschin maruschin

Labels
breaking-change Change that will require a version bump
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

make snapshot.schema() return a SchemaRef
6 participants
@maruschin @zachschuermann @nicklan @scovich @roeap @sebastiantia