feat!(ffi): remove visit_snapshot_schema, add logical_schema
#709
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #709 +/- ##
==========================================
+ Coverage 84.35% 84.36% +0.01%
==========================================
Files 75 75
Lines 17657 17654 -3
Branches 17657 17654 -3
==========================================
Hits 14894 14894
+ Misses 2053 2050 -3
Partials 710 710 ☔ View full report in Codecov by Sentry. |
github-actions
bot
added
the
breaking-change
| @@ -98,6 +98,7 @@ impl Snapshot { | |||
| } | |||
|
|
|||
| /// Table [`Schema`] at this `Snapshot`s version. | |||
| // TODO should this return SchemaRef? | |||
There was a problem hiding this comment.
Yes. It's already a SchemaRef under the hood, and schemas can be quite large (i.e. tables with 10k columns)
There was a problem hiding this comment.
Similar story for the global read schema, I think
| /// | ||
| /// Caller is responsible for passing a valid snapshot handle. | ||
| #[no_mangle] | ||
| pub unsafe extern "C" fn logical_schema(snapshot: Handle<SharedSnapshot>) -> Handle<SharedSchema> { |
There was a problem hiding this comment.
Ah! So logical_schema extracts a schema from the snapshot, which can then be visited in the same way as the scan schema?
There was a problem hiding this comment.
aside: Eventually the logical schema the engine should see has been filtered to remove internal Delta properties, hidden columns (not currently a thing), etc. But then we have three schema types, and logical/physical do make sense already. Maybe the engine should see a "clean" schema or a "public" schema or something?
Or maybe this is the right function for that, and we just need to eventually add the necessary filtering here, instead of returning the "raw" snapshot schema?
There was a problem hiding this comment.
Or maybe this is the right function for that, and we just need to eventually add the necessary filtering here, instead of returning the "raw" snapshot schema?
yea was thinking to keep this as the 'public-facing' snapshot schema. not sure if you think we should keep the name logical_schema still or just snapshot_schema? WDYT?
There was a problem hiding this comment.
This feels like a C way of doing snapshot.logical_schema() vs snapshot.snapshot_schema(). I prefer logical schema cuz snapshot_schema feels redundant.
What changes are proposed in this pull request?
This PR removes the old
visit_snapshot_schemaintroduced in #683 - we should just go ahead and do the 'right thing' with having avisit_schema(introduced in the other PR) and alogical_schema()function (added here) in order to facilitate visiting the snapshot schema. Additionally I've moved the schema-related things up fromscanmodule to top-level in ffi crate. Exact changes listed below; this PR updates tests/examples to leverage the new changes.This PR affects the following public APIs
visit_snapshot_schema()APIlogical_schema(snapshot)API so you can get the schema of a snapshot and use thevisit_schemadirectlyfree_global_read_schemato justfree_schemaSharedSchemaandfree_schemaup frommod scaninto top-levelfficrate.How was this change tested?
existing UT