feat: Add CheckpointManifestReader to process sidecar files #1500

OussamaSaoudi · 2025-11-19T05:35:22Z

🥞 Stacked PR

Use this link to review incremental changes.

stack/dlr_commit [Files changed]
- stack/dlr_manifest [Files changed]
  - stack/dlr_driver [Files changed]
    - stack/dlr_serde [Files changed]
      - stack/dlr_add_rm_dedup [Files changed]
        
        stack/dlr_immutable_add_rm_dedup [Files changed]
        
        stack/dlr_leaf [Files changed]
        
        stack/dlr_executor [Files changed]

What changes are proposed in this pull request?

This PR introduces the CheckpointManifestReader. This is a LogReader that processes manifest files and extracts sidecar paths for later processing. Upon completion, the CheckpointManifestReader returns either:

Done: The entire checkpoint has been processed.
Sidecars: Contains paths to the remaining sidecar checkpoint files that must be processed.

How was this change tested?

The following cases are checked:

Parquet manifest checkpoint produces the correct sidecars
Json manifest checkpoint produces the correct sidecars
Single-part checkpoint with no sidecars
Attempting to finalize the CheckpointManifestReader without reaching completion results in an error.

kernel/src/log_reader/manifest.rs

scovich · 2025-11-20T15:59:52Z

kernel/src/log_reader/checkpoint_manifest.rs

+            ))
+        );
+
+        let sidecars: Vec<_> = self


Type annotation shouldn't be necessary here because AfterManifest::Sidecars forces it

Suggested change

let sidecars: Vec<_> = self

let sidecars = self

Sadly the sidecars.is_empty() somehow doesn't let the type inference work :(

OussamaSaoudi · 2025-11-25T00:02:01Z

Note: Seems that arrow stuff is causing errors in the CI. Will fix in a separate Pr.

nicklan

Mostly looks great. I do wonder if there could be some way to avoid the pattern of "read the iter -> then call a final method to get the sidecars". I think it would be nice if it could just be that the final element of the iterator is the sidecars, but I realize the types don't quite line up. Maybe worth thinking about if we could use Either or something else to make that flow work, and if you think that's better.

nicklan · 2025-11-25T00:33:36Z

kernel/src/log_reader/manifest.rs

+/// Phase that processes single-part checkpoint. This also treats the checkpoint as a manifest file
+/// and extracts the sidecar actions during iteration.
+#[allow(unused)]
+pub(crate) struct ManifestPhase {


I don't love the name ManifestPhase, because it doesn't tell you what it's a phase of. Could this just be a CheckpointManifestProcessor?

I went with CheckpointManifestReader

nicklan · 2025-11-25T00:35:57Z

kernel/src/log_reader/manifest.rs

+    /// - `Sidecars`: Extracted sidecar files
+    /// - `Done`: No sidecars found
+    #[allow(unused)]
+    pub(crate) fn finalize(self) -> DeltaResult<AfterManifest> {


Likewise, it's not clear what we're "finalizing" here. I'd maybe prefer to just call this get_extracted_sidecars or something like that, and it can return Option<Vec<FileMeta>>, unless we think this will return something beyond sidecars or "done" in the future.

Regardless let's make very clear in the doc comment that the iterator needs to be exhausted before you can call this.

good point, simplified to just Vec<FileMeta>. Added docs as well

codecov · 2025-11-25T01:20:39Z

Codecov Report

❌ Patch coverage is 85.65217% with 33 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.56%. Comparing base (6972292) to head (a534df5).

Files with missing lines	Patch %	Lines
kernel/src/log_reader/checkpoint_manifest.rs	85.25%	8 Missing and 15 partials ⚠️
kernel/src/log_reader/commit.rs	87.75%	1 Missing and 5 partials ⚠️
kernel/src/utils.rs	85.71%	0 Missing and 3 partials ⚠️
kernel/src/log_segment.rs	50.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1500      +/-   ##
==========================================
+ Coverage   84.55%   84.56%   +0.01%     
==========================================
  Files         123      125       +2     
  Lines       33366    33584     +218     
  Branches    33366    33584     +218     
==========================================
+ Hits        28213    28401     +188     
- Misses       3768     3775       +7     
- Partials     1385     1408      +23

☔ 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.

OussamaSaoudi · 2025-12-01T23:09:52Z

kernel/src/log_reader/commit.rs

    use std::sync::Arc;
-    use url::Url;
-
-    fn load_test_table(


NOTE: this was moved to utils, but also the semantics changed a bit. Now it tries treating the test file as zipped. If that fails, it tries reading as if it isn't zipped.

nicklan

lgtm, thanks!

nicklan · 2025-12-02T00:09:42Z

kernel/src/log_reader/checkpoint_manifest.rs

+        let log_root = log_segment.log_root.clone();
+        assert_eq!(log_segment.checkpoint_parts.len(), 1);
+        let checkpoint_file = &log_segment.checkpoint_parts[0];
+        let mut manifest_phase =


nit: variable name doesn't make sense anymore :)

Suggested change

let mut manifest_phase =

let mut checkpoint_manifest_reader =

OussamaSaoudi mentioned this pull request Nov 19, 2025

feat: Distributed Log Replay serialization/deserialization #1503

Open

github-actions bot assigned OussamaSaoudi Nov 19, 2025

This was referenced Nov 19, 2025

feat: Distributed Log Replay Sequential Phase #1502

Open

feat: Commit Reader #1499

Open

feat: CheckpointLeaf file Reader #1501

Draft

OussamaSaoudi changed the title ~~manifest v1~~ feat: Manifest File Reader Nov 19, 2025

scovich reviewed Nov 20, 2025

View reviewed changes

OussamaSaoudi force-pushed the stack/dlr_manifest branch 2 times, most recently from 698da9b to 5db45f5 Compare November 24, 2025 23:07

OussamaSaoudi requested review from nicklan and scovich November 24, 2025 23:35

OussamaSaoudi force-pushed the stack/dlr_manifest branch from e54ba02 to 58604ff Compare November 24, 2025 23:48

OussamaSaoudi marked this pull request as ready for review November 25, 2025 00:01

OussamaSaoudi changed the title ~~feat: Manifest File Reader~~ feat: Add CheckpointManifestReader to process sidecar files Nov 25, 2025

nicklan reviewed Nov 25, 2025

View reviewed changes

OussamaSaoudi force-pushed the stack/dlr_manifest branch from a6e206c to 563a6d2 Compare November 25, 2025 01:18

OussamaSaoudi requested a review from nicklan November 25, 2025 19:57

OussamaSaoudi added 11 commits December 1, 2025 14:29

commit reader

9116c74

improve commit

7c04cd7

cleanup

20c2a95

commit reader

703dc4e

manifest v1

fc9c3b6

manifest

b2bf9d3

allow_unused

8ccc3a6

improve test

e63e7a8

fix manifest

f95ab27

more cleanup

3ac0de8

address pr reviews

cba5141

OussamaSaoudi added 3 commits December 1, 2025 14:29

rename to CheckpointManifestReader

bc84caa

address comments

1bcc46e

simplify

a534df5

OussamaSaoudi force-pushed the stack/dlr_manifest branch from 77c7f93 to a534df5 Compare December 1, 2025 23:07

OussamaSaoudi commented Dec 1, 2025

View reviewed changes

nicklan approved these changes Dec 2, 2025

View reviewed changes

This was referenced Dec 3, 2025

feat: Introduce Deduplicator trait to unify mutable and immutable deduplication #1537

Open

feat: Add executor for distributed log replay #1539

Draft

feat: Add immutable deduplicator for checkpoint phase of distributed log replay #1538

Draft

	let mut manifest_phase =
	let mut checkpoint_manifest_reader =

feat: Add CheckpointManifestReader to process sidecar files #1500

Are you sure you want to change the base?

feat: Add CheckpointManifestReader to process sidecar files #1500

Conversation

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

Uh oh!

🥞 Stacked PR

What changes are proposed in this pull request?

How was this change tested?

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

scovich Nov 20, 2025

Choose a reason for hiding this comment

Uh oh!

OussamaSaoudi Nov 24, 2025

Choose a reason for hiding this comment

Uh oh!

OussamaSaoudi commented Nov 25, 2025

Uh oh!

nicklan left a comment

Choose a reason for hiding this comment

Uh oh!

nicklan Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicklan Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

OussamaSaoudi Nov 25, 2025

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

OussamaSaoudi Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

nicklan left a comment

Choose a reason for hiding this comment

Uh oh!

nicklan Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

OussamaSaoudi commented Nov 19, 2025 •

edited

Loading

OussamaSaoudi Nov 25, 2025 •

edited

Loading

codecov bot commented Nov 25, 2025 •

edited

Loading