Assert diffs sum does not change when swapping in spine batches

[DAlperin]

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.

[bkirwi]

I'll follow up with a full review when I can, but two quick things to check in on:

• Code like p.diffs_sum(metrics).unwrap_or(0) suggests that missing sums are equivalent to 0, but in other places None indicates that we haven't calculated the sum. I believe this could cause false positives.
• We can't add methods like pub fn diffs_sum(&self, metrics: &ColumnarMetrics) -> Option<i64> {, because diffs are not guaranteed to be i64. (Though they usually are!) This may need to be a type parameter.

[DAlperin]

Ah yep. I made it generic, but most invocations now call it like

 p.diffs_sum::<Diff>(metrics)
                    .expect("new batch must have diffs sum")

where Diff is the mz_repr::Diff that we seem to be hardcoding basically everywhere we describe diff types. I'm not clear if we should be adding more generics to push further up the the caller or if this is fine.

[bkirwi]

This is a forward-compat hazard, since older versions don't have this field in the schema and would discard it.

Normally I'd suggest doing what you did for other PRs, but in this case I think it may be fine without a new flag... the hollow run stuff is already behind the max_run_len rollout flag, which is off in staging / prod. Though it is on in CI, so we may have to do some tweaking there.

[bkirwi]

Cool! Overall shape seems good.

There remains a bunch of almost-duplicated code with subtle differences, which makes me a bit nervous and is kind of hard to review. I made one suggestion at a small scale (summing diffs of parts) but if there are any other ideas you think have a good cost-benefit ratio I think they're probably worthwhile!

There's a fair bit of red in the nightlies, but I haven't taken a look in detail - let me know if you want a second pair of eyes there.

[bkirwi]

Lots of code movement so this is a little hard to verify... but I think this is good! (And nightlies look pretty clean.) Appreciate all the cleanup.

One nit that I think is worth dealing with before merging, but otherwise this seems good to go.

We've verified that the fixed version of this still errors on the bad change in CI, yeah?

[def-]

We've verified that the fixed version of this still errors on the bad change in CI, yeah?

I did a run here: https://buildkite.com/materialize/nightly/builds/12213. Seeing lots of

platform-checks-materialized-1       | 2025-06-04T16:39:14.746851Z  thread 'persist:0008' panicked at src/persist-client/src/internal/trace.rs:1018:17: assertion `left == right` failed: merge res diffs sum (-1406225) did not match spine batch diffs sum (-919999)

so it seems good!