Persist: Add new API for run based spine replacement #33044

DAlperin · 2025-07-15T21:08:53Z

Motivation

Tips for reviewer

Checklist

This PR has adequate test coverage / QA involvement has been duly considered. (trigger-ci for additional test/nightly runs)
This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
If this PR includes major user-facing behavior changes, I have pinged the relevant PM to schedule a changelog post.

Depends on #33044

Copilot

Pull Request Overview

This PR adds a new API for run-based spine replacement in the persist client, extending the existing merge resolution functionality to support fine-grained replacement of specific runs within hollow batches rather than just entire batches.

Adds RunLocation struct to identify specific runs within hollow batches using SpineId and RunId pairs
Introduces a new apply_merge_res_checked method that can replace individual runs within batches
Extends FueledMergeRes with inputs and new_active_compaction fields to support the new replacement logic

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
src/persist-client/src/internal/trace.rs	Core implementation of run-based spine replacement with new data structures and methods
src/persist-client/src/internal/state_serde.json	Test data updates reflecting the new run-based structure
src/persist-client/src/internal/state_diff.rs	Updates to populate new fields in FueledMergeRes
src/persist-client/src/internal/state.rs	Test utilities and method renaming for compatibility
src/persist-client/src/internal/machine.rs	Updates to construct FueledMergeRes with new fields
src/persist-client/src/internal/compact.rs	Updates to construct FueledMergeRes with new fields
src/persist-client/src/cli/admin.rs	Updates to construct FueledMergeRes with new fields

src/persist-client/src/internal/trace.rs

Depends on MaterializeInc#33044

Depends on #33044

bkirwi

Good changes! If nightlies are happy then so am I.

This PR could use a description also.

bkirwi · 2025-07-23T18:34:16Z

src/persist-client/src/internal/trace.rs

-#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
+#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
+pub enum CompactionInput {
+    /// We don't know what our inputs where, this should only be used for


Suggested change

/// We don't know what our inputs where, this should only be used for

/// We don't know what our inputs were; this should only be used for

bkirwi · 2025-07-23T18:36:51Z

src/persist-client/src/internal/trace.rs

+    Legacy,
+    /// This compaction output is a total replacement for all batches in this id range.
+    #[allow(dead_code)]
+    IdRange(BTreeSet<SpineId>),


This could just be a SpineId instead of a set, since a single id is already a range. (Optional, but would save you some validation below.)

bkirwi · 2025-07-23T18:49:58Z

src/persist-client/src/internal/trace.rs

+        let old_batch_diff_sum = Self::diffs_sum::<D>(batch.parts.iter(), metrics);
+        let old_diffs_sum = Self::diffs_sum_for_runs::<D>(batch, &run_ids, metrics);
+
+        if let (Some(old_diffs_sum), Some(new_diffs_sum)) = (old_diffs_sum, new_diffs_sum) {


We should also assert that the old_diffs_sum is None when the new_diffs_some is None... for new diffs None should always mean 0, and it would be bad if we let that replace data with nonzero sum!

bkirwi · 2025-07-23T18:50:21Z

src/persist-client/src/internal/trace.rs

+        let Some((i, batch)) = part else {
+            return ApplyMergeResult::NotAppliedNoMatch;
+        };
+        let replacement_range = i..i + 1;


Suggested change

let replacement_range = i..i + 1;

let replacement_range = i..(i + 1);

bkirwi · 2025-07-23T18:52:00Z

src/persist-client/src/internal/trace.rs

+        }
+
+        let parts = &self.parts[replacement_range.clone()];
+        let id = SpineId(parts.first().unwrap().id.0, parts.last().unwrap().id.1);


This is oddly baroque... we're only targeting a single batch. Why a single-element range that we then unwrap?

bkirwi · 2025-07-23T18:56:08Z

src/persist-client/src/internal/trace.rs

+            .prop_filter("non-empty batch", |(batch, _)| batch.run_meta.len() >= 1)
+            .prop_flat_map(|(batch, to_replace)| {
+                let batch_len = batch.run_meta.len();
+                (0..batch_len).prop_flat_map(move |start| {


Nit: I'd extract this gen out to a let above so you get proper formatting.

bkirwi · 2025-07-23T19:03:55Z

src/persist-client/src/internal/trace.rs

+                (1..=max_len).prop_map(move |len| {
+                    let range = start..(start + len);
+                    (batch_clone.clone(), to_replace_clone.clone(), range)
+                })


This test still bakes in the "must replace a contiguous range of runs from the original batch" idea that the function under test no longer requires. Might as well choose a random subset instead and get better coverage?

bkirwi · 2025-07-23T21:34:57Z

Looks like you need to opt out the new test from miri... lots of similar examples in our existing proptests.

I find the scalability benchmark result sort of implausible, given the limited stuff you've changed... maybe spurious?

DAlperin requested a review from a team as a code owner July 15, 2025 21:08

DAlperin added a commit that referenced this pull request Jul 15, 2025

Persist: plumb incremental apply APIs through

6f74d0b

Depends on #33044

DAlperin mentioned this pull request Jul 15, 2025

Persist: plumb incremental apply APIs through #33045

Open

5 tasks

DAlperin requested a review from Copilot July 16, 2025 20:26

Copilot AI reviewed Jul 16, 2025

View reviewed changes

bkirwi reviewed Jul 16, 2025

View reviewed changes

DAlperin added a commit to DAlperin/materialize that referenced this pull request Jul 17, 2025

Persist: plumb incremental apply APIs through

ca69a61

Depends on MaterializeInc#33044

DAlperin added a commit to DAlperin/materialize that referenced this pull request Jul 17, 2025

Persist: plumb incremental apply APIs through

2ba013f

Depends on MaterializeInc#33044

DAlperin added a commit to DAlperin/materialize that referenced this pull request Jul 17, 2025

Persist: plumb incremental apply APIs through

c60199e

Depends on MaterializeInc#33044

DAlperin force-pushed the dov/run-based-spine-replacement branch from f035a90 to 23e21c8 Compare July 17, 2025 02:52

DAlperin added a commit that referenced this pull request Jul 17, 2025

Persist: plumb incremental apply APIs through

5779b4d

Depends on #33044

DAlperin force-pushed the dov/run-based-spine-replacement branch from 23e21c8 to f497c4a Compare July 17, 2025 14:26

DAlperin added a commit that referenced this pull request Jul 17, 2025

Persist: plumb incremental apply APIs through

9f945f4

Depends on #33044

DAlperin added a commit that referenced this pull request Jul 17, 2025

Persist: plumb incremental apply APIs through

b75952b

Depends on #33044

DAlperin force-pushed the dov/run-based-spine-replacement branch from f497c4a to ae7312a Compare July 17, 2025 14:39

DAlperin added a commit that referenced this pull request Jul 17, 2025

Persist: plumb incremental apply APIs through

0d9a612

Depends on #33044

DAlperin force-pushed the dov/run-based-spine-replacement branch from ae7312a to 2bb9775 Compare July 17, 2025 15:08

DAlperin added a commit that referenced this pull request Jul 17, 2025

Persist: plumb incremental apply APIs through

43e3595

Depends on #33044

DAlperin force-pushed the dov/run-based-spine-replacement branch from 2bb9775 to a15c651 Compare July 17, 2025 15:11

DAlperin added a commit that referenced this pull request Jul 17, 2025

Persist: plumb incremental apply APIs through

ac52cb3

Depends on #33044

DAlperin force-pushed the dov/run-based-spine-replacement branch from a15c651 to 85910d2 Compare July 17, 2025 15:21

DAlperin added a commit that referenced this pull request Jul 21, 2025

Persist: plumb incremental apply APIs through

1ddef6e

Depends on #33044

DAlperin force-pushed the dov/run-based-spine-replacement branch from 85910d2 to 0274b1f Compare July 21, 2025 15:17

DAlperin added a commit that referenced this pull request Jul 21, 2025

Persist: plumb incremental apply APIs through

b4e20a7

Depends on #33044

DAlperin force-pushed the dov/run-based-spine-replacement branch from 0274b1f to ee85994 Compare July 21, 2025 20:09

DAlperin added a commit that referenced this pull request Jul 21, 2025

Persist: plumb incremental apply APIs through

2ac14ef

Depends on #33044

DAlperin force-pushed the dov/run-based-spine-replacement branch from ee85994 to 3c38325 Compare July 21, 2025 20:53

DAlperin added a commit that referenced this pull request Jul 21, 2025

Persist: plumb incremental apply APIs through

0188a39

Depends on #33044

DAlperin force-pushed the dov/run-based-spine-replacement branch from 3c38325 to a97ff26 Compare July 21, 2025 20:55

Persist: Add new API for run based spine replacement

a946d3c

DAlperin force-pushed the dov/run-based-spine-replacement branch from a97ff26 to a946d3c Compare July 23, 2025 14:12

DAlperin added a commit that referenced this pull request Jul 23, 2025

Persist: plumb incremental apply APIs through

a9fad35

Depends on #33044

bkirwi approved these changes Jul 23, 2025

View reviewed changes

	/// We don't know what our inputs where, this should only be used for
	/// We don't know what our inputs were; this should only be used for

	let replacement_range = i..i + 1;
	let replacement_range = i..(i + 1);

Persist: Add new API for run based spine replacement #33044

Are you sure you want to change the base?

Persist: Add new API for run based spine replacement #33044

Conversation

DAlperin commented Jul 15, 2025

Motivation

Tips for reviewer

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

bkirwi Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

bkirwi Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

bkirwi Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

bkirwi Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

bkirwi Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

bkirwi Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

bkirwi Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

bkirwi commented Jul 23, 2025

Uh oh!

Uh oh!

bkirwi left a comment •

edited

Loading