Add `LogSegmentBuilder` to construct `LogSegment`s #457

OussamaSaoudi-db · 2024-11-06T20:11:13Z

What changes are proposed in this pull request?

This pull request builds on #438 by creating a LogSegmentBuilder. This moves all logic for building a LogSegment out of Snapshot. This change is made in anticipation of TableChanges, which will represent CDFs and must construct its own LogSegment.

The builder allows you to specify the following:

A starting checkpoint
The order of the commit files
The start version
The end version
Whether checkpoint files are omitted from the LogSegment.

How was this change tested?

New tests are added to check the following:

Speciying start_version and start_checkpoint results in an error
Non contiguous commit files should result in an error
commit file ordering is correct in ascending and descending cases
Checkpoint files are correctly omitted if with_omit_checkpoint_files is specified.
start and end version work.

Log segment is going to be used by both snapshot and table changes. It makes sense to separate it into its own module

codecov · 2024-11-06T20:15:00Z

Codecov Report

Attention: Patch coverage is 92.15686% with 36 lines in your changes missing coverage. Please review.

Project coverage is 79.97%. Comparing base (bb1be88) to head (f8457d9).

Files with missing lines	Patch %	Lines
kernel/src/log_segment.rs	92.47%	24 Missing and 10 partials ⚠️
kernel/src/snapshot.rs	71.42%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #457      +/-   ##
==========================================
+ Coverage   79.60%   79.97%   +0.37%     
==========================================
  Files          57       57              
  Lines       12330    12585     +255     
  Branches    12330    12585     +255     
==========================================
+ Hits         9815    10065     +250     
- Misses       1987     1993       +6     
+ Partials      528      527       -1

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

…nto snapshot_cleanup

Co-authored-by: Ryan Johnson <[email protected]>

nicklan

lgtm with one change you should make

kernel/src/log_segment.rs

zachschuermann

few final comments - can we also add a test with a start_checkpoint and an end_version

kernel/src/log_segment.rs

zachschuermann · 2024-11-14T00:05:31Z

Oh and note that LogSegmentBuilder is pub(crate) with developer-visibility pub but everything in it is just pub(crate) maybe we just make it all only pub(crate) for now?

Co-authored-by: Zach Schuermann <[email protected]>

zachschuermann

LGTM thanks for taking some time for this! did we add test from above for a start_checkpoint and an end_version check?

kernel/src/log_segment.rs

scovich · 2024-11-14T12:52:14Z

kernel/src/log_segment.rs

+///     2. Commit file versions will be greater than or equal to `start_version` if specified.
+///     3. If checkpoint(s) is/are present in the range, only commits with versions greater than the most
+///        recent checkpoint version are retained. Checkpoints can be omitted (and this rule skipped)
+///        whenever the `LogSegment` is created. See `LogSegmentBuilder::with_omit_checkpoint_files`.


That doesn't exactly roll off the tongue... should be call it without_checkpoint_files?

scovich · 2024-11-14T12:52:58Z

kernel/src/log_segment.rs

+/// - For a Snapshot at version `n`: Its LogSegment is made up of zero or one checkpoint, and all
+///   commits between the checkpoint and the end version `n`.
+/// - For a TableChanges between versions `a` and `b`: Its LogSegment is made up of zero
+/// checkpoints and all commits between versions `a` and `b`


Suggested change

/// checkpoints and all commits between versions `a` and `b`

/// checkpoints and all commits between versions `a` and `b` (inclusive)

scovich · 2024-11-14T12:56:12Z

kernel/src/log_segment.rs

-    /// checkpoint files in the log segment.
-    pub checkpoint_files: Vec<ParsedLogPath>,
+    /// Checkpoint files in the log segment.
+    pub checkpoint_parts: Vec<ParsedLogPath>,
 }

 impl LogSegment {


aside: can we rename LogSegment::replay to something else? I don't think the name was ever super accurate ("replay" jargon in Delta normally implies dedup logic that is missing here). Really this code is just producing a chunked iterator of log actions.

sounds good - let me take this in my follow up :)

scovich · 2024-11-14T12:59:27Z

kernel/src/log_segment.rs

-                commit_read_schema,
-                meta_predicate.clone(),
-            )?
+            .read_json_files(&commit_files, commit_read_schema, meta_predicate.clone())?


Now that I think about it, I'm surprised these read_xxx_files methods didn't already take FileMeta. Do we have a tracking item to consider changing that?

I believe these take a slice of FileMeta (location is a FileMeta - perhaps we need a rename there?)

Ohhhh! I worried for a moment that we were passing just file names. You're right tho, location is a FileMeta now in this PR.

scovich · 2024-11-14T13:01:02Z

kernel/src/log_segment.rs

+    pub(crate) fn new() -> Self {
+        LogSegmentBuilder {
+            start_checkpoint: None,
+            start_version: None,
+            end_version: None,
+            sort_commit_files_ascending: false,
+            omit_checkpoint_files: false,
+        }
+    }


Should we just #[derive(Default)]?

scovich · 2024-11-14T13:02:15Z

kernel/src/log_segment.rs

+    /// Note: Either `start_version` or `start_checkpoint` may be specified.  Attempting to build a [`LogSegment`]
+    /// with both will result in an error.


In the builder model, is it considered better to catch errors when injected to the builder? Or to wait until the end and check everything at once?

Also, @zachschuermann had a good point IMO:

I'd like to reconsider having some sort of bifurcation for a snapshot segment vs. a cdf segment.

Have we considered whether it might be simpler to have two builders that both produce LogSegment but which take different sets of options? So much of the logic in the build method is just trying to wrangle the various combinations of options, which is super error prone because it's combinatorial.

Split logic analysis

For a snapshot log segment, we have the following:

input parameters, both optional:

checkpoint hint (None means list from zero)

end version (Some means time travel)

Note: Some checkpoint hint above Some end version is very possible and should cause it to be ignored

listing behavior

prefer to use the checkpoint hint as a listing start (list from 0 otherwise)

terminate the listing if a file above end version is found

filter out incomplete checkpoints (multi-part checkpoints may be missing parts)

Only keep the latest complete checkpoint

Only keep log files with higher versions than the latest complete checkpoint (if any)

output

0+ checkpoint part files, all the same version

0+ log files in contiguous descending version order

If a checkpoint is present, it is the next version below the lowest log file version

For a CDF log segment we have the following:

input parameters

start version (required)

end version (optional)

Note: Some end version below the start version is an error and should fail the build()

listing behavior

use the start version as listing start

terminate the listing if a file above end version is found

output

end-start+1 log files in contiguous ascending version order

A nice feature of the split is that the number of possible scenarios, and possible error cases, is vastly reduced. Fewer possible wrong combos means fewer checks to implement means fewer checks that could be implemented incorrectly.

I even start to wonder if we need builders at all, vs. two log segment "constructor" methods?

impl LogSegment { pub(crate) fn for_snapshot( fs_client: &dyn FileSystemClient, table_root: &Url, checkpoint_hint: Option<_>, time_travel_version: Option<Version>, ) -> DeltaResult<Self>; pub(crate) fn for_cdf( fs_client: &dyn FileSystemClient, table_root: &Url, start: Version, end: Option<Version>, ) -> DeltaResult<Self>; // private internal constructor that validates the files it is given fn try_new(checkopint_files: Vec<_>, commit_files: Vec<_>) -> DeltaResult<Self>;

This all suggests that we DO want two independent builders, backed by some common helper methods:

lower-bounded listing: (lower bound, Option) -> Iterator

log segment constructor that verifies its files are gap-free

Consumers of a log segment who need reverse-ordered files (vs. whatever we decide is the "natural" order) should probably just use Iterator::rev?

The main redundancy at that point is that both builders accept an optional upper bound version... and even they are slightly different concepts (tho the effect on listing behavior implementation is the same in practice).

Hey @scovich this is great thanks for the thorough ideas here - do you think I can take this as a follow-up after this gets merged just to unblock @OussamaSaoudi-db's CDF work? I was thinking of something of this sort but ultimately didn't want to derail a ton an spend more time on this. Hopefully these changes will just require only minor modifications to how LogSegments are created so seems like I could work on this as @OussamaSaoudi-db moves on.

Regarding the actual implementation for this: I think I totally agree on everything here - but additionally, I'm wondering if we want two different kinds of LogSegments. We know that a LogSegment is some continuous chunk of commits/checkpoint but we have different invariants based on the kind right? CDF LogSegments only ever have commits, whereas snapshot LogSegments have either commits/checkpoint or only commits. and ordering could/should be different between them?

We could still have a unified LogSegment with sorted_commit_files, checkpoint_parts, and commit_file_sort_order which I think would capture both, but want to bring up the possibility of an enum LogSegment which has LogSegment::CDF and LogSegment::Snapshot? but maybe that's getting overly specific/specialized and we want to keep it more general? curious to hear your thoughts :)

issue here: #476

Oh and yea I agree reversing the vec on consumer side is probably better - just read that part

scovich · 2024-11-14T13:07:08Z

kernel/src/log_segment.rs

+    pub(crate) fn with_start_checkpoint(mut self, start_checkpoint: CheckpointMetadata) -> Self {
+        self.start_checkpoint = Some(start_checkpoint);
+        self
+    }
+    /// Optionally provide a checkpoint metadata to start the log segment. See [`LogSegmentBuilder::with_start_checkpoint`]
+    /// for details. If `start_checkpoint` is `None`, this is a no-op.
+    ///
+    /// Note: Either `start_version` or `start_checkpoint` may be specified.  Attempting to build a [`LogSegment`]
+    /// with both will result in an error.
+    pub(crate) fn with_start_checkpoint_opt(


IMO we should have one "right way" of specifying options and stick to it, rather than providing multiple versions of the same method. Two ideas gleaned from other similar situations:

One method that takes Option<Foo> -- it's easier for unconditional callers to wrap the arg in Some than it is for conditional callers to break the builder chain with if let Some(...) control flow.

One method that takes impl Into<Option<Foo>> and callers can choose what to pass

I like the impl Into<Option<Foo>> approach!

scovich · 2024-11-14T14:13:09Z

kernel/src/log_segment.rs

+/// The commit files are guaranteed to be sorted in ascending order by version. The elements of
+/// `checkpoint_parts` are all the parts of the same checkpoint. Checkpoint parts share the same
+/// version.
+fn list_log_files_with_version(


This method was just moved (unchanged) from snapshot.rs?
Same for list_log_files_with_checkpoint below?

list_log_files_with_checkpoint and list_log_files_with_version actually had much of the same code twice. I've merged the two so that you list and process files in one place.

Co-authored-by: Zach Schuermann <[email protected]>

…kernel-rs into list_log_cleanup

scovich · 2024-11-14T19:23:45Z

kernel/src/log_segment.rs

    pub(crate) fn new() -> Self {
-        LogSegmentBuilder {
-            start_checkpoint: None,
-            start_version: None,
-            end_version: None,
-            sort_commit_files_ascending: false,
-            omit_checkpoint_files: false,
-        }
+        Self::default()


Do we still need a new method at all? I get mixed messages from other rust code, e.g. Vec::new is the same as Vec::default (same for HashMap).

scovich · 2024-11-14T19:28:20Z

kernel/src/log_segment.rs

-    /// Note: Either `start_version` or `start_checkpoint` may be specified.  Attempting to build a [`LogSegment`]
-    /// with both will result in an error.
-    pub(crate) fn with_start_checkpoint_opt(
+    pub(crate) fn with_start_checkpoint(


nit?

Suggested change

pub(crate) fn with_start_checkpoint(

pub(crate) fn with_checkpoint_hint(

…hanges (#495)  ## What changes are proposed in this pull request?  This introduces two methods to construct `LogSegment`. The first is constructing LogSegment for Snapshots using `LogSegment::for_snapshot`. The second constructs LogSegment for the upcoming `TableChanges` type. This PR also refactors log listing functions to reduce duplication in the code. We do so by creating a function `get_parsed_log_files_iter` to list, filter, and parse log files. This adds a test function to `test-utils` called `delta_path_for_multipart_checkpoint`. This function can be used to create a multipart checkpoint path. This replaces the changes proposed in #457  ## How was this change tested?  This change introduces tests for the following: - reading log with out of date checkpoint hint - reading log with up to date checkpoint hint - creating snapshot log segment without a checkpoint hint - Creating snapshot with a multi-part checkpoint - Multipart checkpoint with incorrect number of parts fails. - creating snapshot with a start checkpoint and an end time travel version - Creating a snapshot with a checkpoint hint higher than the time travel version - Creating log segments for table changes - Checking contiguity of the log is always preserved. - Checking that `for_table_changes` fails when the start version > end_version This PR also adds an ignored test that checks for desired behaviour. The test `build_snapshot_with_missing_checkpoint_part_no_hint` checks that an incomplete checkpoint is not used in a LogSegment. A checkpoint is incomplete if it does not have all the parts specified in `LogPathFileType::MultiPartCheckpoint.num_parts`. --------- Co-authored-by: Ryan Johnson <[email protected]> Co-authored-by: Zach Schuermann <[email protected]>

zachschuermann · 2024-11-16T00:12:06Z

done in #495

OussamaSaoudi-db added 7 commits October 28, 2024 15:15

Move log segment into separate module

c7913dc

Log segment is going to be used by both snapshot and table changes. It makes sense to separate it into its own module

Fix tests, make fields pub

6b331ac

Improve comments

f1f9886

Remove table changes

8122113

Merge branch 'main' into snapshot_cleanup

46185ae

change visibility

471a858

Merge branch 'main' into snapshot_cleanup

4ac35da

OussamaSaoudi-db changed the title ~~List log cleanup~~ [WIP] Add LogSegmentBuilder to construct LogSegments Nov 6, 2024

github-actions bot added the breaking-change Change that will require a version bump label Nov 6, 2024

OussamaSaoudi-db and others added 20 commits November 6, 2024 12:55

Merge branch 'main' into snapshot_cleanup

6297805

Merge remote-tracking branch 'refs/remotes/origin/snapshot_cleanup' i…

e569719

…nto snapshot_cleanup

Merge branch 'main' into snapshot_cleanup

5edf4db

Remove old log segment

0b8463a

fix failing tests

5300a7b

Get rid of warnings

81d0de0

Fix failing tests

6b85932

Apply suggestions from code review

1384ea3

Co-authored-by: Ryan Johnson <[email protected]>

Address more pr comments

0182326

fix imports

d053a77

rebase onto git changes

52f57e5

address nits

aa6c9f4

fix visibility issue

dca491c

Use LogSegmentBuilder

bf5cdd4

Introduce start and end versions

748bab9

Remove old code

2a6eb3e

Fix failing tests

841f17f

Most up to date logsegment

1ce29d8

Fix failing test and remove unnecessary code

a2f9810

remove table changes from this commit

5f7a680

OussamaSaoudi-db requested a review from zachschuermann November 13, 2024 23:07

OussamaSaoudi-db added 4 commits November 13, 2024 15:09

fix comment

4c9c96b

Update comments

a53f38a

error message

b0b6514

Merge branch 'main' into list_log_cleanup

5654a6d

nicklan approved these changes Nov 13, 2024

View reviewed changes

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

OussamaSaoudi-db added 3 commits November 13, 2024 15:32

Use delta_path_for_version instead of get_path

2630a9e

Add reference import

db5dddb

Merge branch 'main' into list_log_cleanup

6f9b459

zachschuermann reviewed Nov 14, 2024

View reviewed changes

kernel/src/log_segment.rs Show resolved Hide resolved

kernel/src/log_segment.rs Show resolved Hide resolved

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

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

OussamaSaoudi-db and others added 7 commits November 13, 2024 16:13

Apply suggestions from code review

a1531ee

Co-authored-by: Zach Schuermann <[email protected]>

Update kernel/src/log_segment.rs

33c4039

Co-authored-by: Zach Schuermann <[email protected]>

More nits

1f077b0

Merge branch 'main' into list_log_cleanup

bee5659

Return dev visibility

9d0a24f

make builder methods match builder visibility

f8457d9

Fix doc issue

9c44a69

zachschuermann approved these changes Nov 14, 2024

View reviewed changes

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

zachschuermann assigned OussamaSaoudi-db Nov 14, 2024

scovich requested changes Nov 14, 2024

View reviewed changes

OussamaSaoudi-db and others added 4 commits November 14, 2024 09:57

Address nits

3a214de

Update kernel/src/log_segment.rs

d5adae1

Co-authored-by: Zach Schuermann <[email protected]>

add test, fix bug

6a5484c

Merge branch 'list_log_cleanup' of github.com:OussamaSaoudi-db/delta-…

1f5b7f1

…kernel-rs into list_log_cleanup

scovich reviewed Nov 14, 2024

View reviewed changes

OussamaSaoudi-db mentioned this pull request Nov 15, 2024

Add methods for constructing LogSegment for Snapshot and for TableChanges #495

Merged

zachschuermann closed this Nov 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `LogSegmentBuilder` to construct `LogSegment`s #457

Add `LogSegmentBuilder` to construct `LogSegment`s #457

OussamaSaoudi-db commented Nov 6, 2024 •

edited

Loading

codecov bot commented Nov 6, 2024 •

edited

Loading

nicklan left a comment

zachschuermann left a comment

zachschuermann commented Nov 14, 2024

zachschuermann left a comment

scovich Nov 14, 2024

scovich Nov 14, 2024

scovich Nov 14, 2024

zachschuermann Nov 14, 2024

scovich Nov 14, 2024

zachschuermann Nov 14, 2024

scovich Nov 14, 2024

scovich Nov 14, 2024

scovich Nov 14, 2024

scovich Nov 14, 2024 •

edited

Loading

zachschuermann Nov 14, 2024

zachschuermann Nov 14, 2024

zachschuermann Nov 14, 2024

scovich Nov 14, 2024

OussamaSaoudi-db Nov 14, 2024

scovich Nov 14, 2024

OussamaSaoudi-db Nov 14, 2024

scovich Nov 14, 2024

scovich Nov 14, 2024

zachschuermann commented Nov 16, 2024

	/// checkpoints and all commits between versions `a` and `b`
	/// checkpoints and all commits between versions `a` and `b` (inclusive)

		/// Note: Either `start_version` or `start_checkpoint` may be specified. Attempting to build a [`LogSegment`]
		/// with both will result in an error.

	pub(crate) fn with_start_checkpoint(
	pub(crate) fn with_checkpoint_hint(

Add LogSegmentBuilder to construct LogSegments #457

Add LogSegmentBuilder to construct LogSegments #457

Conversation

OussamaSaoudi-db commented Nov 6, 2024 • edited Loading

What changes are proposed in this pull request?

How was this change tested?

codecov bot commented Nov 6, 2024 • edited Loading

Codecov Report

nicklan left a comment

Choose a reason for hiding this comment

zachschuermann left a comment

Choose a reason for hiding this comment

zachschuermann commented Nov 14, 2024

zachschuermann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scovich Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zachschuermann commented Nov 16, 2024

Add `LogSegmentBuilder` to construct `LogSegment`s #457

Add `LogSegmentBuilder` to construct `LogSegment`s #457

OussamaSaoudi-db commented Nov 6, 2024 •

edited

Loading

codecov bot commented Nov 6, 2024 •

edited

Loading

scovich Nov 14, 2024 •

edited

Loading