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]
        
        stack/dlr_serialization_helpers [Files changed]
        
        stack/dlr_api [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.78%. Comparing base (72cb27c) to head (011d91c).

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.77%   84.78%            
========================================
  Files         126      128     +2     
  Lines       35679    35897   +218     
  Branches    35679    35897   +218     
========================================
+ Hits        30246    30434   +188     
- Misses       3965     3972     +7     
- Partials     1468     1491    +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 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

OussamaSaoudi added 4 commits December 5, 2025 17:31

commit reader

39b6132

improve commit

95291f7

cleanup

ed7888c

commit reader

fd419c8

OussamaSaoudi added 10 commits December 5, 2025 17:31

manifest v1

527b388

manifest

33209b2

allow_unused

548c0da

improve test

aed338a

fix manifest

d5c941e

more cleanup

c28e6bb

address pr reviews

42545f4

rename to CheckpointManifestReader

7353753

address comments

6d676c4

simplify

011d91c

OussamaSaoudi force-pushed the stack/dlr_manifest branch from a534df5 to 011d91c Compare December 8, 2025 17:56

OussamaSaoudi mentioned this pull request Dec 8, 2025

initial api #1547

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