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

feat!(ffi): expose metadata in SchemaEngineVisitor ffi api #659

Merged
merged 6 commits into from
Jan 30, 2025
Merged

feat!(ffi): expose metadata in SchemaEngineVisitor ffi api #659

merged 6 commits into from
Jan 30, 2025
+73 −7

Conversation

kssenii
Copy link
Contributor

@kssenii kssenii commented Jan 23, 2025

  • new feature PR: "Expose metadata in SchemaEngineVisitor ffi api"

What changes are proposed in this pull request?

This PR affects the following public APIs

A new argument "metadata" was added to EngineSchemaVisitor visitor functions.

How was this change tested?

examples/read_table was updated to test the change.

@codecov Codecov
Copy link

codecov bot commented Jan 23, 2025 •
edited
Loading

Codecov Report

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

Project coverage is 84.14%. Comparing base (3305d3a) to head (7382ae4).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
ffi/src/schema.rs 0.00% 14 Missing ⚠️
ffi/src/scan.rs 0.00% 3 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #659      +/-   ##
==========================================
- Coverage   84.22%   84.14%   -0.08%     
==========================================
  Files          77       77              
  Lines       17694    17710      +16     
  Branches    17694    17710      +16     
==========================================
  Hits        14902    14902              
- Misses       2080     2096      +16     
  Partials      712      712

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

zachschuermann
zachschuermann reviewed Jan 23, 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.

looking good, left a couple comments!

ffi/src/scan.rs Outdated Show resolved Hide resolved
ffi/examples/read-table/schema.h Outdated Show resolved Hide resolved
@zachschuermann
Copy link
Collaborator

also looks like builds are red due to unused arg:

/Users/runner/work/delta-kernel-rs/delta-kernel-rs/ffi/examples/read-table/schema.h:88:33: error: unused parameter 'name' [-Werror,-Wunused-parameter]
void print_metadata(const char *name, const CStringMap* metadata)

@zachschuermann
Copy link
Collaborator

Oh I think due to the preprocessing? might need to IFDEF in such at way that it doesn't yell about unused vars..

@kssenii
Copy link
Contributor Author

kssenii commented Jan 23, 2025

also looks like builds are red due to unused arg:

fixed in the last commit

@scovich scovich added the breaking-change Change that will require a version bump label Jan 23, 2025
@scovich scovich changed the title feat(ffi): expose metadata in SchemaEngineVisitor ffi api feat!(ffi): expose metadata in SchemaEngineVisitor ffi api Jan 23, 2025
scovich
scovich reviewed Jan 23, 2025
View reviewed changes
ffi/examples/read-table/schema.h
void print_physical_name(const char *name, const CStringMap* metadata)
{
#ifdef VERBOSE
char* key_str = "delta.columnMapping.physicalName";
Copy link
Collaborator

Choose a reason for hiding this comment

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

NOTE: This is an internal Delta column that should not actually be user-visible, see #583. That needs to be fixed in kernel proper (not here in the FFI layer), but when it does get fixed this example will stop working.

ffi/src/schema.rs Outdated Show resolved Hide resolved
@kssenii kssenii force-pushed the ffi-api-expose-metadata-in-engine-visitor branch from 7bbe83d to 95a89fa Compare January 24, 2025 12:25
scovich
scovich approved these changes Jan 24, 2025
View reviewed changes
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.

LGTM. Couple nits to fix before merge.

ffi/src/scan.rs Outdated Show resolved Hide resolved
ffi/src/schema.rs Outdated Show resolved Hide resolved
scovich
scovich reviewed Jan 28, 2025
View reviewed changes
ffi/src/scan.rs Outdated Show resolved Hide resolved
@hntd187 hntd187 self-requested a review January 29, 2025 16:22
hntd187
hntd187 approved these changes Jan 29, 2025
View reviewed changes
Copy link
Collaborator

@hntd187 hntd187 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 to me, Ryan and Zach got most of the nits on it.

@hntd187
Copy link
Collaborator

hntd187 commented Jan 29, 2025

and @kssenii can you rebase your branch?

@hntd187 hntd187 merged commit 06d8dbb into delta-io:main Jan 30, 2025
19 of 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 left review comments

@scovich scovich scovich approved these changes

@hntd187 hntd187 hntd187 approved these changes

@nicklan nicklan Awaiting requested review from nicklan

Assignees

@kssenii kssenii

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.
4 participants
@kssenii @zachschuermann @hntd187 @scovich