feat: typed commit info #1207
feat: typed commit info #1207
Conversation
roeap
requested review from
houqp,
xianwill,
wjones127,
fvaleye,
rtyler and
mosyp
as code owners
rust/src/action/mod.rs
Outdated
| /// Version number the commit corresponds to | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub version: Option<DeltaDataTypeVersion>, |
There was a problem hiding this comment.
It is, and nobody actually writes it to the commit info in the log. Initially I was following the spark implementation for the optimistic commits quite closely, where this field is being used. Not sure if we still do though. I'll remove it for now, and bring it back in case its needed in the follow up PR...
| pub version: Option<DeltaDataTypeVersion>, | ||
| /// Timestamp in millis when the commit was created | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub timestamp: Option<DeltaDataTypeTimestamp>, |
There was a problem hiding this comment.
As an aside, it would be cool if we swapped out DeltaDataTypeTimestamp for a type whose Debug / Display implementation is a formatted timestamp rather than an integer.
There was a problem hiding this comment.
Absolutely, we may also be able to implement custom serde, so it is parsed as a datetime.
rust/src/operations/transaction.rs
Outdated
| pub(crate) async fn prepare_commit( | ||
| storage: &dyn ObjectStore, | ||
| operation: &DeltaOperation, | ||
| actions: &mut Vec<Action>, |
There was a problem hiding this comment.
Why is this changed to a &mut instead of just an owned value?
There was a problem hiding this comment.
I extracted this from the optimistic commit PR where we are using this in a loop and need access to the actions afterwards. That said, having this mutable in that scenario is likely not the best idea, and we need it only to add the commit info. Changed is so we just take a borrowed value now.
# Description Another PR on the road to delta-io#632 - ~~keeping it a draft, as it is based on delta-io#1206~~ While the `commitInfo` action is defined as completely optional, spark and delta-rs write at the very least interesting, but often also quite helpful information into the commit info. To make it easier to work with and centralize some conventions, we introduce a `CommitInfo` struct, that exposes some of the fields at the top level. Additionally we harmonize a bit between spark and delta-rs conventions. # Related Issue(s) part of delta-io#632 # Documentation <!--- Share links to useful documentation ---> --------- Co-authored-by: Will Jones <[email protected]>
Description
Another PR on the road to #632 -
keeping it a draft, as it is based on #1206While the
commitInfoaction is defined as completely optional, spark and delta-rs write at the very least interesting, but often also quite helpful information into the commit info. To make it easier to work with and centralize some conventions, we introduce aCommitInfostruct, that exposes some of the fields at the top level. Additionally we harmonize a bit between spark and delta-rs conventions.Related Issue(s)
part of #632
Documentation