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

Move schema and expression FFI code to separate module #360

Merged

Conversation

OussamaSaoudi-db
Copy link
Collaborator

This PR addresses points 1 & 2 in #358 by moving the schema visitor to the schema.rs module, and moving the KernelExpressionVisitor to the module expressions.rs.

FFI code for schema and expressions have been moved to their own
modules.

Add prototype of the visitor to enable kernel expresison ->
engine expression
Copy link

codecov bot commented Sep 27, 2024

Codecov Report

Attention: Patch coverage is 0% with 220 lines in your changes missing coverage. Please review.

Project coverage is 74.69%. Comparing base (da206ed) to head (a722343).
Report is 31 commits behind head on main.

Files with missing lines Patch % Lines
ffi/src/expressions.rs 0.00% 164 Missing ⚠️
ffi/src/schema.rs 0.00% 56 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #360      +/-   ##
==========================================
- Coverage   74.71%   74.69%   -0.03%     
==========================================
  Files          43       45       +2     
  Lines        8361     8361              
  Branches     8361     8361              
==========================================
- Hits         6247     6245       -2     
- Misses       1727     1729       +2     
  Partials      387      387              

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

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.

lgtm! thanks

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.

Nice!

Eventually we may also want to move the engine bits to their own file, but that's probably not necessary until we add more complete engine support (e.g. engine data, expressions, etc).

}

// TODO move expression visitors to separate module

// A set that can identify its contents by address
pub struct ReferenceSet<T> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

aside: Right now, this is only used by the expression visitors.
Should we move it there? Or do we anticipate it being useful more generally?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this would be useful in any Engine -> Kernel conversions, like how its used for expressions. Right now, I'm not sure if we'd have usecases where the engine must convert anything besides expressions. Perhaps schemas or some other metadata?

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 flagging this is breaking API change if we shuffle modules/etc. around (at least in rust it would be if e.g. it leads to import path changes)

@OussamaSaoudi-db OussamaSaoudi-db merged commit 135e8d3 into delta-incubator:main Oct 7, 2024
10 of 12 checks passed
@OussamaSaoudi-db OussamaSaoudi-db deleted the expressions_visitor branch October 7, 2024 18:55
@zachschuermann zachschuermann added the breaking-change Change that will require a version bump label Oct 22, 2024
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.

4 participants