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.

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

feat!: add in-commit timestamps table properties #558

Merged
merged 8 commits into from
Feb 6, 2025
37 changes: 21 additions & 16 deletions kernel/src/table_properties/deserialize.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ where
// attempt to parse a key-value pair into a `TableProperties` struct. Returns Some(()) if the key
// was successfully parsed, and None otherwise.
fn try_parse(props: &mut TableProperties, k: &str, v: &str) -> Option<()> {
// NOTE!! we do Some(parse(v)?) instead of just parse(v) because we want to return None if the
// parsing fails. If we simply call 'parse(v)', then we would (incorrectly) return Some(()) and
// just set the property to None.
match k {
"delta.appendOnly" => props.append_only = Some(parse_bool(v)?),
"delta.autoOptimize.autoCompact" => props.auto_compact = Some(parse_bool(v)?),
Expand Down Expand Up @@ -80,10 +83,10 @@ fn try_parse(props: &mut TableProperties, k: &str, v: &str) -> Option<()> {
props.enable_in_commit_timestamps = Some(parse_bool(v)?)
}
"delta.inCommitTimestampEnablementVersion" => {
props.in_commit_timestamp_enablement_version = Some(parse_uint(v)?)
props.in_commit_timestamp_enablement_version = Some(parse_non_negative(v)?)
}
"delta.inCommitTimestampEnablementTimestamp" => {
props.in_commit_timestamp_enablement_timestamp = Some(parse_uint(v)?.try_into().ok()?)
props.in_commit_timestamp_enablement_timestamp = Some(parse_non_negative(v)?)
}
_ => return None,
}
Expand All @@ -94,14 +97,17 @@ fn try_parse(props: &mut TableProperties, k: &str, v: &str) -> Option<()> {
/// if successfully parses, and `None` otherwise.
pub(crate) fn parse_positive_int(s: &str) -> Option<NonZero<u64>> {
// parse as non-negative and verify the result is non-zero
NonZero::new(parse_uint(s)?)
NonZero::new(parse_non_negative(s)?)
}

/// Deserialize a string representing a non-negative integer into an `Option<u64>`. Returns `Some` if
/// successfully parses, and `None` otherwise.
pub(crate) fn parse_uint(s: &str) -> Option<u64> {
// parse to i64 (then to u64) since java doesn't even allow u64
let n: i64 = s.parse().ok()?;
pub(crate) fn parse_non_negative<T>(s: &str) -> Option<T>
where
i64: TryInto<T>,
{
// parse to i64 since java doesn't even allow u64
let n: i64 = s.parse().ok().filter(|&i| i >= 0)?;
n.try_into().ok()
}

Expand Down Expand Up @@ -214,18 +220,17 @@ mod tests {
assert_eq!(parse_bool("whatever"), None);
}

#[test]
fn test_parse_positive_int() {
assert_eq!(parse_positive_int("123").unwrap().get(), 123);
assert_eq!(parse_positive_int("0"), None);
assert_eq!(parse_positive_int("-123"), None);
}

#[test]
fn test_parse_int() {
assert_eq!(parse_uint("123").unwrap(), 123);
assert_eq!(parse_uint("0").unwrap(), 0);
assert_eq!(parse_uint("-123"), None);
assert_eq!(parse_positive_int("12").unwrap().get(), 12);
assert_eq!(parse_positive_int("0"), None);
assert_eq!(parse_positive_int("-12"), None);
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);
Copy link
Collaborator

@scovich scovich Feb 6, 2025

Choose a reason for hiding this comment

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

post-hoc nit:

assert_eq!(parse_non_negative("12").unwrap(), 12_i64);

(make type inference work for you)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah thanks!

assert_eq!(parse_non_negative::<i64>("0").unwrap(), 0);
assert_eq!(parse_non_negative::<i64>("-12"), None);
}

#[test]
Expand Down