feat(python, rust): add feature operation
#2712
Conversation
github-actions
bot
added
binding/python
rtyler
force-pushed
the
feat/add_feature_operation
branch
from
c743970
to
eeb903f
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2712 +/- ##
==========================================
+ Coverage 72.40% 72.42% +0.02%
==========================================
Files 131 133 +2
Lines 40011 40238 +227
Branches 40011 40238 +227
==========================================
+ Hits 28970 29143 +173
- Misses 9165 9242 +77
+ Partials 1876 1853 -23 ☔ View full report in Codecov by Sentry. |
ion-elgreco
force-pushed
the
feat/add_feature_operation
branch
from
edc67b6
to
6e78f47
Compare
ion-elgreco
requested review from
wjones127,
fvaleye,
roeap,
rtyler and
hntd187
as code owners
| /// High level table features | ||
| #[derive(Serialize, Deserialize, Debug, Clone, Eq, PartialEq, Hash)] | ||
| #[serde(rename_all = "camelCase")] | ||
| pub enum TableFeatures { |
There was a problem hiding this comment.
What should the relation between this enum and the DeltaConfigKey in src/table/config.rs be? 🤔 😕
| } | ||
|
|
||
| /// Specify the features to be added | ||
| pub fn with_features<S: Into<TableFeatures>>(mut self, name: Vec<S>) -> Self { |
There was a problem hiding this comment.
Just generally curious (this is by no means wrong), but why is the generic S ?
There was a problem hiding this comment.
I don't remember haha
There was a problem hiding this comment.
I generally like what this does, but I have some questions about it.
- If I understand correctly, this only allows you to add features, not remove them right?
- This doesn't appear to have any mapping of the feature to their specific reader/writer versions correct? Like if you enable CDF, that's 4,1 would this just make the protocol versions 7,3?
| commit_properties: CommitProperties, | ||
| } | ||
|
|
||
| impl super::Operation<()> for AddTableFeatureBuilder {} |
There was a problem hiding this comment.
Nothing at the moment, but @rtyler wanted to use this at some point
There was a problem hiding this comment.
It's mostly placebo at the moment, but this ensures that the AddTableFeatureBuilder adheres to the Operation trait. There's more refactoring that's due to that Operation trait to clean things up 😄
| .await | ||
| .unwrap(); | ||
|
|
||
| let current_protocol = &result.protocol().cloned().unwrap(); |
There was a problem hiding this comment.
So what happens if you read a table with an unknown reader or writer property? Like a databricks table that has some new cool internal table feature in it. Does this fail?
There was a problem hiding this comment.
It won't fail I think, since reader/writerFeature enum has a other, so those probably will end up in there, and just get tagged alon
|
… purpose This also necessitates a version bump of all the crates!
rtyler
force-pushed
the
feat/add_feature_operation
branch
from
d713ccd
to
e5fd2bc
Compare
Description
Adds a new TableFeatures enum, that is a collection of all table features irrespective of reader/writer variant. Users simply select a feature they want, and we handle whether it needs to be added in readerFeatures or writerFeatures.
TableFeatures enum is exposed to python as well.
Related Issue(s)
Documentation