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

OussamaSaoudi-db · 2024-11-15T00:55:03Z

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.

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

…nto snapshot_cleanup

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

zachschuermann

flushing comments

kernel/src/log_segment.rs

zachschuermann · 2024-11-15T02:20:15Z

kernel/src/log_segment.rs

+        // Commit file versions must be greater than the most recent checkpoint version if it exists
+        if let Some(checkpoint_file) = checkpoint_parts.first() {
+            sorted_commit_files.retain(|log_path| checkpoint_file.version < log_path.version);
+        }


should we put this in LogSegment::try_new

The CDF constructor doesn't even have checkpoint files, does it?

I was thinking of try_new as a function that simply validates instead of processing commit/checkpoint files.

@scovich You're right, we filter out all checkpoints.

kernel/src/log_segment.rs

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

scovich

This looks much cleaner than the old code.

kernel/src/log_segment.rs

scovich · 2024-11-15T04:08:47Z

kernel/src/log_segment.rs

+        let version_eff = sorted_commit_files
+            .last()
+            .or(checkpoint_parts.first())
+            .ok_or(Error::MissingVersion)? // TODO: A more descriptive error


This means there are no files at all... so not just a missing version.

Would it make more sense to use an Error::generic here, or create an new error variant?

seems fine to do generic error for now?

"no files in log segment" or EmptyLogSegment new error?

scovich · 2024-11-15T04:09:36Z

kernel/src/log_segment.rs

+        // Commit file versions must be greater than the most recent checkpoint version if it exists
+        if let Some(checkpoint_file) = checkpoint_parts.first() {
+            sorted_commit_files.retain(|log_path| checkpoint_file.version < log_path.version);
+        }


The CDF constructor doesn't even have checkpoint files, does it?

kernel/src/log_segment.rs

scovich · 2024-11-15T04:48:44Z

kernel/src/log_segment.rs

+            latest_checkpoint.version
+        );
+    }
+    if checkpoint_parts.len() != checkpoint_metadata.parts.unwrap_or(1) {


This is an incorrect check (somebody noticed and filed a bug I think). It wrongly fails if a stale checkpoint hint gives a different checkpoint part count than the checkpoint we actually found.

That said, the whole checkpoint listing logic is buggy and needs a rewrite, so maybe just leave it alone and we can land the fixes separately, e.g. #322

This particular one is actually on me. I'd wanted to check that checkpoint parts are correct ,but the case you point out fails. Thx for flagging it!

I do agree we should revisit log segment and give it a more thorough look and give it the time it deserves.

kernel/src/log_segment.rs

scovich · 2024-11-15T04:55:09Z

kernel/src/log_segment.rs

-    /// Sorted commit files in the log segment
-    pub commit_files: Vec<ParsedLogPath>,
+    /// Sorted commit files in the log segment (ascending)
+    pub sorted_commit_files: Vec<ParsedLogPath>,


I'm pretty these used to be sorted in reverse order; did log replay get updated to use a reverse iterator?
(and do we have any tests that could catch such a bug?)

Update: yes, the action iterator was updated with a rev call.

Reverse iterator is in use. I don't have a test for checking that it's reverse sorted because that would require that we replay. This needs a read schema, engine, and some engine data wrangling. This seems out of scope for this PR.

kernel/src/log_segment.rs

scovich · 2024-11-15T05:07:30Z

kernel/src/log_segment.rs

-    /// checkpoint files in the log segment.
-    pub checkpoint_files: Vec<ParsedLogPath>,
+    /// Sorted commit files in the log segment (ascending)
+    pub sorted_commit_files: Vec<ParsedLogPath>,


qq: Would it be more self-describing to call this ascending_commit_files?
(your call, just a wild late-night idea)

hm I agree more self-describing but knee-jerk reaction is it's generally less readable? no strong opinion

scovich

Code looks quite good in general.

Two main questions:

Should we avoid moving code between files in this PR (either do it before or after)?
How to handle ParsedLogPath "error" cases (see comment)

scovich · 2024-11-15T12:57:48Z

kernel/src/log_segment.rs

+
+    // Check that the provided version is less than or equal to the end version if it exists
+    let lte_end_version = move |version: u64| {
+        end_version.is_none() || end_version.is_some_and(|end_version| version <= end_version)


Yes, we probably don't want to take a hard dep on 1.82 so soon (and not for something so minor)

kernel/src/log_segment.rs

scovich · 2024-11-15T13:01:34Z

kernel/src/log_segment/mod.rs

+    pub end_version: Version,
+    pub log_root: Url,
+    /// Sorted commit files in the log segment (ascending)
+    pub sorted_commit_files: Vec<ParsedLogPath>,


Rescuing #495 (comment)

qq: Would it be more self-describing to call this ascending_commit_files?

I can rename it to that 👍

scovich · 2024-11-15T13:04:32Z

kernel/src/log_segment/mod.rs

+            require!(checkpoint_file.version + 1 == commit_file.version,
+            Error::generic(format!("Expected commit file version {} to be next version to checkpoint file version {}", commit_file.version, checkpoint_file.version )))


weird indent? cargo fmt?

Also, maybe the message can be less wordy, e.g

"Gap between checkpoint version {} and next commit {}"

scovich · 2024-11-15T13:09:52Z

kernel/src/log_segment/mod.rs

+        let version_eff = sorted_commit_files
+            .last()
+            .or(checkpoint_parts.first())
+            .ok_or(Error::MissingVersion)? // TODO: A more descriptive error


Rescuing #495 (comment)

Would it make more sense to use an Error::generic here, or create an new error variant?

Generic seems fine for now.

We may want a tracking issue to make a new error class to cover all missing file situations we might encounter during log listing?

kernel/src/log_segment/mod.rs

scovich · 2024-11-15T13:15:55Z

kernel/src/log_segment/mod.rs

+    Ok(fs_client
+        .list_from(&start_from)?
+        .map(|meta| ParsedLogPath::try_from(meta?))
+        // TODO this filters out .crc files etc which start with "." - how do we want to use these kind of files?


rescuing #495 (comment)

Do we even use or care about .crc files? They seem to be something pyspark associates with every write? Would we ever try to read one?

scovich · 2024-11-15T13:29:27Z

kernel/src/log_segment/mod.rs

+        .filter_map_ok(identity)
+        .take_while(move |path_res| match path_res {
+            Ok(path) => end_version.is_none_or(|end_version| path.version <= end_version),
+            Err(_) => true,


Rescuing #495 (comment)

Hmm is it a good idea to be filtering out errors though?

This is a good question we should probably track as a separate discussion/issue:

The ParsedLogPath code recognizes four general classes of files:

"Known" versioned files, e.g. Ok(Some(SinglePartCheckpoint))

"Unknown" versioned files that should be ignored, Ok(Some(Unknown))

Non-versioned files that should be ignored, Ok(None)

Versioned files that resemble a known type but which fail to parse, Err(_)

The Delta spec doesn't specifically say what to do with unrecognized file types, but it is pretty clear that we should ignore unrecognized things in general:

Since breaking changes must be accompanied by an increase in the protocol version recorded in a table or by the addition of a table feature, clients can assume that unrecognized actions, fields, and/or metadata domains are never required in order to correctly interpret the transaction log. Clients must ignore such unrecognized fields, and should not produce an error when reading a table that contains unrecognized fields.

... and some Delta table features like v2 checkpoints and log compaction rely on older clients to ignore unrecognized files/dirs.

So then the question is whether file names like these:

# hex instead of decimal 00000000deadbeef.commit.json # bogus part numbering 00000000000000000000.checkpoint.0000000010.0000000000.parquet # v2 checkpoint, as seen by a client that doesn't understand that feature 00000000000000000010.checkpoint.80a083e8-7026-4e79-81be-64bd76c43a11.json # compacted log file, as seen by a client that doesn't understand that feature 00000000000000000004.00000000000000000006.compacted.json

... are simply "unrecognized files" or actual errors to care about. Your code is treating them like actual errors; @zachschuermann is suggesting to just filter them out.

Maybe we should take the middle ground and filter them out with a warn! so there's at least a possibility to know they happened?

Note: The original code (before this PR) was propagating file name parsing errors, so we should probably keep that behavior for now and file an issue for separate follow-up?

yea agree with the last comment -- let's keep behavior and file a follow up (and if we want to add a warn! for some other cases that's fine too)

Thx for the context @scovich!! Agreed that it's something to address. No need to fail if we don't have to. I'll talk to @zachschuermann and get an issue up

kernel/src/log_segment/mod.rs

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

…ernel-rs into log_builder_new

zachschuermann

few comments but LGTM

kernel/src/log_segment.rs

zachschuermann · 2024-11-15T20:09:10Z

kernel/src/log_segment.rs

+    /// between versions `start_version` (inclusive) and `end_version` (inclusive). If no `end_version`
+    /// is specified it will be the most recent version by default.
+    #[allow(unused)]
+    #[cfg_attr(feature = "developer-visibility", visibility::make(pub))]


Suggested change

#[cfg_attr(feature = "developer-visibility", visibility::make(pub))]

zachschuermann · 2024-11-15T20:10:29Z

kernel/src/log_segment.rs

+    Ok(fs_client
+        .list_from(&start_from)?
+        .map(|meta| ParsedLogPath::try_from(meta?))
+        // TODO this filters out .crc files etc which start with "." - how do we want to use these kind of files?


@OussamaSaoudi-db can you create an issue for follow-up?

zachschuermann · 2024-11-15T20:11:10Z

kernel/src/log_segment.rs

+    Ok(fs_client
+        .list_from(&start_from)?
+        .map(|meta| ParsedLogPath::try_from(meta?))
+        // TODO this filters out .crc files etc which start with "." - how do we want to use these kind of files?


looks like #496 ? thanks!

zachschuermann · 2024-11-15T20:13:14Z

kernel/src/log_segment/tests.rs

+#[ignore]
+#[test]
+fn build_snapshot_with_missing_checkpoint_part_no_hint() {
+    // TODO(Oussam): Hande checkpoints correctly so that this test passes


follow up issue?

kernel/src/log_segment/tests.rs

zachschuermann · 2024-11-15T20:16:35Z

test-utils/src/lib.rs

+// get an ObjectStore path for a checkpoint file, based on version, part number, and total number of parts
+pub fn delta_path_for_multipart_checkpoint(version: u64, part_num: u64, num_parts: u64) -> Path {
+    let path =
+        format!("_delta_log/{version:020}.checkpoint.{part_num:010}.{num_parts:010}.parquet");
+    Path::from(path.as_str())
+}


not sure if this should go here or just in some test-only code right now? can always pull it out if we need it here?

Agree test-only code is best for now. If we did want it in prod code, it would belong in path.rs, not here? And it should probably involve LogFileType, e.g.

fn delta_file_name(version: Version, ty: LogFileType) -> String { let suffix = match ty { ... MultiPartCheckpoint { part_num, num_parts } => { format!("checkpoint.{part_num:010}.{num_parts:010}.parquet") } ... }; format!("{version:020}.{suffix}") }

(best done as a follow-up)

scovich

Few nits, but pretty much ready to merge!

scovich · 2024-11-15T20:27:37Z

kernel/src/log_segment.rs

+    /// between versions `start_version` (inclusive) and `end_version` (inclusive). If no `end_version`
+    /// is specified it will be the most recent version by default.
+    #[allow(unused)]
+    #[cfg_attr(feature = "developer-visibility", visibility::make(pub))]


test-utils/src/lib.rs

scovich · 2024-11-15T20:36:28Z

test-utils/src/lib.rs

+// get an ObjectStore path for a checkpoint file, based on version, part number, and total number of parts
+pub fn delta_path_for_multipart_checkpoint(version: u64, part_num: u64, num_parts: u64) -> Path {
+    let path =
+        format!("_delta_log/{version:020}.checkpoint.{part_num:010}.{num_parts:010}.parquet");
+    Path::from(path.as_str())
+}


Agree test-only code is best for now. If we did want it in prod code, it would belong in path.rs, not here? And it should probably involve LogFileType, e.g.

fn delta_file_name(version: Version, ty: LogFileType) -> String { let suffix = match ty { ... MultiPartCheckpoint { part_num, num_parts } => { format!("checkpoint.{part_num:010}.{num_parts:010}.parquet") } ... }; format!("{version:020}.{suffix}") }

(best done as a follow-up)

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

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

…ernel-rs into log_builder_new

OussamaSaoudi-db and others added 30 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

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

remove table_changes

6d8e35f

Merge code

6b78d56

Update log segment to latest version

c959b1d

zachschuermann reviewed Nov 15, 2024

View reviewed changes

OussamaSaoudi-db and others added 2 commits November 14, 2024 20:47

Apply suggestions from code review

e8f3405

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

Address pr reviews

f31ec8a

scovich reviewed Nov 15, 2024

View reviewed changes

OussamaSaoudi-db added 5 commits November 14, 2024 23:23

Move log segment tests to separate file

634661a

Address more comments

b94e2dd

Make naming consistent

ff8ad65

Address more nits

60a8a08

Merge branch 'main' into log_builder_new

04d2665

scovich reviewed Nov 15, 2024

View reviewed changes

OussamaSaoudi-db and others added 5 commits November 15, 2024 10:17

Update kernel/src/log_segment/mod.rs

2f02d19

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

address more pr comments

38fb7d3

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

6a7a624

…ernel-rs into log_builder_new

Move back log_segment.rs

13da708

fix typo

8d20941

OussamaSaoudi-db requested review from scovich and zachschuermann November 15, 2024 19:48

OussamaSaoudi-db mentioned this pull request Nov 15, 2024

Failed parsing of log files should be ignored #496

Open

zachschuermann approved these changes Nov 15, 2024

View reviewed changes

scovich approved these changes Nov 15, 2024

View reviewed changes

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

Apply suggestions from code review

7c45564

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

Apply suggestions from code review

1718d85

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

more comments

5819e49

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

ef28095

…ernel-rs into log_builder_new

Address nits

e49fdfa

fix clippy

b83dafe

Merge branch 'main' into log_builder_new

358ed16

OussamaSaoudi-db merged commit 3e7ad45 into delta-io:main Nov 15, 2024
19 checks passed

zachschuermann mentioned this pull request Nov 16, 2024

Add LogSegmentBuilder to construct LogSegments #457

Closed

OussamaSaoudi-db mentioned this pull request Dec 14, 2024

[tracking issue] Change Data Feed read path #440

Open

21 tasks

		require!(checkpoint_file.version + 1 == commit_file.version,
		Error::generic(format!("Expected commit file version {} to be next version to checkpoint file version {}", commit_file.version, checkpoint_file.version )))

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

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

Conversation

OussamaSaoudi-db commented Nov 15, 2024 • edited Loading

What changes are proposed in this pull request?

How was this change tested?

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

scovich 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

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 left a comment • 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

scovich Nov 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

scovich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

OussamaSaoudi-db commented Nov 15, 2024 •

edited

Loading

scovich left a comment •

edited

Loading

scovich Nov 15, 2024 •

edited

Loading