feat!: add in-commit timestamps table properties #558
Conversation
github-actions
bot
added
the
breaking-change
|
Mention: #559 |
zachschuermann
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #558 +/- ##
==========================================
+ Coverage 84.08% 84.09% +0.01%
==========================================
Files 77 77
Lines 17777 17805 +28
Branches 17777 17805 +28
==========================================
+ Hits 14948 14974 +26
Misses 2115 2115
- Partials 714 716 +2 ☔ View full report in Codecov by Sentry. |
zachschuermann
requested review from
scovich and
OussamaSaoudi
and removed request for
OussamaSaoudi-db
| props.enable_in_commit_timestamps = Some(parse_bool(v)?) | ||
| } | ||
| "delta.inCommitTimestampEnablementVersion" => { | ||
| props.in_commit_timestamp_enablement_version = Some(parse_uint(v)?) |
There was a problem hiding this comment.
parse_uint already returns an option. Why not just:
| props.in_commit_timestamp_enablement_version = Some(parse_uint(v)?) | |
| props.in_commit_timestamp_enablement_version = parse_uint(v) |
There was a problem hiding this comment.
ah i actually had to think about this for a sec too lol - we actually want to return None in the case it doesn't parse. if we leave it as just parse_uint(v) then it will set the value to None and return Some
perhaps we should rethink this code and try to rewrite it more nicely considering both you and I have now stumbled on it :)
There was a problem hiding this comment.
Ah you're right! Thx for the clarification
There was a problem hiding this comment.
This also took a solid minute to grok, does the class/method have a doc comment explaining the intended behavior?
Basically, we want the entire method to return None if a required property fails to parse (indicating failure) -- not merely set that property to None (it was probably None already)?
There was a problem hiding this comment.
yep exactly. added a comment to clarify but given this has tripped up three of us now I think we should rewrite lol
made a quick issue: #682
zachschuermann
changed the title
| props.enable_in_commit_timestamps = Some(parse_bool(v)?) | ||
| } | ||
| "delta.inCommitTimestampEnablementVersion" => { | ||
| props.in_commit_timestamp_enablement_version = Some(parse_uint(v)?) |
There was a problem hiding this comment.
This also took a solid minute to grok, does the class/method have a doc comment explaining the intended behavior?
Basically, we want the entire method to return None if a required property fails to parse (indicating failure) -- not merely set that property to None (it was probably None already)?
| assert_eq!(parse_non_negative::<u64>("12").unwrap(), 12); | ||
| assert_eq!(parse_non_negative::<u64>("0").unwrap(), 0); | ||
| assert_eq!(parse_non_negative::<u64>("-12"), None); | ||
| assert_eq!(parse_non_negative::<i64>("12").unwrap(), 12); |
There was a problem hiding this comment.
post-hoc nit:
assert_eq!(parse_non_negative("12").unwrap(), 12_i64);(make type inference work for you)
What changes are proposed in this pull request?
Parse:
delta.enableInCommitTimestamps,delta.inCommitTimestampEnablementVersion, anddelta.inCommitTimestampEnablementTimestampand expose table property methods to access.This PR affects the following public APIs
New pub fields in
TableProperties:enable_in_commit_timestampsin_commit_timestamp_enablement_versionin_commit_timestamp_enablement_timestampHow was this change tested?
new UT