feat: CRC for PM replay #982

zachschuermann · 2025-05-27T17:13:39Z

What changes are proposed in this pull request?

Leverage CRC files for P+M replay. We do this by modifying our PM log replay to:

If we have CRC at this version, we can just read the Protocol and Metadata from it.
Else, create a 'pruned' (CRC-rooted) log segment that is only the commits after the latest CRC (if there is no CRC, we just use the whole log segment)
Do PM replay on the pruned log segment to find the latest Protocol/Metadata actions.
If no Protocol/Metadata was found, return Protocol/Metadata from CRC (if CRC doesn't exist, we are guaranteed to get a Protocol/Metadata from checkpoint in (3)).

How was this change tested?

new UTs

codecov · 2025-05-27T17:16:03Z

Codecov Report

Attention: Patch coverage is 92.72727% with 4 lines in your changes missing coverage. Please review.

Project coverage is 85.07%. Comparing base (f139209) to head (76c99c4).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
kernel/src/actions/crc.rs	78.57%	0 Missing and 3 partials ⚠️
kernel/src/log_segment.rs	85.71%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #982      +/-   ##
==========================================
- Coverage   85.07%   85.07%   -0.01%     
==========================================
  Files          90       90              
  Lines       23387    23410      +23     
  Branches    23387    23410      +23     
==========================================
+ Hits        19896    19915      +19     
  Misses       2479     2479              
- Partials     1012     1016       +4

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

kernel/src/actions/crc.rs

zachschuermann · 2025-06-12T20:18:26Z

kernel/src/actions/crc.rs

+    pub(crate) protocol: Option<Protocol>,
+    pub(crate) metadata: Option<Metadata>,


this is fixing an incorrect usage of Default --> we want Default to just set up 'empty' P/M for us to fill in after visiting - not to generate some default P/M itself.

zachschuermann · 2025-06-12T20:30:32Z

kernel/src/actions/crc.rs

+        });
+
+        let json = crc_json.to_string().into_bytes();
+        // let (engine, store) = DefaultEngine::new_memory();


anyone know why I can't seem to get the extension trait working here?

We know now and can remove the commented out line (sadly)

nicklan · 2025-06-18T21:41:06Z

kernel/src/actions/crc.rs

@@ -94,14 +93,46 @@ pub(crate) struct DeletedRecordCountsHistogram {
    pub(crate) deleted_record_counts: Vec<i64>,
 }

+#[derive(Debug, Clone, PartialEq, Eq)]
+pub(crate) struct ProtocolMetadata {


Seems like we're just using this here to return a protocol and metadata? We have lots of other places where we pair them together as either arguments, or a (protocol, metadata). I'd say here can we just do:

fn try_protocol_and_metadata_from_crc(path: FileMeta, engine: &dyn Engine) -> DeltaResult<(Protocol, Metadata)> {}

We could pass through everything and unify on a single struct in the future, but for now this is just confusing

nicklan · 2025-06-18T21:41:46Z

kernel/src/actions/crc.rs

+    /// protocol/metadata cannot be found in the CRC file (specification requires both to be
+    /// present).
+    pub(crate) fn try_from_crc(path: FileMeta, engine: &dyn Engine) -> DeltaResult<Self> {
+        let json = engine.json_handler();


micro_nit:

Suggested change

let json = engine.json_handler();

let json_handler = engine.json_handler();

nicklan · 2025-06-18T21:43:16Z

kernel/src/actions/crc.rs

+        });
+
+        let json = crc_json.to_string().into_bytes();
+        // let (engine, store) = DefaultEngine::new_memory();


We know now and can remove the commented out line (sadly)

nicklan · 2025-06-18T21:44:40Z

kernel/src/log_segment.rs

+        let protocol_metadata_log_segment: Cow<'_, Self> =
+            if let Some(latest_crc_file) = &self.latest_crc_file {
+                let mut log_segment = self.clone();
+                log_segment.checkpoint_version = None;


Have we pruned here so we know that any crc has to be after the checkpoint?

nicklan · 2025-06-18T21:48:27Z

kernel/src/log_segment.rs

+        // 2. construct a "protocol-metadata log segment"
+        //    a) if we have a CRC file, we only need commits (CRC_version, target_version]
+        //    b) if we don't have a CRC, we proceed as normal
+        let protocol_metadata_log_segment: Cow<'_, Self> =


Another option would be to add a start_version: Option<Version> to replay_for_metadata. In that case, find_commit_cover could quite easily just drop any versions less than start_version, and we wouldn't need this code really at all (and could avoid the clone)

nicklan · 2025-06-18T21:53:03Z

kernel/src/log_segment.rs

+        if let Some(latest_crc_file) = &self.latest_crc_file {
+            let path = &latest_crc_file.location;
+            let pm = ProtocolMetadata::try_from_crc(path.clone(), engine)?;
+            metadata_opt.get_or_insert(pm.metadata);
+            protocol_opt.get_or_insert(pm.protocol);
+        }
+
        Ok((metadata_opt, protocol_opt))


Suggested change

if let Some(latest_crc_file) = &self.latest_crc_file {

let path = &latest_crc_file.location;

let pm = ProtocolMetadata::try_from_crc(path.clone(), engine)?;

metadata_opt.get_or_insert(pm.metadata);

protocol_opt.get_or_insert(pm.protocol);

}

Ok((metadata_opt, protocol_opt))

if let Some(latest_crc_file) = &self.latest_crc_file {

let path = &latest_crc_file.location;

let pm = ProtocolMetadata::try_from_crc(path.clone(), engine)?;

Ok((metadata_opt.or(Some(pm.metadata)), protocol_opt.or(Some(pm.protocol))))

} else {

Ok((metadata_opt, protocol_opt))

}

nicklan · 2025-06-18T21:54:59Z

test-utils/src/lib.rs

@@ -164,4 +166,15 @@ impl DefaultEngineExtension for DefaultEngine<TokioBackgroundExecutor> {
            TokioBackgroundExecutor::new().into(),
        ))
    }
+
+    fn new_memory() -> (Arc<DefaultEngine<TokioBackgroundExecutor>>, Arc<InMemory>) {


probably can remove this now right, or move it to the kernel crate

wip: CRC for PM replay

76c99c4

github-actions bot assigned zachschuermann May 27, 2025

scovich reviewed May 30, 2025

View reviewed changes

kernel/src/actions/crc.rs Outdated Show resolved Hide resolved

need to sort out new_memory() helper

15ddc3f

zachschuermann marked this pull request as ready for review June 12, 2025 20:16

zachschuermann changed the title ~~[wip] feat: CRC for PM replay~~ feat: CRC for PM replay Jun 12, 2025

Merge remote-tracking branch 'upstream/main' into crc-pm-replay-3

114cf79

zachschuermann commented Jun 12, 2025

View reviewed changes

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

zachschuermann requested review from nicklan and scovich June 12, 2025 20:26

zachschuermann commented Jun 12, 2025

View reviewed changes

nicklan reviewed Jun 18, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: CRC for PM replay #982

feat: CRC for PM replay #982

Uh oh!

zachschuermann commented May 27, 2025 •

edited

Loading

Uh oh!

codecov bot commented May 27, 2025

Uh oh!

Uh oh!

zachschuermann Jun 12, 2025

Uh oh!

zachschuermann Jun 12, 2025

Uh oh!

nicklan Jun 18, 2025

Uh oh!

nicklan Jun 18, 2025

Uh oh!

nicklan Jun 18, 2025

Uh oh!

nicklan Jun 18, 2025

Uh oh!

nicklan Jun 18, 2025

Uh oh!

nicklan Jun 18, 2025

Uh oh!

nicklan Jun 18, 2025

Uh oh!

nicklan Jun 18, 2025

Uh oh!

Uh oh!

		pub(crate) protocol: Option<Protocol>,
		pub(crate) metadata: Option<Metadata>,

	let json = engine.json_handler();
	let json_handler = engine.json_handler();

feat: CRC for PM replay #982

Are you sure you want to change the base?

feat: CRC for PM replay #982

Uh oh!

Conversation

zachschuermann commented May 27, 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 May 27, 2025

Codecov Report

Uh oh!

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!

Uh oh!

zachschuermann commented May 27, 2025 •

edited

Loading