feat: Add log segment constructor for timestamp to version conversion #895

OussamaSaoudi · 2025-04-28T05:12:28Z

What changes are proposed in this pull request?

This is a stacked PR. Please look at the LAST commit of each PR for the files changed.

LogSegment constructor for timestamp conversion
- Commit to timestamp helper function
  - ICT search bounds checking
    - High level timestamp conversion APIs

This PR adds a LogSegment constructor that will be used in timestamp to version conversion. The function LogSegment::for_timestamp_conversion creates a LogSegment made entirely of commits up to an optional end version.

LogSegment::for_timestamp_conversion goes back to the earliest possible commit while maintaining contiguity of the LogSegment.

How was this change tested?

This checks that we construct a log segment in the following cases:

A normal range of commits
A delta log that is missing a commit.
A delta log that only has a checkpoint.

codecov · 2025-04-28T05:14:54Z

Codecov Report

Attention: Patch coverage is 75.51020% with 12 lines in your changes missing coverage. Please review.

Project coverage is 84.66%. Comparing base (717fd48) to head (a4318bf).

Files with missing lines	Patch %	Lines
kernel/src/log_segment.rs	84.21%	4 Missing and 2 partials ⚠️
kernel/src/log_segment/tests.rs	45.45%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #895      +/-   ##
==========================================
- Coverage   84.68%   84.66%   -0.02%     
==========================================
  Files          92       92              
  Lines       23025    23074      +49     
  Branches    23025    23074      +49     
==========================================
+ Hits        19499    19536      +37     
- Misses       2564     2574      +10     
- Partials      962      964       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

scovich · 2025-04-28T22:02:22Z

kernel/src/log_segment.rs

+        log_root: Url,
+        end_version: Option<Version>,
+    ) -> DeltaResult<Self> {
+        let ascending_commit_files: Vec<_> = list_log_files(storage, &log_root, None, end_version)?


Listing without a hint can be really painfully slow on a big table. But guess that's unavoidable, since the resulting log segment is supposed to cover the entire reachable history of the table? That said, an optimization in delta-spark lets callers limit the amount of history they want to retrieve. Then, use either an existing snapshot or the _last_checkpoint hint to get a guess of where the end of the table is, and start listing from max(0, hint.version - limit). It might produce a few extra versions if the hint was stale, but is generally quite effective.

aha interesting! The LogHistoryManager is supposed to take a Snapshot anyway (for TableConfig), so I can totally get that hint.version. I'll try to incorporate limit 👍

Updated to use limit and require the end_version.

scovich · 2025-04-28T22:11:21Z

kernel/src/log_segment.rs

+        let mut contiguous_commits = ascending_commit_files
+            .into_iter()
+            // Start from the last commit
+            .rev()
+            .fold_while(vec![], |mut out, commit| {


I think it will be simpler to process a non-materialized listing in arrival order:

let ascending_commit_files = list_log_files(storage, &log_root, None, end_version)? .filter_ok(ParsedLogPath::is_commit); let mut contiguous_commits = vec![]; for commit in ascending_commit_files { // Should we actually blow up on error? Or just skip it and let the next commit // treat it like a gap, if it was trying to be a commit file? (if it was not trying // to be a commit file, we probably don't even care?) let commit = commit?; if contiguous_commits .last() .is_some_and(|prev_commit| prev_commit.version != commit.version - 1) { // We found a gap, so throw away all earlier versions contiguous_commits.clear(); } contiguous_commits.push(commit); } // If we have a non-empty commit list and a requested end version, verify they match. // // NOTE: No need to check for an empty commit list, `LogSegment::try_new` fails in that case. match contiguous_commits.last().zip(end_version) { Some((last_commit, end_version)) if last_commit.version != end_version => { // warning or error? } _ => (), } LogSegment::try_new(contiguous_commits, vec![], log_root, end_version)

// Should we actually blow up on error? Or just skip it and let the next commit // treat it like a gap, if it was trying to be a commit file? (if it was not trying // to be a commit file, we probably don't even care?)

This is a place where better error types would make these decisions easier 😔 I'll take a look and see what kinds of errors we're returning there and see if we can ignore some.

For parsed file names, specifically, we have four cases:

The file is a valid commit (happy path)

It's a valid file type, but not a commit (filter it out and move on)

The file is just plain invalid (unrecognized type, version can't parse, etc). Not relevant to commit gap searches, we should ignore it.

The file attempts to be a commit, but is invalid (e.g. version couldn't parse). This one is slightly tricky, but we should probably treat it like a gap. Nobody will read it as a commit later, but they may still be able to read whatever came after it so we should keep going.

kernel/src/log_segment.rs

zachschuermann

few comments but generally looking good!

zachschuermann · 2025-05-15T23:38:18Z

kernel/src/log_segment.rs

+    /// Constructs a [`LogSegment`] to be used for timestamp conversion. This [`LogSegment`] will consist
+    /// only of contiguous commit files. If an `end_version` is specified, the commit range will
+    /// include commits up to the `end_version` commit (inclusive). If present, `limit` specifies the
+    /// maximum length of the returned LogSegment.
+    pub(crate) fn for_timestamp_conversion(


I wonder if we should make this specific like for_timestamp_conversion or make slightly more general to just be a LogSegment::new_continuous_commits(...)

I realize we have the existing pattern of for_snapshot or for_table_changes, but I think moving more towards less specific constructors may be useful

The distinction is that table_changes requires that the LogSegment contains [start_version,end_version].

On the other hand, timestamp conversion usually cares about an end version, and a maximum log segment size (limit). It's not necessary for the logsegment to have everything in [(end_version-limit), end_version].

zachschuermann · 2025-05-15T23:39:04Z

kernel/src/log_segment.rs

+        storage: &dyn StorageHandler,
+        log_root: Url,
+        end_version: Version,
+        limit: Option<usize>,


why usize? should we just do u64?

logically, the limit is the "max size of the log segment". I'm also using it as the default length of VecDeque, which demands a usize instead of u64. I also compare limit against the length down belowcontiguous_commits.len() > limit, and this also demands a usize.

usize works better more often in this function.

Also, should we consider limit: Option<NonZero<usize>>? A limit of zero is nonsensical and would anyway break the log segment constructor. Downside is, NonZero is kind of a pain to use in practice.

Yeah I'm trying NonZero out and it's a bit annoying. But given that it is an internal api, I think it's fine to use for that extra safety.

zachschuermann · 2025-05-15T23:45:11Z

kernel/src/log_segment.rs

+    /// Constructs a [`LogSegment`] to be used for timestamp conversion. This [`LogSegment`] will consist
+    /// only of contiguous commit files. If an `end_version` is specified, the commit range will
+    /// include commits up to the `end_version` commit (inclusive). If present, `limit` specifies the
+    /// maximum length of the returned LogSegment.


does limit reduce the length we 'look back'?

is the log segment always one of:

[0, latest] no limit, no end

(latest-limit, latest] limit, no end

(end-limit, end] limit, end

oh just realized end_version isn't optional - should it be? or we just think the caller will be the one to always decide 'latest'? would that necessitate another file listing that we could avoid if we push the Optional version down here?

When doing timestamp conversion, the end version will be fixed. This is because you always need to know the P&M at that end version.

We may do further listings back, in which case we also have a fixed end version.

Ex:

Create LogHistoryManager for snapshot at version x.

List back 1000 versions from x.

To give better intuition:
(end-limit, end] is the largest possible LogSegment that can be produced.

But we may get a shorter one if the commit at (end-limit) is not present, or (end-limit +1), etc.

zachschuermann · 2025-05-15T23:46:00Z

kernel/src/log_segment.rs

+                let Ok(limit) = TryInto::<u64>::try_into(limit) else {
+                    return Err(Error::generic(
+                        "Failed to convert limit into u64
+                         when building log segment in timestamp conversion",
+                    ));
+                };


should we just make it u64 to start? (related comment above)

Given that end_version is Version, we should use that for start_from as well, no?

Currently start_from is an Option<Version>.

zachschuermann · 2025-05-15T23:50:30Z

kernel/src/log_segment.rs

+        // List the commits greater than or equal to `start_from`. For large tables, listing with
+        // `start_from` can be a significant speedup over listing _all_ the files in the log.


maybe move the 'listing from start_from recommendation to above (for docs on limit) where callers will interact with this

zachschuermann · 2025-05-15T23:53:10Z

kernel/src/log_segment.rs

+        // If limit is not specified, then use 10 as a default. Typically, there are 10 commits per
+        // checkpoint, so start with that.
+        let mut contiguous_commits: VecDeque<ParsedLogPath> =
+            VecDeque::with_capacity(limit.unwrap_or(10));


I don't think this is necessarily true (but we actually have this same const elsewhere in the code). We should probably either: (1) consolidate these consts to a central place where we can make our best guess at a useful number or (2) don't guess and remove entirely (just relying on the normal reallocations that Vec(Deque)s use to grow)

related comment: #936 (comment)

cool. Set it to the default if no limit is defined.

zachschuermann · 2025-05-16T00:01:02Z

kernel/src/log_segment.rs

+            // If the number of commits exceeds the limit, remove the earliest one.
+            if limit.is_some_and(|limit| contiguous_commits.len() > limit) {
+                contiguous_commits.pop_front();
+            }


so limit specifies when to start the listing but also how many we will store at once?

I don't think we need this any more, now that the upper bound is required?
Under what circumstances could we end up with more commit files than the limit allows?

Update: Because listing start/end bounds are inclusive, we could end up with limit+1 commits.
Seems like we should just fix that off-by-one error when computing start_from?
Then we could also use a normal Vec here.

scovich · 2025-05-28T13:48:05Z

kernel/src/log_segment.rs

+        let mut contiguous_commits: VecDeque<ParsedLogPath> = match limit {
+            Some(limit) => VecDeque::with_capacity(limit),
+            None => VecDeque::new(),
+        };


nit:

Suggested change

let mut contiguous_commits: VecDeque<ParsedLogPath> = match limit {

Some(limit) => VecDeque::with_capacity(limit),

None => VecDeque::new(),

};

let mut contiguous_commits: VecDeque<ParsedLogPath> =

limit.map_or_else(VecDeque::new, VecDeque::with_capacity);

scovich · 2025-05-28T14:08:03Z

kernel/src/log_segment.rs

+        let start_from = limit
+            .map(|limit| {
+                let Ok(limit) = TryInto::<u64>::try_into(limit) else {
+                    return Err(Error::generic(
+                        "Failed to convert limit into u64
+                         when building log segment in timestamp conversion",
+                    ));
+                };
+                match (limit.cmp(&end_version)) {
+                    Ordering::Less | Ordering::Equal => Ok(end_version - limit),
+                    Ordering::Greater => Ok(0),
+                }
+            })
+            .transpose()?;


Suggested change

let start_from = limit

.map(|limit| {

let Ok(limit) = TryInto::<u64>::try_into(limit) else {

return Err(Error::generic(

"Failed to convert limit into u64

when building log segment in timestamp conversion",

));

};

match (limit.cmp(&end_version)) {

Ordering::Less | Ordering::Equal => Ok(end_version - limit),

Ordering::Greater => Ok(0),

}

})

.transpose()?;

let start_from = limit

.map(|limit| match Version::try_from(limit) {

Ok(limit) if limit > 0 => Ok(Version::saturating_sub(end_version, limit - 1)),

_ => Err(Error::generic(format!(

"Invalid limit {limit} when building log segment in timestamp conversion",

))),

})

.transpose()?;

(fixes an off-by-one error as well)

I wasn't familiar with all the sub varieties. Ty!

scovich · 2025-05-28T14:08:07Z

kernel/src/log_segment.rs

+                let Ok(limit) = TryInto::<u64>::try_into(limit) else {
+                    return Err(Error::generic(
+                        "Failed to convert limit into u64
+                         when building log segment in timestamp conversion",
+                    ));
+                };


Given that end_version is Version, we should use that for start_from as well, no?

scovich · 2025-05-28T14:11:03Z

kernel/src/log_segment.rs

+            // If the number of commits exceeds the limit, remove the earliest one.
+            if limit.is_some_and(|limit| contiguous_commits.len() > limit) {
+                contiguous_commits.pop_front();
+            }


I don't think we need this any more, now that the upper bound is required?
Under what circumstances could we end up with more commit files than the limit allows?

scovich · 2025-05-28T14:15:16Z

kernel/src/log_segment.rs

+            // If the number of commits exceeds the limit, remove the earliest one.
+            if limit.is_some_and(|limit| contiguous_commits.len() > limit) {
+                contiguous_commits.pop_front();
+            }


Update: Because listing start/end bounds are inclusive, we could end up with limit+1 commits.
Seems like we should just fix that off-by-one error when computing start_from?
Then we could also use a normal Vec here.

scovich · 2025-05-28T14:17:49Z

kernel/src/log_segment.rs

+        storage: &dyn StorageHandler,
+        log_root: Url,
+        end_version: Version,
+        limit: Option<usize>,


Also, should we consider limit: Option<NonZero<usize>>? A limit of zero is nonsensical and would anyway break the log segment constructor. Downside is, NonZero is kind of a pain to use in practice.

kernel/src/log_segment.rs

scovich

LGTM

nicklan

lgtm with one nit

nicklan · 2025-06-03T17:06:30Z

kernel/src/log_segment.rs

+
+        for commit_res in list_log_files(storage, &log_root, start_from, Some(end_version))? {
+            let commit = match commit_res {
+                Ok(file) if file.is_commit() => file,


I'd like to get rid of the is_commit method, so we don't have multiple ways of doing the same thing. So maybe:

Suggested change

Ok(file) if file.is_commit() => file,

Ok(file) if matches!(file.file_type, LogPathFileType::Commit) => file,

github-actions bot assigned OussamaSaoudi Apr 28, 2025

github-actions bot added the breaking-change Change that require a major version bump label Apr 28, 2025

OussamaSaoudi force-pushed the history_log_segment branch from 74c1ee6 to deb1026 Compare April 28, 2025 06:43

OussamaSaoudi removed the breaking-change Change that require a major version bump label Apr 28, 2025

OussamaSaoudi marked this pull request as ready for review April 28, 2025 07:13

OussamaSaoudi requested review from nicklan, scovich and zachschuermann and removed request for nicklan April 28, 2025 16:16

scovich reviewed Apr 28, 2025

View reviewed changes

OussamaSaoudi force-pushed the history_log_segment branch from deb1026 to 52fab21 Compare April 29, 2025 07:46

OussamaSaoudi requested a review from scovich April 29, 2025 07:47

github-actions bot added the breaking-change Change that require a major version bump label Apr 29, 2025

OussamaSaoudi force-pushed the history_log_segment branch 5 times, most recently from c651ec6 to a9a9e51 Compare April 30, 2025 18:56

scovich reviewed May 2, 2025

View reviewed changes

kernel/src/log_segment.rs Outdated Show resolved Hide resolved

kernel/src/log_segment.rs Outdated Show resolved Hide resolved

OussamaSaoudi force-pushed the history_log_segment branch from a9a9e51 to 280e2ff Compare May 12, 2025 14:37

OussamaSaoudi requested a review from scovich May 12, 2025 14:37

zachschuermann reviewed May 16, 2025

View reviewed changes

OussamaSaoudi force-pushed the history_log_segment branch 3 times, most recently from 59ed19b to fc8cb58 Compare May 25, 2025 23:25

scovich reviewed May 28, 2025

View reviewed changes

OussamaSaoudi force-pushed the history_log_segment branch 2 times, most recently from 2b58cb3 to 5a64d76 Compare June 2, 2025 06:22

LogSegment for timestamp conversion

a4318bf

OussamaSaoudi force-pushed the history_log_segment branch from 5a64d76 to a4318bf Compare June 2, 2025 06:25

OussamaSaoudi requested review from zachschuermann and scovich June 2, 2025 06:27

scovich approved these changes Jun 2, 2025

View reviewed changes

OussamaSaoudi removed the breaking-change Change that require a major version bump label Jun 2, 2025

OussamaSaoudi changed the title ~~feat: Add log segment helper function for timestamp to version conversion~~ feat: Add log segment constructor for timestamp to version conversion Jun 3, 2025

nicklan approved these changes Jun 3, 2025

View reviewed changes

		// List the commits greater than or equal to `start_from`. For large tables, listing with
		// `start_from` can be a significant speedup over listing _all_ the files in the log.

	Ok(file) if file.is_commit() => file,
	Ok(file) if matches!(file.file_type, LogPathFileType::Commit) => file,

feat: Add log segment constructor for timestamp to version conversion #895

Are you sure you want to change the base?

feat: Add log segment constructor for timestamp to version conversion #895

Uh oh!

Conversation

OussamaSaoudi commented Apr 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes are proposed in this pull request?

How was this change tested?

Uh oh!

codecov bot commented Apr 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zachschuermann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

OussamaSaoudi commented Apr 28, 2025 •

edited

Loading

codecov bot commented Apr 28, 2025 •

edited

Loading