CHA-545 | key the cold segment cache by content hash, not row uuid

[nhobin219]

Closes CHA-545.

The problem

SegmentCache was keyed by the cold-artifact row's uuid. Reference copies mint
a new row uuid over an unchanged (object_uri, offset, length):

• insert_carried_snapshot_segments / insert_carried_segment_indexes carry a
  snapshot forward (CHA-531), and
• fork_copy materializes a fork's cold references at fork time (CHA-539).

Nobody rewrites the bytes. A uuid-keyed cache therefore stored N identical
decodes of one byte range — N× the memory under a fixed byte budget, and N cold
reads to fill it. A fork's cold footprint is mostly its parent's files, so N
grows with the branch count, which is the thing Penca expects to be cheap.

The ticket asked whether this is worth solving. It is: the duplication scales
with exactly the operation the product markets as free.

The change

Every cold-artifact metadata row — persist segments, snapshot segments, and
segment index sidecars — now carries a content_hash: an xxh3_128 digest of
the typed in-memory Arrow batch, computed once at write time
(penca_core::digest::segment_content_hash). SegmentCache is keyed by that
hash, and every reference copy inherits the source row's hash verbatim — that
inheritance is the dedup.

Hashing the decoded batch rather than the encoded file bytes is deliberate: the
key has to name one decoded batch (equal hash must mean equal decode), and the
digest is available where it is taken — at write time, before the format writer
encodes. It also makes dedup insensitive to encoding choices, so two
independently written segments holding the same rows share an entry even if the
writer picked different row-group boundaries or dictionary encodings.

The part that is not just a key swap

A hash-keyed entry is shared across callers whose schemas can differ. Carry-
forward and fork copy select old.content_hash verbatim, so when a fork ALTERs
a column its rows still read the parent's bytes under the parent's hash — parent
and fork share one cache entry while disagreeing about that column's type. No
hashing scheme can separate them, because neither row's digest was ever taken
under its own read schema.

So the cached value had to become the file-native decode, with caller
shaping (projection + null-fill of columns added by a later
ALTER TABLE ADD COLUMN) moved after the lookup:
FormatReader::read_segment_native + reader::shape_to_schema. A caller-shaped
entry would carry the schema of whichever branch decoded first, and the second
branch's read would fail on a type mismatch its own metadata never justified.
test_fork_and_parent_diverge_a_columns_type_over_one_shared_slice is the
regression guard.

Why the storage format is deliberately not in the key

Digesting the batch pre-encode is exactly what lets one hash name files in two
formats, once OBJECT_STORAGE_FORMAT has been flipped between two writes of the
same content. Those two files decode to the same Arrow batch under Penca's
storage contract — a write followed by a read returns the batch that was written
— so the hash alone names one decode and the two files share one entry.

A review pass folded the format into the key on the argument that the cached
file-native decode is a per-format artifact (6d755e2); 2a6cd96 reverses it.
That inverts the contract: a Parquet/Lance divergence would mean one format's
round trip is wrong, fixed in that format or by dropping the type from
CanonicalType — not a fact for a cache key to absorb. Nothing asserts the
contract today, but keying around an unverified defect gives up a real
cross-format share permanently and would mask the defect if it existed, turning
the loud cross-format type mismatch into silence. CHA-548 adds round-trip
identity coverage driven from CanonicalType.

The alternative that was rejected

The symmetric design fingerprints the read-time schema into the key and caches a
value already adapted to it — in its strongest form, the type-cast table, with
projection and null-fill still applied per caller. It is implementable and
correct; the fingerprint separates the two branches above. It loses because the
key then moves whenever the schema moves while the data does not: one
ADD COLUMN re-fingerprints a footprint nobody rewrote, and a fork stops sharing
with its parent at their first divergent ALTER — which is the dedup this PR
exists to create. Its one genuine advantage, serving a read whose declared type
differs from the file's, has nothing to bite on while schema evolution is
ADD COLUMN-only; and if type evolution lands, casting belongs in the shaping
tail where it is per-caller and visible, not hidden behind a cache key.
docs/design-decisions.md carries the full comparison and a worked example,
since none of this is self-evident from the code.

Index sidecars are in scope for the same reason base segments are: they come out
of object storage through the same SegmentCache and are reference-copied by the
same two mechanisms.

content_hash is UUID NOT NULL with no default on all three tables. Every
writer computes it and a catalog predating the column is recreated rather than
migrated, so there is no legacy row to default — the uuid fallback key space is
deleted, not kept alive. tx_log_persist_segment_metadata deliberately has no
content_hash: it is never cache-read and never reference-copied.

Commits

Red tests

• 0d0a9fe test(cold) — failing cross-branch segment cache dedup tests
• 1343c25 test(cold) — cross-branch schema-divergence regression guard

Digest + schema

• 401f38e feat(cold) — segment_content_hash over typed Arrow batches
• 0bd3200 fix(cold) — drop the no-op normalization, pin dictionary handling
• 50f177d feat(schema) — content_hash column on segment + sidecar metadata

Write + copy paths

• 8b1482c feat(cold) — compute segment_content_hash at every fresh cold write
• 3dddb34 feat(cold) — inherit content_hash on every reference copy

Read path

• f244986 refactor(query) — cache the file-native decode, shape after the lookup
• 75cb567 feat(query) — key SegmentCache by content_hash
• 6435833 perf(query) — take matched rows before shaping on the index-seek path

Cleanup pass (orch:run-cleanup)

• f401463 refactor(cold) — key SegmentCache by Uuid, not a stringified hash
• 7043699 refactor(cold) — extract requested_columns from the format readers
• e1fe2e5 refactor(cold) — name the projection-less shaping call in the driver
• 6687637 refactor(cold) — collapse three decode-and-cache copies into one helper
• ec961b6 feat(cold) — report the sidecar cache miss as cached vs uncached

Plus test-fixture and docs commits (bb60e6e, 55102b6, 48e6eaa, b4b1539,
b8310a2, 1a16aab, 3fd2cef, b1c4ac8, 1b7736f).

Review follow-ups (/review-pr, folded in rather than deferred)

• 6d755e2 fix(cold) — fold the storage format into the segment cache key
• 97ad00a chore — cargo fmt the segment cache key call site
• b2100a4 docs(cold) — reconcile the sidecar collision note with the key space
• 7308732 refactor(cold) — name the segment cache key as a type
• a3ae1bd docs(cold) — record why the cache holds the native decode
• 2a6cd96 refactor(cold) — drop the format from the segment cache key
  (reverses 6d755e2; see the section above)
• 2b57ac7 perf(cold) — compact byte-view buffers before hashing a segment
• a78f281 fix(cold) — keep the row count when compacting a zero-size fixed list

Those last two are worth calling out. arrow-ipc emits a view array's variadic data
buffers whole rather than prove no surviving view points into a pruned one,
so a digest of a slice encoded the whole parent's string payload — and compact
and the snapshot packer both hash once per slice of one concat_batches result,
so a wave of N inputs re-serialized every input's bytes N times, on exactly
the many-small-segments workload compaction exists for. gc-ing the byte-view
columns first (descending one level into a list child, which is as deep as
CanonicalType allows a view to sit) bounds each digest to its own rows and
makes it canonical for those types: two segments holding the same values now
agree even when their parents laid the bytes out differently. The digest's
contract loses its slice-invariance exception rather than documenting one.

a78f281 fixes a bug that compaction introduced: FixedSizeListArray::try_new
does not preserve its input's length — at size == 0 it re-derives len off the
null buffer, so a null-free FixedSizeList(Utf8View, 0) rebuilt as zero rows
and two such columns digested identically regardless of row count. CanonicalType
accepts any i32 size including 0, so nothing upstream rejects it.
try_new_with_length carries the length through. The same commit covers the
BinaryView, LargeList<view> and FixedSizeList<view> compaction branches,
which the first pass left untested — including a guard pinning the arrow behavior
the FixedSizeList arm rests on (slice narrows its child, unlike the offset
flavours), so a future arrow release that changed it would fail loudly instead of
silently re-absorbing neighbouring rows' bytes.

Rebases onto merged work

#23 (CHA-539). Merged first; the rebase landed the fork_copy.rs half
intact: all three copy statements (table_persist_segment_metadata,
segment_index_metadata, snapshot_segment_metadata) select old.content_hash
verbatim. No TODO(CHA-545) is left anywhere in the tree.

#37 (CHA-546). Merged during review; rebased onto it, conflicting in
fork_copy.rs and persist.rs where both changes edit the same SQL. #37 made
the copy statements name the parent and child partitions separately
(FROM {seg_old} old) and bind the already-parsed branch uuid; this PR adds
old.content_hash to the same SELECT lists and a content_hash bind to
repoint_table_persist_segment. Both halves are kept in each of the four
resolutions.

Out of scope

Round-trip identity coverage. CHA-548 — write a fixture batch per
CanonicalType variant through each FormatWriter, read it back via
read_segment_native, assert schema and data equality, and assert the two
formats agree. That is the invariant this PR's key assumes; penca-format is the
one consumer that never matches on CanonicalType, so CHA-386's cross-crate
exhaustiveness guarantee does not currently reach it.

Checksum reuse. CHA-545 names it as a possible second use of this digest. It
is not available yet: the digest is taken on the in-memory batch before the
format writer encodes, so verifying a stored file against it means re-deriving it
from a decode — sound only once round-trip identity is established. CHA-548 is
that gate too.

Schema-directed decode. Neither reader consults the table's stored
point-in-time arrow_schema; Parquet decodes under builder.schema() and Lance
under reader.metadata().file_schema, and nothing in penca-format casts. That
predates this PR — old read_segment and new read_segment_native share the
same read_in_file_schema. Directing the decode from the stored schema would
make the decoded types a function of metadata rather than of the encoder, which
would turn the cross-format assumption above into a guarantee rather than
something CHA-548 has to keep testing. Recorded as an open question rather than
built here.

Verification

Red-verified. The two red-test tasks were written first and observed failing
before any implementation: the dedup tests (0d0a9fe) and the cross-branch
schema-divergence guard (1343c25). The latter is the one that forced the
native-decode split — with a caller-shaped cache value it fails on a type
mismatch the fork's own metadata never justified.

Green.

• just check — full workspace gate, green on the final tree
• just cargo-clippy — clean on the final tree
• Full integration suite on a freshly-wiped --profile=test stack, against the
  final tree: 729 passed, 16 skipped, 0 failed (serial phase 66, parallel
  phase 663)

Branch CI skips integration entirely — it runs on merge_group, so the queue is
the authoritative gate — which is why the suite was run by hand from scratch
after each round of review follow-ups, after the rebase onto f557d1c, and once
more on the final tree. Counts moved only by what main added (721 → 729 as #37
landed); zero failures throughout.

That full-suite run is what caught 1b7736f: a white-box test hand-INSERTs a
fake segment row from an explicit column list that predated content_hash, and
the new NOT NULL broke it. No other white-box insert touches an affected table
— the purge_tx_log ones write tx_log_persist_segment_metadata, which
deliberately has no content_hash.

🤖 Generated with Claude Code

[nhobin219]

Penca PR Review — CHA-545 content-hash-keyed segment cache

Verdict: comment-only (own PR — event=COMMENT). No Critical findings. The load-bearing correctness split — cache the file-native decode, shape per caller after the lookup — is implemented correctly and completely. One Important finding on the key space, one Suggestion on contradictory docstrings.

CI is fully green (Rust clippy + fmt + test, Python unit tests, CodeQL, all Analyze jobs).

What I verified (rather than took on trust)

• Exactly one cache.insert in the whole workspace (crates/penca-dl/src/driver.rs:236, inside decode_and_cache_native), and it always stores reader.read_segment_native(...). No path caches a caller-shaped batch; no path shapes before caching.
• All three read entry points shape after the lookup, including on the hit branch: read_cached_snapshot_segment (via _unshaped + CachedSegment), read_cached_persist_segment (return shape_native(&full, full_schema) on hit), read_cached_index_sidecar (segment_index_schema(key_types) computed before the lookup, applied after). non_nullable_missing_column_errors_on_a_cache_hit_too pins exactly the branch a regression would hide in.
• take_matched_rows takes matched rows before shaping, keeping the seek path O(matches) rather than O(segment); guarded by seek_snapshot_point_null_fills_a_column_absent_from_the_file.
• Every fresh cold write hashes the exact typed batch it hands the writer: persist and snapshot step.batch (durable_writer.rs), per-partition slices in packer.rs::flush / single_partition_file, the empty-merge placeholder, and the sidecar batch that write_segment_index actually writes.
• compact.rs correctly recomputes per repointed slice instead of carrying the input row's hash, and the (cumulative, meta.row_count) slice offsets match what the repoint writes. The rationale comment (concat_batches normalizes to one segment_schema, so a slice can decode wider than its input file) is the right reason.
• Every reference copy inherits old.content_hash verbatim: fork_copy.rs ×3 (snapshot segments, sidecars, persist segments), plus both CHA-531 carry-forwards in snapshot.rs and segment_index.rs. No path mints a fresh hash on a copy.
• No cold write path into the three tables omits content_hash. The remaining penca-storage-meta INSERTs target parent/header tables that legitimately have none, and there is no Python writer for these tables.
• The "recreate, don't migrate" claim checks out against the pre-existing comment in crates/penca-db/src/dialect/pg.rs:586 ("This DDL only runs at CreateCatalog; pre-release there is no in-place migration path — recreate catalogs that predate a schema change"). No legacy rows, so no uuid fallback key space is needed.
• The nil-hash escape is genuinely unreachable. The two production ..PersistSegment::default() sites (query/mod.rs:2059, :2541) both feed ColdStorageClient::read_persist_segments, which goes through read_one_persist_segment — not read_cached_persist_segment. provider.rs:233 is the only production cache-read construction and its segments come from meta_plan.rs, built field-by-field off a NOT NULL column. The plan.rs comment defending the Default is accurate.
• Weight accounting did not regress. admits() and the weigher are unchanged and still charge size_bytes (the in-memory Arrow footprint, same unit as the budget). The native decode is if anything smaller than the old caller-shaped value.
• Skill invariants clean: no bare SessionContext::new() on any cold path (all hits are tests or comments); penca-api has no datafusion dependency and no use datafusion. All 24 commit headlines are conventional with in-repo scopes (cold, schema, query).

Strengths

• The read_segment_native / shape_to_schema split is the right factoring, not a workaround: read_in_file_schema is shared by both arms in parquet.rs and lance.rs, and the concat_batches anchor correctly moved to batches[0].schema() (file types) rather than the caller's.
• crates/penca-core/src/digest.rs documents the slice-invariance contract by Arrow type class and pins the one case where it fails (Utf8View/BinaryView, because the writer emits variadic data buffers whole) with a test named for it — sliced_view_column_is_the_documented_non_invariant_case. That is exactly the kind of boundary that would otherwise be discovered in production.
• meta_plan.rs::decode_child_sidecar deliberately uses a non-Option row.get::<Uuid, _>("sidecar_content_hash") with a comment saying why unwrap_or(Uuid::nil()) would alias every sidecar onto one entry. Correct instinct, correctly documented.
• The integration guard doesn't just assert the divergent read — it joins on content_hash across both the persist and snapshot tiers and raises if shared == 0, so the test can't silently degrade into a tautology when the fork stops sharing slices.
• IndexSidecar deliberately having no Default (unlike the two segment types) is a well-reasoned asymmetry, not an oversight.

[nhobin219]

Important: format lost its functional dependency on the key, but stayed out of the key.

The docstring this line replaces justified the omission with a premise the PR removes:

format is intentionally absent from the key — it is functionally determined by the uuid, and every value is a format-agnostic decoded RecordBatch

Under uuid keying that held: one segment_uuid → one metadata row → one format. Under content_hash keying it does not. content_hash is a digest of one pre-encode typed batch, so two metadata rows carrying the same hash may name files written by different FormatWriters. The replacement text keeps the conclusion but substitutes a premise that this PR makes false for the value: after the native/shape split the cached value is deliberately the file-native decode (read_segment_native) — precisely the format-dependent artifact, not a format-agnostic one.

Mechanically, the lookup happens before the reader is chosen. In all three call sites (driver.rs:314, :452, :515) cache.get(&segment.content_hash) runs above segment.format.as_wire_code(), so a hit never consults the row's format at all.

Reachability: ObjectStorageConfig::build_readers() populates a HashMap<i32, R> of every configured format and dispatch is per metadata row, so one process can hold parquet- and lance-decoded values in the same cache. OBJECT_STORAGE_FORMAT is deployment-wide at write time, so a cross-format duplicate needs the same typed batch written once under each format — an OBJECT_STORAGE_FORMAT flip is the reachable path, and this PR itself names the content most likely to recur across one: snapshot_op.rs::empty_merge_placeholder_step ("Every empty placeholder over one schema is the same zero-row content, so they legitimately collapse onto one cache entry").

Blast radius is bounded — the shaping tail (shape_to_schema → RecordBatch::try_new) hard-errors on a type mismatch rather than silently returning wrong types — but "which reader decoded this" is no longer a property the key preserves, and where the two natives happen to agree structurally the wrong-format value is served without complaint.

Cheap fix, either shape:

• key on (Uuid, i32) with format.as_wire_code() as the second component, or
• fold the wire code into segment_content_hash so the key stays a plain Uuid.

Either restores the functional dependency the pre-PR comment was resting on. At minimum the sentence should stop asserting the value is format-agnostic, since the paragraph three lines above it says the opposite.

[nhobin219]

Suggestion: this sentence and the SegmentCache docstring take opposite positions on cross-artifact-class collisions.

Here: the sidecar's hash "differs from any base segment's, so the two never collide in one cache."

crates/penca-dl/src/cache.rs:51-55: "two entries collide only when their decoded batches are identical, in which case sharing one entry is the correct answer rather than a bug — a base segment and a sidecar that decode to the same batch may safely share."

The cache.rs framing is the sound one, and it's the one that makes the flat key space defensible: a same-content collision is by construction safe, so no per-class prefix is needed. The "never collide" claim here is both stronger than needed and not enforced anywhere — it happens to hold because the sidecar schema is (key_0..key_{n-1}, row_offset) and a base segment's never is, but nothing checks that, and if it ever stopped holding the cache.rs argument says it still wouldn't be a bug.

Suggest dropping the "so the two never collide in one cache" clause and pointing at SegmentCache's key-space paragraph instead, so there's one statement of the invariant rather than two that disagree.