From 0d0a9fe9518ce2269548d62f1e2182363e69c60f Mon Sep 17 00:00:00 2001 From: Nico Bautista Hobin Date: Fri, 31 Jul 2026 01:18:22 +0000 Subject: [PATCH 01/32] test(cold): add failing cross-branch segment cache dedup tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reference copies (snapshot carry-forward CHA-531, fork copy CHA-539) mint a new row uuid over an unchanged (uri, offset, length), so the uuid-keyed SegmentCache stores N identical decodes of one byte range. Add content_hash: Uuid to PersistSegment, SnapshotSegment, and IndexSidecar plus the two Default impls, and three driver tests asserting that two rows sharing a content_hash decode once. All three fail today (counter 2, want 1) — one per artifact class, because the scope rule is that segments and index sidecars dedup identically. CHA-545 Co-Authored-By: Claude Opus 5 --- crates/penca-core/src/plan.rs | 27 +++++++ crates/penca-dl/src/driver.rs | 128 ++++++++++++++++++++++++++++++++ crates/penca-dl/src/provider.rs | 12 +++ 3 files changed, 167 insertions(+) diff --git a/crates/penca-core/src/plan.rs b/crates/penca-core/src/plan.rs index ae0c921..95d2e10 100644 --- a/crates/penca-core/src/plan.rs +++ b/crates/penca-core/src/plan.rs @@ -13,6 +13,7 @@ //! [`CommittedAtBounds`] rather than the proto `TimestampFilter`. use crate::Format; +use uuid::Uuid; /// Microsecond window on `commit_micros`: inclusive lower /// (`min_micros`), exclusive upper (`max_micros`); both optional. Native @@ -96,6 +97,12 @@ pub struct PersistSegment { /// `..PersistSegment::default()`: a cold `tx_log` segment reuses this type /// but holds commit metadata, not data rows, and has no ceiling. pub max_commit_seq_num: Option, + /// `xxh3_128` of the segment's typed in-memory Arrow batch, recorded at + /// write time and inherited verbatim by reference copies. Keys the segment + /// cache, so a fork and its parent share one decoded entry for a byte range + /// they both reference — which the row uuid cannot express, because a + /// reference copy mints a new uuid over unchanged bytes (CHA-545). + pub content_hash: Uuid, } /// The internal `row_uuid` index sidecar attached to a snapshot segment — the @@ -118,6 +125,11 @@ pub struct IndexSidecar { pub segment_index_uuid: String, /// In-memory Arrow footprint, for the shared segment cache's byte budget. pub size_bytes: i64, + /// `xxh3_128` of the sidecar's typed in-memory Arrow batch. Same role as on + /// [`SnapshotSegment`]: sidecars are read through the same cache and + /// reference-copied by the same paths, so they duplicate for the same + /// reason and dedup by the same key (CHA-545). + pub content_hash: Uuid, } /// A snapshot segment in cold storage (read-optimized baseline). @@ -154,6 +166,10 @@ pub struct SnapshotSegment { /// tables, never a planner candidate). The internal identity sidecar stays /// in its dedicated `row_uuid_index_sidecar` slot. pub index_sidecars: Vec<(String, IndexSidecar)>, + /// `xxh3_128` of the segment's typed in-memory Arrow batch, recorded at + /// write time and inherited verbatim by reference copies (carry-forward, + /// fork copy). Keys the segment cache — see [`PersistSegment::content_hash`]. + pub content_hash: Uuid, } /// A user secondary index declared for the plan's snapshot (CHA-485) — the @@ -276,6 +292,15 @@ pub struct Plan { // can `..Default::default()`. `Format` itself stays default-free by // design (no `Unspecified` — see `format.rs`), so the segments pick // `Format::Parquet` as the placeholder; any test that cares sets it. +// +// `content_hash` defaults to `Uuid::nil()`, which would be a shared cache key +// if it ever reached the cache. It cannot: the only production caller of these +// `Default`s is the cold `tx_log` carrier path, whose segments hold commit +// metadata and are read by `read_tx_log_batches`, never through `SegmentCache` +// (CHA-545). Every cache-read segment is built field-by-field from a metadata +// row whose `content_hash` is `NOT NULL`. `IndexSidecar` deliberately has no +// `Default` for the same reason — it has no such non-cached carrier path, so a +// nil hash there would be reachable. impl Default for PersistSegment { fn default() -> Self { Self { @@ -289,6 +314,7 @@ impl Default for PersistSegment { offset: None, length: None, max_commit_seq_num: None, + content_hash: Uuid::nil(), } } } @@ -309,6 +335,7 @@ impl Default for SnapshotSegment { statistics: Vec::new(), row_uuid_index_sidecar: None, index_sidecars: Vec::new(), + content_hash: Uuid::nil(), } } } diff --git a/crates/penca-dl/src/driver.rs b/crates/penca-dl/src/driver.rs index e72b558..c6b4eaa 100644 --- a/crates/penca-dl/src/driver.rs +++ b/crates/penca-dl/src/driver.rs @@ -1088,6 +1088,7 @@ mod tests { format: Format::Parquet, segment_index_uuid: format!("idx-{name}"), size_bytes: 256, + content_hash: Uuid::nil(), }), ..Default::default() } @@ -1251,6 +1252,7 @@ mod tests { format: Format::Parquet, segment_index_uuid: format!("idx-{name}"), size_bytes: 256, + content_hash: Uuid::nil(), }), ..Default::default() } @@ -1521,6 +1523,7 @@ mod tests { format: Format::Parquet, segment_index_uuid: "idx-identity-never-read".to_string(), size_bytes: 256, + content_hash: Uuid::nil(), }); let dl = routing_driver(cache, by_uri); @@ -1550,6 +1553,7 @@ mod tests { format: Format::Parquet, segment_index_uuid: "idx-identity".to_string(), size_bytes: 256, + content_hash: Uuid::nil(), }); let name_index = Uuid::new_v4(); let res = dl @@ -1591,6 +1595,7 @@ mod tests { format: Format::Parquet, segment_index_uuid: "idx-other".to_string(), size_bytes: 256, + content_hash: Uuid::nil(), }, )]; let requested = Uuid::new_v4(); // != the keyed index present @@ -1635,6 +1640,7 @@ mod tests { format: Format::Parquet, segment_index_uuid: "idx-keyed-never-read".to_string(), size_bytes: 256, + content_hash: Uuid::nil(), }, )]; let dl = routing_driver(cache, by_uri); @@ -1756,6 +1762,128 @@ mod tests { ); } + /// Two metadata rows over one byte range — exactly what snapshot + /// carry-forward (CHA-531) and a fork's reference copy (CHA-539) produce: a + /// NEW row uuid over an unchanged `(uri, offset, length)`. Keyed by uuid the + /// cache stores both decodes of identical bytes; keyed by `content_hash` it + /// stores one (CHA-545). + /// + /// One test per artifact class because the scope rule is precisely that all + /// three behave identically — a fix that dedups segments but leaves sidecars + /// uuid-keyed would pass a single-class test. + #[tokio::test] + async fn snapshot_segments_sharing_content_hash_decode_once() { + let schema = test_schema(); + let cache = Arc::new(SegmentCache::new(1 << 20)); + let (dl, reads) = driver_with(cache, test_batch(&schema)); + + let shared = Uuid::from_u128(0xc0ffee); + let mut parent = segment("parent-seg", 128); + parent.uri = "s3://t/shared.parquet".into(); + parent.content_hash = shared; + let mut fork = segment("fork-seg", 128); + fork.uri = parent.uri.clone(); + fork.content_hash = shared; + + let first = read_seg(&dl, &parent, &schema, &schema).await.unwrap(); + let second = read_seg(&dl, &fork, &schema, &schema).await.unwrap(); + + assert_eq!( + reads.load(Ordering::SeqCst), + 1, + "distinct segment uuids over one content hash must share a cache entry" + ); + assert_eq!(first, second, "both rows resolve to the same decoded batch"); + } + + #[tokio::test] + async fn persist_segments_sharing_content_hash_decode_once() { + let schema = test_schema(); + let cache = Arc::new(SegmentCache::new(1 << 20)); + let (dl, reads) = driver_with(cache, test_batch(&schema)); + + let shared = Uuid::from_u128(0xc0ffee); + let seg = |uuid: &str| PersistSegment { + segment_uuid: uuid.to_string(), + uri: "s3://t/shared.parquet".into(), + format: Format::Parquet, + size_bytes: 256, + content_hash: shared, + ..Default::default() + }; + + let first = read_cached_persist_segment( + dl.readers.as_ref(), + &dl.cache, + &seg("parent-p"), + &schema, + &schema, + ) + .await + .unwrap(); + let second = read_cached_persist_segment( + dl.readers.as_ref(), + &dl.cache, + &seg("fork-p"), + &schema, + &schema, + ) + .await + .unwrap(); + + assert_eq!( + reads.load(Ordering::SeqCst), + 1, + "distinct persist segment uuids over one content hash must share a cache entry" + ); + assert_eq!(first, second, "both rows resolve to the same decoded batch"); + } + + /// `CountingFormatReader::read_segment` ignores its `schema` argument and + /// returns a fixed batch, so no sidecar-shaped fixture is needed — the + /// `key_types` slice only has to reach the reader. + #[tokio::test] + async fn index_sidecars_sharing_content_hash_decode_once() { + let schema = test_schema(); + let cache = Arc::new(SegmentCache::new(1 << 20)); + let (dl, reads) = driver_with(cache, test_batch(&schema)); + + let shared = Uuid::from_u128(0xbeef); + let sidecar = |uuid: &str| IndexSidecar { + object_uri: "s3://t/shared.idx".into(), + offset: 0, + length: 3, + format: Format::Parquet, + segment_index_uuid: uuid.to_string(), + size_bytes: 256, + content_hash: shared, + }; + + let first = read_cached_index_sidecar( + dl.readers.as_ref(), + &dl.cache, + &sidecar("parent-idx"), + &[DataType::Utf8], + ) + .await + .unwrap(); + let second = read_cached_index_sidecar( + dl.readers.as_ref(), + &dl.cache, + &sidecar("fork-idx"), + &[DataType::Utf8], + ) + .await + .unwrap(); + + assert_eq!( + reads.load(Ordering::SeqCst), + 1, + "distinct sidecar uuids over one content hash must share a cache entry" + ); + assert_eq!(first, second, "both rows resolve to the same decoded batch"); + } + #[tokio::test] async fn oversized_segment_not_cached_uses_uncached_read() { let schema = test_schema(); diff --git a/crates/penca-dl/src/provider.rs b/crates/penca-dl/src/provider.rs index ca480f5..27cbb23 100644 --- a/crates/penca-dl/src/provider.rs +++ b/crates/penca-dl/src/provider.rs @@ -674,6 +674,7 @@ mod tests { use datafusion::datasource::MemTable; use penca_core::{Format, PersistPlan}; use penca_format::reader::{AnyFormatReader, FormatError}; + use uuid::Uuid; fn user_schema() -> SchemaRef { Arc::new(Schema::new(vec![ @@ -815,6 +816,7 @@ mod tests { offset: None, length: None, max_commit_seq_num: None, + content_hash: Uuid::nil(), } } @@ -1323,6 +1325,7 @@ mod tests { format: Format::Parquet, segment_index_uuid: "sidecar1".to_string(), size_bytes: 256, + content_hash: Uuid::nil(), }), ..Default::default() }; @@ -1396,6 +1399,7 @@ mod tests { format: Format::Parquet, segment_index_uuid: "sidecar1".to_string(), size_bytes: 256, + content_hash: Uuid::nil(), }), ..Default::default() }; @@ -1483,6 +1487,7 @@ mod tests { format: Format::Parquet, segment_index_uuid: "sc".to_string(), size_bytes: 64, + content_hash: Uuid::nil(), }); let provider = SnapshotTableProvider::new( vec![seg], @@ -1551,6 +1556,7 @@ mod tests { format: Format::Parquet, segment_index_uuid: "sidecar-identity".to_string(), size_bytes: 256, + content_hash: Uuid::nil(), }), index_sidecars: vec![( user_index_uuid.to_string(), @@ -1561,6 +1567,7 @@ mod tests { format: Format::Parquet, segment_index_uuid: "sidecar-user".to_string(), size_bytes: 256, + content_hash: Uuid::nil(), }, )], ..Default::default() @@ -1642,6 +1649,7 @@ mod tests { format: Format::Parquet, segment_index_uuid: "sidecar-identity".to_string(), size_bytes: 256, + content_hash: Uuid::nil(), }), // No keyed sidecars: the user entry below cannot resolve. ..Default::default() @@ -1693,6 +1701,7 @@ mod tests { format: Format::Parquet, segment_index_uuid: "id-sc".to_string(), size_bytes: 1, + content_hash: Uuid::nil(), }), index_sidecars: vec![( user.to_string(), @@ -1703,6 +1712,7 @@ mod tests { format: Format::Parquet, segment_index_uuid: "user-sc".to_string(), size_bytes: 1, + content_hash: Uuid::nil(), }, )], ..Default::default() @@ -1777,6 +1787,7 @@ mod tests { format: Format::Parquet, segment_index_uuid: "sidecar-value".to_string(), size_bytes: 256, + content_hash: Uuid::nil(), }, )], ..Default::default() @@ -1888,6 +1899,7 @@ mod tests { format: Format::Parquet, segment_index_uuid: "sidecar-x".to_string(), size_bytes: 128, + content_hash: Uuid::nil(), }, )], ..Default::default() From 401f38ee96e85e1c81a521c2fec51e27bb516c96 Mon Sep 17 00:00:00 2001 From: Nico Bautista Hobin Date: Fri, 31 Jul 2026 01:22:48 +0000 Subject: [PATCH 02/32] feat(cold): add segment_content_hash digest over typed Arrow batches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit xxh3_128 of the decoded in-memory batch, canonically encoded via Arrow IPC with explicitly pinned options (alignment, no compression, MetadataVersion V5) so the digest cannot shift under an arrow-rs default change. Hashing the typed batch rather than the encoded file bytes is what makes it schema-sensitive: two segments over the same (uri, offset, length) slice under different schemas are different decoded objects and must not collide. Doc comment records that this is write-time only — a pre-write digest is not known to survive a format round-trip, so the checksum reuse CHA-545 mentions needs round-trip stability established first. CHA-545 Co-Authored-By: Claude Opus 5 --- crates/penca-core/src/digest.rs | 227 ++++++++++++++++++++++++++++++++ crates/penca-core/src/lib.rs | 1 + 2 files changed, 228 insertions(+) create mode 100644 crates/penca-core/src/digest.rs diff --git a/crates/penca-core/src/digest.rs b/crates/penca-core/src/digest.rs new file mode 100644 index 0000000..d406ecd --- /dev/null +++ b/crates/penca-core/src/digest.rs @@ -0,0 +1,227 @@ +//! Content digest of a decoded cold segment (CHA-545). + +use arrow::array::RecordBatch; +use arrow::error::ArrowError; +use arrow::ipc::MetadataVersion; +use arrow::ipc::writer::{IpcWriteOptions, StreamWriter}; +use uuid::Uuid; +use xxhash_rust::xxh3::xxh3_128; + +/// Alignment for the canonical IPC encoding. Pinned rather than defaulted so +/// the digest does not silently change under an arrow-rs upgrade. +const IPC_ALIGNMENT: usize = 8; + +/// `xxh3_128` of a segment's **typed in-memory Arrow batch**, as a [`Uuid`]. +/// +/// The input is the decoded batch, never the encoded file bytes. That is what +/// makes the digest schema-sensitive: two segments referencing the same +/// `(object_uri, offset, length)` slice under different schemas must hash +/// differently, because they are two different decoded objects and a cache +/// keyed by this value would otherwise hand one caller the other's types. +/// +/// **Write-time only.** A digest taken before the format writer encodes a batch +/// is *not* guaranteed to equal the digest of a later decode of that file: +/// Parquet may widen a type, re-dictionary-encode, or normalize on round-trip. +/// Dedup never compares the two — the value is computed once at write and +/// inherited by reference copies, never recomputed. CHA-545 names checksum +/// reuse as a possible future use of this digest; that use requires +/// establishing round-trip stability first, and this function does not. +/// +/// Changing the normalization, the IPC options, or the hash changes every +/// future digest. That is safe — stored digests are opaque identity, never +/// recomputed and compared against a stored value — but it must stay +/// deliberate, which is why nothing here relies on a library default. +pub fn segment_content_hash(batch: &RecordBatch) -> Result { + // A batch from `slice()` stays a view onto its parent's buffers, so those + // buffers physically hold bytes outside the slice. Measured on arrow-rs + // 57.3: the IPC writer already encodes only the logical range, so for + // primitive and Utf8 columns this concat is redundant (removing it leaves + // `slice_hashes_equal_to_an_independently_built_batch` green). It stays + // because that redundancy is an arrow-rs implementation detail and this + // digest must be representation-independent for every column type, + // including the nested ones no test here covers. + let compacted = arrow::compute::concat_batches(&batch.schema(), std::slice::from_ref(batch))?; + + let options = IpcWriteOptions::try_new(IPC_ALIGNMENT, false, MetadataVersion::V5)?; + let mut bytes = Vec::new(); + let mut writer = StreamWriter::try_new_with_options(&mut bytes, &compacted.schema(), options)?; + writer.write(&compacted)?; + writer.finish()?; + drop(writer); + + Ok(Uuid::from_u128(xxh3_128(&bytes))) +} + +#[cfg(test)] +mod tests { + use std::sync::Arc; + + use arrow::array::{Int64Array, StringArray}; + use arrow::datatypes::{DataType, Field, Schema}; + + use super::*; + + fn batch(fields: Vec, columns: Vec) -> RecordBatch { + RecordBatch::try_new(Arc::new(Schema::new(fields)), columns).expect("valid fixture") + } + + fn i64_col(name: &str, values: Vec>) -> (Field, arrow::array::ArrayRef) { + ( + Field::new(name, DataType::Int64, true), + Arc::new(Int64Array::from(values)), + ) + } + + fn one_i64(name: &str, values: Vec>) -> RecordBatch { + let (f, c) = i64_col(name, values); + batch(vec![f], vec![c]) + } + + fn hash(b: &RecordBatch) -> Uuid { + segment_content_hash(b).expect("digest") + } + + #[test] + fn identical_batches_hash_equal() { + let a = one_i64("a", vec![Some(1), Some(2), Some(3)]); + let b = one_i64("a", vec![Some(1), Some(2), Some(3)]); + assert_eq!(hash(&a), hash(&a), "same batch hashed twice is stable"); + assert_eq!( + hash(&a), + hash(&b), + "independently built identical batches agree" + ); + } + + #[test] + fn differing_values_hash_differently() { + let a = one_i64("a", vec![Some(1), Some(2), Some(3)]); + let b = one_i64("a", vec![Some(1), Some(9), Some(3)]); + assert_ne!( + hash(&a), + hash(&b), + "one differing cell must change the hash" + ); + } + + #[test] + fn null_differs_from_value_in_the_same_cell() { + let a = one_i64("a", vec![Some(1), Some(2)]); + let b = one_i64("a", vec![Some(1), None]); + assert_ne!( + hash(&a), + hash(&b), + "null is not the same content as a value" + ); + } + + /// The schema-divergence case the whole ticket exists for: same column name, + /// same values as rendered, different type. A byte-level hash of the file + /// would collide these; hashing the typed batch must not. + #[test] + fn same_values_under_different_column_type_hash_differently() { + let as_int = one_i64("a", vec![Some(1), Some(2)]); + let as_utf8 = batch( + vec![Field::new("a", DataType::Utf8, true)], + vec![Arc::new(StringArray::from(vec![Some("1"), Some("2")]))], + ); + assert_ne!( + hash(&as_int), + hash(&as_utf8), + "Int64 and Utf8 columns are different decoded objects" + ); + } + + #[test] + fn column_name_is_part_of_the_hash() { + let a = one_i64("a", vec![Some(1), Some(2)]); + let b = one_i64("b", vec![Some(1), Some(2)]); + assert_ne!(hash(&a), hash(&b), "column name must change the hash"); + } + + #[test] + fn nullability_flag_is_part_of_the_hash() { + let nullable = batch( + vec![Field::new("a", DataType::Int64, true)], + vec![Arc::new(Int64Array::from(vec![1_i64, 2]))], + ); + let non_nullable = batch( + vec![Field::new("a", DataType::Int64, false)], + vec![Arc::new(Int64Array::from(vec![1_i64, 2]))], + ); + assert_ne!( + hash(&nullable), + hash(&non_nullable), + "nullability is schema, and schema is part of the identity" + ); + } + + #[test] + fn column_order_is_part_of_the_hash() { + let (fa, ca) = i64_col("a", vec![Some(1), Some(2)]); + let (fb, cb) = i64_col("b", vec![Some(3), Some(4)]); + let ab = batch(vec![fa.clone(), fb.clone()], vec![ca.clone(), cb.clone()]); + let ba = batch(vec![fb, fa], vec![cb, ca]); + assert_ne!(hash(&ab), hash(&ba), "column order must change the hash"); + } + + #[test] + fn row_count_is_part_of_the_hash() { + let empty = one_i64("a", vec![]); + let one = one_i64("a", vec![Some(1)]); + let many = one_i64("a", vec![Some(1), Some(2), Some(3)]); + assert_ne!(hash(&empty), hash(&one)); + assert_ne!(hash(&one), hash(&many)); + assert_eq!( + hash(&empty), + hash(&one_i64("a", vec![])), + "zero rows still hashes deterministically" + ); + } + + /// A sliced batch and an independently built batch of the same logical rows + /// must hash equal — the digest is over content, not representation. + /// + /// Uses Utf8 because that is where a slice demonstrably stays a *view* — + /// the guard below asserts the slice still shares its parent's values + /// buffer, so the buffer physically holds bytes outside the slice. Without + /// normalization the encoding could pick those up. (`RecordBatch::slice` + /// reports `offset() == 0` even here, so asserting on the offset would + /// silently pass on a fixture that proves nothing.) + #[test] + fn slice_hashes_equal_to_an_independently_built_batch() { + let parent = batch( + vec![Field::new("s", DataType::Utf8, true)], + vec![Arc::new(StringArray::from(vec!["aa", "bb", "cc", "dd"]))], + ); + let sliced = parent.slice(1, 2); + let standalone = batch( + vec![Field::new("s", DataType::Utf8, true)], + vec![Arc::new(StringArray::from(vec!["bb", "cc"]))], + ); + + assert_eq!( + parent.column(0).to_data().buffers()[1].as_ptr(), + sliced.column(0).to_data().buffers()[1].as_ptr(), + "fixture must still be a view onto the parent's buffer, or this proves nothing" + ); + assert_eq!( + hash(&sliced), + hash(&standalone), + "the digest is over logical content, not buffer position" + ); + } + + /// The primitive twin. `slice()` rebases the data pointer here rather than + /// keeping a logical offset, so this is the cheaper case — asserted anyway + /// because both paths reach `segment_content_hash` in production. + #[test] + fn primitive_slice_hashes_equal_to_an_independently_built_batch() { + let parent = one_i64("a", vec![Some(1), None, Some(3), Some(4)]); + assert_eq!( + hash(&parent.slice(1, 2)), + hash(&one_i64("a", vec![None, Some(3)])), + "sliced primitive rows hash as their own content" + ); + } +} diff --git a/crates/penca-core/src/lib.rs b/crates/penca-core/src/lib.rs index 2d24e65..47e12f5 100644 --- a/crates/penca-core/src/lib.rs +++ b/crates/penca-core/src/lib.rs @@ -1,4 +1,5 @@ pub mod config; +pub mod digest; pub mod error; pub mod format; pub mod log_kind; From 3fd2cefb68a1a6f45915cff80b500e6ca856fd8e Mon Sep 17 00:00:00 2001 From: Nico Bautista Hobin Date: Fri, 31 Jul 2026 01:23:43 +0000 Subject: [PATCH 03/32] docs(cold): correct the slice-invariance note in segment_content_hash The test doc claimed the normalization was what kept a sliced batch from encoding its parent's out-of-range bytes. Measured: removing concat_batches leaves the test green, because arrow-rs's IPC writer already encodes only the logical range. Say what was observed rather than what was assumed. CHA-545 Co-Authored-By: Claude Opus 5 --- crates/penca-core/src/digest.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/crates/penca-core/src/digest.rs b/crates/penca-core/src/digest.rs index d406ecd..3a1fac5 100644 --- a/crates/penca-core/src/digest.rs +++ b/crates/penca-core/src/digest.rs @@ -182,12 +182,15 @@ mod tests { /// A sliced batch and an independently built batch of the same logical rows /// must hash equal — the digest is over content, not representation. /// - /// Uses Utf8 because that is where a slice demonstrably stays a *view* — - /// the guard below asserts the slice still shares its parent's values - /// buffer, so the buffer physically holds bytes outside the slice. Without - /// normalization the encoding could pick those up. (`RecordBatch::slice` + /// Uses Utf8 because that is where a slice demonstrably stays a *view* — the + /// guard below asserts it still shares its parent's values buffer, so that + /// buffer physically holds bytes outside the slice. (`RecordBatch::slice` /// reports `offset() == 0` even here, so asserting on the offset would /// silently pass on a fixture that proves nothing.) + /// + /// This currently passes with or without the `concat_batches` normalization + /// — arrow-rs's IPC writer compacts on its own. It is a guard on the + /// property, not proof that the normalization is reachable. #[test] fn slice_hashes_equal_to_an_independently_built_batch() { let parent = batch( From 50f177db340b376cd85194dc2ca078d1d3fbbfaa Mon Sep 17 00:00:00 2001 From: Nico Bautista Hobin Date: Fri, 31 Jul 2026 02:10:00 +0000 Subject: [PATCH 04/32] feat(schema): content_hash column on segment + index-sidecar metadata MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add `content_hash UUID NOT NULL` to the three cold-artifact metadata tables (persist segment, snapshot segment, snapshot segment index) and carry it from row to plan struct on every read path. NOT NULL with no default: catalog metadata has no in-place migration path, so a catalog predating this change lacks the column, not the value — it is recreated. There is no nullable arm and no fallback key space. `decode_child_sidecar` reads it non-Option, following `segment_index_uuid` rather than the `size_bytes` `unwrap_or(0)` beside it: a nil default there would be one shared cache key for every sidecar, serving segment X's index for segment Y. Test fixtures derive a per-name hash via `deterministic_uuid_from` rather than defaulting to nil, so distinct fixtures stay distinct once the cache keys on this column. Restores workspace compilation after the field additions in the red commit. Verified with `cargo check --workspace --all-targets` and `cargo test -p penca-core`. CHA-545 --- crates/penca-api/src/lifecycle/compact.rs | 1 + crates/penca-api/src/query/meta_plan.rs | 22 ++++++++++++----- crates/penca-core/src/plan.rs | 6 +++-- crates/penca-db/src/dialect/pg.rs | 7 ++++++ crates/penca-merge/benches/floor_support.rs | 2 ++ crates/penca-merge/src/lib.rs | 2 ++ crates/penca-storage-cold/src/lib.rs | 3 +++ crates/penca-storage-meta/src/persist.rs | 1 + docs/schema-reference.md | 27 +++++++++++++++++++++ 9 files changed, 63 insertions(+), 8 deletions(-) diff --git a/crates/penca-api/src/lifecycle/compact.rs b/crates/penca-api/src/lifecycle/compact.rs index ef5c126..61813b1 100644 --- a/crates/penca-api/src/lifecycle/compact.rs +++ b/crates/penca-api/src/lifecycle/compact.rs @@ -159,6 +159,7 @@ impl<'a> PersistScope<'a> { // honoring the ceiling AND the slice arithmetic has to be reworked // for short reads. max_commit_seq_num: Some(row.get("max_commit_seq_num")), + content_hash: row.get("content_hash"), }) } } diff --git a/crates/penca-api/src/query/meta_plan.rs b/crates/penca-api/src/query/meta_plan.rs index c72e864..b362c24 100644 --- a/crates/penca-api/src/query/meta_plan.rs +++ b/crates/penca-api/src/query/meta_plan.rs @@ -784,7 +784,7 @@ impl QueryManager { }; let snapshot_sql = format!( "SELECT seg.table_snapshot_segment_uuid, seg.object_uri, \ - seg.\"offset\", seg.length, seg.format, \ + seg.\"offset\", seg.length, seg.format, seg.content_hash, \ snap.snapshotted_at_micros, snap.commit_seq_num, \ seg.table_snapshot_uuid, seg.row_count, \ seg.size_bytes, seg.metadata, seg.statistics, \ @@ -794,7 +794,8 @@ impl QueryManager { c.object_uri AS sidecar_object_uri, \ c.\"offset\" AS sidecar_offset, c.length AS sidecar_length, \ c.format AS sidecar_format, c.size_bytes AS sidecar_size_bytes, \ - c.segment_index_uuid AS sidecar_segment_index_uuid \ + c.segment_index_uuid AS sidecar_segment_index_uuid, \ + c.content_hash AS sidecar_content_hash \ FROM {seg_table} seg \ INNER JOIN {snap_table} snap \ ON seg.table_snapshot_uuid = snap.table_snapshot_uuid \ @@ -909,6 +910,7 @@ impl QueryManager { size_bytes: row.get("size_bytes"), metadata_json: metadata.to_string(), statistics: statistics.unwrap_or_default(), + content_hash: row.get("content_hash"), row_uuid_index_sidecar: None, index_sidecars: Vec::new(), }); @@ -1264,7 +1266,7 @@ impl QueryManager { "SELECT tfm.log_kind, \ seg.table_persist_segment_uuid AS segment_uuid, \ seg.object_uri, seg.\"offset\", seg.length, \ - seg.row_count, seg.format, \ + seg.row_count, seg.format, seg.content_hash, \ seg.min_tx_commit_micros, \ seg.max_tx_commit_micros, \ seg.max_commit_seq_num, \ @@ -1340,6 +1342,7 @@ impl QueryManager { // segment this equals the file's true maximum, so the ceiling // costs a no-op filter and needs no per-row special case. max_commit_seq_num: Some(row.get("max_commit_seq_num")), + content_hash: row.get("content_hash"), }; match log_kind { @@ -1450,9 +1453,10 @@ pub(crate) struct HotTableNames { /// Decode one row's LEFT-JOINed child-sidecar columns into an /// [`IndexSidecar`], or `None` when no child matched (NULL /// `sidecar_object_uri`). The object_uri-presence gate stands in for "the -/// whole child row matched": offset/length/format/segment_index_uuid are all -/// NOT NULL in the schema, so the non-Option `row.get`s cannot hit a NULL -/// while object_uri is present. Revisit if a nullable child column is added. +/// whole child row matched": offset/length/format/segment_index_uuid/ +/// content_hash are all NOT NULL in the schema, so the non-Option `row.get`s +/// cannot hit a NULL while object_uri is present. Revisit if a nullable child +/// column is added. fn decode_child_sidecar(row: &PgRow) -> Result> { row.get::, _>("sidecar_object_uri") .map(|object_uri| -> Result { @@ -1471,6 +1475,10 @@ fn decode_child_sidecar(row: &PgRow) -> Result> { format: sidecar_format, segment_index_uuid: row.get::("sidecar_segment_index_uuid").to_string(), size_bytes: row.get::, _>("sidecar_size_bytes").unwrap_or(0), + // Non-Option deliberately, unlike `size_bytes` directly above: + // an `unwrap_or(Uuid::nil())` here would hand every sidecar the + // same cache key and serve segment X's index for segment Y. + content_hash: row.get::("sidecar_content_hash"), }) }) .transpose() @@ -1785,6 +1793,7 @@ mod assemble_tests { offset: None, length: None, max_commit_seq_num: None, + content_hash: penca_core::naming::deterministic_uuid_from(&[uuid]), } } @@ -1956,6 +1965,7 @@ mod assemble_tests { format: Format::Parquet, segment_index_uuid: uri.to_string(), size_bytes: 1, + content_hash: penca_core::naming::deterministic_uuid_from(&[uri]), } } diff --git a/crates/penca-core/src/plan.rs b/crates/penca-core/src/plan.rs index 95d2e10..2ae3c02 100644 --- a/crates/penca-core/src/plan.rs +++ b/crates/penca-core/src/plan.rs @@ -120,8 +120,10 @@ pub struct IndexSidecar { pub length: i64, /// Columnar file format of the sidecar. pub format: Format, - /// Globally-unique id of the sidecar — its segment-cache key, a distinct - /// deterministic-UUID namespace from `table_snapshot_segment_uuid`. + /// Globally-unique id of the sidecar row, in a distinct deterministic-UUID + /// namespace from `table_snapshot_segment_uuid`. Row identity only — the + /// segment-cache key is `content_hash`, because a reference copy mints a + /// fresh uuid over bytes it did not rewrite (CHA-545). pub segment_index_uuid: String, /// In-memory Arrow footprint, for the shared segment cache's byte budget. pub size_bytes: i64, diff --git a/crates/penca-db/src/dialect/pg.rs b/crates/penca-db/src/dialect/pg.rs index 634f521..176c9b8 100644 --- a/crates/penca-db/src/dialect/pg.rs +++ b/crates/penca-db/src/dialect/pg.rs @@ -487,6 +487,10 @@ impl PgDialect { // active merged file, `true` for rows of a previously-sealed merged // file. Sealed rows never participate in another compact wave, and the // false → true transition is one-way. + // `content_hash` — on this and the two other cold-artifact tables below + // — is the segment cache key (CHA-545). NOT NULL with no default: + // every writer computes it, and a catalog predating the column is + // recreated rather than migrated, so there is no legacy row to default. driver .execute_no_result(&format!( r#"CREATE TABLE IF NOT EXISTS {qi} ( @@ -504,6 +508,7 @@ impl PgDialect { length BIGINT, row_count BIGINT NOT NULL, format TEXT NOT NULL, + content_hash UUID NOT NULL, size_bytes BIGINT DEFAULT 0, metadata JSONB DEFAULT '{{}}'::jsonb, statistics BYTEA, @@ -626,6 +631,7 @@ impl PgDialect { length BIGINT NOT NULL, size_bytes BIGINT DEFAULT 0, format TEXT NOT NULL, + content_hash UUID NOT NULL, metadata JSONB DEFAULT '{{}}'::jsonb, statistics BYTEA, row_count BIGINT NOT NULL, @@ -752,6 +758,7 @@ impl PgDialect { "offset" BIGINT NOT NULL, length BIGINT NOT NULL, format TEXT NOT NULL, + content_hash UUID NOT NULL, size_bytes BIGINT DEFAULT 0, statistics BYTEA, written_at_micros BIGINT DEFAULT {epoch}, diff --git a/crates/penca-merge/benches/floor_support.rs b/crates/penca-merge/benches/floor_support.rs index a07f5f1..37626e3 100644 --- a/crates/penca-merge/benches/floor_support.rs +++ b/crates/penca-merge/benches/floor_support.rs @@ -259,7 +259,9 @@ pub fn base_segment_with_sidecar(size_bytes: i64, rows: i64) -> SnapshotSegment format: Format::Parquet, segment_index_uuid: "floor-sidecar".to_string(), size_bytes: 1 << 16, + content_hash: penca_core::naming::deterministic_uuid_from(&["floor-sidecar"]), }), + content_hash: penca_core::naming::deterministic_uuid_from(&["floor-seg"]), ..Default::default() } } diff --git a/crates/penca-merge/src/lib.rs b/crates/penca-merge/src/lib.rs index 2633ea3..97308a7 100644 --- a/crates/penca-merge/src/lib.rs +++ b/crates/penca-merge/src/lib.rs @@ -1631,6 +1631,7 @@ mod tests { statistics: Vec::new(), row_uuid_index_sidecar: None, index_sidecars: Vec::new(), + content_hash: penca_core::naming::deterministic_uuid_from(&[uuid]), } } @@ -1661,6 +1662,7 @@ mod tests { offset: None, length: None, max_commit_seq_num: None, + content_hash: penca_core::naming::deterministic_uuid_from(&[uuid]), } } diff --git a/crates/penca-storage-cold/src/lib.rs b/crates/penca-storage-cold/src/lib.rs index 2bc3e65..61a4b78 100644 --- a/crates/penca-storage-cold/src/lib.rs +++ b/crates/penca-storage-cold/src/lib.rs @@ -414,6 +414,9 @@ mod tests { format: Format::Parquet, segment_index_uuid: "idx".to_string(), size_bytes: 0, + // Never reaches a cache — this asserts format dispatch fails before + // any I/O — so the nil `Uuid::default()` cannot alias anything. + content_hash: Default::default(), }; let schema = penca_format::index::segment_index_schema(&[DataType::Utf8]); let err = ColdStorageClient::read_segment_index(&readers, &sidecar, &schema) diff --git a/crates/penca-storage-meta/src/persist.rs b/crates/penca-storage-meta/src/persist.rs index 4e881fb..9bd6fea 100644 --- a/crates/penca-storage-meta/src/persist.rs +++ b/crates/penca-storage-meta/src/persist.rs @@ -700,6 +700,7 @@ impl LifecycleManager { let mut sql = format!( "SELECT seg.table_persist_segment_uuid, seg.table_persist_uuid, seg.object_uri, \ seg.\"offset\", seg.length, seg.format, seg.row_count, seg.size_bytes, \ + seg.content_hash, \ seg.table_uuid, seg.min_tx_commit_micros, seg.max_tx_commit_micros, \ seg.max_commit_seq_num, seg.is_sealed, tfm.log_kind \ FROM {seg} seg \ diff --git a/docs/schema-reference.md b/docs/schema-reference.md index dea19fd..5cbdde3 100644 --- a/docs/schema-reference.md +++ b/docs/schema-reference.md @@ -378,6 +378,7 @@ the `log_kind` classification (CHA-218: only `upsert_log` and | `length` | int64 (set at compact time) | | `row_count` | int64 | | `format` | text (parquet, lance) | +| `content_hash` | UUID NOT NULL (CHA-545, segment-cache key) | | `size_bytes` | int64 | | `metadata` | JSONB | | `statistics` | JSONB | @@ -387,6 +388,17 @@ the `log_kind` classification (CHA-218: only `upsert_log` and Indices: implicit `(branch_uuid, table_persist_segment_uuid)` PK + per-branch partition. +`content_hash` is the `xxh3_128` of the segment's typed in-memory Arrow +batch, computed once at write time and inherited verbatim by every +reference copy (carry-forward, CHA-539 fork copy). It keys the in-process +`SegmentCache`, so a fork and its parent share one decoded entry for a byte +range they both reference — which the row uuid cannot express, since a +reference copy mints a fresh uuid over bytes it did not rewrite. `NOT NULL` +with no default: every writer computes it, and a catalog predating the +column is recreated rather than migrated (there is no in-place migration +path for catalog metadata), so there is no legacy row to default. Not +indexed — reads carry the value through, they never look a row up by it. + Each level's `written_at_micros` / `commit_micros` pair supports the three-level commit decoupling. Rows with `commit_micros IS NULL` (at any level) are invisible to reads @@ -417,6 +429,12 @@ committed rows gates `PurgeTxLog`; reads seek the sorted `commit_seq_num` / | `format` | text (parquet, lance) | | `committed_at_micros` | int64 (NULL until per-segment commit) | +Deliberately carries **no** `content_hash`, unlike the three cold-artifact +tables around it (14, 16, 17): a cold `tx_log` file is read by +`read_tx_log_batches`, never through `SegmentCache`, and is never +reference-copied — so it has neither of the two properties the hash exists +to serve. + **15. Table snapshot metadata** — `{catalog_uuid}_table_snapshot_metadata` Per-catalog, LIST-partitioned by `branch_uuid` (CHA-198). One row per @@ -464,6 +482,7 @@ completes). | `length` | int64 NOT NULL (row count of the range) | | `size_bytes` | int64 | | `format` | text (lance, parquet) | +| `content_hash` | UUID NOT NULL (CHA-545, segment-cache key — see table 14) | | `metadata` | JSON (format-specific, e.g., row group size) | | `statistics` | JSON (column stats: min/max for filterable columns) | | `row_count` | int64 | @@ -520,6 +539,7 @@ forward by reference with its base segment and participates in the ref-counted G | `offset` | int64 | | `length` | int64 | | `format` | text (lance, parquet) | +| `content_hash` | UUID NOT NULL (CHA-545, segment-cache key — see table 14) | | `size_bytes` | int64 | | `statistics` | bytes (indexed-key min/max bounds; binary, decoded in-planner by the CHA-454 seek in the `SnapshotTableProvider`) | | `written_at_micros` | int64 (micros, auto-generated) | @@ -531,6 +551,13 @@ Indices: implicit `(branch_uuid, …_uuid)` PKs + per-branch partitions; child `index_uuid IS NULL` (role: internal vs user secondary) and `index_type` (physical layout, on `__penca_system__.indexes`) are orthogonal. +Sidecars carry `content_hash` for the same reason base segments do: they are +read out of object storage through the same `SegmentCache` and copied by +reference by both carry-forward and the CHA-539 fork copy. `segment_index_uuid` +is stable across a carry-forward *within* a branch, but a fork derives it from +the child's own `segment_uuid` and so mints a new id over unchanged bytes — +which is exactly the duplication the hash collapses. + ## Data tables (per-branch, per-table) Both user tables and the two system tables (`__penca_system__.schemas`, From bb60e6e2c6be1216b3c4c6ebb1cb1a69157fd201 Mon Sep 17 00:00:00 2001 From: Nico Bautista Hobin Date: Fri, 31 Jul 2026 02:15:59 +0000 Subject: [PATCH 05/32] test(cold): derive distinct content_hash values for cache fixtures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every pre-existing penca-dl fixture was seeded `Uuid::nil()`, so all distinct fixtures in a test shared one future cache key. Once SegmentCache keys on content_hash that turns `evicted_segment_re_reads_from_storage` into a hit on the wrong entry, and makes the provider's identity-vs-user sidecar pair silently serve one sidecar's batch for the other — a wrong-result path a passing test would mask. Fixture helpers and every explicitly-seeded literal now derive the hash from their own identity field via `naming::deterministic_uuid_from`, so distinct fixtures stay distinct; only the tests deliberately asserting dedup share a value. Left as-is: five single-segment persist fixtures whose test builds its own SegmentCache, where there is no second entry to alias with. The three CHA-545 dedup tests remain red for their own assertion (2 reads != 1); the other 82 penca-dl tests pass. CHA-545 --- crates/penca-dl/src/driver.rs | 22 ++++++++++++++++------ crates/penca-dl/src/provider.rs | 25 +++++++++++++------------ 2 files changed, 29 insertions(+), 18 deletions(-) diff --git a/crates/penca-dl/src/driver.rs b/crates/penca-dl/src/driver.rs index c6b4eaa..8f3154d 100644 --- a/crates/penca-dl/src/driver.rs +++ b/crates/penca-dl/src/driver.rs @@ -974,11 +974,15 @@ mod tests { .unwrap() } + /// `content_hash` is derived from the name so distinct fixtures stay + /// distinct under content-hash cache keying; tests asserting dedup + /// overwrite it with a deliberately shared value. fn segment(uuid: &str, size_bytes: i64) -> SnapshotSegment { SnapshotSegment { table_snapshot_segment_uuid: uuid.to_string(), format: Format::Parquet, size_bytes, + content_hash: penca_core::naming::deterministic_uuid_from(&[uuid]), ..Default::default() } } @@ -1088,7 +1092,9 @@ mod tests { format: Format::Parquet, segment_index_uuid: format!("idx-{name}"), size_bytes: 256, - content_hash: Uuid::nil(), + content_hash: penca_core::naming::deterministic_uuid_from(&[&format!( + "idx-{name}" + )]), }), ..Default::default() } @@ -1252,7 +1258,9 @@ mod tests { format: Format::Parquet, segment_index_uuid: format!("idx-{name}"), size_bytes: 256, - content_hash: Uuid::nil(), + content_hash: penca_core::naming::deterministic_uuid_from(&[&format!( + "idx-{name}" + )]), }), ..Default::default() } @@ -1523,7 +1531,7 @@ mod tests { format: Format::Parquet, segment_index_uuid: "idx-identity-never-read".to_string(), size_bytes: 256, - content_hash: Uuid::nil(), + content_hash: penca_core::naming::deterministic_uuid_from(&["idx-identity-never-read"]), }); let dl = routing_driver(cache, by_uri); @@ -1553,7 +1561,7 @@ mod tests { format: Format::Parquet, segment_index_uuid: "idx-identity".to_string(), size_bytes: 256, - content_hash: Uuid::nil(), + content_hash: penca_core::naming::deterministic_uuid_from(&["idx-identity"]), }); let name_index = Uuid::new_v4(); let res = dl @@ -1595,7 +1603,7 @@ mod tests { format: Format::Parquet, segment_index_uuid: "idx-other".to_string(), size_bytes: 256, - content_hash: Uuid::nil(), + content_hash: penca_core::naming::deterministic_uuid_from(&["idx-other"]), }, )]; let requested = Uuid::new_v4(); // != the keyed index present @@ -1640,7 +1648,9 @@ mod tests { format: Format::Parquet, segment_index_uuid: "idx-keyed-never-read".to_string(), size_bytes: 256, - content_hash: Uuid::nil(), + content_hash: penca_core::naming::deterministic_uuid_from(&[ + "idx-keyed-never-read", + ]), }, )]; let dl = routing_driver(cache, by_uri); diff --git a/crates/penca-dl/src/provider.rs b/crates/penca-dl/src/provider.rs index 27cbb23..42a1a21 100644 --- a/crates/penca-dl/src/provider.rs +++ b/crates/penca-dl/src/provider.rs @@ -674,7 +674,6 @@ mod tests { use datafusion::datasource::MemTable; use penca_core::{Format, PersistPlan}; use penca_format::reader::{AnyFormatReader, FormatError}; - use uuid::Uuid; fn user_schema() -> SchemaRef { Arc::new(Schema::new(vec![ @@ -816,7 +815,7 @@ mod tests { offset: None, length: None, max_commit_seq_num: None, - content_hash: Uuid::nil(), + content_hash: penca_core::naming::deterministic_uuid_from(&[uuid]), } } @@ -1030,6 +1029,7 @@ mod tests { table_snapshot_segment_uuid: uuid.to_string(), format: Format::Parquet, size_bytes, + content_hash: penca_core::naming::deterministic_uuid_from(&[uuid]), ..Default::default() } } @@ -1325,7 +1325,7 @@ mod tests { format: Format::Parquet, segment_index_uuid: "sidecar1".to_string(), size_bytes: 256, - content_hash: Uuid::nil(), + content_hash: penca_core::naming::deterministic_uuid_from(&["sidecar1"]), }), ..Default::default() }; @@ -1399,7 +1399,7 @@ mod tests { format: Format::Parquet, segment_index_uuid: "sidecar1".to_string(), size_bytes: 256, - content_hash: Uuid::nil(), + content_hash: penca_core::naming::deterministic_uuid_from(&["sidecar1"]), }), ..Default::default() }; @@ -1487,7 +1487,7 @@ mod tests { format: Format::Parquet, segment_index_uuid: "sc".to_string(), size_bytes: 64, - content_hash: Uuid::nil(), + content_hash: penca_core::naming::deterministic_uuid_from(&["sc"]), }); let provider = SnapshotTableProvider::new( vec![seg], @@ -1556,7 +1556,7 @@ mod tests { format: Format::Parquet, segment_index_uuid: "sidecar-identity".to_string(), size_bytes: 256, - content_hash: Uuid::nil(), + content_hash: penca_core::naming::deterministic_uuid_from(&["sidecar-identity"]), }), index_sidecars: vec![( user_index_uuid.to_string(), @@ -1567,7 +1567,7 @@ mod tests { format: Format::Parquet, segment_index_uuid: "sidecar-user".to_string(), size_bytes: 256, - content_hash: Uuid::nil(), + content_hash: penca_core::naming::deterministic_uuid_from(&["sidecar-user"]), }, )], ..Default::default() @@ -1649,7 +1649,7 @@ mod tests { format: Format::Parquet, segment_index_uuid: "sidecar-identity".to_string(), size_bytes: 256, - content_hash: Uuid::nil(), + content_hash: penca_core::naming::deterministic_uuid_from(&["sidecar-identity"]), }), // No keyed sidecars: the user entry below cannot resolve. ..Default::default() @@ -1701,7 +1701,7 @@ mod tests { format: Format::Parquet, segment_index_uuid: "id-sc".to_string(), size_bytes: 1, - content_hash: Uuid::nil(), + content_hash: penca_core::naming::deterministic_uuid_from(&["id-sc"]), }), index_sidecars: vec![( user.to_string(), @@ -1712,7 +1712,7 @@ mod tests { format: Format::Parquet, segment_index_uuid: "user-sc".to_string(), size_bytes: 1, - content_hash: Uuid::nil(), + content_hash: penca_core::naming::deterministic_uuid_from(&["user-sc"]), }, )], ..Default::default() @@ -1787,7 +1787,7 @@ mod tests { format: Format::Parquet, segment_index_uuid: "sidecar-value".to_string(), size_bytes: 256, - content_hash: Uuid::nil(), + content_hash: penca_core::naming::deterministic_uuid_from(&["sidecar-value"]), }, )], ..Default::default() @@ -1899,7 +1899,7 @@ mod tests { format: Format::Parquet, segment_index_uuid: "sidecar-x".to_string(), size_bytes: 128, - content_hash: Uuid::nil(), + content_hash: penca_core::naming::deterministic_uuid_from(&["sidecar-x"]), }, )], ..Default::default() @@ -1977,6 +1977,7 @@ mod by_plan_order_tests { uri: uri.to_string(), format: Format::Parquet, size_bytes: 64, + content_hash: penca_core::naming::deterministic_uuid_from(&[uuid]), ..Default::default() } } From 0bd32001b1a50197d02c04d67b9f70982afa9320 Mon Sep 17 00:00:00 2001 From: Nico Bautista Hobin Date: Fri, 31 Jul 2026 02:20:19 +0000 Subject: [PATCH 06/32] fix(cold): drop the no-op digest normalization, pin dictionary handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `concat_batches` was credited with making the digest representation-independent for nested columns. It cannot: `concat` short-circuits single-array input to `slice(0, len)`, rebuilding nothing. Measured on arrow-rs 57.3 across Int32/Int64/Utf8/List — with and without the call — slice-invariance holds either way and rests solely on the IPC writer encoding the logical range. Dictionary columns are the one measured exception, and `concat_batches` does not fix them either; Penca rejects `DataType::Dictionary` at the type boundary, so none reaches this function. Drops the call, adds a List slice test (the nested case the old comment gestured at but never covered) guarded on the child array staying whole, and pins `DictionaryHandling::Resend` so the claim that nothing here relies on a library default is actually true. CHA-545 --- crates/penca-core/src/digest.rs | 92 ++++++++++++++++++++++++--------- 1 file changed, 68 insertions(+), 24 deletions(-) diff --git a/crates/penca-core/src/digest.rs b/crates/penca-core/src/digest.rs index 3a1fac5..ebda2b8 100644 --- a/crates/penca-core/src/digest.rs +++ b/crates/penca-core/src/digest.rs @@ -3,7 +3,7 @@ use arrow::array::RecordBatch; use arrow::error::ArrowError; use arrow::ipc::MetadataVersion; -use arrow::ipc::writer::{IpcWriteOptions, StreamWriter}; +use arrow::ipc::writer::{DictionaryHandling, IpcWriteOptions, StreamWriter}; use uuid::Uuid; use xxhash_rust::xxh3::xxh3_128; @@ -27,25 +27,32 @@ const IPC_ALIGNMENT: usize = 8; /// reuse as a possible future use of this digest; that use requires /// establishing round-trip stability first, and this function does not. /// -/// Changing the normalization, the IPC options, or the hash changes every -/// future digest. That is safe — stored digests are opaque identity, never -/// recomputed and compared against a stored value — but it must stay -/// deliberate, which is why nothing here relies on a library default. +/// **Slice-invariant for every column type Penca supports.** A batch from +/// `slice()` stays a view onto its parent's buffers, which physically hold +/// bytes outside the slice, but the IPC writer encodes only the logical range — +/// measured on arrow-rs 57.3 for Int32/Int64/Utf8/List, which with LargeList +/// and FixedSizeList are the whole supported set (everything else is +/// `UnsupportedType`, see [`crate::types`]). The one measured exception is +/// dictionary-encoded columns, where `DictionaryHandling::Resend` emits the +/// full dictionary regardless of which rows the slice covers; Penca rejects +/// `DataType::Dictionary` at the type boundary, so no such column reaches here. +/// Were one to, the cost is a *missed dedup* — two cache entries for one +/// logical content — never a wrong result, because the digest is computed once +/// and inherited, never recomputed and compared. +/// +/// Changing the IPC options or the hash changes every future digest. That is +/// safe — stored digests are opaque identity, never recomputed and compared +/// against a stored value — but it must stay deliberate, which is why nothing +/// here relies on a library default. pub fn segment_content_hash(batch: &RecordBatch) -> Result { - // A batch from `slice()` stays a view onto its parent's buffers, so those - // buffers physically hold bytes outside the slice. Measured on arrow-rs - // 57.3: the IPC writer already encodes only the logical range, so for - // primitive and Utf8 columns this concat is redundant (removing it leaves - // `slice_hashes_equal_to_an_independently_built_batch` green). It stays - // because that redundancy is an arrow-rs implementation detail and this - // digest must be representation-independent for every column type, - // including the nested ones no test here covers. - let compacted = arrow::compute::concat_batches(&batch.schema(), std::slice::from_ref(batch))?; - - let options = IpcWriteOptions::try_new(IPC_ALIGNMENT, false, MetadataVersion::V5)?; + let options = IpcWriteOptions::try_new(IPC_ALIGNMENT, false, MetadataVersion::V5)? + // Pinned for the same reason as the alignment: the default is `Resend` + // today, and a future flip to `Delta` would make a batch's digest + // depend on what the writer had already emitted. + .with_dictionary_handling(DictionaryHandling::Resend); let mut bytes = Vec::new(); - let mut writer = StreamWriter::try_new_with_options(&mut bytes, &compacted.schema(), options)?; - writer.write(&compacted)?; + let mut writer = StreamWriter::try_new_with_options(&mut bytes, &batch.schema(), options)?; + writer.write(batch)?; writer.finish()?; drop(writer); @@ -56,8 +63,8 @@ pub fn segment_content_hash(batch: &RecordBatch) -> Result { mod tests { use std::sync::Arc; - use arrow::array::{Int64Array, StringArray}; - use arrow::datatypes::{DataType, Field, Schema}; + use arrow::array::{Int64Array, ListArray, StringArray}; + use arrow::datatypes::{DataType, Field, Int64Type, Schema}; use super::*; @@ -187,10 +194,6 @@ mod tests { /// buffer physically holds bytes outside the slice. (`RecordBatch::slice` /// reports `offset() == 0` even here, so asserting on the offset would /// silently pass on a fixture that proves nothing.) - /// - /// This currently passes with or without the `concat_batches` normalization - /// — arrow-rs's IPC writer compacts on its own. It is a guard on the - /// property, not proof that the normalization is reachable. #[test] fn slice_hashes_equal_to_an_independently_built_batch() { let parent = batch( @@ -227,4 +230,45 @@ mod tests { "sliced primitive rows hash as their own content" ); } + + /// The nested twin, and the reason `segment_content_hash` needs no + /// compaction step. Slicing a `List` rebases the offsets buffer but leaves + /// the *child* array whole — the guard below pins that it still holds all + /// four rows' values, shared with the parent — and the IPC writer still + /// encodes only the logical range. + /// + /// This is the case an earlier revision assumed a `concat_batches` call was + /// protecting. It was not: `concat` short-circuits single-array input to + /// `slice(0, len)`, which rebuilds nothing. + #[test] + fn list_slice_hashes_equal_to_an_independently_built_batch() { + fn lists(rows: Vec>) -> RecordBatch { + let arr = ListArray::from_iter_primitive::( + rows.into_iter() + .map(|r| Some(r.into_iter().map(Some).collect::>())), + ); + let field = Field::new( + "l", + DataType::List(Arc::new(Field::new("item", DataType::Int64, true))), + true, + ); + batch(vec![field], vec![Arc::new(arr)]) + } + + let parent = lists(vec![vec![1], vec![2, 2], vec![3, 3, 3], vec![4]]); + let sliced = parent.slice(1, 2); + + let sliced_data = sliced.column(0).to_data(); + let parent_data = parent.column(0).to_data(); + assert_eq!( + sliced_data.child_data()[0].len(), + parent_data.child_data()[0].len(), + "fixture must keep the parent's whole child array, or this proves nothing" + ); + assert_eq!( + hash(&sliced), + hash(&lists(vec![vec![2, 2], vec![3, 3, 3]])), + "a sliced list column hashes as its own logical rows" + ); + } } From 55102b68b07974d506ab153f7c38f0d6755fd3ec Mon Sep 17 00:00:00 2001 From: Nico Bautista Hobin Date: Fri, 31 Jul 2026 02:25:38 +0000 Subject: [PATCH 07/32] test(cold): give multi-segment dl fixtures distinct content hashes `indexed_segment` and `composite_indexed_segment` set an explicit `table_snapshot_segment_uuid` but left `content_hash` at the nil Default. Two tests feed two of those base segments to one shared `SegmentCache`, so once the cache keys on the hash both would collide onto one entry and serve segment A's batch for segment B. CHA-545 --- crates/penca-dl/src/driver.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/crates/penca-dl/src/driver.rs b/crates/penca-dl/src/driver.rs index 8f3154d..7d8c704 100644 --- a/crates/penca-dl/src/driver.rs +++ b/crates/penca-dl/src/driver.rs @@ -1080,6 +1080,7 @@ mod tests { by_uri.insert(side_uri.clone(), sidecar_batch); SnapshotSegment { table_snapshot_segment_uuid: format!("seg-{name}"), + content_hash: penca_core::naming::deterministic_uuid_from(&[&format!("seg-{name}")]), uri: base_uri, format: Format::Parquet, length: keys.len() as i64, @@ -1246,6 +1247,7 @@ mod tests { by_uri.insert(side_uri.clone(), sidecar_batch); SnapshotSegment { table_snapshot_segment_uuid: format!("seg-{name}"), + content_hash: penca_core::naming::deterministic_uuid_from(&[&format!("seg-{name}")]), uri: base_uri, format: Format::Parquet, length: key0s.len() as i64, From 48e6eaaa9434854a587dcca5f4fe82e7bb9037de Mon Sep 17 00:00:00 2001 From: Nico Bautista Hobin Date: Fri, 31 Jul 2026 02:29:32 +0000 Subject: [PATCH 08/32] docs(cold): correct the supported set in segment_content_hash's contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The slice-invariance note enumerated the supported set as Int32/Int64/Utf8 plus the three list types, and named dictionaries the only exception. Both were wrong: `CanonicalType::from_arrow` accepts every scalar, decimal, and temporal type plus `Utf8View`/`BinaryView`, and it is the *view* encodings — not dictionaries, which are rejected at the type boundary — that are the reachable exception. Measured across the supported set on arrow-rs 57.3, including `Boolean` at a non-byte-aligned offset. The writer truncates a view array's views buffer but emits each variadic data buffer whole, so a sliced view column above the 12-byte inline threshold carries bytes for rows it does not cover, top-level or as a list child. Pinned by a canary test so the contract cannot drift again. Cost is a missed dedup between independently written segments, never a wrong result and never lost fork sharing — a reference copy inherits the stored hash rather than recomputing it. CHA-545 --- crates/penca-core/src/digest.rs | 67 ++++++++++++++++++++++++++------- 1 file changed, 54 insertions(+), 13 deletions(-) diff --git a/crates/penca-core/src/digest.rs b/crates/penca-core/src/digest.rs index ebda2b8..2ce5268 100644 --- a/crates/penca-core/src/digest.rs +++ b/crates/penca-core/src/digest.rs @@ -27,18 +27,25 @@ const IPC_ALIGNMENT: usize = 8; /// reuse as a possible future use of this digest; that use requires /// establishing round-trip stability first, and this function does not. /// -/// **Slice-invariant for every column type Penca supports.** A batch from -/// `slice()` stays a view onto its parent's buffers, which physically hold -/// bytes outside the slice, but the IPC writer encodes only the logical range — -/// measured on arrow-rs 57.3 for Int32/Int64/Utf8/List, which with LargeList -/// and FixedSizeList are the whole supported set (everything else is -/// `UnsupportedType`, see [`crate::types`]). The one measured exception is -/// dictionary-encoded columns, where `DictionaryHandling::Resend` emits the -/// full dictionary regardless of which rows the slice covers; Penca rejects -/// `DataType::Dictionary` at the type boundary, so no such column reaches here. -/// Were one to, the cost is a *missed dedup* — two cache entries for one -/// logical content — never a wrong result, because the digest is computed once -/// and inherited, never recomputed and compared. +/// **Slice-invariant except for view-encoded columns.** A batch from `slice()` +/// stays a view onto its parent's buffers, which physically hold bytes outside +/// the slice, but the IPC writer encodes only the logical range. Measured on +/// arrow-rs 57.3 across the supported set (see [`crate::types`]): primitives, +/// decimals, dates/times/timestamps, `Boolean` at a non-byte-aligned offset, +/// `Utf8`/`LargeUtf8`/`Binary`, and `List`/`LargeList`/`FixedSizeList` all hold. +/// +/// `Utf8View`/`BinaryView` do not, top-level or as a list child: the writer +/// truncates the *views* buffer but emits each variadic data buffer whole, so a +/// slice carries bytes belonging to rows it does not cover. Only above the +/// 12-byte inline threshold — shorter values live in the view itself and are +/// invariant. `Dictionary` fails the same way under `DictionaryHandling::Resend` +/// but is rejected at the type boundary and cannot reach here. +/// +/// Neither costs correctness, and neither costs the dedup this digest exists +/// for: a reference copy *inherits* the stored hash rather than recomputing it, +/// so fork and copy sharing is unaffected. What is lost is dedup between two +/// segments written independently whose identical rows happened to be sliced out +/// of differently-shaped parent batches. /// /// Changing the IPC options or the hash changes every future digest. That is /// safe — stored digests are opaque identity, never recomputed and compared @@ -63,7 +70,7 @@ pub fn segment_content_hash(batch: &RecordBatch) -> Result { mod tests { use std::sync::Arc; - use arrow::array::{Int64Array, ListArray, StringArray}; + use arrow::array::{Int64Array, ListArray, StringArray, StringViewArray}; use arrow::datatypes::{DataType, Field, Int64Type, Schema}; use super::*; @@ -271,4 +278,38 @@ mod tests { "a sliced list column hashes as its own logical rows" ); } + + /// The one exception in the contract above, pinned so the contract cannot go + /// stale: arrow-rs 57.3 truncates a view array's *views* buffer on slice but + /// emits each variadic data buffer whole. + /// + /// A failure here means arrow started compacting those buffers — delete this + /// test and the exception paragraph it guards. + #[test] + fn sliced_view_column_is_the_documented_non_invariant_case() { + let view = |v: Vec<&str>| { + batch( + vec![Field::new("s", DataType::Utf8View, true)], + vec![Arc::new(StringViewArray::from(v))], + ) + }; + // Above the 12-byte inline threshold, so the values live in a data + // buffer rather than in the view word itself. + let long = [ + "aaaaaaaaaaaaaaaaaaaa", + "bbbbbbbbbbbbbbbbbbbb", + "cccccccccccccccccccc", + ]; + + assert_ne!( + hash(&view(long.to_vec()).slice(1, 2)), + hash(&view(vec![long[1], long[2]])), + "arrow now truncates variadic data buffers — update the contract above" + ); + assert_eq!( + hash(&view(vec!["aa", "bb", "cc"]).slice(1, 2)), + hash(&view(vec!["bb", "cc"])), + "inline-length views carry no data buffer, so they stay invariant" + ); + } } From 1343c25b361be7e90819a20c4f656162466e8a66 Mon Sep 17 00:00:00 2001 From: Nico Bautista Hobin Date: Fri, 31 Jul 2026 02:31:23 +0000 Subject: [PATCH 09/32] test(cold): restore the cross-branch schema-divergence regression guard Removed in 86dee41 along with the slice-keying design it was written against. The scenario it pins is independent of how the cache is keyed: any keying that lets a fork and its parent share one entry can hand a caller-shaped decode to a branch expecting a different type for the column it ALTERed independently. Content-hash keying reaches it the same way slice keying did, so the guard comes back with the docstring re-grounded on CHA-545's mechanism. Both load-bearing properties preserved: the child-then-parent read order (the child's decode is the miss that populates the cache; only the parent's assertion is the guard) and the precondition that RAISES across both cold tiers rather than asserting, so a fixture that stopped producing a shared slice fails loudly instead of silently turning the guard into a no-op. CHA-545 --- ...integration_branch_persist_at_fork_test.py | 124 ++++++++++++++++++ 1 file changed, 124 insertions(+) diff --git a/tests/integration/integration_branch_persist_at_fork_test.py b/tests/integration/integration_branch_persist_at_fork_test.py index 3f58524..a5c23c8 100644 --- a/tests/integration/integration_branch_persist_at_fork_test.py +++ b/tests/integration/integration_branch_persist_at_fork_test.py @@ -1106,3 +1106,127 @@ def test_fork_audit_does_not_double_match_a_straddling_parent_tx_log(): f"{post_fork!r} was committed on main above the fork and must not " f"appear in the child's audit. Full list: {names}" ) + + +def test_fork_and_parent_diverge_a_columns_type_over_one_shared_slice(): + """Two branches whose schemas diverged must not share a cached decode. + + CHA-545 keys the segment cache on `content_hash`, so a fork's copied row and + the parent's row over the same bytes share one entry — that is the point, + since a fork's cold footprint is mostly the parent's files and the copy + inherits the parent's hash rather than minting its own. + + But the cached value is not the file's native decode: `read_segment` + null-fills to the CALLER's schema, so an entry carries the schema of + whichever branch decoded it first. Since CHA-539 the fork names the parent's + `(uri, offset, length)` while ALTERing independently, so that entry can be + handed to a branch expecting a different type for the same column: whichever + branch missed first caches `extra` as an all-null array of its own type and + the other branch's projection fails on the mismatch — a query error driven by + cache state, on a branch whose own metadata is perfectly consistent. What + keeps that from happening is caching the file's NATIVE decode and null-filling + to the caller's schema after the lookup, not before it. + + Reads child-then-parent, and that one order is sufficient AND deterministic: + within a process the child misses, caches `extra` as Utf8, and the parent's + read is the hit that used to fail with "expected Int64 but found Utf8". Only + the PARENT assertion is the regression guard — the child's read is always the + miss, so its assertion holds with or without the fix and is a sanity check. + The reverse direction (parent poisons child) is not covered here; it needs a + second fixture on a fresh catalog, since after this pair the file is already + cached under its content hash. + """ + client = make_client() + schema_uuid, table_uuid, catalog_uuid, main_branch = setup_schema(client) + scope = { + "catalog_uuid": catalog_uuid, + "schema_uuid": schema_uuid, + "table_uuid": table_uuid, + } + + fork_seq = _write_committed_rows( + client, + branch_uuid=main_branch, + rows={"name": ["shared"], "value": [1]}, + **scope, + ) + client.persist(branch_uuid=main_branch, **scope) + client.snapshot(branch_uuid=main_branch, **scope) + + child = client.create_branch( + "kid", "t", "fork", commit_seq_num=fork_seq, catalog_uuid=catalog_uuid + ).branch_uuid + + # Same new column, incompatible types, one on each branch — over the single + # base slice the fork inherited by reference. + parent_schema = pa.schema( + [*list(USER_SCHEMA), pa.field("extra", pa.int64(), nullable=True)] + ) + child_schema = pa.schema( + [*list(USER_SCHEMA), pa.field("extra", pa.string(), nullable=True)] + ) + client.update_table( + parent_schema, + branch_uuid=main_branch, + primary_keys=["name"], + author="test", + comment="parent adds extra BIGINT", + **scope, + ) + client.update_table( + child_schema, + branch_uuid=child, + primary_keys=["name"], + author="test", + comment="fork adds extra TEXT", + **scope, + ) + + # The precondition the scenario rests on: the fork's cold row and the + # parent's must name the SAME slice, or the two branches decode + # independently, every assertion below still passes, and this test covers + # nothing. Metadata-only, so it does not warm the cache it is about to probe. + # + # BOTH tiers, because which one carries the shared slice depends on the + # fixture: here the snapshot covers the whole persist, so `copy_inherited_ + # persist` finds nothing above the baseline watermark and the inheritance is + # entirely snapshot segments. Checking only the persist table looked correct + # and reported "no shared slice" on a fixture that has one. + shared = 0 + for base in (TABLE_PERSIST_SEGMENT_METADATA, TABLE_SNAPSHOT_SEGMENT_METADATA): + rows = get_pg_driver().execute( + SQL( + "SELECT count(*) FROM {tbl} p JOIN {tbl} c" + ' ON p.object_uri = c.object_uri AND p."offset" IS NOT DISTINCT FROM c."offset"' + " WHERE p.branch_uuid = %s AND c.branch_uuid = %s AND p.table_uuid = %s" + ).format(tbl=Identifier(f"{catalog_uuid}_{base}")), + (main_branch, child, table_uuid), + ) + shared += rows[0][0] + + if shared == 0: + raise RuntimeError( + "setup failed: the fork holds no cold row naming the parent's slice " + "in either tier, so both branches would decode independently and the " + "cross-branch cache sharing this test pins is unreachable" + ) + + # Child first, then parent: the child's decode populates the cache and the + # parent must not be served it. + child_rows = client.read_data(branch_uuid=child, **scope) + parent_rows = client.read_data(branch_uuid=main_branch, **scope) + + assert child_rows.column("name").to_pylist() == ["shared"], ( + "the fork lost its inherited row" + ) + assert parent_rows.column("name").to_pylist() == ["shared"], ( + "the parent lost its own row" + ) + assert pa.types.is_string(child_rows.schema.field("extra").type), ( + "the fork's read must honour ITS schema for the diverged column, not the " + f"branch that decoded the shared slice first: {child_rows.schema}" + ) + assert pa.types.is_int64(parent_rows.schema.field("extra").type), ( + "the parent's read must honour ITS schema for the diverged column, not " + f"the branch that decoded the shared slice first: {parent_rows.schema}" + ) From 8b1482cf45fdebe210d857bd4e1fff74bb3d7dd1 Mon Sep 17 00:00:00 2001 From: Nico Bautista Hobin Date: Fri, 31 Jul 2026 02:41:25 +0000 Subject: [PATCH 10/32] feat(cold): compute segment_content_hash at every fresh cold write `content_hash` was added as NOT NULL with no writer, so every cold write failed on the not-null constraint. Populates it at the four live insert sites plus the currently-callerless snapshot `write_segment` arm, each hashing the same in-memory batch the sibling `compute_segment_statistics` call already consumes: - persist segments, in `PersistSegmentScope::insert_segment` - packed snapshot segments, per partition slice in the packer, so the hash is partition-tight like `size_bytes`/`statistics` rather than covering the whole packed file - index sidecars, over the sidecar batch rather than its base segment: two segments with different rows can build identical indexes, and it is the index batch the cache entry holds - the empty-merge placeholder, where every zero-row batch over one schema legitimately collapses onto a single entry Compact re-points recompute per slice instead of carrying the input row's hash. Rows survive a merge 1:1, but `concat_batches` normalizes every input to one `segment_schema`, so a slice of the merged batch can decode to a wider type than its input file did; carrying the old value would leave two different decodes sharing one cache key. CHA-545 --- crates/penca-api/src/lifecycle/compact.rs | 10 ++++++++++ .../penca-api/src/lifecycle/durable_writer.rs | 20 ++++++++++++++++--- crates/penca-api/src/lifecycle/packer.rs | 11 +++++++--- crates/penca-api/src/lifecycle/snapshot_op.rs | 17 ++++++++++++---- crates/penca-storage-meta/src/persist.rs | 18 +++++++++++------ .../penca-storage-meta/src/segment_index.rs | 10 +++++++--- crates/penca-storage-meta/src/snapshot.rs | 10 +++++++--- 7 files changed, 74 insertions(+), 22 deletions(-) diff --git a/crates/penca-api/src/lifecycle/compact.rs b/crates/penca-api/src/lifecycle/compact.rs index 61813b1..12263a4 100644 --- a/crates/penca-api/src/lifecycle/compact.rs +++ b/crates/penca-api/src/lifecycle/compact.rs @@ -321,6 +321,15 @@ where } else { 0 }; + // Recomputed per slice rather than carried over from the input row. + // The rows are preserved 1:1, but `concat_batches` normalizes every + // input to one `segment_schema`, so a slice of the merged batch can + // decode to a wider type than its input file did. Carrying the old hash + // would leave two different decodes under one cache key. + let slice_hash = penca_core::digest::segment_content_hash( + &merged.slice(cumulative as usize, meta.row_count as usize), + ) + .map_err(ApiError::Arrow)?; LifecycleManager::repoint_table_persist_segment( &tx, &catalog_str, @@ -332,6 +341,7 @@ where proportional_size, storage_format_text, &merged_stats, + &slice_hash, false, ) .await?; diff --git a/crates/penca-api/src/lifecycle/durable_writer.rs b/crates/penca-api/src/lifecycle/durable_writer.rs index be8db2c..ee8bc5a 100644 --- a/crates/penca-api/src/lifecycle/durable_writer.rs +++ b/crates/penca-api/src/lifecycle/durable_writer.rs @@ -24,6 +24,7 @@ use penca_db::driver::pg::PgDriver; use penca_format::writer::FormatWriter; use penca_storage_cold::ColdStorageClient; use penca_storage_meta::LifecycleManager; +use uuid::Uuid; use crate::error::ApiError; @@ -219,6 +220,12 @@ impl<'a> SegmentScope for PersistSegmentScope<'a> { async fn insert_segment(&self, pool: &PgDriver, step: &Self::Step) -> Result<(), ApiError> { let statistics = penca_dl::stats::compute_segment_statistics(&step.batch); + // Hashed here, on the in-memory batch, rather than after `write_file`: + // the digest identifies the logical content handed to the format writer, + // which is what the segment cache stores post-decode. It is deliberately + // not a checksum of the object on S3 — see `segment_content_hash`. + let content_hash = + penca_core::digest::segment_content_hash(&step.batch).map_err(ApiError::Arrow)?; LifecycleManager::insert_table_persist_segment( pool, self.catalog_str, @@ -235,6 +242,7 @@ impl<'a> SegmentScope for PersistSegmentScope<'a> { step.num_rows, self.storage_format.extension(), &statistics, + &content_hash, ) .await?; Ok(()) @@ -324,6 +332,8 @@ impl<'a> SegmentScope for SnapshotSegmentScope<'a> { async fn insert_segment(&self, pool: &PgDriver, step: &Self::Step) -> Result<(), ApiError> { let statistics = penca_dl::stats::compute_segment_statistics(&step.batch); + let content_hash = + penca_core::digest::segment_content_hash(&step.batch).map_err(ApiError::Arrow)?; LifecycleManager::insert_snapshot_segment( pool, self.catalog_str, @@ -341,6 +351,7 @@ impl<'a> SegmentScope for SnapshotSegmentScope<'a> { step.num_rows, self.storage_format.extension(), &statistics, + &content_hash, ) .await?; Ok(()) @@ -409,9 +420,10 @@ pub(super) struct SnapshotFileStep { /// One per-partition segment metadata row inside a packed file. /// `offset`/`length` are the partition's row range within the file — /// `length` doubles as the catalog `row_count` (a packed row IS its -/// row range; both columns are NOT NULL). `size_bytes` -/// and `statistics` are computed over the slice only, so pruning -/// stats stay partition-tight. +/// row range; both columns are NOT NULL). `size_bytes`, +/// `statistics` and `content_hash` are computed over the slice only, so +/// pruning stats stay partition-tight and the hash identifies the rows +/// this one row covers rather than the whole packed file. pub(super) struct SnapshotSegmentRowSpec { pub seg_uuid_str: String, pub chunk_idx: u32, @@ -425,6 +437,7 @@ pub(super) struct SnapshotSegmentRowSpec { pub length: i64, pub size_bytes: i64, pub statistics: Vec, + pub content_hash: Uuid, } impl<'a> DurableSegmentWriter> { @@ -456,6 +469,7 @@ impl<'a> DurableSegmentWriter> { row.length, self.scope.storage_format.extension(), &row.statistics, + &row.content_hash, ) .await?; self.current_group() diff --git a/crates/penca-api/src/lifecycle/packer.rs b/crates/penca-api/src/lifecycle/packer.rs index 25d9679..90babb5 100644 --- a/crates/penca-api/src/lifecycle/packer.rs +++ b/crates/penca-api/src/lifecycle/packer.rs @@ -268,9 +268,10 @@ impl SegmentPacker { } /// Concatenate the buffered partitions into one file step with one - /// segment row per partition. `size_bytes` and `statistics` are - /// computed per partition slice, not per file, so pruning stats - /// stay partition-tight. + /// segment row per partition. `size_bytes`, `statistics` and + /// `content_hash` are computed per partition slice, not per file, so + /// pruning stats stay partition-tight and each row's hash identifies + /// its own rows rather than the packed file they share. fn flush(&mut self) -> Result, ApiError> { if self.buffered.is_empty() { return Ok(None); @@ -297,6 +298,8 @@ impl SegmentPacker { length: num_rows, size_bytes: batch_in_memory_bytes(batch)?, statistics: penca_dl::stats::compute_segment_statistics(batch), + content_hash: penca_core::digest::segment_content_hash(batch) + .map_err(ApiError::Arrow)?, }); self.chunk_idx += 1; offset += num_rows; @@ -327,6 +330,8 @@ impl SegmentPacker { length: num_rows, size_bytes: chunk_bytes, statistics: penca_dl::stats::compute_segment_statistics(&chunk), + content_hash: penca_core::digest::segment_content_hash(&chunk) + .map_err(ApiError::Arrow)?, }; self.chunk_idx += 1; Ok(SnapshotFileStep { diff --git a/crates/penca-api/src/lifecycle/snapshot_op.rs b/crates/penca-api/src/lifecycle/snapshot_op.rs index 1945247..4b7a480 100644 --- a/crates/penca-api/src/lifecycle/snapshot_op.rs +++ b/crates/penca-api/src/lifecycle/snapshot_op.rs @@ -82,7 +82,7 @@ fn empty_merge_placeholder_step( base_uri: &str, storage_format_text: &str, user_schema: &SchemaRef, -) -> SnapshotFileStep { +) -> Result { let placeholder_batch = RecordBatch::new_empty(penca_merge::snapshot_read_schema(user_schema)); let seg_uuid = table_snapshot_segment_uuid(snap_uuid, 0); let uri = snapshot_segment_uri( @@ -94,7 +94,11 @@ fn empty_merge_placeholder_step( storage_format_text, ); let statistics = penca_dl::stats::compute_segment_statistics(&placeholder_batch); - SnapshotFileStep { + // Every empty placeholder over one schema is the same zero-row content, so + // they legitimately collapse onto one cache entry. + let content_hash = + penca_core::digest::segment_content_hash(&placeholder_batch).map_err(ApiError::Arrow)?; + Ok(SnapshotFileStep { snap_uuid_str: snap_str.to_string(), uri, file_batch: placeholder_batch, @@ -106,8 +110,9 @@ fn empty_merge_placeholder_step( length: 0, size_bytes: 0, statistics, + content_hash, }], - } + }) } impl LifecycleManager { @@ -1539,7 +1544,7 @@ impl LifecycleManager { &self.base_uri, ctx.storage_format_text, ctx.user_schema, - ); + )?; seg_writer .write_segment_group(pool, writer, &placeholder) .await?; @@ -1912,6 +1917,10 @@ async fn build_one_segment_sidecar( // sorted the keys, so the bounds are the first/last entry). Deferred so // the seek owns the exact bound encoding rather than guessing it here. &[], + // The sidecar's own content, not the base segment's: two segments with + // different rows can build byte-identical indexes, and it is the index + // batch that this row's cache entry holds. + &penca_core::digest::segment_content_hash(&sidecar).map_err(ApiError::Arrow)?, ) .await?; built_uris.push(uri); diff --git a/crates/penca-storage-meta/src/persist.rs b/crates/penca-storage-meta/src/persist.rs index 9bd6fea..c0d40ac 100644 --- a/crates/penca-storage-meta/src/persist.rs +++ b/crates/penca-storage-meta/src/persist.rs @@ -332,6 +332,7 @@ impl LifecycleManager { row_count: i64, format_text: &str, statistics: &[u8], + content_hash: &Uuid, ) -> Result<()> { let catalog = parse_uuid(catalog_uuid); let branch = parse_uuid(branch_uuid); @@ -344,13 +345,14 @@ impl LifecycleManager { (table_persist_segment_uuid, table_persist_uuid, branch_uuid, table_uuid, \ chunk_idx, min_tx_commit_micros, max_tx_commit_micros, \ min_commit_seq_num, max_commit_seq_num, \ - object_uri, row_count, format, statistics) \ - VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13) \ + object_uri, row_count, format, statistics, content_hash) \ + VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, $14) \ ON CONFLICT (branch_uuid, table_persist_segment_uuid) DO UPDATE \ SET object_uri = EXCLUDED.object_uri, \ row_count = EXCLUDED.row_count, \ format = EXCLUDED.format, \ - statistics = EXCLUDED.statistics", + statistics = EXCLUDED.statistics, \ + content_hash = EXCLUDED.content_hash", table = qi(&table), ); driver @@ -370,6 +372,7 @@ impl LifecycleManager { SqlValue::Int64(row_count), SqlValue::Text(format_text.to_string()), SqlValue::Bytes(statistics.to_vec()), + SqlValue::Uuid(*content_hash), ], ) .await?; @@ -413,6 +416,7 @@ impl LifecycleManager { size_bytes: i64, format_text: &str, statistics: &[u8], + content_hash: &Uuid, seal_now: bool, ) -> Result<()> { let catalog = parse_uuid(catalog_uuid); @@ -426,9 +430,10 @@ impl LifecycleManager { length = $3, \ size_bytes = $4, \ format = $5, \ - statistics = $6{seal_clause} \ - WHERE branch_uuid = $7 \ - AND table_persist_segment_uuid = $8", + statistics = $6, \ + content_hash = $7{seal_clause} \ + WHERE branch_uuid = $8 \ + AND table_persist_segment_uuid = $9", table = qi(&table), ); driver @@ -441,6 +446,7 @@ impl LifecycleManager { SqlValue::Int64(size_bytes), SqlValue::Text(format_text.to_string()), SqlValue::Bytes(statistics.to_vec()), + SqlValue::Uuid(*content_hash), SqlValue::Uuid(branch), SqlValue::uuid_str(table_persist_segment_uuid)?, ], diff --git a/crates/penca-storage-meta/src/segment_index.rs b/crates/penca-storage-meta/src/segment_index.rs index 0378008..aec9a50 100644 --- a/crates/penca-storage-meta/src/segment_index.rs +++ b/crates/penca-storage-meta/src/segment_index.rs @@ -255,6 +255,7 @@ impl LifecycleManager { format_text: &str, size_bytes: i64, statistics: &[u8], + content_hash: &Uuid, ) -> Result<()> { let catalog = parse_uuid(catalog_uuid); let branch = parse_uuid(branch_uuid); @@ -262,8 +263,9 @@ impl LifecycleManager { let sql = format!( "INSERT INTO {table} \ (segment_index_uuid, branch_uuid, segment_uuid, table_snapshot_index_uuid, \ - object_uri, \"offset\", length, format, size_bytes, statistics) \ - VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10) \ + object_uri, \"offset\", length, format, size_bytes, statistics, \ + content_hash) \ + VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11) \ ON CONFLICT (branch_uuid, segment_index_uuid) DO UPDATE \ SET segment_uuid = EXCLUDED.segment_uuid, \ table_snapshot_index_uuid = EXCLUDED.table_snapshot_index_uuid, \ @@ -272,7 +274,8 @@ impl LifecycleManager { length = EXCLUDED.length, \ format = EXCLUDED.format, \ size_bytes = EXCLUDED.size_bytes, \ - statistics = EXCLUDED.statistics", + statistics = EXCLUDED.statistics, \ + content_hash = EXCLUDED.content_hash", table = qi(&table), ); driver @@ -289,6 +292,7 @@ impl LifecycleManager { SqlValue::Text(format_text.to_string()), SqlValue::Int64(size_bytes), SqlValue::Bytes(statistics.to_vec()), + SqlValue::Uuid(*content_hash), ], ) .await?; diff --git a/crates/penca-storage-meta/src/snapshot.rs b/crates/penca-storage-meta/src/snapshot.rs index f1b3e17..6c454c1 100644 --- a/crates/penca-storage-meta/src/snapshot.rs +++ b/crates/penca-storage-meta/src/snapshot.rs @@ -132,6 +132,7 @@ impl LifecycleManager { row_count: i64, format_text: &str, statistics: &[u8], + content_hash: &Uuid, ) -> Result<()> { let catalog = parse_uuid(catalog_uuid); let branch = parse_uuid(branch_uuid); @@ -139,15 +140,17 @@ impl LifecycleManager { let sql = format!( "INSERT INTO {table} \ (table_snapshot_segment_uuid, table_snapshot_uuid, branch_uuid, table_uuid, \ - chunk_idx, object_uri, \"offset\", length, row_count, format, statistics) \ - VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11) \ + chunk_idx, object_uri, \"offset\", length, row_count, format, statistics, \ + content_hash) \ + VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12) \ ON CONFLICT (branch_uuid, table_snapshot_segment_uuid) DO UPDATE \ SET object_uri = EXCLUDED.object_uri, \ \"offset\" = EXCLUDED.\"offset\", \ length = EXCLUDED.length, \ row_count = EXCLUDED.row_count, \ format = EXCLUDED.format, \ - statistics = EXCLUDED.statistics", + statistics = EXCLUDED.statistics, \ + content_hash = EXCLUDED.content_hash", table = qi(&table), ); driver @@ -165,6 +168,7 @@ impl LifecycleManager { SqlValue::Int64(row_count), SqlValue::Text(format_text.to_string()), SqlValue::Bytes(statistics.to_vec()), + SqlValue::Uuid(*content_hash), ], ) .await?; From 3dddb3442b6a584d64cb8a2f124c5091fcc9b933 Mon Sep 17 00:00:00 2001 From: Nico Bautista Hobin Date: Fri, 31 Jul 2026 02:46:14 +0000 Subject: [PATCH 11/32] feat(cold): inherit content_hash on every reference copy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both reference-copy families mint a new row uuid over an object nobody rewrote, which is exactly the duplication CHA-545 is about and the only place cross-branch dedup is established. Each copy now carries the source row's `content_hash` verbatim — no recompute, no file read — by adding one column to the existing explicit `INSERT … SELECT old.` projections: - carry-forward: `insert_carried_snapshot_segments` and its sidecar twin `insert_carried_segment_indexes`, both also extending `DO UPDATE SET` so a crash-retry refreshes the column like its neighbours - fork copy: the snapshot-segment, index-sidecar and persist-segment arms in `fork_copy.rs`, which are `DO NOTHING` and gain no update clause The parent-header arms (`table_snapshot_metadata`, its index parent, and `table_persist_metadata`) are deliberately untouched: they name no object-storage artifact, so a `content_hash` there would be a column with no writer and no reader. Asserted on the steady-state carry test, which already proves carry-by- reference via a shared `object_uri` — base segment and sidecar both. CHA-545 --- crates/penca-storage-meta/src/fork_copy.rs | 14 ++--- .../penca-storage-meta/src/segment_index.rs | 8 +-- crates/penca-storage-meta/src/snapshot.rs | 8 +-- .../integration_cold_user_index_build_test.py | 53 ++++++++++++++++++- 4 files changed, 70 insertions(+), 13 deletions(-) diff --git a/crates/penca-storage-meta/src/fork_copy.rs b/crates/penca-storage-meta/src/fork_copy.rs index b73cc8f..011cea4 100644 --- a/crates/penca-storage-meta/src/fork_copy.rs +++ b/crates/penca-storage-meta/src/fork_copy.rs @@ -253,10 +253,12 @@ impl LifecycleManager { "INSERT INTO {seg} \ (table_snapshot_segment_uuid, table_snapshot_uuid, branch_uuid, \ table_uuid, chunk_idx, object_uri, \"offset\", length, size_bytes, \ - format, metadata, statistics, row_count, commit_micros) \ + format, metadata, statistics, row_count, content_hash, \ + commit_micros) \ SELECT m.new_uuid, $1, $2, old.table_uuid, m.chunk_idx, old.object_uri, \ old.\"offset\", old.length, old.size_bytes, old.format, \ - old.metadata, old.statistics, old.row_count, $4 \ + old.metadata, old.statistics, old.row_count, \ + old.content_hash, $4 \ FROM {seg_old} old \ JOIN unnest({new_arr}, {old_arr}, {chunk_arr}) \ AS m(new_uuid, old_uuid, chunk_idx) \ @@ -369,10 +371,10 @@ impl LifecycleManager { "INSERT INTO {sidecar} \ (segment_index_uuid, branch_uuid, segment_uuid, \ table_snapshot_index_uuid, object_uri, \"offset\", length, \ - format, size_bytes, statistics, commit_micros) \ + format, size_bytes, statistics, content_hash, commit_micros) \ SELECT m.new_sidecar, $1, m.new_seg, m.new_parent, old.object_uri, \ old.\"offset\", old.length, old.format, old.size_bytes, \ - old.statistics, $3 \ + old.statistics, old.content_hash, $3 \ FROM {sidecar_old} old \ JOIN unnest({a1}, {a2}, {a3}, {a4}, {a5}) \ AS m(new_sidecar, new_seg, new_parent, old_seg, old_parent) \ @@ -611,13 +613,13 @@ impl LifecycleManager { table_uuid, chunk_idx, min_tx_commit_micros, max_tx_commit_micros, \ min_commit_seq_num, max_commit_seq_num, object_uri, \"offset\", \ length, row_count, format, size_bytes, metadata, statistics, \ - is_sealed, commit_micros) \ + content_hash, is_sealed, commit_micros) \ SELECT m.new_uuid, $1, $2, old.table_uuid, old.chunk_idx, \ old.min_tx_commit_micros, old.max_tx_commit_micros, \ old.min_commit_seq_num, LEAST(old.max_commit_seq_num, $4), \ old.object_uri, old.\"offset\", old.length, old.row_count, \ old.format, old.size_bytes, old.metadata, old.statistics, \ - TRUE, $5 \ + old.content_hash, TRUE, $5 \ FROM {seg_old} old \ JOIN unnest({new_arr}, {old_arr}) AS m(new_uuid, old_uuid) \ ON old.table_persist_segment_uuid = m.old_uuid \ diff --git a/crates/penca-storage-meta/src/segment_index.rs b/crates/penca-storage-meta/src/segment_index.rs index aec9a50..5671854 100644 --- a/crates/penca-storage-meta/src/segment_index.rs +++ b/crates/penca-storage-meta/src/segment_index.rs @@ -572,10 +572,11 @@ impl LifecycleManager { let sql = format!( "INSERT INTO {table} \ (segment_index_uuid, branch_uuid, segment_uuid, table_snapshot_index_uuid, \ - object_uri, \"offset\", length, format, size_bytes, statistics) \ + object_uri, \"offset\", length, format, size_bytes, statistics, \ + content_hash) \ SELECT n.new_sidecar, $1, n.new_seg, $2, \ old.object_uri, old.\"offset\", old.length, old.format, \ - old.size_bytes, old.statistics \ + old.size_bytes, old.statistics, old.content_hash \ FROM UNNEST({new_seg_arr}, {new_sidecar_arr}, {prior_sidecar_arr}) \ AS n(new_seg, new_sidecar, prior_sidecar) \ JOIN {source_table} old \ @@ -590,7 +591,8 @@ impl LifecycleManager { length = EXCLUDED.length, \ format = EXCLUDED.format, \ size_bytes = EXCLUDED.size_bytes, \ - statistics = EXCLUDED.statistics", + statistics = EXCLUDED.statistics, \ + content_hash = EXCLUDED.content_hash", table = qi(&table), source_table = qi(&source_table), ); diff --git a/crates/penca-storage-meta/src/snapshot.rs b/crates/penca-storage-meta/src/snapshot.rs index 6c454c1..edf773f 100644 --- a/crates/penca-storage-meta/src/snapshot.rs +++ b/crates/penca-storage-meta/src/snapshot.rs @@ -570,10 +570,11 @@ impl LifecycleManager { "INSERT INTO {table} \ (table_snapshot_segment_uuid, table_snapshot_uuid, branch_uuid, \ table_uuid, chunk_idx, object_uri, \"offset\", length, \ - row_count, size_bytes, format, metadata, statistics) \ + row_count, size_bytes, format, metadata, statistics, content_hash) \ SELECT new.uuid, $1, $2, old.table_uuid, new.idx, \ old.object_uri, old.\"offset\", old.length, old.row_count, \ - old.size_bytes, old.format, old.metadata, old.statistics \ + old.size_bytes, old.format, old.metadata, old.statistics, \ + old.content_hash \ FROM UNNEST({new_arr}, {idx_arr}, {prior_arr}) \ AS new(uuid, idx, old_uuid) \ JOIN {source_table} old \ @@ -589,7 +590,8 @@ impl LifecycleManager { size_bytes = EXCLUDED.size_bytes, \ format = EXCLUDED.format, \ metadata = EXCLUDED.metadata, \ - statistics = EXCLUDED.statistics \ + statistics = EXCLUDED.statistics, \ + content_hash = EXCLUDED.content_hash \ RETURNING table_snapshot_segment_uuid", table = qi(&table), source_table = qi(&source_table), diff --git a/tests/integration/integration_cold_user_index_build_test.py b/tests/integration/integration_cold_user_index_build_test.py index 091640e..bb6c5d4 100644 --- a/tests/integration/integration_cold_user_index_build_test.py +++ b/tests/integration/integration_cold_user_index_build_test.py @@ -222,6 +222,35 @@ def _user_child(catalog_uuid, branch_uuid, segment_uuid, index_uuid): return None +def _segment_content_hash(catalog_uuid, branch_uuid, segment_uuid): + """A base segment's ``content_hash``, as text.""" + rows = get_pg_driver().execute( + SQL( + "SELECT content_hash::text FROM {tbl}" + " WHERE branch_uuid = %s AND table_snapshot_segment_uuid = %s" + ).format(tbl=Identifier(f"{catalog_uuid}_{TABLE_SNAPSHOT_SEGMENT_METADATA}")), + (branch_uuid, segment_uuid), + ) + return rows[0][0] + + +def _sidecar_content_hash(catalog_uuid, branch_uuid, segment_uuid, link): + """The ``content_hash`` of ``segment_uuid``'s sidecar under one parent + index, as text. ``link`` is the parent ``table_snapshot_index_uuid`` + returned by :func:`_user_child`.""" + rows = get_pg_driver().execute( + SQL( + "SELECT content_hash::text FROM {tbl}" + " WHERE branch_uuid = %s AND segment_uuid = %s" + " AND table_snapshot_index_uuid = %s" + ).format( + tbl=Identifier(f"{catalog_uuid}_{TABLE_SNAPSHOT_SEGMENT_INDEX_METADATA}") + ), + (branch_uuid, segment_uuid, link), + ) + return rows[0][0] + + class TestUserIndexBuild: """A cold snapshot materializes user ``CREATE INDEX`` definitions: a committed parent (``index_uuid`` non-NULL) + one committed sidecar per base @@ -509,12 +538,18 @@ def test_user_index_sidecar_carries_forward_by_reference(self): table_uuid, pa.table({"name": ["alice", "carol"], "value": [1, 3]}, schema=USER_SCHEMA), ) - # Map each snap1 base file -> its user sidecar uri. + # Map each snap1 base file -> its user sidecar uri, and both rows' + # content hashes for the inheritance assertion below. snap1_sidecar_by_file = {} + snap1_hashes_by_file = {} for seg, uri, off in _base_segment_tuples(catalog_uuid, branch, snap1): child = _user_child(catalog_uuid, branch, seg, index_uuid) assert child is not None, "snap1 must build a user sidecar per segment" snap1_sidecar_by_file[(uri, off)] = child[1] # object_uri + snap1_hashes_by_file[(uri, off)] = ( + _segment_content_hash(catalog_uuid, branch, seg), + _sidecar_content_hash(catalog_uuid, branch, seg, child[0]), + ) # snap2 rewrites only alice -> carol's segment carries forward, and its # already-built sidecar must carry by reference (same object_uri). @@ -540,3 +575,19 @@ def test_user_index_sidecar_carries_forward_by_reference(self): f" by reference, no rebuild): {child[1]} !=" f" {snap1_sidecar_by_file[(uri, off)]}" ) + # CHA-545: a carried row is a fresh uuid over bytes nobody rewrote, + # so it must inherit the prior row's content_hash verbatim. That + # inheritance is the whole dedup — recomputing, or defaulting, would + # give the same bytes two cache entries. + prior_seg_hash, prior_sidecar_hash = snap1_hashes_by_file[(uri, off)] + assert _segment_content_hash(catalog_uuid, branch, seg) == prior_seg_hash, ( + "a carried base segment must inherit its prior row's" + f" content_hash, expected {prior_seg_hash}" + ) + assert ( + _sidecar_content_hash(catalog_uuid, branch, seg, child[0]) + == prior_sidecar_hash + ), ( + "a carried sidecar must inherit its prior row's content_hash," + f" expected {prior_sidecar_hash}" + ) From b4b153927b0456d40d7337e24c1a618098a4acbd Mon Sep 17 00:00:00 2001 From: Nico Bautista Hobin Date: Fri, 31 Jul 2026 02:51:36 +0000 Subject: [PATCH 12/32] docs(cold): scope the view-slice exception to the parent array The narrowing clause said view columns stay invariant below the 12-byte inline threshold. Measured false for a mixed-length parent: the IPC writer emits each variadic data buffer whole, and one long value anywhere in the parent gives the array those buffers, so a slice of only short rows still hashes differently from a standalone batch of the same rows. A reader whose view columns are short would have concluded invariance from the old wording. Pins the mixed case in the canary test alongside the all-inline one, so the assertion messages no longer state a rule broader than the fixture. CHA-545 --- crates/penca-core/src/digest.rs | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/crates/penca-core/src/digest.rs b/crates/penca-core/src/digest.rs index 2ce5268..9fa6134 100644 --- a/crates/penca-core/src/digest.rs +++ b/crates/penca-core/src/digest.rs @@ -36,10 +36,13 @@ const IPC_ALIGNMENT: usize = 8; /// /// `Utf8View`/`BinaryView` do not, top-level or as a list child: the writer /// truncates the *views* buffer but emits each variadic data buffer whole, so a -/// slice carries bytes belonging to rows it does not cover. Only above the -/// 12-byte inline threshold — shorter values live in the view itself and are -/// invariant. `Dictionary` fails the same way under `DictionaryHandling::Resend` -/// but is rejected at the type boundary and cannot reach here. +/// slice carries bytes belonging to rows it does not cover. The condition is a +/// property of the parent array, not of the sliced rows — a single value above +/// the 12-byte inline threshold anywhere in the parent gives the array data +/// buffers, and every slice of it then carries them in full. Invariance holds +/// only when *no* value in the parent is longer than that. `Dictionary` fails +/// the same way under `DictionaryHandling::Resend` but is rejected at the type +/// boundary and cannot reach here. /// /// Neither costs correctness, and neither costs the dedup this digest exists /// for: a reference copy *inherits* the stored hash rather than recomputing it, @@ -309,7 +312,15 @@ mod tests { assert_eq!( hash(&view(vec!["aa", "bb", "cc"]).slice(1, 2)), hash(&view(vec!["bb", "cc"])), - "inline-length views carry no data buffer, so they stay invariant" + "an all-inline parent has no data buffer at all, so it stays invariant" + ); + // The narrow reading of the line above — "short rows are invariant" — + // is false: one long value anywhere in the parent gives the array a + // data buffer, and a slice of only short rows still carries it. + assert_ne!( + hash(&view(vec!["aa", "bb", &long[0]]).slice(0, 2)), + hash(&view(vec!["aa", "bb"])), + "the exception is a property of the parent array, not of the sliced rows" ); } } From b8310a2f6c7243c9c3dac3762b4a271c0ac06d85 Mon Sep 17 00:00:00 2001 From: Nico Bautista Hobin Date: Fri, 31 Jul 2026 02:52:51 +0000 Subject: [PATCH 13/32] test(cold): join the fork-divergence precondition on content_hash MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The precondition probed the old slice-keying condition — matching object_uri and offset. Under content-hash keying that predicate no longer names what makes the fork and the parent share a cache entry, so a copy that minted a fresh hash instead of inheriting one would leave the addressing identical, pass the precondition, and turn the guard into the silent no-op its raise exists to prevent. Joins on content_hash first, keeping the addressing columns (now including length) to pin that the collision comes from a reference copy rather than two independent writes landing on the same bytes. CHA-545 --- ...integration_branch_persist_at_fork_test.py | 20 ++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/tests/integration/integration_branch_persist_at_fork_test.py b/tests/integration/integration_branch_persist_at_fork_test.py index a5c23c8..38e9091 100644 --- a/tests/integration/integration_branch_persist_at_fork_test.py +++ b/tests/integration/integration_branch_persist_at_fork_test.py @@ -1183,10 +1183,17 @@ def test_fork_and_parent_diverge_a_columns_type_over_one_shared_slice(): ) # The precondition the scenario rests on: the fork's cold row and the - # parent's must name the SAME slice, or the two branches decode + # parent's must land on the SAME cache entry, or the two branches decode # independently, every assertion below still passes, and this test covers # nothing. Metadata-only, so it does not warm the cache it is about to probe. # + # `content_hash` is the join that matters — it is the cache key (CHA-545). + # Matching `object_uri`/`offset`/`length` too pins WHY they collide: the + # fork's row is a reference copy of the parent's slice that inherited its + # hash, not two writes that happened to produce the same bytes. Joining on + # addressing alone would pass even if the copy minted a fresh hash, which + # is the failure mode that would silently retire this test. + # # BOTH tiers, because which one carries the shared slice depends on the # fixture: here the snapshot covers the whole persist, so `copy_inherited_ # persist` finds nothing above the baseline watermark and the inheritance is @@ -1197,7 +1204,9 @@ def test_fork_and_parent_diverge_a_columns_type_over_one_shared_slice(): rows = get_pg_driver().execute( SQL( "SELECT count(*) FROM {tbl} p JOIN {tbl} c" - ' ON p.object_uri = c.object_uri AND p."offset" IS NOT DISTINCT FROM c."offset"' + " ON p.content_hash = c.content_hash AND p.object_uri = c.object_uri" + ' AND p."offset" IS NOT DISTINCT FROM c."offset"' + " AND p.length IS NOT DISTINCT FROM c.length" " WHERE p.branch_uuid = %s AND c.branch_uuid = %s AND p.table_uuid = %s" ).format(tbl=Identifier(f"{catalog_uuid}_{base}")), (main_branch, child, table_uuid), @@ -1206,9 +1215,10 @@ def test_fork_and_parent_diverge_a_columns_type_over_one_shared_slice(): if shared == 0: raise RuntimeError( - "setup failed: the fork holds no cold row naming the parent's slice " - "in either tier, so both branches would decode independently and the " - "cross-branch cache sharing this test pins is unreachable" + "setup failed: the fork holds no cold row sharing a content_hash " + "with the parent's slice in either tier, so both branches would " + "decode independently and the cross-branch cache sharing this test " + "pins is unreachable" ) # Child first, then parent: the child's decode populates the cache and the From f24498657866fb4d21f0eb221d39a7544aa48297 Mon Sep 17 00:00:00 2001 From: Nico Bautista Hobin Date: Fri, 31 Jul 2026 03:55:45 +0000 Subject: [PATCH 14/32] refactor(query): cache the file-native decode, shape after the lookup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every cache-populating read decoded to the caller's schema and cached that. Once two callers with different schemas share one entry — which is what content-hash keying is about to make routine — the second gets the first's types. Adds FormatReader::read_segment_native, which returns the file's own columns in the file's own types with only the (offset, length) slice applied, so row count and row order still line up with the sidecar offsets seek_row_offsets and take_matched_rows index into. Both real readers now route their projected and native reads through one read_in_file_schema, so the native path is not a second read implementation and the projected path keeps its column pushdown. The shaping tail — project_schema then null_fill_to_schema — is lifted verbatim into shape_to_schema and re-run in penca-dl after the cache lookup, on the hit and miss paths alike, for base segments, persist segments and index sidecars. Sidecars are the same bug, not an exemption: the file carries the writing branch's key column types while key_types comes from the reading branch's schema. Pins that NonNullableMissingColumn still fires on a hit, red-verified by returning the cached batch unshaped and watching it pass instead. Cache keying is untouched here — still per-uuid. CHA-545 --- crates/penca-dl/src/driver.rs | 140 +++++++++++++------- crates/penca-dl/src/provider.rs | 31 +++++ crates/penca-format/src/reader/lance.rs | 92 +++++++++---- crates/penca-format/src/reader/mod.rs | 41 ++++++ crates/penca-format/src/reader/parquet.rs | 107 ++++++++++----- crates/penca-merge/benches/floor_support.rs | 22 +++ 6 files changed, 321 insertions(+), 112 deletions(-) diff --git a/crates/penca-dl/src/driver.rs b/crates/penca-dl/src/driver.rs index 7d8c704..d0fa929 100644 --- a/crates/penca-dl/src/driver.rs +++ b/crates/penca-dl/src/driver.rs @@ -19,7 +19,7 @@ use async_trait::async_trait; use datafusion::execution::SendableRecordBatchStream; use datafusion::execution::context::{SessionContext, SessionState}; use penca_core::{ColdStoragePlan, IndexSidecar, PersistSegment, SnapshotSegment}; -use penca_format::reader::{FormatError, FormatReader}; +use penca_format::reader::{FormatError, FormatReader, shape_to_schema}; use penca_storage_cold::{COMMIT_SEQ_NUM_COLUMN, ColdStorageError}; use tracing::Instrument as _; use uuid::Uuid; @@ -204,29 +204,18 @@ impl DatafusionDlDriver { } /// Cacheable miss: decode the WHOLE segment (all columns, no filter -/// pushdown) so the cached entry is reusable across any projection, insert -/// it under `weight`, and return the full superset. The caller has already -/// decided this segment is admissible. +/// pushdown, no caller shaping) so the cached entry is reusable across any +/// projection AND any caller schema, insert it under `weight`, and return the +/// native batch. The caller has already decided this segment is admissible, +/// and shapes the result itself once it holds it. async fn read_and_cache_full( reader: &R, cache: &SegmentCache, segment: &SnapshotSegment, - full_schema: &SchemaRef, weight: u64, -) -> Result { - let full_cols: Vec<&str> = full_schema - .fields() - .iter() - .map(|f| f.name().as_str()) - .collect(); +) -> Result, DlError> { let batch = reader - .read_segment( - &segment.uri, - Some(segment.offset), - Some(segment.length), - full_schema, - Some(&full_cols), - ) + .read_segment_native(&segment.uri, Some(segment.offset), Some(segment.length)) .await .map_err(ColdStorageError::from)?; let batch = Arc::new(batch); @@ -239,7 +228,7 @@ async fn read_and_cache_full( rows = batch.num_rows(), "snapshot segment cached full decode" ); - Ok((*batch).clone()) + Ok(batch) } /// Non-cacheable miss: a projected read of just `out_schema`, not cached. @@ -276,6 +265,10 @@ async fn read_projected_uncached( /// superset on hit / miss-cached, or the projected `out_schema` batch on the /// non-cacheable (oversized) path. No predicate is pushed (ADR 0023); the /// caller projects / null-fills downstream. +/// +/// The cached value is the file-native decode; shaping it to `full_schema` +/// happens here, after the lookup, so two branches sharing one entry each get +/// their own types rather than whichever branch decoded first (CHA-545). #[tracing::instrument( level = "debug", skip_all, @@ -298,7 +291,7 @@ pub(crate) async fn read_cached_snapshot_segment( if let Some(full) = cache.get(uuid) { span.record("cache", "hit"); tracing::debug!(rows = full.num_rows(), "snapshot segment cache hit"); - return Ok((*full).clone()); + return Ok(shape_to_schema(&full, full_schema, None).map_err(ColdStorageError::from)?); } let code = segment.format.as_wire_code(); @@ -311,36 +304,25 @@ pub(crate) async fn read_cached_snapshot_segment( if cache.admits(weight) { span.record("cache", "miss-cached"); - read_and_cache_full(reader, cache, segment, full_schema, weight).await + let native = read_and_cache_full(reader, cache, segment, weight).await?; + Ok(shape_to_schema(&native, full_schema, None).map_err(ColdStorageError::from)?) } else { span.record("cache", "miss-uncached"); read_projected_uncached(reader, segment, out_schema).await } } -/// Cacheable persist miss: decode the WHOLE persist segment (all columns) so the -/// cached entry serves any projection, insert it under `weight`, and return the -/// full superset. +/// Cacheable persist miss: decode the WHOLE persist segment (all columns, no +/// caller shaping) so the cached entry serves any projection and any caller +/// schema, insert it under `weight`, and return the native batch. async fn read_and_cache_full_persist( reader: &R, cache: &SegmentCache, segment: &PersistSegment, - full_schema: &SchemaRef, weight: u64, -) -> Result { - let full_cols: Vec<&str> = full_schema - .fields() - .iter() - .map(|f| f.name().as_str()) - .collect(); +) -> Result, DlError> { let batch = reader - .read_segment( - &segment.uri, - segment.offset, - segment.length, - full_schema, - Some(&full_cols), - ) + .read_segment_native(&segment.uri, segment.offset, segment.length) .await .map_err(ColdStorageError::from)?; let batch = Arc::new(batch); @@ -349,7 +331,7 @@ async fn read_and_cache_full_persist( rows = batch.num_rows(), "persist segment decoded and cached" ); - Ok((*batch).clone()) + Ok(batch) } /// Non-cacheable persist miss: a projected read of just `out_schema`, not cached. @@ -439,7 +421,7 @@ pub(crate) async fn read_cached_persist_segment( if let Some(full) = cache.get(uuid) { span.record("cache", "hit"); tracing::debug!(rows = full.num_rows(), "persist segment cache hit"); - return Ok((*full).clone()); + return Ok(shape_to_schema(&full, full_schema, None).map_err(ColdStorageError::from)?); } let code = segment.format.as_wire_code(); @@ -453,7 +435,8 @@ pub(crate) async fn read_cached_persist_segment( if cache.admits(weight) { span.record("cache", "miss-cached"); - read_and_cache_full_persist(reader, cache, segment, full_schema, weight).await + let native = read_and_cache_full_persist(reader, cache, segment, weight).await?; + Ok(shape_to_schema(&native, full_schema, None).map_err(ColdStorageError::from)?) } else { span.record("cache", "miss-uncached"); read_projected_uncached_persist(reader, segment, out_schema, full_schema).await @@ -463,6 +446,12 @@ pub(crate) async fn read_cached_persist_segment( /// Load a sorted `(key, row_offset)` index sidecar through the shared snapshot /// cache, keyed by its own `segment_index_uuid` — a distinct deterministic-UUID /// namespace from the base segment uuid, so the two never collide in one cache. +/// +/// Cached natively and shaped to `key_types` after the lookup, for the same +/// reason as a base segment: the file was written with the *writing* branch's +/// key column types, `key_types` comes from the *reading* branch's schema, and +/// a fork that diverges an indexed column's type makes the two disagree +/// (CHA-545). #[tracing::instrument( level = "debug", skip_all, @@ -478,25 +467,23 @@ async fn read_cached_index_sidecar( key_types: &[arrow::datatypes::DataType], ) -> Result { let span = tracing::Span::current(); + // The sidecar's key schema is the indexed columns' native types; the + // identity/name sidecars are the all-Utf8 special case. + let schema = penca_format::index::segment_index_schema(key_types); if let Some(batch) = cache.get(&sidecar.segment_index_uuid) { span.record("cache", "hit"); - return Ok((*batch).clone()); + return Ok(shape_to_schema(&batch, &schema, None).map_err(ColdStorageError::from)?); } span.record("cache", "miss"); let code = sidecar.format.as_wire_code(); let reader = readers .get(&code) .ok_or(ColdStorageError::UnknownFormat(code))?; - // The sidecar's key schema is the indexed columns' native types; the - // identity/name sidecars are the all-Utf8 special case. - let schema = penca_format::index::segment_index_schema(key_types); let batch = reader - .read_segment( + .read_segment_native( &sidecar.object_uri, Some(sidecar.offset), Some(sidecar.length), - &schema, - None, ) .await .map_err(ColdStorageError::from)?; @@ -508,7 +495,7 @@ async fn read_cached_index_sidecar( Arc::clone(&batch), sidecar.size_bytes.max(0) as u64, ); - Ok((*batch).clone()) + Ok(shape_to_schema(&batch, &schema, None).map_err(ColdStorageError::from)?) } /// Index-driven selective read: binary-search the segment's index sidecar for @@ -954,6 +941,18 @@ mod tests { Ok(self.batch.project(&indices)?) } + + /// Delegates so the read counter stays in one place: a native decode is + /// a storage hit like any other, and the cache tests count both. + async fn read_segment_native( + &self, + uri: &str, + offset: Option, + length: Option, + ) -> Result { + self.read_segment(uri, offset, length, &self.batch.schema(), None) + .await + } } fn test_schema() -> SchemaRef { @@ -1040,6 +1039,17 @@ mod tests { .unwrap_or_else(|| panic!("unexpected read of {uri}")) .clone()) } + + async fn read_segment_native( + &self, + uri: &str, + offset: Option, + length: Option, + ) -> Result { + // `read_segment` ignores its schema argument here — it routes on uri. + self.read_segment(uri, offset, length, &Arc::new(Schema::empty()), None) + .await + } } fn routing_driver( @@ -2351,6 +2361,38 @@ mod tests { ); } + /// Shaping moved after the cache lookup (CHA-545), so a caller asking for a + /// non-nullable column the file lacks must fail the same way whether or not + /// an earlier caller already warmed the entry. The first read caches the + /// native decode and *then* fails to shape it, so the second read takes the + /// hit path — the one that would silently return an unshaped batch if the + /// tail were skipped there. + #[tokio::test] + async fn non_nullable_missing_column_errors_on_a_cache_hit_too() { + let demanding: SchemaRef = Arc::new(Schema::new(vec![ + Field::new("row_uuid", DataType::Utf8, false), + Field::new("absent", DataType::Int32, false), + ])); + let cache = Arc::new(SegmentCache::new(1 << 20)); + let (dl, reads) = driver_with(cache, test_batch(&test_schema())); + let seg = segment("seg-missing", 128); + + for pass in ["miss", "hit"] { + let err = read_seg(&dl, &seg, &demanding, &demanding) + .await + .expect_err("non-nullable column absent from the segment must error"); + assert!( + err.to_string().contains("absent"), + "{pass} pass must name the missing column, got: {err}" + ); + } + assert_eq!( + reads.load(Ordering::SeqCst), + 1, + "the second pass errored off the cached entry, not a re-read" + ); + } + #[tokio::test] async fn scan_snapshot_schema_tolerance() { // Segment decoded against the OLDER narrow schema {row_uuid, name}; diff --git a/crates/penca-dl/src/provider.rs b/crates/penca-dl/src/provider.rs index 42a1a21..3310c65 100644 --- a/crates/penca-dl/src/provider.rs +++ b/crates/penca-dl/src/provider.rs @@ -1022,6 +1022,15 @@ mod tests { ) -> Result { Ok(self.batch.clone()) } + + async fn read_segment_native( + &self, + _uri: &str, + _offset: Option, + _length: Option, + ) -> Result { + Ok(self.batch.clone()) + } } fn snapshot_seg(uuid: &str, size_bytes: i64) -> SnapshotSegment { @@ -1284,6 +1293,17 @@ mod tests { self.reads.lock().unwrap().push(uri.to_string()); Ok(self.batches.get(uri).cloned().expect("uri registered")) } + + /// Delegates so one read is recorded per storage hit, native or not. + async fn read_segment_native( + &self, + uri: &str, + offset: Option, + length: Option, + ) -> Result { + self.read_segment(uri, offset, length, &Arc::new(Schema::empty()), None) + .await + } } #[tokio::test] @@ -2002,6 +2022,17 @@ mod by_plan_order_tests { } Ok(self.batches[uri].clone()) } + + /// Delegates so the deliberate per-uri sleep applies to native reads too. + async fn read_segment_native( + &self, + uri: &str, + offset: Option, + length: Option, + ) -> Result { + self.read_segment(uri, offset, length, &Arc::new(Schema::empty()), None) + .await + } } fn session_for( diff --git a/crates/penca-format/src/reader/lance.rs b/crates/penca-format/src/reader/lance.rs index 1d73488..e99b2bb 100644 --- a/crates/penca-format/src/reader/lance.rs +++ b/crates/penca-format/src/reader/lance.rs @@ -16,7 +16,7 @@ use lance_io::ReadBatchParams; use lance_io::scheduler::{ScanScheduler, SchedulerConfig}; use super::{ - FormatError, FormatReader, empty_batch, null_fill_to_schema, present_columns, project_schema, + FormatError, FormatReader, empty_batch, present_columns, project_schema, shape_to_schema, }; use crate::uri::uri_to_object_path; @@ -76,10 +76,11 @@ impl LanceFormatReader { /// `params` selects the row range (`RangeFull` for whole-file, /// `Range(start..end)` for a slice). When `projection` is `Some`, /// dispatches through `read_stream_projected`; otherwise through - /// `read_stream`. + /// `read_stream`. `empty_schema` is used only when the slice decodes to no + /// batches at all; otherwise the decoded batches' own schema is kept. async fn read_with_params( reader: &FileReader, - output_schema: &SchemaRef, + empty_schema: &SchemaRef, projection: Option, params: ReadBatchParams, ) -> Result { @@ -113,9 +114,51 @@ impl LanceFormatReader { } if batches.is_empty() { - return Ok(empty_batch(output_schema)); + return Ok(empty_batch(empty_schema)); } - Ok(concat_batches(output_schema, &batches)?) + Ok(concat_batches(&batches[0].schema(), &batches)?) + } + + /// The read itself, in the file's own schema. `projection`, when `Some`, + /// is narrowed to the columns physically present and pushed into the Lance + /// reader; `None` reads every column the file has. + /// + /// Shaping to a caller's schema is the caller's job — `read_segment` runs + /// it immediately, `read_segment_native`'s callers run it after their + /// cache lookup. + async fn read_in_file_schema( + &self, + uri: &str, + offset: Option, + length: Option, + projection: Option<&[&str]>, + ) -> Result { + let reader = self.open_file(uri).await?; + let file_schema: SchemaRef = Arc::new(arrow::datatypes::Schema::from( + reader.metadata().file_schema.as_ref(), + )); + // A column added by a later `ALTER TABLE ADD COLUMN` is absent from an + // older segment, and `ReaderProjection::from_column_names` would reject + // the missing name; the shaping tail null-fills it instead. + // `present_names` is never empty: every segment carries `row_uuid` and + // every read requests it, so the read yields a row count. + let (proj, read_schema) = match projection { + Some(names) => { + let present_names = present_columns(&file_schema, names); + ( + Self::build_projection(&reader, Some(&present_names))?, + project_schema(&file_schema, Some(&present_names))?, + ) + } + None => (None, file_schema), + }; + let params = match (offset, length) { + (Some(offset), Some(length)) => { + ReadBatchParams::Range(offset as usize..offset as usize + length as usize) + } + _ => ReadBatchParams::RangeFull, + }; + Self::read_with_params(&reader, &read_schema, proj, params).await } } @@ -149,33 +192,28 @@ impl FormatReader for LanceFormatReader { schema: &SchemaRef, projection: Option<&[&str]>, ) -> Result { - let output_schema = project_schema(schema, projection)?; - let reader = self.open_file(uri).await?; - // Project only the requested columns physically present in this - // segment file, then null-fill the rest to `output_schema`. A column - // added by a later `ALTER TABLE ADD COLUMN` is absent from an older - // segment; `ReaderProjection::from_column_names` would otherwise reject - // the missing name. `present_names` is never empty: every segment carries - // `row_uuid` and every read requests it, so the read yields a row count. - let file_arrow: SchemaRef = Arc::new(arrow::datatypes::Schema::from( - reader.metadata().file_schema.as_ref(), - )); let column_names: Vec<&str> = match projection { Some(cols) => cols.to_vec(), None => schema.fields().iter().map(|f| f.name().as_str()).collect(), }; - let present_names = present_columns(&file_arrow, &column_names); - let present_schema = project_schema(schema, Some(&present_names))?; - let proj = Self::build_projection(&reader, Some(&present_names))?; - let params = match (offset, length) { - (Some(offset), Some(length)) => { - ReadBatchParams::Range(offset as usize..offset as usize + length as usize) - } - _ => ReadBatchParams::RangeFull, - }; - let present = Self::read_with_params(&reader, &present_schema, proj, params).await?; - let out = null_fill_to_schema(&present, &output_schema)?; + let present = self + .read_in_file_schema(uri, offset, length, Some(&column_names)) + .await?; + let out = shape_to_schema(&present, schema, projection)?; tracing::Span::current().record("rows", out.num_rows()); Ok(out) } + + #[tracing::instrument( + skip_all, + fields(uri = %uri, offset = ?offset, length = ?length, format = "lance"), + )] + async fn read_segment_native( + &self, + uri: &str, + offset: Option, + length: Option, + ) -> Result { + self.read_in_file_schema(uri, offset, length, None).await + } } diff --git a/crates/penca-format/src/reader/mod.rs b/crates/penca-format/src/reader/mod.rs index 8a50d80..814b89c 100644 --- a/crates/penca-format/src/reader/mod.rs +++ b/crates/penca-format/src/reader/mod.rs @@ -55,6 +55,23 @@ pub trait FormatReader: Send + Sync { schema: &SchemaRef, projection: Option<&[&str]>, ) -> impl Future> + Send; + + /// Read one segment file as it was written: the file's own columns, in the + /// file's own types, with no projection and no null-fill. The `(offset, + /// length)` slice still applies, so row count and row order match what + /// [`read_segment`](Self::read_segment) would return for the same slice. + /// + /// Shape the result to a caller's schema afterwards with + /// [`shape_to_schema`] — the same tail `read_segment` runs internally. + /// Splitting the two is what lets one decode be cached and served to + /// callers whose schemas disagree: a caller-shaped cache entry would hand + /// the second caller the first caller's types (CHA-545). + fn read_segment_native( + &self, + uri: &str, + offset: Option, + length: Option, + ) -> impl Future> + Send; } /// Errors from format read/write operations. @@ -115,6 +132,18 @@ impl FormatReader for AnyFormatReader { } } } + + async fn read_segment_native( + &self, + uri: &str, + offset: Option, + length: Option, + ) -> Result { + match self { + Self::Parquet(r) => r.read_segment_native(uri, offset, length).await, + Self::Lance(r) => r.read_segment_native(uri, offset, length).await, + } + } } /// Create an empty `RecordBatch` matching the given schema. @@ -122,6 +151,18 @@ pub fn empty_batch(schema: &SchemaRef) -> RecordBatch { RecordBatch::new_empty(schema.clone()) } +/// Adapt a natively-decoded `batch` to `schema`/`projection`: the tail every +/// [`FormatReader::read_segment`] impl runs after its own read, exposed so a +/// caller that decoded via [`FormatReader::read_segment_native`] can run it +/// later — after a cache lookup — with identical behavior. +pub fn shape_to_schema( + batch: &RecordBatch, + schema: &SchemaRef, + projection: Option<&[&str]>, +) -> Result { + null_fill_to_schema(batch, &project_schema(schema, projection)?) +} + /// Resolve the effective output schema given an optional column projection. /// /// Returns `schema` unchanged when `projection` is `None`; otherwise returns diff --git a/crates/penca-format/src/reader/parquet.rs b/crates/penca-format/src/reader/parquet.rs index 265a5f2..e16c0b9 100644 --- a/crates/penca-format/src/reader/parquet.rs +++ b/crates/penca-format/src/reader/parquet.rs @@ -18,7 +18,7 @@ use parquet::arrow::arrow_reader::{RowSelection, RowSelector}; use parquet::arrow::async_reader::{ParquetObjectReader, ParquetRecordBatchStreamBuilder}; use super::{ - FormatError, FormatReader, empty_batch, null_fill_to_schema, present_columns, project_schema, + FormatError, FormatReader, empty_batch, present_columns, project_schema, shape_to_schema, }; use crate::uri::uri_to_object_path; @@ -45,6 +45,59 @@ impl ParquetFormatReader { pub fn new(store: Arc, base_uri: String) -> Self { Self { store, base_uri } } + + /// The read itself, in the file's own schema. `projection`, when `Some`, + /// is narrowed to the columns physically present and pushed down as a + /// [`ProjectionMask`] so only those byte ranges are fetched; `None` reads + /// every column the file has. + /// + /// Shaping to a caller's schema is the caller's job — `read_segment` runs + /// it immediately, `read_segment_native`'s callers run it after their + /// cache lookup. + async fn read_in_file_schema( + &self, + uri: &str, + offset: Option, + length: Option, + projection: Option<&[&str]>, + ) -> Result { + let path = uri_to_object_path(&self.base_uri, uri); + let reader = ParquetObjectReader::new(self.store.clone(), path); + let mut builder = ParquetRecordBatchStreamBuilder::new(reader).await?; + let file_schema = builder.schema().clone(); + + // A column added by a later `ALTER TABLE ADD COLUMN` is absent from an + // older segment; it is null-filled by the shaping tail rather than + // erroring the projection here. `present_names` is never empty: every + // Penca segment carries `row_uuid` and every read requests it + // (`snapshot_read_schema` prepends it), so the read below always + // yields a row count to null-fill against. + let read_schema = match projection { + Some(names) => { + let present_names = present_columns(&file_schema, names); + let parquet_schema = builder.parquet_schema().clone(); + let mask = ProjectionMask::columns(&parquet_schema, present_names.iter().copied()); + builder = builder.with_projection(mask); + project_schema(&file_schema, Some(&present_names))? + } + None => file_schema, + }; + + if let (Some(offset), Some(length)) = (offset, length) { + builder = builder.with_row_selection(slice_selection(offset as usize, length as usize)); + } + + let mut stream = builder.build()?; + let mut batches = Vec::new(); + while let Some(batch) = stream.next().await { + batches.push(batch?); + } + + if batches.is_empty() { + return Ok(empty_batch(&read_schema)); + } + Ok(concat_batches(&batches[0].schema(), &batches)?) + } } impl FormatReader for ParquetFormatReader { @@ -75,46 +128,28 @@ impl FormatReader for ParquetFormatReader { schema: &SchemaRef, projection: Option<&[&str]>, ) -> Result { - let output_schema = project_schema(schema, projection)?; let column_names: Vec<&str> = match projection { Some(cols) => cols.to_vec(), None => schema.fields().iter().map(|f| f.name().as_str()).collect(), }; - - let path = uri_to_object_path(&self.base_uri, uri); - let reader = ParquetObjectReader::new(self.store.clone(), path); - let mut builder = ParquetRecordBatchStreamBuilder::new(reader).await?; - - let parquet_schema = builder.parquet_schema().clone(); - // Project only the requested columns that physically exist in - // this segment file. A column added by a later `ALTER TABLE ADD COLUMN` - // is absent from an older segment; it is null-filled to `output_schema` - // after the read rather than erroring the projection. `present_names` - // is never empty: every Penca segment carries `row_uuid` and every - // read requests it (`snapshot_read_schema` prepends it), so the read - // below always yields a row count to null-fill against. - let present_names = present_columns(builder.schema(), &column_names); - let output_mask = ProjectionMask::columns(&parquet_schema, present_names.iter().copied()); - builder = builder.with_projection(output_mask); - - if let (Some(offset), Some(length)) = (offset, length) { - builder = builder.with_row_selection(slice_selection(offset as usize, length as usize)); - } - - let mut stream = builder.build()?; - let mut batches = Vec::new(); - while let Some(batch) = stream.next().await { - batches.push(batch?); - } - - if batches.is_empty() { - tracing::Span::current().record("rows", 0); - return Ok(empty_batch(&output_schema)); - } - - let present = concat_batches(&batches[0].schema(), &batches)?; - let out = null_fill_to_schema(&present, &output_schema)?; + let present = self + .read_in_file_schema(uri, offset, length, Some(&column_names)) + .await?; + let out = shape_to_schema(&present, schema, projection)?; tracing::Span::current().record("rows", out.num_rows()); Ok(out) } + + #[tracing::instrument( + skip_all, + fields(uri = %uri, offset = ?offset, length = ?length, format = "parquet"), + )] + async fn read_segment_native( + &self, + uri: &str, + offset: Option, + length: Option, + ) -> Result { + self.read_in_file_schema(uri, offset, length, None).await + } } diff --git a/crates/penca-merge/benches/floor_support.rs b/crates/penca-merge/benches/floor_support.rs index 37626e3..a2c066c 100644 --- a/crates/penca-merge/benches/floor_support.rs +++ b/crates/penca-merge/benches/floor_support.rs @@ -84,6 +84,15 @@ impl FormatReader for InMemoryFormatReader { ) -> Result { Ok(self.batch.clone()) } + + async fn read_segment_native( + &self, + _uri: &str, + _offset: Option, + _length: Option, + ) -> Result { + Ok(self.batch.clone()) + } } /// Build a real `DatafusionDlDriver` whose only reader (Parquet) serves `batch` @@ -228,6 +237,19 @@ impl FormatReader for SeekFormatReader { Ok(self.base.clone()) } } + + async fn read_segment_native( + &self, + uri: &str, + _offset: Option, + _length: Option, + ) -> Result { + if uri == "mem://sidecar" { + Ok(self.sidecar.clone()) + } else { + Ok(self.base.clone()) + } + } } /// A `DatafusionDlDriver` whose reader serves `base` + its `row_uuid` sidecar. From 75cb5674cff3daa805b6de51ac6c8e05db957e60 Mon Sep 17 00:00:00 2001 From: Nico Bautista Hobin Date: Fri, 31 Jul 2026 04:05:15 +0000 Subject: [PATCH 15/32] feat(query): key SegmentCache by content_hash MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The cache stored one decoded entry per metadata row uuid, so a fork and its parent held two copies of byte-identical cold data: snapshot carry-forward (CHA-531) and a fork's cold materialization (CHA-539) both mint a new row uuid over bytes nobody rewrote. Keying by the segment's `content_hash` — the digest of the typed in-memory batch, inherited verbatim by every reference copy — stores one entry per distinct content instead, so the second branch's read is a hit. All three cached artifact classes move together: snapshot segments, persist data segments and index sidecars. The uuids stay in the `#[tracing::instrument]` span fields, which is what correlates a log line to a metadata row; `content_hash` joins them rather than replacing them. CHA-545 --- crates/penca-dl/src/cache.rs | 72 ++++++++++++++++++++-------------- crates/penca-dl/src/driver.rs | 74 +++++++++++++++++++++-------------- 2 files changed, 87 insertions(+), 59 deletions(-) diff --git a/crates/penca-dl/src/cache.rs b/crates/penca-dl/src/cache.rs index a6eeb5b..c913dbc 100644 --- a/crates/penca-dl/src/cache.rs +++ b/crates/penca-dl/src/cache.rs @@ -2,15 +2,13 @@ //! //! A repeat read of the same cold segment within the process lifetime is served //! as an `Arc::clone` of the already-decoded Arrow batches, skipping the S3 GET + -//! Parquet/Lance decode. It holds both snapshot segments (keyed by -//! `table_snapshot_segment_uuid`) and persist data segments (keyed by -//! `segment_uuid`) under one byte budget. Each per-uuid segment file is -//! immutable, so the key→value mapping is stable and needs no invalidation: a -//! snapshot uuid resolves to identical bytes for the life of the process, and -//! although a *resolved persist tier* is mutable under retention compaction, an -//! individual persist *file* (keyed by uuid) is not. There is no TTL — -//! immutability makes W-TinyLFU eviction the whole reclaim mechanism for both -//! tiers. +//! Parquet/Lance decode. It holds snapshot segments, persist data segments and +//! index sidecars under one byte budget, all keyed by `content_hash`. The +//! mapping is stable and needs no invalidation: a hash names one decoded value +//! by construction, and cold artifacts are immutable — although a *resolved +//! persist tier* is mutable under retention compaction, an individual persist +//! *file* is not. There is no TTL — immutability makes W-TinyLFU eviction the +//! whole reclaim mechanism for every tier. //! //! Eviction is W-TinyLFU (frequency-based, scan-resistant, aged) via `moka`, //! bounded by a byte budget: each entry is weighed by its segment's @@ -31,16 +29,31 @@ use std::sync::Arc; use arrow::record_batch::RecordBatch; use moka::sync::Cache; -/// In-process W-TinyLFU cache of decoded cold segments, keyed by segment uuid -/// (`table_snapshot_segment_uuid` for snapshot, `segment_uuid` for persist) and -/// bounded by a byte budget. +/// In-process W-TinyLFU cache of decoded cold segments, keyed by `content_hash` +/// and bounded by a byte budget. /// -/// The key is the segment uuid alone: it is globally unique, immutable, and -/// resolves to identical bytes, so it fully identifies the decoded value. -/// `format` is intentionally absent from the key — it is functionally -/// determined by the uuid, and every value is a format-agnostic decoded -/// [`RecordBatch`]; `format` is consulted only on the miss path (by the -/// caller) to pick the reader. +/// `content_hash` is the digest of the typed in-memory Arrow batch, recorded +/// once at write time and inherited verbatim by every reference copy — snapshot +/// carry-forward (CHA-531) and a fork's cold materialization (CHA-539) both mint +/// a new row uuid over bytes nobody rewrote. Keying by uuid stored one decode +/// per *row*; keying by hash stores one per distinct *content*, which is what +/// lets a fork and its parent share a single entry for a shared slice (CHA-545). +/// +/// **The cached value must be the file-native decode**, not one caller's +/// shaping. A hash-keyed entry is shared across callers whose schemas can +/// differ — a fork that retypes a column still reads the parent's bytes — so a +/// caller-shaped value would hand the second caller the first caller's types. +/// Callers shape after the lookup via `penca_format::reader::shape_to_schema`; +/// `test_fork_and_parent_diverge_a_columns_type_over_one_shared_slice` is the +/// regression guard. +/// +/// One flat key space, no per-artifact-class prefix. With every artifact keyed +/// by a hash of its own typed content, 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. `format` is likewise absent from the +/// key: every value is a format-agnostic decoded [`RecordBatch`], and `format` +/// is consulted only on the miss path (by the caller) to pick the reader. /// /// Cheaply cloneable: `moka::sync::Cache` is internally an `Arc`, so callers /// typically hold a `SegmentCache` behind one outer `Arc` shared @@ -63,7 +76,7 @@ impl SegmentCache { .max_capacity(budget_bytes) // Weight in the same byte unit as `max_capacity` so the budget is a // real RAM bound; the stored `u32` is the entry's `size_bytes`. - .weigher(|_uuid: &String, (_batch, weight): &(Arc, u32)| *weight) + .weigher(|_hash: &String, (_batch, weight): &(Arc, u32)| *weight) .build(); Self { inner, @@ -103,21 +116,22 @@ impl SegmentCache { && weight_bytes <= u32::MAX as u64 } - /// Fetch a decoded segment by uuid, bumping its frequency estimate. A hit - /// is an `Arc::clone` — no buffer copy. - pub fn get(&self, uuid: &str) -> Option> { - self.inner.get(uuid).map(|(batch, _weight)| batch) + /// Fetch a decoded segment by content hash, bumping its frequency estimate. + /// A hit is an `Arc::clone` — no buffer copy. + pub fn get(&self, content_hash: &str) -> Option> { + self.inner.get(content_hash).map(|(batch, _weight)| batch) } - /// Insert a decoded segment under its uuid, charged `weight_bytes` against - /// the budget. No-op when the segment is not [`admits`](Self::admits)-ible. - /// moka enforces `max_capacity` via W-TinyLFU; there is no manual eviction - /// loop here. - pub fn insert(&self, uuid: String, batch: Arc, weight_bytes: u64) { + /// Insert a decoded segment under its content hash, charged `weight_bytes` + /// against the budget. No-op when the segment is not + /// [`admits`](Self::admits)-ible. moka enforces `max_capacity` via + /// W-TinyLFU; there is no manual eviction loop here. + pub fn insert(&self, content_hash: String, batch: Arc, weight_bytes: u64) { if !self.admits(weight_bytes) { return; } - self.inner.insert(uuid, (batch, weight_bytes as u32)); + self.inner + .insert(content_hash, (batch, weight_bytes as u32)); } /// Force pending eviction/maintenance to run synchronously. moka does diff --git a/crates/penca-dl/src/driver.rs b/crates/penca-dl/src/driver.rs index d0fa929..7602772 100644 --- a/crates/penca-dl/src/driver.rs +++ b/crates/penca-dl/src/driver.rs @@ -219,11 +219,7 @@ async fn read_and_cache_full( .await .map_err(ColdStorageError::from)?; let batch = Arc::new(batch); - cache.insert( - segment.table_snapshot_segment_uuid.clone(), - Arc::clone(&batch), - weight, - ); + cache.insert(segment.content_hash.to_string(), Arc::clone(&batch), weight); tracing::debug!( rows = batch.num_rows(), "snapshot segment cached full decode" @@ -274,6 +270,7 @@ async fn read_projected_uncached( skip_all, fields( segment_uuid = %segment.table_snapshot_segment_uuid, + content_hash = %segment.content_hash, format = %segment.format, cache = tracing::field::Empty, ), @@ -286,9 +283,8 @@ pub(crate) async fn read_cached_snapshot_segment( out_schema: &SchemaRef, ) -> Result { let span = tracing::Span::current(); - let uuid = segment.table_snapshot_segment_uuid.as_str(); - if let Some(full) = cache.get(uuid) { + if let Some(full) = cache.get(&segment.content_hash.to_string()) { span.record("cache", "hit"); tracing::debug!(rows = full.num_rows(), "snapshot segment cache hit"); return Ok(shape_to_schema(&full, full_schema, None).map_err(ColdStorageError::from)?); @@ -326,7 +322,7 @@ async fn read_and_cache_full_persist( .await .map_err(ColdStorageError::from)?; let batch = Arc::new(batch); - cache.insert(segment.segment_uuid.clone(), Arc::clone(&batch), weight); + cache.insert(segment.content_hash.to_string(), Arc::clone(&batch), weight); tracing::debug!( rows = batch.num_rows(), "persist segment decoded and cached" @@ -390,20 +386,20 @@ async fn read_projected_uncached_persist( } /// Cache-aware read of a single persist segment. A persist segment file is -/// immutable once written and keyed by its globally-unique `segment_uuid`, so it -/// shares the process-lifetime [`SegmentCache`] with snapshot segments under one -/// byte budget, with NO TTL — W-TinyLFU eviction plus the `admits` budget gate -/// is the whole mechanism. (The *resolved* persist tier is mutable under -/// retention compaction, which is why the tier is re-resolved live on every -/// read; the per-uuid *file bytes* this caches are not.) Returns the full -/// decoded superset on hit / miss-cached, or the projected `out_schema` batch on -/// the non-cacheable (oversized) path; the caller projects / null-fills -/// downstream. +/// immutable once written, so it shares the process-lifetime [`SegmentCache`] +/// with snapshot segments under one byte budget and one `content_hash` key +/// space, with NO TTL — W-TinyLFU eviction plus the `admits` budget gate is the +/// whole mechanism. (The *resolved* persist tier is mutable under retention +/// compaction, which is why the tier is re-resolved live on every read; the +/// *file bytes* this caches are not.) Returns the full decoded superset on hit / +/// miss-cached, or the projected `out_schema` batch on the non-cacheable +/// (oversized) path; the caller projects / null-fills downstream. #[tracing::instrument( level = "debug", skip_all, fields( segment_uuid = %segment.segment_uuid, + content_hash = %segment.content_hash, format = %segment.format, cache = tracing::field::Empty, ), @@ -416,9 +412,8 @@ pub(crate) async fn read_cached_persist_segment( out_schema: &SchemaRef, ) -> Result { let span = tracing::Span::current(); - let uuid = segment.segment_uuid.as_str(); - if let Some(full) = cache.get(uuid) { + if let Some(full) = cache.get(&segment.content_hash.to_string()) { span.record("cache", "hit"); tracing::debug!(rows = full.num_rows(), "persist segment cache hit"); return Ok(shape_to_schema(&full, full_schema, None).map_err(ColdStorageError::from)?); @@ -444,8 +439,9 @@ pub(crate) async fn read_cached_persist_segment( } /// Load a sorted `(key, row_offset)` index sidecar through the shared snapshot -/// cache, keyed by its own `segment_index_uuid` — a distinct deterministic-UUID -/// namespace from the base segment uuid, so the two never collide in one cache. +/// cache, keyed by the sidecar's own `content_hash` — a digest of the sidecar +/// batch, which differs from any base segment's, so the two never collide in one +/// cache. /// /// Cached natively and shaped to `key_types` after the lookup, for the same /// reason as a base segment: the file was written with the *writing* branch's @@ -457,6 +453,7 @@ pub(crate) async fn read_cached_persist_segment( skip_all, fields( segment_index_uuid = %sidecar.segment_index_uuid, + content_hash = %sidecar.content_hash, cache = tracing::field::Empty, ), )] @@ -470,7 +467,7 @@ async fn read_cached_index_sidecar( // The sidecar's key schema is the indexed columns' native types; the // identity/name sidecars are the all-Utf8 special case. let schema = penca_format::index::segment_index_schema(key_types); - if let Some(batch) = cache.get(&sidecar.segment_index_uuid) { + if let Some(batch) = cache.get(&sidecar.content_hash.to_string()) { span.record("cache", "hit"); return Ok(shape_to_schema(&batch, &schema, None).map_err(ColdStorageError::from)?); } @@ -491,7 +488,7 @@ async fn read_cached_index_sidecar( // `insert` self-gates on `cache.admits(weight)`, so an oversize sidecar is // decoded-but-not-cached rather than evicting the whole budget. cache.insert( - sidecar.segment_index_uuid.clone(), + sidecar.content_hash.to_string(), Arc::clone(&batch), sidecar.size_bytes.max(0) as u64, ); @@ -1861,14 +1858,21 @@ mod tests { assert_eq!(first, second, "both rows resolve to the same decoded batch"); } - /// `CountingFormatReader::read_segment` ignores its `schema` argument and - /// returns a fixed batch, so no sidecar-shaped fixture is needed — the - /// `key_types` slice only has to reach the reader. + /// The fixture is sidecar-shaped because the sidecar read shapes its cached + /// native batch to `segment_index_schema(key_types)` after the lookup. #[tokio::test] async fn index_sidecars_sharing_content_hash_decode_once() { - let schema = test_schema(); + let schema = penca_format::index::segment_index_schema(&[DataType::Utf8]); + let batch = RecordBatch::try_new( + schema.clone(), + vec![ + Arc::new(StringArray::from(vec!["k0", "k1", "k2"])), + Arc::new(arrow::array::Int64Array::from(vec![0i64, 1, 2])), + ], + ) + .unwrap(); let cache = Arc::new(SegmentCache::new(1 << 20)); - let (dl, reads) = driver_with(cache, test_batch(&schema)); + let (dl, reads) = driver_with(cache, batch); let shared = Uuid::from_u128(0xbeef); let sidecar = |uuid: &str| IndexSidecar { @@ -1921,7 +1925,10 @@ mod tests { 2, "oversized segment is never cached — both accesses re-read storage" ); - assert!(cache.get("big").is_none(), "oversized segment not stored"); + assert!( + cache.get(&seg.content_hash.to_string()).is_none(), + "oversized segment not stored" + ); } #[tokio::test] @@ -1941,7 +1948,14 @@ mod tests { // moka evicted one of {a,b} to honor the budget (we don't assert which // — that is moka's W-TinyLFU choice). Re-reading the evicted key must // hit storage again. - let evicted = if cache.get("a").is_none() { "a" } else { "b" }; + let evicted = if cache + .get(&segment("a", 150).content_hash.to_string()) + .is_none() + { + "a" + } else { + "b" + }; read_seg(&dl, &segment(evicted, 150), &schema, &schema) .await .unwrap(); From 643583398be554cf0b5f439cdb4bd74b496171ea Mon Sep 17 00:00:00 2001 From: Nico Bautista Hobin Date: Fri, 31 Jul 2026 04:13:09 +0000 Subject: [PATCH 16/32] perf(query): take matched rows before shaping on the index-seek path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Shaping moved behind the cache lookup so each caller gets its own types, but `take_matched_rows` then shaped the whole cached segment to `full_schema` before narrowing it to the matched offsets. For a segment written before an `ALTER TABLE ADD COLUMN`, that allocates a segment-length null array on every probe — O(rows) work reintroduced on the path whose whole point is O(matches). Split the cache-aware read into an unshaped `CachedSegment` so the seek path can `take` first and null-fill only the taken rows. The scan path is unchanged: it needs every row shaped anyway. CHA-545 --- crates/penca-dl/src/driver.rs | 124 +++++++++++++++++++++++++++++----- 1 file changed, 106 insertions(+), 18 deletions(-) diff --git a/crates/penca-dl/src/driver.rs b/crates/penca-dl/src/driver.rs index 7602772..be47459 100644 --- a/crates/penca-dl/src/driver.rs +++ b/crates/penca-dl/src/driver.rs @@ -257,14 +257,25 @@ async fn read_projected_uncached( Ok(batch) } -/// Cache-aware read of a single snapshot segment. Returns the full decoded -/// superset on hit / miss-cached, or the projected `out_schema` batch on the -/// non-cacheable (oversized) path. No predicate is pushed (ADR 0023); the -/// caller projects / null-fills downstream. +/// One cache-aware segment read, before any shaping — the two outcomes differ +/// in what shaping they still owe. +enum CachedSegment { + /// The file-native decode, cached (or freshly cached) as-is. Still owes + /// `shape_to_schema` to the caller's `full_schema`; deferring that to the + /// caller is what lets the index-seek path `take` its O(matches) rows + /// first and null-fill only those. + Native(Arc), + /// The non-cacheable (oversized) path, already read projected to + /// `out_schema`. Owes nothing. + Projected(RecordBatch), +} + +/// Cache-aware read of a single snapshot segment, unshaped. No predicate is +/// pushed (ADR 0023); the caller projects / null-fills downstream. /// -/// The cached value is the file-native decode; shaping it to `full_schema` -/// happens here, after the lookup, so two branches sharing one entry each get -/// their own types rather than whichever branch decoded first (CHA-545). +/// The cached value is the file-native decode, so shaping happens after the +/// lookup and two branches sharing one entry each get their own types rather +/// than whichever branch decoded first (CHA-545). #[tracing::instrument( level = "debug", skip_all, @@ -275,19 +286,18 @@ async fn read_projected_uncached( cache = tracing::field::Empty, ), )] -pub(crate) async fn read_cached_snapshot_segment( +async fn read_cached_snapshot_segment_unshaped( readers: &HashMap, cache: &SegmentCache, segment: &SnapshotSegment, - full_schema: &SchemaRef, out_schema: &SchemaRef, -) -> Result { +) -> Result { let span = tracing::Span::current(); if let Some(full) = cache.get(&segment.content_hash.to_string()) { span.record("cache", "hit"); tracing::debug!(rows = full.num_rows(), "snapshot segment cache hit"); - return Ok(shape_to_schema(&full, full_schema, None).map_err(ColdStorageError::from)?); + return Ok(CachedSegment::Native(full)); } let code = segment.format.as_wire_code(); @@ -300,11 +310,32 @@ pub(crate) async fn read_cached_snapshot_segment( if cache.admits(weight) { span.record("cache", "miss-cached"); - let native = read_and_cache_full(reader, cache, segment, weight).await?; - Ok(shape_to_schema(&native, full_schema, None).map_err(ColdStorageError::from)?) + Ok(CachedSegment::Native( + read_and_cache_full(reader, cache, segment, weight).await?, + )) } else { span.record("cache", "miss-uncached"); - read_projected_uncached(reader, segment, out_schema).await + Ok(CachedSegment::Projected( + read_projected_uncached(reader, segment, out_schema).await?, + )) + } +} + +/// Cache-aware read of a single snapshot segment. Returns the full decoded +/// superset on hit / miss-cached, or the projected `out_schema` batch on the +/// non-cacheable (oversized) path. +pub(crate) async fn read_cached_snapshot_segment( + readers: &HashMap, + cache: &SegmentCache, + segment: &SnapshotSegment, + full_schema: &SchemaRef, + out_schema: &SchemaRef, +) -> Result { + match read_cached_snapshot_segment_unshaped(readers, cache, segment, out_schema).await? { + CachedSegment::Native(native) => { + Ok(shape_to_schema(&native, full_schema, None).map_err(ColdStorageError::from)?) + } + CachedSegment::Projected(batch) => Ok(batch), } } @@ -552,6 +583,11 @@ async fn seek_entry_offsets( /// decode entirely on zero matches. A candidate segment that passes coarse /// pruning but doesn't contain the probed key must not pay a full base /// decode just to `take` zero rows (the common cross-segment-lookup miss). +/// +/// `take` runs BEFORE the shaping tail so the seek path stays O(matches): a +/// segment written before an `ALTER TABLE ADD COLUMN` null-fills the added +/// column, and null-filling the whole cached batch first would allocate a +/// segment-length array on every probe. async fn take_matched_rows( readers: &HashMap, cache: &SegmentCache, @@ -563,11 +599,14 @@ async fn take_matched_rows( if offsets.is_empty() { return Ok(RecordBatch::new_empty(full_schema.clone())); } - let base = - read_cached_snapshot_segment(readers, cache, segment, full_schema, out_schema).await?; let indices = arrow::array::Int64Array::from(offsets); - let taken = arrow::compute::take_record_batch(&base, &indices)?; - Ok(taken) + match read_cached_snapshot_segment_unshaped(readers, cache, segment, out_schema).await? { + CachedSegment::Native(native) => { + let taken = arrow::compute::take_record_batch(&native, &indices)?; + Ok(shape_to_schema(&taken, full_schema, None).map_err(ColdStorageError::from)?) + } + CachedSegment::Projected(batch) => Ok(arrow::compute::take_record_batch(&batch, &indices)?), + } } /// Seek SEVERAL resolved entries against one segment and decode the @@ -1169,6 +1208,55 @@ mod tests { assert_eq!(res, expected); } + /// The seek path `take`s the matched offsets before it null-fills, so this + /// pins that the added column's nulls land on the taken rows and the other + /// columns stay aligned with them. + #[tokio::test] + async fn seek_snapshot_point_null_fills_a_column_absent_from_the_file() { + let cache = Arc::new(SegmentCache::new(1 << 20)); + let mut by_uri = HashMap::new(); + let seg = indexed_segment( + &mut by_uri, + &test_schema(), // what the segment file actually holds + "added", + &["r0", "r1", "r2"], + &[0, 1, 2], + ); + let dl = routing_driver(cache, by_uri); + + // `added` arrived via a later ALTER TABLE ADD COLUMN: it is in the + // table schema but not in this segment's file. + let full_schema: SchemaRef = Arc::new(Schema::new(vec![ + Field::new("row_uuid", DataType::Utf8, false), + Field::new("v", DataType::Int32, false), + Field::new("added", DataType::Int32, true), + ])); + + let res = dl + .seek_snapshot_point( + std::slice::from_ref(&seg), + &[vec!["r1".to_string()]], + None, + &[], + &full_schema, + &full_schema, + ) + .await + .unwrap() + .expect("indexed segment => Some"); + + let expected = RecordBatch::try_new( + full_schema.clone(), + vec![ + Arc::new(StringArray::from(vec!["r1"])), + Arc::new(Int32Array::from(vec![1])), + Arc::new(Int32Array::from(vec![None::])), + ], + ) + .unwrap(); + assert_eq!(res, expected); + } + #[tokio::test] async fn seek_snapshot_point_empty_segments_yields_empty_batch() { let cache = Arc::new(SegmentCache::new(1 << 20)); From 1a16aab67e4338230b6e3351b1cd860e413aeaaa Mon Sep 17 00:00:00 2001 From: Nico Bautista Hobin Date: Fri, 31 Jul 2026 05:08:11 +0000 Subject: [PATCH 17/32] docs(cold): record the segment-cache content-hash decision MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CHA-545's acceptance criterion is a decision, not a behavior: a content hash on every cold-artifact metadata row is non-obvious complexity, so record why it exists where a future reader finds it — including the two points most likely to be re-litigated, why the digest is over the typed batch rather than the encoded file bytes, and why index sidecars are in scope alongside base segments. CHA-545 --- docs/design-decisions.md | 54 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/docs/design-decisions.md b/docs/design-decisions.md index c82e70d..ef5f1da 100644 --- a/docs/design-decisions.md +++ b/docs/design-decisions.md @@ -180,3 +180,57 @@ reads footers, `register_listing_table` hits object storage, Tracked upstream: a real `clone_ctx()` in `datafusion-python` would let Python match Rust's mechanics exactly; until then the factory is the correct shape. + +--- + +## Cold segments are cached by content hash, not by row uuid + +Every cold-artifact metadata row — persist segments, snapshot segments, and +segment index sidecars — 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 it. + +**The problem.** Reference copies mint a new row uuid over an unchanged +`(object_uri, offset, length)`: `insert_carried_snapshot_segments` and +`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. + +**Why hash the decoded batch, not the encoded file bytes.** Two rows may +reference the same byte slice under different schemas — that is exactly what a +fork that `ALTER`s a column produces. Hashing the file bytes would collide them +and hand one caller the other's types. Hashing the typed batch keeps the key a +name for one decoded value. + +The same argument forced the cached *value* to be 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` plus `reader::shape_to_schema`. A +caller-shaped entry carries +the schema of whichever branch decoded first, so the second branch's read fails +on a type mismatch its own metadata never justified. + +**Why the scope is uniform across artifact classes.** Base segments and index +sidecars both come out of object storage through the same `SegmentCache` and are +both reference-copied by carry-forward and fork copy, so they duplicate for the +same reason and dedup the same way. Sidecars were briefly scoped out on the +argument that `segment_index_schema(key_types)` makes the cached value +caller-dependent — but that argument is wrong for the same reason it is wrong +for base segments, and the native-decode split answers it. + +`content_hash` is `NOT NULL` with no default on all three tables +(`penca-db/src/dialect/pg.rs`). Every writer computes it and a catalog predating +the column is recreated rather than migrated, so there is no legacy row to +default and no uuid-fallback key space to keep alive. + +**Non-goal.** `tx_log_persist_segment_metadata` has no `content_hash`: it is +never cache-read and never reference-copied. + +**Open question.** CHA-545 names checksum reuse 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 it, and Parquet may widen a type or +re-dictionary-encode on round-trip, so using it as a stored-file checksum needs +round-trip stability established first. From f401463da34967b85598c9e8dd504dcdc0a51f51 Mon Sep 17 00:00:00 2001 From: Nico Bautista Hobin Date: Fri, 31 Jul 2026 05:39:07 +0000 Subject: [PATCH 18/32] refactor(cold): key SegmentCache by Uuid instead of a stringified hash MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit content_hash is a Uuid everywhere it is defined (plan.rs) and stored (UUID NOT NULL in all three cold-artifact tables). SegmentCache was the one boundary that downgraded it to a String, so every crossing rendered a 16-byte value as a 36-char hex string — cache.get allocated a String per segment on every cold read, hit or miss, purely to index a hash map. Key the moka cache by Uuid directly. The borrowed-get / owned-insert split is unchanged (moka owns the key on insert; &Uuid on lookup matches the workspace convention for Uuid parameters), so the only call-site change is dropping .to_string(). CHA-545 --- crates/penca-dl/src/cache.rs | 41 ++++++++++++++++++++++------------- crates/penca-dl/src/driver.rs | 19 +++++++--------- 2 files changed, 34 insertions(+), 26 deletions(-) diff --git a/crates/penca-dl/src/cache.rs b/crates/penca-dl/src/cache.rs index c913dbc..2fcb564 100644 --- a/crates/penca-dl/src/cache.rs +++ b/crates/penca-dl/src/cache.rs @@ -28,6 +28,7 @@ use std::sync::Arc; use arrow::record_batch::RecordBatch; use moka::sync::Cache; +use uuid::Uuid; /// In-process W-TinyLFU cache of decoded cold segments, keyed by `content_hash` /// and bounded by a byte budget. @@ -61,7 +62,7 @@ use moka::sync::Cache; pub struct SegmentCache { /// Value carries its own weight so the weigher can charge the /// caller-supplied `size_bytes` rather than the batch's runtime memory. - inner: Cache, u32)>, + inner: Cache, u32)>, budget_bytes: u64, } @@ -76,7 +77,7 @@ impl SegmentCache { .max_capacity(budget_bytes) // Weight in the same byte unit as `max_capacity` so the budget is a // real RAM bound; the stored `u32` is the entry's `size_bytes`. - .weigher(|_hash: &String, (_batch, weight): &(Arc, u32)| *weight) + .weigher(|_hash: &Uuid, (_batch, weight): &(Arc, u32)| *weight) .build(); Self { inner, @@ -118,7 +119,7 @@ impl SegmentCache { /// Fetch a decoded segment by content hash, bumping its frequency estimate. /// A hit is an `Arc::clone` — no buffer copy. - pub fn get(&self, content_hash: &str) -> Option> { + pub fn get(&self, content_hash: &Uuid) -> Option> { self.inner.get(content_hash).map(|(batch, _weight)| batch) } @@ -126,7 +127,7 @@ impl SegmentCache { /// against the budget. No-op when the segment is not /// [`admits`](Self::admits)-ible. moka enforces `max_capacity` via /// W-TinyLFU; there is no manual eviction loop here. - pub fn insert(&self, content_hash: String, batch: Arc, weight_bytes: u64) { + pub fn insert(&self, content_hash: Uuid, batch: Arc, weight_bytes: u64) { if !self.admits(weight_bytes) { return; } @@ -157,6 +158,7 @@ mod tests { use arrow::array::Int32Array; use arrow::datatypes::{DataType, Field, Schema}; use arrow::record_batch::RecordBatch; + use uuid::Uuid; use super::SegmentCache; @@ -185,10 +187,10 @@ mod tests { #[test] fn zero_weight_segment_is_not_cached() { let cache = SegmentCache::new(1 << 20); - cache.insert("zero".into(), batch(4), 0); + cache.insert(Uuid::from_u128(1), batch(4), 0); cache.run_pending(); assert!( - cache.get("zero").is_none(), + cache.get(&Uuid::from_u128(1)).is_none(), "weight-0 segment must not be pinned in the cache" ); } @@ -208,22 +210,31 @@ mod tests { ); assert!(cache.admits(u32::MAX as u64), "exactly u32::MAX is fine"); - cache.insert("huge".into(), batch(8), over_u32); + cache.insert(Uuid::from_u128(2), batch(8), over_u32); cache.run_pending(); - assert!(cache.get("huge").is_none(), "over-u32 weight never stored"); + assert!( + cache.get(&Uuid::from_u128(2)).is_none(), + "over-u32 weight never stored" + ); } #[test] fn over_budget_insert_is_noop() { let cache = SegmentCache::new(100); - cache.insert("too-big".into(), batch(8), 200); + cache.insert(Uuid::from_u128(3), batch(8), 200); cache.run_pending(); - assert!(cache.get("too-big").is_none(), "over-budget never stored"); + assert!( + cache.get(&Uuid::from_u128(3)).is_none(), + "over-budget never stored" + ); let disabled = SegmentCache::disabled(); - disabled.insert("x".into(), batch(8), 1); + disabled.insert(Uuid::from_u128(4), batch(8), 1); disabled.run_pending(); - assert!(disabled.get("x").is_none(), "disabled never stores"); + assert!( + disabled.get(&Uuid::from_u128(4)).is_none(), + "disabled never stores" + ); } #[test] @@ -234,7 +245,7 @@ mod tests { // moka's W-TinyLFU choice, not Penca's contract). let cache = SegmentCache::new(100); for i in 0..5 { - cache.insert(format!("seg-{i}"), batch(10), 40); + cache.insert(Uuid::from_u128(i), batch(10), 40); } cache.run_pending(); assert!( @@ -248,9 +259,9 @@ mod tests { fn hit_returns_arc_clone_same_buffers() { let cache = SegmentCache::new(1_000); let original = batch(16); - cache.insert("seg".into(), original.clone(), 40); + cache.insert(Uuid::from_u128(5), original.clone(), 40); cache.run_pending(); - let hit = cache.get("seg").expect("cached"); + let hit = cache.get(&Uuid::from_u128(5)).expect("cached"); // Same backing column buffer — a hit is a refcount bump, no copy. assert_eq!( original.column(0).to_data().buffers()[0].as_ptr(), diff --git a/crates/penca-dl/src/driver.rs b/crates/penca-dl/src/driver.rs index be47459..8edfc10 100644 --- a/crates/penca-dl/src/driver.rs +++ b/crates/penca-dl/src/driver.rs @@ -219,7 +219,7 @@ async fn read_and_cache_full( .await .map_err(ColdStorageError::from)?; let batch = Arc::new(batch); - cache.insert(segment.content_hash.to_string(), Arc::clone(&batch), weight); + cache.insert(segment.content_hash, Arc::clone(&batch), weight); tracing::debug!( rows = batch.num_rows(), "snapshot segment cached full decode" @@ -294,7 +294,7 @@ async fn read_cached_snapshot_segment_unshaped( ) -> Result { let span = tracing::Span::current(); - if let Some(full) = cache.get(&segment.content_hash.to_string()) { + if let Some(full) = cache.get(&segment.content_hash) { span.record("cache", "hit"); tracing::debug!(rows = full.num_rows(), "snapshot segment cache hit"); return Ok(CachedSegment::Native(full)); @@ -353,7 +353,7 @@ async fn read_and_cache_full_persist( .await .map_err(ColdStorageError::from)?; let batch = Arc::new(batch); - cache.insert(segment.content_hash.to_string(), Arc::clone(&batch), weight); + cache.insert(segment.content_hash, Arc::clone(&batch), weight); tracing::debug!( rows = batch.num_rows(), "persist segment decoded and cached" @@ -444,7 +444,7 @@ pub(crate) async fn read_cached_persist_segment( ) -> Result { let span = tracing::Span::current(); - if let Some(full) = cache.get(&segment.content_hash.to_string()) { + if let Some(full) = cache.get(&segment.content_hash) { span.record("cache", "hit"); tracing::debug!(rows = full.num_rows(), "persist segment cache hit"); return Ok(shape_to_schema(&full, full_schema, None).map_err(ColdStorageError::from)?); @@ -498,7 +498,7 @@ async fn read_cached_index_sidecar( // The sidecar's key schema is the indexed columns' native types; the // identity/name sidecars are the all-Utf8 special case. let schema = penca_format::index::segment_index_schema(key_types); - if let Some(batch) = cache.get(&sidecar.content_hash.to_string()) { + if let Some(batch) = cache.get(&sidecar.content_hash) { span.record("cache", "hit"); return Ok(shape_to_schema(&batch, &schema, None).map_err(ColdStorageError::from)?); } @@ -519,7 +519,7 @@ async fn read_cached_index_sidecar( // `insert` self-gates on `cache.admits(weight)`, so an oversize sidecar is // decoded-but-not-cached rather than evicting the whole budget. cache.insert( - sidecar.content_hash.to_string(), + sidecar.content_hash, Arc::clone(&batch), sidecar.size_bytes.max(0) as u64, ); @@ -2014,7 +2014,7 @@ mod tests { "oversized segment is never cached — both accesses re-read storage" ); assert!( - cache.get(&seg.content_hash.to_string()).is_none(), + cache.get(&seg.content_hash).is_none(), "oversized segment not stored" ); } @@ -2036,10 +2036,7 @@ mod tests { // moka evicted one of {a,b} to honor the budget (we don't assert which // — that is moka's W-TinyLFU choice). Re-reading the evicted key must // hit storage again. - let evicted = if cache - .get(&segment("a", 150).content_hash.to_string()) - .is_none() - { + let evicted = if cache.get(&segment("a", 150).content_hash).is_none() { "a" } else { "b" From 7043699d57d588aef857da478d9180fecc99722b Mon Sep 17 00:00:00 2001 From: Nico Bautista Hobin Date: Fri, 31 Jul 2026 05:42:50 +0000 Subject: [PATCH 19/32] refactor(cold): extract requested_columns from the format readers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both FormatReader::read_segment impls opened with a byte-identical four-line match resolving 'projection when given, otherwise every column in schema' — a rule the trait documents once (reader/mod.rs) but that was implemented twice, with nothing forcing the two to stay in lockstep. Lift it into reader/mod.rs beside present_columns, which is the same shape: a pub(crate) reader helper with the same two call sites. CHA-545 --- crates/penca-format/src/reader/lance.rs | 8 +++---- crates/penca-format/src/reader/mod.rs | 29 +++++++++++++++++++++++ crates/penca-format/src/reader/parquet.rs | 8 +++---- 3 files changed, 35 insertions(+), 10 deletions(-) diff --git a/crates/penca-format/src/reader/lance.rs b/crates/penca-format/src/reader/lance.rs index e99b2bb..27c0895 100644 --- a/crates/penca-format/src/reader/lance.rs +++ b/crates/penca-format/src/reader/lance.rs @@ -16,7 +16,8 @@ use lance_io::ReadBatchParams; use lance_io::scheduler::{ScanScheduler, SchedulerConfig}; use super::{ - FormatError, FormatReader, empty_batch, present_columns, project_schema, shape_to_schema, + FormatError, FormatReader, empty_batch, present_columns, project_schema, requested_columns, + shape_to_schema, }; use crate::uri::uri_to_object_path; @@ -192,10 +193,7 @@ impl FormatReader for LanceFormatReader { schema: &SchemaRef, projection: Option<&[&str]>, ) -> Result { - let column_names: Vec<&str> = match projection { - Some(cols) => cols.to_vec(), - None => schema.fields().iter().map(|f| f.name().as_str()).collect(), - }; + let column_names = requested_columns(schema, projection); let present = self .read_in_file_schema(uri, offset, length, Some(&column_names)) .await?; diff --git a/crates/penca-format/src/reader/mod.rs b/crates/penca-format/src/reader/mod.rs index 814b89c..9222ff0 100644 --- a/crates/penca-format/src/reader/mod.rs +++ b/crates/penca-format/src/reader/mod.rs @@ -215,6 +215,20 @@ pub(crate) fn null_fill_to_schema( Ok(RecordBatch::try_new(output_schema.clone(), columns)?) } +/// The columns a segment read should request from the file: the projection when +/// given, otherwise every column in `schema`. The `None` case is what makes a +/// projection-less [`FormatReader::read_segment`] return the caller's whole +/// schema, null-filling any column the file predates. +pub(crate) fn requested_columns<'a>( + schema: &'a SchemaRef, + projection: Option<&[&'a str]>, +) -> Vec<&'a str> { + match projection { + Some(cols) => cols.to_vec(), + None => schema.fields().iter().map(|f| f.name().as_str()).collect(), + } +} + /// The subset of `names` that actually exist as columns in `file_schema`, /// preserving the requested order. Used by readers to project only the columns /// physically present in a segment before null-filling the rest. @@ -263,6 +277,21 @@ mod tests { assert!(matches!(err, FormatError::UnknownProjectionColumn(name) if name == "zzz")); } + #[test] + fn requested_columns_passes_projection_through_and_expands_none() { + let schema = test_schema(); + assert_eq!( + requested_columns(&schema, Some(&["c", "a"])), + vec!["c", "a"], + "a projection is returned verbatim, in the requested order" + ); + assert_eq!( + requested_columns(&schema, None), + vec!["a", "b", "c"], + "no projection means every column in the caller's schema" + ); + } + #[test] fn present_columns_keeps_only_existing_in_file_order() { // file has {a, b}; request {b, missing, a} -> keep {b, a} in request order. diff --git a/crates/penca-format/src/reader/parquet.rs b/crates/penca-format/src/reader/parquet.rs index e16c0b9..c8e048f 100644 --- a/crates/penca-format/src/reader/parquet.rs +++ b/crates/penca-format/src/reader/parquet.rs @@ -18,7 +18,8 @@ use parquet::arrow::arrow_reader::{RowSelection, RowSelector}; use parquet::arrow::async_reader::{ParquetObjectReader, ParquetRecordBatchStreamBuilder}; use super::{ - FormatError, FormatReader, empty_batch, present_columns, project_schema, shape_to_schema, + FormatError, FormatReader, empty_batch, present_columns, project_schema, requested_columns, + shape_to_schema, }; use crate::uri::uri_to_object_path; @@ -128,10 +129,7 @@ impl FormatReader for ParquetFormatReader { schema: &SchemaRef, projection: Option<&[&str]>, ) -> Result { - let column_names: Vec<&str> = match projection { - Some(cols) => cols.to_vec(), - None => schema.fields().iter().map(|f| f.name().as_str()).collect(), - }; + let column_names = requested_columns(schema, projection); let present = self .read_in_file_schema(uri, offset, length, Some(&column_names)) .await?; From b1c4ac89b8a3c20129790137a457557170f01f2d Mon Sep 17 00:00:00 2001 From: Nico Bautista Hobin Date: Fri, 31 Jul 2026 05:45:38 +0000 Subject: [PATCH 20/32] docs(cold): correct what the content hash does and does not separate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The rationale claimed hashing the typed batch is what keeps a fork's rows from colliding with the parent's over a shared byte slice. It is not: every reference-copy path selects old.content_hash verbatim (fork_copy.rs, snapshot.rs, segment_index.rs) and it is never recomputed, so parent and fork carry the SAME hash under any scheme — that inheritance is the dedup. No digest can separate them because neither row's was taken under its own read schema. That contradicted the very next paragraph, and a reader who believed it could conclude the native-decode split is redundant and reintroduce the cross-branch type mismatch it exists to prevent. State the property the digest actually buys (equal hash means equal decode, plus dedup that survives re-encoding) and name the split as the schema-divergence answer. Same correction in segment_content_hash's own doc. CHA-545 --- crates/penca-core/src/digest.rs | 19 ++++++++++++++----- crates/penca-dl/src/cache.rs | 4 ++-- docs/design-decisions.md | 31 +++++++++++++++++++++---------- 3 files changed, 37 insertions(+), 17 deletions(-) diff --git a/crates/penca-core/src/digest.rs b/crates/penca-core/src/digest.rs index 9fa6134..a8a1f2a 100644 --- a/crates/penca-core/src/digest.rs +++ b/crates/penca-core/src/digest.rs @@ -13,11 +13,20 @@ const IPC_ALIGNMENT: usize = 8; /// `xxh3_128` of a segment's **typed in-memory Arrow batch**, as a [`Uuid`]. /// -/// The input is the decoded batch, never the encoded file bytes. That is what -/// makes the digest schema-sensitive: two segments referencing the same -/// `(object_uri, offset, length)` slice under different schemas must hash -/// differently, because they are two different decoded objects and a cache -/// keyed by this value would otherwise hand one caller the other's types. +/// The input is the decoded batch, never the encoded file bytes, so everything +/// that distinguishes two decoded batches is in the digest: values, types, column +/// names, nullability, column order, row count. That is the soundness property a +/// cache keyed by this value needs — equal hash must mean equal decode, or two +/// unrelated segments would share one entry. +/// +/// It is **not** a defence against a reader whose schema differs from the +/// writer's. A reference copy inherits this value verbatim (`fork_copy`, snapshot +/// carry-forward) and it is never recomputed, so a fork that `ALTER`s a column +/// still reads the parent's bytes under the parent's hash — no digest could +/// separate the two, because neither row's was ever taken under its own read +/// schema. Serving both from one entry safely is the cached *value's* job: it +/// holds the file-native decode and callers shape after the lookup. See +/// `penca_dl::cache::SegmentCache`. /// /// **Write-time only.** A digest taken before the format writer encodes a batch /// is *not* guaranteed to equal the digest of a later decode of that file: diff --git a/crates/penca-dl/src/cache.rs b/crates/penca-dl/src/cache.rs index 2fcb564..7ac1176 100644 --- a/crates/penca-dl/src/cache.rs +++ b/crates/penca-dl/src/cache.rs @@ -4,8 +4,8 @@ //! as an `Arc::clone` of the already-decoded Arrow batches, skipping the S3 GET + //! Parquet/Lance decode. It holds snapshot segments, persist data segments and //! index sidecars under one byte budget, all keyed by `content_hash`. The -//! mapping is stable and needs no invalidation: a hash names one decoded value -//! by construction, and cold artifacts are immutable — although a *resolved +//! mapping is stable and needs no invalidation: a hash names one file-native +//! decode by construction, and cold artifacts are immutable — although a *resolved //! persist tier* is mutable under retention compaction, an individual persist //! *file* is not. There is no TTL — immutability makes W-TinyLFU eviction the //! whole reclaim mechanism for every tier. diff --git a/docs/design-decisions.md b/docs/design-decisions.md index ef5f1da..429ea5a 100644 --- a/docs/design-decisions.md +++ b/docs/design-decisions.md @@ -199,19 +199,30 @@ decodes of one byte range — N× the memory under a fixed byte budget, and N co 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. -**Why hash the decoded batch, not the encoded file bytes.** Two rows may -reference the same byte slice under different schemas — that is exactly what a -fork that `ALTER`s a column produces. Hashing the file bytes would collide them -and hand one caller the other's types. Hashing the typed batch keeps the key a -name for one decoded value. - -The same argument forced the cached *value* to be the file-native decode, with +**Why hash the decoded batch, not the encoded file bytes.** The key has to name +one decoded batch — equal hash must mean equal decode, or two unrelated segments +would share an entry. Hashing the typed batch gives that directly, and it is what +is available where the digest is taken: write time, *before* the format writer +encodes. Hashing the stored object would mean reading back what was just written. +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. + +**What the key deliberately does *not* do is separate a reference copy from its +source.** Carry-forward and fork copy select `old.content_hash` verbatim +(`fork_copy.rs`, `snapshot.rs`, `segment_index.rs`) and it is never recomputed — +that inheritance *is* the dedup. So when a fork `ALTER`s 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, and no hashing scheme could +separate them, because neither row's digest was ever taken under its own read +schema. + +That is what forces the cached *value* to be 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` plus `reader::shape_to_schema`. A -caller-shaped entry carries -the schema of whichever branch decoded first, so the second branch's read fails -on a type mismatch its own metadata never justified. +caller-shaped entry carries the schema of whichever branch decoded first, so the +second branch's read fails on a type mismatch its own metadata never justified. **Why the scope is uniform across artifact classes.** Base segments and index sidecars both come out of object storage through the same `SegmentCache` and are From e1fe2e501d91f1fa12a6540a49e26d75625ef369 Mon Sep 17 00:00:00 2001 From: Nico Bautista Hobin Date: Fri, 31 Jul 2026 05:48:04 +0000 Subject: [PATCH 21/32] refactor(cold): name the projection-less shaping call in the driver Six sites repeated shape_to_schema(.., None).map_err(ColdStorageError::from)?, restating an invariant none of them asserted: after the decode/shape split every cache-path shaping call passes the caller's FULL schema with no projection, because pruning is DataFusion's job (ADR 0023). A reader had to notice the None at all six sites to learn that. CHA-545 --- crates/penca-dl/src/driver.rs | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/crates/penca-dl/src/driver.rs b/crates/penca-dl/src/driver.rs index 8edfc10..d1dffc4 100644 --- a/crates/penca-dl/src/driver.rs +++ b/crates/penca-dl/src/driver.rs @@ -257,11 +257,20 @@ async fn read_projected_uncached( Ok(batch) } +/// Shape a natively-decoded segment to `schema`, in [`DlError`] terms. +/// +/// Always projection-less: the cache stores the file-native decode (CHA-545) and +/// every consumer shapes to its own *full* schema after the lookup, leaving +/// column pruning to DataFusion (ADR 0023). +fn shape_native(batch: &RecordBatch, schema: &SchemaRef) -> Result { + Ok(shape_to_schema(batch, schema, None).map_err(ColdStorageError::from)?) +} + /// One cache-aware segment read, before any shaping — the two outcomes differ /// in what shaping they still owe. enum CachedSegment { /// The file-native decode, cached (or freshly cached) as-is. Still owes - /// `shape_to_schema` to the caller's `full_schema`; deferring that to the + /// [`shape_native`] to the caller's `full_schema`; deferring that to the /// caller is what lets the index-seek path `take` its O(matches) rows /// first and null-fill only those. Native(Arc), @@ -332,9 +341,7 @@ pub(crate) async fn read_cached_snapshot_segment( out_schema: &SchemaRef, ) -> Result { match read_cached_snapshot_segment_unshaped(readers, cache, segment, out_schema).await? { - CachedSegment::Native(native) => { - Ok(shape_to_schema(&native, full_schema, None).map_err(ColdStorageError::from)?) - } + CachedSegment::Native(native) => shape_native(&native, full_schema), CachedSegment::Projected(batch) => Ok(batch), } } @@ -447,7 +454,7 @@ pub(crate) async fn read_cached_persist_segment( if let Some(full) = cache.get(&segment.content_hash) { span.record("cache", "hit"); tracing::debug!(rows = full.num_rows(), "persist segment cache hit"); - return Ok(shape_to_schema(&full, full_schema, None).map_err(ColdStorageError::from)?); + return shape_native(&full, full_schema); } let code = segment.format.as_wire_code(); @@ -462,7 +469,7 @@ pub(crate) async fn read_cached_persist_segment( if cache.admits(weight) { span.record("cache", "miss-cached"); let native = read_and_cache_full_persist(reader, cache, segment, weight).await?; - Ok(shape_to_schema(&native, full_schema, None).map_err(ColdStorageError::from)?) + shape_native(&native, full_schema) } else { span.record("cache", "miss-uncached"); read_projected_uncached_persist(reader, segment, out_schema, full_schema).await @@ -500,7 +507,7 @@ async fn read_cached_index_sidecar( let schema = penca_format::index::segment_index_schema(key_types); if let Some(batch) = cache.get(&sidecar.content_hash) { span.record("cache", "hit"); - return Ok(shape_to_schema(&batch, &schema, None).map_err(ColdStorageError::from)?); + return shape_native(&batch, &schema); } span.record("cache", "miss"); let code = sidecar.format.as_wire_code(); @@ -523,7 +530,7 @@ async fn read_cached_index_sidecar( Arc::clone(&batch), sidecar.size_bytes.max(0) as u64, ); - Ok(shape_to_schema(&batch, &schema, None).map_err(ColdStorageError::from)?) + shape_native(&batch, &schema) } /// Index-driven selective read: binary-search the segment's index sidecar for @@ -603,7 +610,7 @@ async fn take_matched_rows( match read_cached_snapshot_segment_unshaped(readers, cache, segment, out_schema).await? { CachedSegment::Native(native) => { let taken = arrow::compute::take_record_batch(&native, &indices)?; - Ok(shape_to_schema(&taken, full_schema, None).map_err(ColdStorageError::from)?) + shape_native(&taken, full_schema) } CachedSegment::Projected(batch) => Ok(arrow::compute::take_record_batch(&batch, &indices)?), } From 6687637864071c6070574b445b46dbd0a1768e3d Mon Sep 17 00:00:00 2001 From: Nico Bautista Hobin Date: Fri, 31 Jul 2026 05:53:32 +0000 Subject: [PATCH 22/32] refactor(cold): collapse three decode-and-cache copies into one helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The snapshot, persist and index-sidecar cacheable-miss paths each had their own read_segment_native → Arc::new → cache.insert → debug! sequence, two as named helpers and one open-coded. They differed only in which fields of which segment type supplied the uri, slice bounds, content hash and weight — nothing about the decode or the insert. decode_and_cache_native takes those four as plain arguments, mirroring FormatReader::read_segment_native, so no artifact type reaches it. The sidecar path gains the `rows` debug line the other two already had, and the three per-path log texts collapse to one "segment decoded and cached"; the span already carries content_hash and the artifact's own uuid, so the message never carried the distinction. CHA-545 --- crates/penca-dl/src/driver.rs | 101 +++++++++++++++++----------------- 1 file changed, 51 insertions(+), 50 deletions(-) diff --git a/crates/penca-dl/src/driver.rs b/crates/penca-dl/src/driver.rs index d1dffc4..0dc2765 100644 --- a/crates/penca-dl/src/driver.rs +++ b/crates/penca-dl/src/driver.rs @@ -203,27 +203,38 @@ impl DatafusionDlDriver { } } -/// Cacheable miss: decode the WHOLE segment (all columns, no filter +/// Cacheable miss: decode the WHOLE segment file (all columns, no filter /// pushdown, no caller shaping) so the cached entry is reusable across any -/// projection AND any caller schema, insert it under `weight`, and return the -/// native batch. The caller has already decided this segment is admissible, -/// and shapes the result itself once it holds it. -async fn read_and_cache_full( +/// projection AND any caller schema (CHA-545), offer it to the cache under +/// `content_hash`, and return the native batch. The caller shapes the result +/// itself once it holds it. +/// +/// Takes `uri` + `(offset, length)` rather than any segment type, mirroring +/// [`FormatReader::read_segment_native`] — persist segments, snapshot segments +/// and index sidecars all reach it, and none of them is the reader's or the +/// cache's concern. +/// +/// [`SegmentCache::insert`] self-gates on +/// [`admits`](SegmentCache::admits), so an artifact too large for the budget is +/// decoded-but-not-cached rather than evicting everything and then itself. A +/// caller with a narrower read available should check `admits` first and take +/// that path instead. +async fn decode_and_cache_native( reader: &R, cache: &SegmentCache, - segment: &SnapshotSegment, + uri: &str, + offset: Option, + length: Option, + content_hash: Uuid, weight: u64, ) -> Result, DlError> { let batch = reader - .read_segment_native(&segment.uri, Some(segment.offset), Some(segment.length)) + .read_segment_native(uri, offset, length) .await .map_err(ColdStorageError::from)?; let batch = Arc::new(batch); - cache.insert(segment.content_hash, Arc::clone(&batch), weight); - tracing::debug!( - rows = batch.num_rows(), - "snapshot segment cached full decode" - ); + cache.insert(content_hash, Arc::clone(&batch), weight); + tracing::debug!(rows = batch.num_rows(), "segment decoded and cached"); Ok(batch) } @@ -320,7 +331,16 @@ async fn read_cached_snapshot_segment_unshaped( if cache.admits(weight) { span.record("cache", "miss-cached"); Ok(CachedSegment::Native( - read_and_cache_full(reader, cache, segment, weight).await?, + decode_and_cache_native( + reader, + cache, + &segment.uri, + Some(segment.offset), + Some(segment.length), + segment.content_hash, + weight, + ) + .await?, )) } else { span.record("cache", "miss-uncached"); @@ -346,28 +366,6 @@ pub(crate) async fn read_cached_snapshot_segment( } } -/// Cacheable persist miss: decode the WHOLE persist segment (all columns, no -/// caller shaping) so the cached entry serves any projection and any caller -/// schema, insert it under `weight`, and return the native batch. -async fn read_and_cache_full_persist( - reader: &R, - cache: &SegmentCache, - segment: &PersistSegment, - weight: u64, -) -> Result, DlError> { - let batch = reader - .read_segment_native(&segment.uri, segment.offset, segment.length) - .await - .map_err(ColdStorageError::from)?; - let batch = Arc::new(batch); - cache.insert(segment.content_hash, Arc::clone(&batch), weight); - tracing::debug!( - rows = batch.num_rows(), - "persist segment decoded and cached" - ); - Ok(batch) -} - /// Non-cacheable persist miss: a projected read of just `out_schema`, not cached. /// An oversized persist segment is read narrow rather than widened only to be /// discarded. @@ -468,7 +466,16 @@ pub(crate) async fn read_cached_persist_segment( if cache.admits(weight) { span.record("cache", "miss-cached"); - let native = read_and_cache_full_persist(reader, cache, segment, weight).await?; + let native = decode_and_cache_native( + reader, + cache, + &segment.uri, + segment.offset, + segment.length, + segment.content_hash, + weight, + ) + .await?; shape_native(&native, full_schema) } else { span.record("cache", "miss-uncached"); @@ -514,22 +521,16 @@ async fn read_cached_index_sidecar( let reader = readers .get(&code) .ok_or(ColdStorageError::UnknownFormat(code))?; - let batch = reader - .read_segment_native( - &sidecar.object_uri, - Some(sidecar.offset), - Some(sidecar.length), - ) - .await - .map_err(ColdStorageError::from)?; - let batch = Arc::new(batch); - // `insert` self-gates on `cache.admits(weight)`, so an oversize sidecar is - // decoded-but-not-cached rather than evicting the whole budget. - cache.insert( + let batch = decode_and_cache_native( + reader, + cache, + &sidecar.object_uri, + Some(sidecar.offset), + Some(sidecar.length), sidecar.content_hash, - Arc::clone(&batch), sidecar.size_bytes.max(0) as u64, - ); + ) + .await?; shape_native(&batch, &schema) } From ec961b69134fa6fb845eb5e51b09b4745de449fd Mon Sep 17 00:00:00 2001 From: Nico Bautista Hobin Date: Fri, 31 Jul 2026 05:57:56 +0000 Subject: [PATCH 23/32] feat(cold): report the sidecar cache miss as cached vs uncached MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CHA-545 unified the three cold-artifact classes into one SegmentCache under one content_hash key space, but the sidecar span reported a two-valued `cache` field where the two base-segment paths report "hit" / "miss-cached" / "miss-uncached", so no single query spanned the cache without special-casing one of the three classes just unified. It also left the oversize case invisible: read_cached_index_sidecar relies on insert's self-gate rather than calling admits, so a sidecar larger than the budget is re-decoded from S3 on every read forever and logged as a plain "miss", indistinguishable from a first touch. The admits call added here is telemetry-only — a sidecar has no narrower read to fall back to, so it is still decoded whole. CHA-545 --- crates/penca-dl/src/driver.rs | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/crates/penca-dl/src/driver.rs b/crates/penca-dl/src/driver.rs index 0dc2765..ce42710 100644 --- a/crates/penca-dl/src/driver.rs +++ b/crates/penca-dl/src/driver.rs @@ -516,7 +516,20 @@ async fn read_cached_index_sidecar( span.record("cache", "hit"); return shape_native(&batch, &schema); } - span.record("cache", "miss"); + let weight = sidecar.size_bytes.max(0) as u64; + // Telemetry only — unlike a base segment there is no narrower read to fall + // back to, so a non-admissible sidecar is still decoded whole (`insert` + // self-gates). Reporting the base paths' three-valued vocabulary is what + // makes a sidecar that can *never* cache — re-decoded from S3 on every read + // — distinguishable from an ordinary first touch. + span.record( + "cache", + if cache.admits(weight) { + "miss-cached" + } else { + "miss-uncached" + }, + ); let code = sidecar.format.as_wire_code(); let reader = readers .get(&code) @@ -528,7 +541,7 @@ async fn read_cached_index_sidecar( Some(sidecar.offset), Some(sidecar.length), sidecar.content_hash, - sidecar.size_bytes.max(0) as u64, + weight, ) .await?; shape_native(&batch, &schema) From 1b7736f093460fb87444768b27fbd9c6e7265f35 Mon Sep 17 00:00:00 2001 From: Nico Bautista Hobin Date: Fri, 31 Jul 2026 15:34:54 +0000 Subject: [PATCH 24/32] test(cold): supply content_hash in the fake persist-segment insert MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The white-box INSERT builds a fake table_persist_segment row from an explicit column list, which predates content_hash. That column is now UUID NOT NULL with no default, so the raw insert fails before the test can assert what it is about — that a segment with NULL commit_micros stays out of the read plan. CHA-545 Co-Authored-By: Claude Opus 5 --- tests/integration/integration_lifecycle_test.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/integration/integration_lifecycle_test.py b/tests/integration/integration_lifecycle_test.py index e0c470b..e021a0f 100644 --- a/tests/integration/integration_lifecycle_test.py +++ b/tests/integration/integration_lifecycle_test.py @@ -665,11 +665,11 @@ def test_uncommitted_segment_invisible_to_reads(self): SQL( "INSERT INTO {tbl}" " (table_persist_segment_uuid, table_persist_uuid, branch_uuid," - " table_uuid," + " table_uuid, content_hash," " min_tx_commit_micros, max_tx_commit_micros," " min_commit_seq_num, max_commit_seq_num," " object_uri, row_count, format)" - " VALUES (%s, %s, %s, %s, 0, 0," + " VALUES (%s, %s, %s, %s, %s, 0, 0," " 0, 0," " 'fake://upsert', 999, 'parquet')" ).format(tbl=Identifier(segment_parent)), @@ -678,6 +678,10 @@ def test_uncommitted_segment_invisible_to_reads(self): fake_table_persist_uuid, branch.branch_uuid, table_uuid, + # NOT NULL with no default (CHA-545). Irrelevant to what this + # test asserts, but random rather than fixed so the fake row + # can never collide with a real segment's cache key. + str(uuid4()), ), ) From 6d755e21c9c1a749a020c81d8cb0f9ae80f4b473 Mon Sep 17 00:00:00 2001 From: Nico Bautista Hobin Date: Fri, 31 Jul 2026 16:28:53 +0000 Subject: [PATCH 25/32] fix(cold): fold the storage format into the segment cache key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Keying on content_hash alone assumed the cached value was format-agnostic. It is not: this PR deliberately caches the *file-native* decode, which is a per-format artifact — a round-trip may widen a type or re-dictionary-encode. And content_hash digests the typed batch *before* a FormatWriter encodes it, so one hash can name a Parquet file and a Lance file once OBJECT_STORAGE_FORMAT has been flipped between two writes of the same content. All three lookups ran cache.get above format.as_wire_code(), so a hit never consulted the row's format at all, and one format's decode could be served for the other's file. The key becomes (content_hash, format wire code). Folding the format into the key rather than into the digest keeps content_hash a pure function of the batch, which is what lets a reference copy inherit it verbatim. Red-verified: neutering the format half of the key fails same_hash_under_two_formats_does_not_share_an_entry on the Lance lookup. CHA-545 Co-Authored-By: Claude Opus 5 --- crates/penca-dl/src/cache.rs | 106 ++++++++++++++++++++++++---------- crates/penca-dl/src/driver.rs | 26 ++++++--- docs/design-decisions.md | 12 +++- docs/schema-reference.md | 6 +- 4 files changed, 106 insertions(+), 44 deletions(-) diff --git a/crates/penca-dl/src/cache.rs b/crates/penca-dl/src/cache.rs index 7ac1176..f1ff575 100644 --- a/crates/penca-dl/src/cache.rs +++ b/crates/penca-dl/src/cache.rs @@ -3,9 +3,10 @@ //! A repeat read of the same cold segment within the process lifetime is served //! as an `Arc::clone` of the already-decoded Arrow batches, skipping the S3 GET + //! Parquet/Lance decode. It holds snapshot segments, persist data segments and -//! index sidecars under one byte budget, all keyed by `content_hash`. The -//! mapping is stable and needs no invalidation: a hash names one file-native -//! decode by construction, and cold artifacts are immutable — although a *resolved +//! index sidecars under one byte budget, all keyed by `(content_hash, format)`. +//! The mapping is stable and needs no invalidation: that pair names one +//! file-native decode by construction, and cold artifacts are immutable — +//! although a *resolved //! persist tier* is mutable under retention compaction, an individual persist //! *file* is not. There is no TTL — immutability makes W-TinyLFU eviction the //! whole reclaim mechanism for every tier. @@ -30,8 +31,8 @@ use arrow::record_batch::RecordBatch; use moka::sync::Cache; use uuid::Uuid; -/// In-process W-TinyLFU cache of decoded cold segments, keyed by `content_hash` -/// and bounded by a byte budget. +/// In-process W-TinyLFU cache of decoded cold segments, keyed by +/// `(content_hash, format)` and bounded by a byte budget. /// /// `content_hash` is the digest of the typed in-memory Arrow batch, recorded /// once at write time and inherited verbatim by every reference copy — snapshot @@ -52,17 +53,25 @@ use uuid::Uuid; /// by a hash of its own typed content, 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. `format` is likewise absent from the -/// key: every value is a format-agnostic decoded [`RecordBatch`], and `format` -/// is consulted only on the miss path (by the caller) to pick the reader. +/// to the same batch may safely share. +/// +/// The key is `(content_hash, format)`, not the hash alone. `content_hash` +/// digests the typed batch *before* a `FormatWriter` encodes it, so one hash can +/// name files in two formats once `OBJECT_STORAGE_FORMAT` has been flipped — +/// while the value is deliberately the *file-native* decode, a per-format +/// artifact (a round-trip may widen a type or re-dictionary-encode). Keying on +/// content alone would serve one format's decode for the other format's file. +/// Folding the format into the key rather than into the digest is what keeps +/// `content_hash` a pure function of the batch. /// /// Cheaply cloneable: `moka::sync::Cache` is internally an `Arc`, so callers /// typically hold a `SegmentCache` behind one outer `Arc` shared /// across the process. pub struct SegmentCache { - /// Value carries its own weight so the weigher can charge the - /// caller-supplied `size_bytes` rather than the batch's runtime memory. - inner: Cache, u32)>, + /// Keyed by `(content_hash, format wire code)`. Value carries its own weight + /// so the weigher can charge the caller-supplied `size_bytes` rather than + /// the batch's runtime memory. + inner: Cache<(Uuid, i32), (Arc, u32)>, budget_bytes: u64, } @@ -77,7 +86,7 @@ impl SegmentCache { .max_capacity(budget_bytes) // Weight in the same byte unit as `max_capacity` so the budget is a // real RAM bound; the stored `u32` is the entry's `size_bytes`. - .weigher(|_hash: &Uuid, (_batch, weight): &(Arc, u32)| *weight) + .weigher(|_key: &(Uuid, i32), (_batch, weight): &(Arc, u32)| *weight) .build(); Self { inner, @@ -117,22 +126,30 @@ impl SegmentCache { && weight_bytes <= u32::MAX as u64 } - /// Fetch a decoded segment by content hash, bumping its frequency estimate. - /// A hit is an `Arc::clone` — no buffer copy. - pub fn get(&self, content_hash: &Uuid) -> Option> { - self.inner.get(content_hash).map(|(batch, _weight)| batch) + /// Fetch a decoded segment by content hash and format wire code, bumping its + /// frequency estimate. A hit is an `Arc::clone` — no buffer copy. + pub fn get(&self, content_hash: &Uuid, format_code: i32) -> Option> { + self.inner + .get(&(*content_hash, format_code)) + .map(|(batch, _weight)| batch) } - /// Insert a decoded segment under its content hash, charged `weight_bytes` - /// against the budget. No-op when the segment is not + /// Insert a decoded segment under its content hash and format wire code, + /// charged `weight_bytes` against the budget. No-op when the segment is not /// [`admits`](Self::admits)-ible. moka enforces `max_capacity` via /// W-TinyLFU; there is no manual eviction loop here. - pub fn insert(&self, content_hash: Uuid, batch: Arc, weight_bytes: u64) { + pub fn insert( + &self, + content_hash: Uuid, + format_code: i32, + batch: Arc, + weight_bytes: u64, + ) { if !self.admits(weight_bytes) { return; } self.inner - .insert(content_hash, (batch, weight_bytes as u32)); + .insert((content_hash, format_code), (batch, weight_bytes as u32)); } /// Force pending eviction/maintenance to run synchronously. moka does @@ -158,6 +175,7 @@ mod tests { use arrow::array::Int32Array; use arrow::datatypes::{DataType, Field, Schema}; use arrow::record_batch::RecordBatch; + use penca_core::Format; use uuid::Uuid; use super::SegmentCache; @@ -169,6 +187,11 @@ mod tests { Arc::new(RecordBatch::try_new(schema, vec![Arc::new(col)]).unwrap()) } + /// The format half of the key, for tests that only exercise the hash half. + fn parquet() -> i32 { + Format::Parquet.as_wire_code() + } + #[test] fn admits_predicate() { let cache = SegmentCache::new(100); @@ -187,10 +210,10 @@ mod tests { #[test] fn zero_weight_segment_is_not_cached() { let cache = SegmentCache::new(1 << 20); - cache.insert(Uuid::from_u128(1), batch(4), 0); + cache.insert(Uuid::from_u128(1), parquet(), batch(4), 0); cache.run_pending(); assert!( - cache.get(&Uuid::from_u128(1)).is_none(), + cache.get(&Uuid::from_u128(1), parquet()).is_none(), "weight-0 segment must not be pinned in the cache" ); } @@ -210,10 +233,10 @@ mod tests { ); assert!(cache.admits(u32::MAX as u64), "exactly u32::MAX is fine"); - cache.insert(Uuid::from_u128(2), batch(8), over_u32); + cache.insert(Uuid::from_u128(2), parquet(), batch(8), over_u32); cache.run_pending(); assert!( - cache.get(&Uuid::from_u128(2)).is_none(), + cache.get(&Uuid::from_u128(2), parquet()).is_none(), "over-u32 weight never stored" ); } @@ -221,18 +244,18 @@ mod tests { #[test] fn over_budget_insert_is_noop() { let cache = SegmentCache::new(100); - cache.insert(Uuid::from_u128(3), batch(8), 200); + cache.insert(Uuid::from_u128(3), parquet(), batch(8), 200); cache.run_pending(); assert!( - cache.get(&Uuid::from_u128(3)).is_none(), + cache.get(&Uuid::from_u128(3), parquet()).is_none(), "over-budget never stored" ); let disabled = SegmentCache::disabled(); - disabled.insert(Uuid::from_u128(4), batch(8), 1); + disabled.insert(Uuid::from_u128(4), parquet(), batch(8), 1); disabled.run_pending(); assert!( - disabled.get(&Uuid::from_u128(4)).is_none(), + disabled.get(&Uuid::from_u128(4), parquet()).is_none(), "disabled never stores" ); } @@ -245,7 +268,7 @@ mod tests { // moka's W-TinyLFU choice, not Penca's contract). let cache = SegmentCache::new(100); for i in 0..5 { - cache.insert(Uuid::from_u128(i), batch(10), 40); + cache.insert(Uuid::from_u128(i), parquet(), batch(10), 40); } cache.run_pending(); assert!( @@ -255,13 +278,34 @@ mod tests { ); } + /// `content_hash` digests the batch before the writer encodes it, so one + /// hash can name a Parquet file and a Lance file after an + /// `OBJECT_STORAGE_FORMAT` flip. Their file-native decodes are different + /// artifacts, so the format has to separate them. + #[test] + fn same_hash_under_two_formats_does_not_share_an_entry() { + let cache = SegmentCache::new(1_000); + let shared = Uuid::from_u128(6); + cache.insert(shared, Format::Parquet.as_wire_code(), batch(4), 40); + cache.run_pending(); + + assert!( + cache.get(&shared, Format::Lance.as_wire_code()).is_none(), + "a Lance row must not be served the Parquet decode" + ); + assert!( + cache.get(&shared, Format::Parquet.as_wire_code()).is_some(), + "its own format still hits" + ); + } + #[test] fn hit_returns_arc_clone_same_buffers() { let cache = SegmentCache::new(1_000); let original = batch(16); - cache.insert(Uuid::from_u128(5), original.clone(), 40); + cache.insert(Uuid::from_u128(5), parquet(), original.clone(), 40); cache.run_pending(); - let hit = cache.get(&Uuid::from_u128(5)).expect("cached"); + let hit = cache.get(&Uuid::from_u128(5), parquet()).expect("cached"); // Same backing column buffer — a hit is a refcount bump, no copy. assert_eq!( original.column(0).to_data().buffers()[0].as_ptr(), diff --git a/crates/penca-dl/src/driver.rs b/crates/penca-dl/src/driver.rs index ce42710..2fd1539 100644 --- a/crates/penca-dl/src/driver.rs +++ b/crates/penca-dl/src/driver.rs @@ -226,6 +226,7 @@ async fn decode_and_cache_native( offset: Option, length: Option, content_hash: Uuid, + format_code: i32, weight: u64, ) -> Result, DlError> { let batch = reader @@ -233,7 +234,7 @@ async fn decode_and_cache_native( .await .map_err(ColdStorageError::from)?; let batch = Arc::new(batch); - cache.insert(content_hash, Arc::clone(&batch), weight); + cache.insert(content_hash, format_code, Arc::clone(&batch), weight); tracing::debug!(rows = batch.num_rows(), "segment decoded and cached"); Ok(batch) } @@ -313,14 +314,14 @@ async fn read_cached_snapshot_segment_unshaped( out_schema: &SchemaRef, ) -> Result { let span = tracing::Span::current(); + let code = segment.format.as_wire_code(); - if let Some(full) = cache.get(&segment.content_hash) { + if let Some(full) = cache.get(&segment.content_hash, code) { span.record("cache", "hit"); tracing::debug!(rows = full.num_rows(), "snapshot segment cache hit"); return Ok(CachedSegment::Native(full)); } - let code = segment.format.as_wire_code(); let reader = readers .get(&code) .ok_or(ColdStorageError::UnknownFormat(code))?; @@ -338,6 +339,7 @@ async fn read_cached_snapshot_segment_unshaped( Some(segment.offset), Some(segment.length), segment.content_hash, + code, weight, ) .await?, @@ -448,14 +450,14 @@ pub(crate) async fn read_cached_persist_segment( out_schema: &SchemaRef, ) -> Result { let span = tracing::Span::current(); + let code = segment.format.as_wire_code(); - if let Some(full) = cache.get(&segment.content_hash) { + if let Some(full) = cache.get(&segment.content_hash, code) { span.record("cache", "hit"); tracing::debug!(rows = full.num_rows(), "persist segment cache hit"); return shape_native(&full, full_schema); } - let code = segment.format.as_wire_code(); let reader = readers .get(&code) .ok_or(ColdStorageError::UnknownFormat(code))?; @@ -473,6 +475,7 @@ pub(crate) async fn read_cached_persist_segment( segment.offset, segment.length, segment.content_hash, + code, weight, ) .await?; @@ -509,10 +512,11 @@ async fn read_cached_index_sidecar( key_types: &[arrow::datatypes::DataType], ) -> Result { let span = tracing::Span::current(); + let code = sidecar.format.as_wire_code(); // The sidecar's key schema is the indexed columns' native types; the // identity/name sidecars are the all-Utf8 special case. let schema = penca_format::index::segment_index_schema(key_types); - if let Some(batch) = cache.get(&sidecar.content_hash) { + if let Some(batch) = cache.get(&sidecar.content_hash, code) { span.record("cache", "hit"); return shape_native(&batch, &schema); } @@ -530,7 +534,6 @@ async fn read_cached_index_sidecar( "miss-uncached" }, ); - let code = sidecar.format.as_wire_code(); let reader = readers .get(&code) .ok_or(ColdStorageError::UnknownFormat(code))?; @@ -541,6 +544,7 @@ async fn read_cached_index_sidecar( Some(sidecar.offset), Some(sidecar.length), sidecar.content_hash, + code, weight, ) .await?; @@ -2035,7 +2039,7 @@ mod tests { "oversized segment is never cached — both accesses re-read storage" ); assert!( - cache.get(&seg.content_hash).is_none(), + cache.get(&seg.content_hash, seg.format.as_wire_code()).is_none(), "oversized segment not stored" ); } @@ -2057,7 +2061,11 @@ mod tests { // moka evicted one of {a,b} to honor the budget (we don't assert which // — that is moka's W-TinyLFU choice). Re-reading the evicted key must // hit storage again. - let evicted = if cache.get(&segment("a", 150).content_hash).is_none() { + let a = segment("a", 150); + let evicted = if cache + .get(&a.content_hash, a.format.as_wire_code()) + .is_none() + { "a" } else { "b" diff --git a/docs/design-decisions.md b/docs/design-decisions.md index 429ea5a..9d9a2b6 100644 --- a/docs/design-decisions.md +++ b/docs/design-decisions.md @@ -188,7 +188,8 @@ shape. Every cold-artifact metadata row — persist segments, snapshot segments, and segment index sidecars — 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 it. +(`penca_core::digest::segment_content_hash`). `SegmentCache` is keyed by +`(content_hash, format)`. **The problem.** Reference copies mint a new row uuid over an unchanged `(object_uri, offset, length)`: `insert_carried_snapshot_segments` and @@ -208,6 +209,15 @@ It also makes dedup insensitive to encoding choices, so two independently writte segments holding the same rows share an entry even if the writer picked different row-group boundaries or dictionary encodings. +**Why `format` is the other half of 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. +The cached *value*, meanwhile, is the file-native decode — a per-format artifact, +since a round-trip may widen a type or re-dictionary-encode. So the format joins +the cache key rather than the digest: the pair names one native decode, and +`content_hash` stays a pure function of the batch, which is what lets a reference +copy inherit it verbatim. + **What the key deliberately does *not* do is separate a reference copy from its source.** Carry-forward and fork copy select `old.content_hash` verbatim (`fork_copy.rs`, `snapshot.rs`, `segment_index.rs`) and it is never recomputed — diff --git a/docs/schema-reference.md b/docs/schema-reference.md index 5cbdde3..d6858b6 100644 --- a/docs/schema-reference.md +++ b/docs/schema-reference.md @@ -378,7 +378,7 @@ the `log_kind` classification (CHA-218: only `upsert_log` and | `length` | int64 (set at compact time) | | `row_count` | int64 | | `format` | text (parquet, lance) | -| `content_hash` | UUID NOT NULL (CHA-545, segment-cache key) | +| `content_hash` | UUID NOT NULL (CHA-545, segment-cache key with `format`) | | `size_bytes` | int64 | | `metadata` | JSONB | | `statistics` | JSONB | @@ -482,7 +482,7 @@ completes). | `length` | int64 NOT NULL (row count of the range) | | `size_bytes` | int64 | | `format` | text (lance, parquet) | -| `content_hash` | UUID NOT NULL (CHA-545, segment-cache key — see table 14) | +| `content_hash` | UUID NOT NULL (CHA-545, segment-cache key with `format` — see table 14) | | `metadata` | JSON (format-specific, e.g., row group size) | | `statistics` | JSON (column stats: min/max for filterable columns) | | `row_count` | int64 | @@ -539,7 +539,7 @@ forward by reference with its base segment and participates in the ref-counted G | `offset` | int64 | | `length` | int64 | | `format` | text (lance, parquet) | -| `content_hash` | UUID NOT NULL (CHA-545, segment-cache key — see table 14) | +| `content_hash` | UUID NOT NULL (CHA-545, segment-cache key with `format` — see table 14) | | `size_bytes` | int64 | | `statistics` | bytes (indexed-key min/max bounds; binary, decoded in-planner by the CHA-454 seek in the `SnapshotTableProvider`) | | `written_at_micros` | int64 (micros, auto-generated) | From 97ad00a4bec2f80180817697bd06fc5da242a70a Mon Sep 17 00:00:00 2001 From: Nico Bautista Hobin Date: Fri, 31 Jul 2026 16:29:33 +0000 Subject: [PATCH 26/32] chore: cargo fmt the segment cache key call site CHA-545 Co-Authored-By: Claude Opus 5 --- crates/penca-dl/src/driver.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/crates/penca-dl/src/driver.rs b/crates/penca-dl/src/driver.rs index 2fd1539..f0d688d 100644 --- a/crates/penca-dl/src/driver.rs +++ b/crates/penca-dl/src/driver.rs @@ -2039,7 +2039,9 @@ mod tests { "oversized segment is never cached — both accesses re-read storage" ); assert!( - cache.get(&seg.content_hash, seg.format.as_wire_code()).is_none(), + cache + .get(&seg.content_hash, seg.format.as_wire_code()) + .is_none(), "oversized segment not stored" ); } From b2100a423d4423be678061cba0ac3bc5c1bd55d6 Mon Sep 17 00:00:00 2001 From: Nico Bautista Hobin Date: Fri, 31 Jul 2026 16:30:36 +0000 Subject: [PATCH 27/32] docs(cold): reconcile the sidecar collision note with the key space MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit read_cached_index_sidecar claimed a sidecar and a base segment "never collide in one cache"; SegmentCache's key-space paragraph argues the opposite — that a same-content collision between two artifact classes is the correct answer rather than a bug. The cache's framing is the sound one, and the "never collide" claim was both stronger than needed and unenforced (it happens to hold because a sidecar's schema is (key_0..key_n-1, row_offset), but nothing checks that). Point at SegmentCache instead of restating a second, disagreeing invariant. CHA-545 Co-Authored-By: Claude Opus 5 --- crates/penca-dl/src/driver.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/crates/penca-dl/src/driver.rs b/crates/penca-dl/src/driver.rs index f0d688d..0551191 100644 --- a/crates/penca-dl/src/driver.rs +++ b/crates/penca-dl/src/driver.rs @@ -487,9 +487,10 @@ pub(crate) async fn read_cached_persist_segment( } /// Load a sorted `(key, row_offset)` index sidecar through the shared snapshot -/// cache, keyed by the sidecar's own `content_hash` — a digest of the sidecar -/// batch, which differs from any base segment's, so the two never collide in one -/// cache. +/// cache, under the sidecar's own `content_hash` — a digest of the sidecar +/// batch, not of the base segment it indexes. Sidecars and base segments share +/// one flat key space; see [`SegmentCache`] for why a same-content collision +/// between the two classes would be correct rather than a bug. /// /// Cached natively and shaped to `key_types` after the lookup, for the same /// reason as a base segment: the file was written with the *writing* branch's From 73087321eeaa0d42f85eb71ac298808abb040f97 Mon Sep 17 00:00:00 2001 From: Nico Bautista Hobin Date: Fri, 31 Jul 2026 16:37:51 +0000 Subject: [PATCH 28/32] refactor(cold): name the segment cache key as a type Folding the storage format into the cache key pushed `decode_and_cache_native` to 8 parameters, one over clippy's `too_many_arguments` threshold, failing `just check`. Rather than silence the lint, give the pair a name: `SegmentCacheKey` is what `SegmentCache` is keyed by, so the two halves travel together through the driver instead of as adjacent positional arguments a caller could transpose. CHA-545 --- crates/penca-dl/src/cache.rs | 103 ++++++++++++++++++---------------- crates/penca-dl/src/driver.rs | 36 +++++++----- 2 files changed, 75 insertions(+), 64 deletions(-) diff --git a/crates/penca-dl/src/cache.rs b/crates/penca-dl/src/cache.rs index f1ff575..aae3b42 100644 --- a/crates/penca-dl/src/cache.rs +++ b/crates/penca-dl/src/cache.rs @@ -31,8 +31,28 @@ use arrow::record_batch::RecordBatch; use moka::sync::Cache; use uuid::Uuid; +/// What names one cached decode: the content hash of the typed batch, paired +/// with the wire code of the format the file it landed in is written in. +/// +/// See [`SegmentCache`] for why both halves are needed and why the format is +/// part of the key rather than part of the digest. +#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)] +pub struct SegmentCacheKey { + pub content_hash: Uuid, + pub format_code: i32, +} + +impl SegmentCacheKey { + pub fn new(content_hash: Uuid, format_code: i32) -> Self { + Self { + content_hash, + format_code, + } + } +} + /// In-process W-TinyLFU cache of decoded cold segments, keyed by -/// `(content_hash, format)` and bounded by a byte budget. +/// [`SegmentCacheKey`] and bounded by a byte budget. /// /// `content_hash` is the digest of the typed in-memory Arrow batch, recorded /// once at write time and inherited verbatim by every reference copy — snapshot @@ -68,10 +88,9 @@ use uuid::Uuid; /// typically hold a `SegmentCache` behind one outer `Arc` shared /// across the process. pub struct SegmentCache { - /// Keyed by `(content_hash, format wire code)`. Value carries its own weight - /// so the weigher can charge the caller-supplied `size_bytes` rather than - /// the batch's runtime memory. - inner: Cache<(Uuid, i32), (Arc, u32)>, + /// The value carries its own weight so the weigher can charge the + /// caller-supplied `size_bytes` rather than the batch's runtime memory. + inner: Cache, u32)>, budget_bytes: u64, } @@ -86,7 +105,7 @@ impl SegmentCache { .max_capacity(budget_bytes) // Weight in the same byte unit as `max_capacity` so the budget is a // real RAM bound; the stored `u32` is the entry's `size_bytes`. - .weigher(|_key: &(Uuid, i32), (_batch, weight): &(Arc, u32)| *weight) + .weigher(|_key: &SegmentCacheKey, (_batch, weight): &(Arc, u32)| *weight) .build(); Self { inner, @@ -126,30 +145,20 @@ impl SegmentCache { && weight_bytes <= u32::MAX as u64 } - /// Fetch a decoded segment by content hash and format wire code, bumping its - /// frequency estimate. A hit is an `Arc::clone` — no buffer copy. - pub fn get(&self, content_hash: &Uuid, format_code: i32) -> Option> { - self.inner - .get(&(*content_hash, format_code)) - .map(|(batch, _weight)| batch) + /// Fetch a decoded segment, bumping its frequency estimate. A hit is an + /// `Arc::clone` — no buffer copy. + pub fn get(&self, key: &SegmentCacheKey) -> Option> { + self.inner.get(key).map(|(batch, _weight)| batch) } - /// Insert a decoded segment under its content hash and format wire code, - /// charged `weight_bytes` against the budget. No-op when the segment is not - /// [`admits`](Self::admits)-ible. moka enforces `max_capacity` via - /// W-TinyLFU; there is no manual eviction loop here. - pub fn insert( - &self, - content_hash: Uuid, - format_code: i32, - batch: Arc, - weight_bytes: u64, - ) { + /// Insert a decoded segment charged `weight_bytes` against the budget. No-op + /// when the segment is not [`admits`](Self::admits)-ible. moka enforces + /// `max_capacity` via W-TinyLFU; there is no manual eviction loop here. + pub fn insert(&self, key: SegmentCacheKey, batch: Arc, weight_bytes: u64) { if !self.admits(weight_bytes) { return; } - self.inner - .insert((content_hash, format_code), (batch, weight_bytes as u32)); + self.inner.insert(key, (batch, weight_bytes as u32)); } /// Force pending eviction/maintenance to run synchronously. moka does @@ -178,7 +187,7 @@ mod tests { use penca_core::Format; use uuid::Uuid; - use super::SegmentCache; + use super::{SegmentCache, SegmentCacheKey}; /// One-column batch of `n` i32 rows; a stand-in decoded segment. fn batch(n: usize) -> Arc { @@ -187,9 +196,9 @@ mod tests { Arc::new(RecordBatch::try_new(schema, vec![Arc::new(col)]).unwrap()) } - /// The format half of the key, for tests that only exercise the hash half. - fn parquet() -> i32 { - Format::Parquet.as_wire_code() + /// A Parquet key over hash `n`, for tests that only exercise the hash half. + fn parquet(n: u128) -> SegmentCacheKey { + SegmentCacheKey::new(Uuid::from_u128(n), Format::Parquet.as_wire_code()) } #[test] @@ -210,10 +219,10 @@ mod tests { #[test] fn zero_weight_segment_is_not_cached() { let cache = SegmentCache::new(1 << 20); - cache.insert(Uuid::from_u128(1), parquet(), batch(4), 0); + cache.insert(parquet(1), batch(4), 0); cache.run_pending(); assert!( - cache.get(&Uuid::from_u128(1), parquet()).is_none(), + cache.get(&parquet(1)).is_none(), "weight-0 segment must not be pinned in the cache" ); } @@ -233,10 +242,10 @@ mod tests { ); assert!(cache.admits(u32::MAX as u64), "exactly u32::MAX is fine"); - cache.insert(Uuid::from_u128(2), parquet(), batch(8), over_u32); + cache.insert(parquet(2), batch(8), over_u32); cache.run_pending(); assert!( - cache.get(&Uuid::from_u128(2), parquet()).is_none(), + cache.get(&parquet(2)).is_none(), "over-u32 weight never stored" ); } @@ -244,20 +253,14 @@ mod tests { #[test] fn over_budget_insert_is_noop() { let cache = SegmentCache::new(100); - cache.insert(Uuid::from_u128(3), parquet(), batch(8), 200); + cache.insert(parquet(3), batch(8), 200); cache.run_pending(); - assert!( - cache.get(&Uuid::from_u128(3), parquet()).is_none(), - "over-budget never stored" - ); + assert!(cache.get(&parquet(3)).is_none(), "over-budget never stored"); let disabled = SegmentCache::disabled(); - disabled.insert(Uuid::from_u128(4), parquet(), batch(8), 1); + disabled.insert(parquet(4), batch(8), 1); disabled.run_pending(); - assert!( - disabled.get(&Uuid::from_u128(4), parquet()).is_none(), - "disabled never stores" - ); + assert!(disabled.get(&parquet(4)).is_none(), "disabled never stores"); } #[test] @@ -268,7 +271,7 @@ mod tests { // moka's W-TinyLFU choice, not Penca's contract). let cache = SegmentCache::new(100); for i in 0..5 { - cache.insert(Uuid::from_u128(i), parquet(), batch(10), 40); + cache.insert(parquet(i), batch(10), 40); } cache.run_pending(); assert!( @@ -286,15 +289,17 @@ mod tests { fn same_hash_under_two_formats_does_not_share_an_entry() { let cache = SegmentCache::new(1_000); let shared = Uuid::from_u128(6); - cache.insert(shared, Format::Parquet.as_wire_code(), batch(4), 40); + let as_parquet = SegmentCacheKey::new(shared, Format::Parquet.as_wire_code()); + let as_lance = SegmentCacheKey::new(shared, Format::Lance.as_wire_code()); + cache.insert(as_parquet, batch(4), 40); cache.run_pending(); assert!( - cache.get(&shared, Format::Lance.as_wire_code()).is_none(), + cache.get(&as_lance).is_none(), "a Lance row must not be served the Parquet decode" ); assert!( - cache.get(&shared, Format::Parquet.as_wire_code()).is_some(), + cache.get(&as_parquet).is_some(), "its own format still hits" ); } @@ -303,9 +308,9 @@ mod tests { fn hit_returns_arc_clone_same_buffers() { let cache = SegmentCache::new(1_000); let original = batch(16); - cache.insert(Uuid::from_u128(5), parquet(), original.clone(), 40); + cache.insert(parquet(5), original.clone(), 40); cache.run_pending(); - let hit = cache.get(&Uuid::from_u128(5), parquet()).expect("cached"); + let hit = cache.get(&parquet(5)).expect("cached"); // Same backing column buffer — a hit is a refcount bump, no copy. assert_eq!( original.column(0).to_data().buffers()[0].as_ptr(), diff --git a/crates/penca-dl/src/driver.rs b/crates/penca-dl/src/driver.rs index 0551191..8dc5c2c 100644 --- a/crates/penca-dl/src/driver.rs +++ b/crates/penca-dl/src/driver.rs @@ -24,7 +24,7 @@ use penca_storage_cold::{COMMIT_SEQ_NUM_COLUMN, ColdStorageError}; use tracing::Instrument as _; use uuid::Uuid; -use crate::cache::SegmentCache; +use crate::cache::{SegmentCache, SegmentCacheKey}; use crate::provider::{build_persist_session, build_snapshot_session}; use crate::schema::LogSchemas; use crate::session_template::derive_cold_session; @@ -225,8 +225,7 @@ async fn decode_and_cache_native( uri: &str, offset: Option, length: Option, - content_hash: Uuid, - format_code: i32, + key: SegmentCacheKey, weight: u64, ) -> Result, DlError> { let batch = reader @@ -234,7 +233,7 @@ async fn decode_and_cache_native( .await .map_err(ColdStorageError::from)?; let batch = Arc::new(batch); - cache.insert(content_hash, format_code, Arc::clone(&batch), weight); + cache.insert(key, Arc::clone(&batch), weight); tracing::debug!(rows = batch.num_rows(), "segment decoded and cached"); Ok(batch) } @@ -315,8 +314,9 @@ async fn read_cached_snapshot_segment_unshaped( ) -> Result { let span = tracing::Span::current(); let code = segment.format.as_wire_code(); + let key = SegmentCacheKey::new(segment.content_hash, code); - if let Some(full) = cache.get(&segment.content_hash, code) { + if let Some(full) = cache.get(&key) { span.record("cache", "hit"); tracing::debug!(rows = full.num_rows(), "snapshot segment cache hit"); return Ok(CachedSegment::Native(full)); @@ -338,8 +338,7 @@ async fn read_cached_snapshot_segment_unshaped( &segment.uri, Some(segment.offset), Some(segment.length), - segment.content_hash, - code, + key, weight, ) .await?, @@ -452,7 +451,9 @@ pub(crate) async fn read_cached_persist_segment( let span = tracing::Span::current(); let code = segment.format.as_wire_code(); - if let Some(full) = cache.get(&segment.content_hash, code) { + let key = SegmentCacheKey::new(segment.content_hash, code); + + if let Some(full) = cache.get(&key) { span.record("cache", "hit"); tracing::debug!(rows = full.num_rows(), "persist segment cache hit"); return shape_native(&full, full_schema); @@ -474,8 +475,7 @@ pub(crate) async fn read_cached_persist_segment( &segment.uri, segment.offset, segment.length, - segment.content_hash, - code, + key, weight, ) .await?; @@ -514,10 +514,11 @@ async fn read_cached_index_sidecar( ) -> Result { let span = tracing::Span::current(); let code = sidecar.format.as_wire_code(); + let key = SegmentCacheKey::new(sidecar.content_hash, code); // The sidecar's key schema is the indexed columns' native types; the // identity/name sidecars are the all-Utf8 special case. let schema = penca_format::index::segment_index_schema(key_types); - if let Some(batch) = cache.get(&sidecar.content_hash, code) { + if let Some(batch) = cache.get(&key) { span.record("cache", "hit"); return shape_native(&batch, &schema); } @@ -544,8 +545,7 @@ async fn read_cached_index_sidecar( &sidecar.object_uri, Some(sidecar.offset), Some(sidecar.length), - sidecar.content_hash, - code, + key, weight, ) .await?; @@ -2041,7 +2041,10 @@ mod tests { ); assert!( cache - .get(&seg.content_hash, seg.format.as_wire_code()) + .get(&SegmentCacheKey::new( + seg.content_hash, + seg.format.as_wire_code() + )) .is_none(), "oversized segment not stored" ); @@ -2066,7 +2069,10 @@ mod tests { // hit storage again. let a = segment("a", 150); let evicted = if cache - .get(&a.content_hash, a.format.as_wire_code()) + .get(&SegmentCacheKey::new( + a.content_hash, + a.format.as_wire_code(), + )) .is_none() { "a" From a3ae1bd91457c83988d163f0440848a11dd29b60 Mon Sep 17 00:00:00 2001 From: Nico Bautista Hobin Date: Fri, 31 Jul 2026 18:10:13 +0000 Subject: [PATCH 29/32] docs(cold): record why the cache holds the native decode The read-time schema question is the one a reader of this cache asks first, and neither the ADR nor the type doc answered it: they said the value must be the file-native decode without saying that read-time shaping still happens, just after the lookup. That reads as if read-time schema had been rejected outright, when only the decode is segment-scoped -- because only the decode is shared. So state the mechanics: a worked parent/fork example over one shared slice, and the symmetric design that fingerprints the read schema into the key. Evaluate that one at its strongest -- caching the type-cast table, projection still per caller -- since the weaker variant that caches the fully shaped table loses to an avoidable problem and is not the real comparison. The strong form is implementable and correct; it is rejected because its key moves whenever the schema moves while the data does not, so 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 case this key exists for. Its one real 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 that changes the cast belongs in the shaping tail where it is per caller and visible. Also record what actually directs the decode today. Both readers take the schema the format engine embedded, nothing in penca-format casts, and the table's stored point-in-time `arrow_schema` is never consulted -- which is precisely why `format` sits in the cache key. Directing the decode from the stored schema instead would make it droppable; logged as an open question rather than built here. CHA-545 --- crates/penca-dl/src/cache.rs | 9 +++ crates/penca-format/src/reader/mod.rs | 6 ++ docs/design-decisions.md | 87 +++++++++++++++++++++++++++ 3 files changed, 102 insertions(+) diff --git a/crates/penca-dl/src/cache.rs b/crates/penca-dl/src/cache.rs index aae3b42..de7eeb2 100644 --- a/crates/penca-dl/src/cache.rs +++ b/crates/penca-dl/src/cache.rs @@ -69,6 +69,15 @@ impl SegmentCacheKey { /// `test_fork_and_parent_diverge_a_columns_type_over_one_shared_slice` is the /// regression guard. /// +/// The read-time schema still governs the *output* — it governs it at that +/// shaping step, per caller. Only the decode has to be segment-scoped, and only +/// because the decode is what gets shared. Fingerprinting the read schema into +/// the key instead is correct, but then the key moves whenever the schema does +/// while the data does not: one `ALTER TABLE ADD COLUMN` re-fingerprints a +/// footprint nobody rewrote, and a fork stops sharing with its parent at their +/// first divergent `ALTER` — the case this key exists for. See +/// `docs/design-decisions.md` — "Cold segments are cached by content hash". +/// /// One flat key space, no per-artifact-class prefix. With every artifact keyed /// by a hash of its own typed content, two entries collide only when their /// decoded batches are identical, in which case sharing one entry is the diff --git a/crates/penca-format/src/reader/mod.rs b/crates/penca-format/src/reader/mod.rs index 9222ff0..eb1c3b4 100644 --- a/crates/penca-format/src/reader/mod.rs +++ b/crates/penca-format/src/reader/mod.rs @@ -61,6 +61,12 @@ pub trait FormatReader: Send + Sync { /// length)` slice still applies, so row count and row order match what /// [`read_segment`](Self::read_segment) would return for the same slice. /// + /// "The file's own types" means the schema the format engine embedded and + /// returns, not the table's stored `arrow_schema` — nothing here casts, so + /// the types are the encoder's answer rather than the catalog's. That is why + /// `SegmentCache` keys on the format alongside the content hash; see the + /// open question in `docs/design-decisions.md`. + /// /// Shape the result to a caller's schema afterwards with /// [`shape_to_schema`] — the same tail `read_segment` runs internally. /// Splitting the two is what lets one decode be cached and served to diff --git a/docs/design-decisions.md b/docs/design-decisions.md index 9d9a2b6..f55582f 100644 --- a/docs/design-decisions.md +++ b/docs/design-decisions.md @@ -234,6 +234,78 @@ caller-shaping (projection + null-fill of columns added by a later caller-shaped entry carries the schema of whichever branch decoded first, so the second branch's read fails on a type mismatch its own metadata never justified. +Concretely, with a file holding `{row_uuid, name, value}` that both branches +reference, parent having run `ADD COLUMN extra Int64` and fork `ADD COLUMN extra +Utf8`: + + caching the shaped batch — the bug + fork miss → decode + fill → {row_uuid, name, value, extra: Utf8} + parent HIT → gets extra: Utf8, its own metadata says Int64 ✗ + + caching the native decode — what ships + fork miss → decode → {row_uuid, name, value} + → shape to fork → extra: Utf8 ✓ + parent HIT → {row_uuid, name, value} + → shape to parent → extra: Int64 ✓ + +The read-time schema still governs the output — it just governs it at the +shaping step, per caller, rather than at the decode. Only the decode has to be +segment-scoped, because only the decode is shared. + +### Rejected alternative: fold the read schema into the key + +The symmetric design adds a fingerprint of the read-time schema to the key and +caches a value already adapted to it. Both halves are known before the read, so +it is implementable, and it is correct: the fingerprint separates the two +branches above. It comes in two strengths, and the weaker one is not worth +arguing against. + +Caching the fully **shaped** table — projection included — fragments the key +space by query: `SELECT name` and `SELECT name, value` over one segment become +different entries, combinatorial in the column subsets queries touch. That is +decisive but it is also avoidable, so it is not the real comparison. + +The strong form caches the **type-cast** table: present columns cast to the +read-time schema's declared types, with projection and null-fill still applied +per caller after the lookup. Its cost is that the key still moves when the +schema does, and only the *key* moves — the data does not: + +- **`ALTER TABLE ADD COLUMN` evicts a footprint that did not change.** The added + column is in no file and affects no cast, yet it re-fingerprints every segment + of the table, so the whole cold footprint is re-fetched and re-decoded without + a byte having been rewritten. Narrowing the fingerprint to only the columns a + file actually holds would fix this, but which columns those are is not known + until after the read. +- **It collapses the dedup this entry exists for.** A fork and its parent share + entries only while their schemas agree; the first `ALTER` on either side + duplicates the entire shared footprint — the sharing degrades exactly when a + branch is used for what branches are for. That is the motivating case, not an + edge case. + +Its advantage is real but currently unrealizable: a cached cast would let a read +succeed where the file's decoded type differs from the declared one, which the +shipped path cannot do — `null_fill_to_schema` takes present columns verbatim and +`RecordBatch::try_new` then rejects a mismatch. Penca's schema evolution is +`ADD COLUMN` only, so no supported operation produces that divergence. If type +evolution is added later, the answer is to cast in the shaping tail, where it +applies per caller and is visible as a data-semantics decision — not to hide it +behind a cache key. + +### What directs the decode today, and what should + +Neither reader consults the stored schema. Parquet decodes under the file's +embedded Arrow schema (`builder.schema()`) and Lance under +`reader.metadata().file_schema`; the caller's `SchemaRef` only selects column +*names* to request (`requested_columns`) and drives the shaping tail. No cast or +type coercion happens anywhere in `penca-format`'s readers or writers, so the +decoded types are whatever the encoder chose to write and the decoder chose to +return. + +That is why `format` is in the key: the file's embedded schema is a per-format +artifact, and `shape_to_schema` cannot absorb a divergence — `null_fill_to_schema` +takes present columns by name verbatim, and `RecordBatch::try_new` then validates +their types against the output schema, so a mismatch is a hard read error. + **Why the scope is uniform across artifact classes.** Base segments and index sidecars both come out of object storage through the same `SegmentCache` and are both reference-copied by carry-forward and fork copy, so they duplicate for the @@ -255,3 +327,18 @@ digest. It is not available yet: the digest is taken on the in-memory batch before the format writer encodes it, and Parquet may widen a type or re-dictionary-encode on round-trip, so using it as a stored-file checksum needs round-trip stability established first. + +**Open question.** The decode should be directed by the segment's *stored* +write-time schema rather than by the file's embedded one. `__penca_system__.tables` +already holds it — `arrow_schema` is Arrow IPC and every row is a complete table +definition at a point in time — and a segment row carries `branch_uuid`, +`table_uuid`, and `max_commit_seq_num`, so the definition in force when it was +written is an as-of read away. Doing that would make the decoded schema a function +of metadata instead of of the encoder's round-trip behavior, let a file whose +physical schema contradicts its declared one be rejected rather than silently +trusted, and make `format` droppable from the cache key — both formats would +decode to the same declared types, so `content_hash` would name one decode on its +own. It must be the segment's write-time schema and not the reader's: the cache +key is a property of the segment alone, so anything that varies per reader cannot +direct a shared decode. The cost is resolving that schema per segment, which is +plumbing through the read path rather than a change local to `penca-format`. From 2a6cd96dc76634c823f14a0261b57fcadaf009a6 Mon Sep 17 00:00:00 2001 From: Nico Bautista Hobin Date: Fri, 31 Jul 2026 19:02:25 +0000 Subject: [PATCH 30/32] refactor(cold): drop the format from the segment cache key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit e7e68c7 paired the storage format with content_hash on the argument that the cached file-native decode is a per-format artifact — a round trip may widen a type or re-dictionary-encode. That inverts the contract. Writing an Arrow batch and reading it back is supposed to return that batch, so Parquet and Lance decoding one content_hash differently would mean one of them is wrong, not that the cache should model the difference. Nothing asserts the contract today (format_reader.rs checks row and column counts through the shaped path only), but keying around an unverified defect costs a real cross-format share permanently and would mask the defect if it existed — the cross-format type mismatch is exactly the signal that should be loud. CHA-548 adds round-trip identity coverage driven from CanonicalType. SegmentCacheKey is deleted; the key is Uuid again. Drops same_hash_under_two_formats_does_not_share_an_entry, which asserted the behavior being removed. CHA-545 Co-Authored-By: Claude Opus 5 --- crates/penca-dl/src/cache.rs | 116 ++++++++------------------ crates/penca-dl/src/driver.rs | 38 +++------ crates/penca-format/src/reader/mod.rs | 8 +- docs/design-decisions.md | 57 ++++++++----- docs/schema-reference.md | 6 +- 5 files changed, 92 insertions(+), 133 deletions(-) diff --git a/crates/penca-dl/src/cache.rs b/crates/penca-dl/src/cache.rs index de7eeb2..bfd2ff1 100644 --- a/crates/penca-dl/src/cache.rs +++ b/crates/penca-dl/src/cache.rs @@ -3,10 +3,9 @@ //! A repeat read of the same cold segment within the process lifetime is served //! as an `Arc::clone` of the already-decoded Arrow batches, skipping the S3 GET + //! Parquet/Lance decode. It holds snapshot segments, persist data segments and -//! index sidecars under one byte budget, all keyed by `(content_hash, format)`. -//! The mapping is stable and needs no invalidation: that pair names one -//! file-native decode by construction, and cold artifacts are immutable — -//! although a *resolved +//! index sidecars under one byte budget, all keyed by `content_hash`. +//! The mapping is stable and needs no invalidation: the hash names one decode +//! by construction, and cold artifacts are immutable — although a *resolved //! persist tier* is mutable under retention compaction, an individual persist //! *file* is not. There is no TTL — immutability makes W-TinyLFU eviction the //! whole reclaim mechanism for every tier. @@ -31,28 +30,8 @@ use arrow::record_batch::RecordBatch; use moka::sync::Cache; use uuid::Uuid; -/// What names one cached decode: the content hash of the typed batch, paired -/// with the wire code of the format the file it landed in is written in. -/// -/// See [`SegmentCache`] for why both halves are needed and why the format is -/// part of the key rather than part of the digest. -#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)] -pub struct SegmentCacheKey { - pub content_hash: Uuid, - pub format_code: i32, -} - -impl SegmentCacheKey { - pub fn new(content_hash: Uuid, format_code: i32) -> Self { - Self { - content_hash, - format_code, - } - } -} - -/// In-process W-TinyLFU cache of decoded cold segments, keyed by -/// [`SegmentCacheKey`] and bounded by a byte budget. +/// In-process W-TinyLFU cache of decoded cold segments, keyed by `content_hash` +/// and bounded by a byte budget. /// /// `content_hash` is the digest of the typed in-memory Arrow batch, recorded /// once at write time and inherited verbatim by every reference copy — snapshot @@ -84,14 +63,16 @@ impl SegmentCacheKey { /// correct answer rather than a bug — a base segment and a sidecar that decode /// to the same batch may safely share. /// -/// The key is `(content_hash, format)`, not the hash alone. `content_hash` -/// digests the typed batch *before* a `FormatWriter` encodes it, so one hash can -/// name files in two formats once `OBJECT_STORAGE_FORMAT` has been flipped — -/// while the value is deliberately the *file-native* decode, a per-format -/// artifact (a round-trip may widen a type or re-dictionary-encode). Keying on -/// content alone would serve one format's decode for the other format's file. -/// Folding the format into the key rather than into the digest is what keeps -/// `content_hash` a pure function of the batch. +/// The storage format is deliberately *not* in the key. `content_hash` digests +/// the typed batch *before* a `FormatWriter` encodes it, so after an +/// `OBJECT_STORAGE_FORMAT` flip one hash can name both a Parquet file and a +/// Lance file — and they are supposed to decode to the same batch, since a write +/// followed by a read returns what was written. A divergence there is a broken +/// round trip to fix in that format (or a type to drop from `CanonicalType`), not +/// a fact for this key to encode: adding the format would trade one real +/// cross-format share for a permanent second entry, and would hide the defect +/// rather than surface it. CHA-548 adds the round-trip identity coverage that +/// asserts the invariant this key assumes. /// /// Cheaply cloneable: `moka::sync::Cache` is internally an `Arc`, so callers /// typically hold a `SegmentCache` behind one outer `Arc` shared @@ -99,7 +80,7 @@ impl SegmentCacheKey { pub struct SegmentCache { /// The value carries its own weight so the weigher can charge the /// caller-supplied `size_bytes` rather than the batch's runtime memory. - inner: Cache, u32)>, + inner: Cache, u32)>, budget_bytes: u64, } @@ -114,7 +95,7 @@ impl SegmentCache { .max_capacity(budget_bytes) // Weight in the same byte unit as `max_capacity` so the budget is a // real RAM bound; the stored `u32` is the entry's `size_bytes`. - .weigher(|_key: &SegmentCacheKey, (_batch, weight): &(Arc, u32)| *weight) + .weigher(|_key: &Uuid, (_batch, weight): &(Arc, u32)| *weight) .build(); Self { inner, @@ -156,18 +137,19 @@ impl SegmentCache { /// Fetch a decoded segment, bumping its frequency estimate. A hit is an /// `Arc::clone` — no buffer copy. - pub fn get(&self, key: &SegmentCacheKey) -> Option> { - self.inner.get(key).map(|(batch, _weight)| batch) + pub fn get(&self, content_hash: &Uuid) -> Option> { + self.inner.get(content_hash).map(|(batch, _weight)| batch) } /// Insert a decoded segment charged `weight_bytes` against the budget. No-op /// when the segment is not [`admits`](Self::admits)-ible. moka enforces /// `max_capacity` via W-TinyLFU; there is no manual eviction loop here. - pub fn insert(&self, key: SegmentCacheKey, batch: Arc, weight_bytes: u64) { + pub fn insert(&self, content_hash: Uuid, batch: Arc, weight_bytes: u64) { if !self.admits(weight_bytes) { return; } - self.inner.insert(key, (batch, weight_bytes as u32)); + self.inner + .insert(content_hash, (batch, weight_bytes as u32)); } /// Force pending eviction/maintenance to run synchronously. moka does @@ -193,10 +175,9 @@ mod tests { use arrow::array::Int32Array; use arrow::datatypes::{DataType, Field, Schema}; use arrow::record_batch::RecordBatch; - use penca_core::Format; use uuid::Uuid; - use super::{SegmentCache, SegmentCacheKey}; + use super::SegmentCache; /// One-column batch of `n` i32 rows; a stand-in decoded segment. fn batch(n: usize) -> Arc { @@ -205,9 +186,9 @@ mod tests { Arc::new(RecordBatch::try_new(schema, vec![Arc::new(col)]).unwrap()) } - /// A Parquet key over hash `n`, for tests that only exercise the hash half. - fn parquet(n: u128) -> SegmentCacheKey { - SegmentCacheKey::new(Uuid::from_u128(n), Format::Parquet.as_wire_code()) + /// A distinct content hash per `n`. + fn hash(n: u128) -> Uuid { + Uuid::from_u128(n) } #[test] @@ -228,10 +209,10 @@ mod tests { #[test] fn zero_weight_segment_is_not_cached() { let cache = SegmentCache::new(1 << 20); - cache.insert(parquet(1), batch(4), 0); + cache.insert(hash(1), batch(4), 0); cache.run_pending(); assert!( - cache.get(&parquet(1)).is_none(), + cache.get(&hash(1)).is_none(), "weight-0 segment must not be pinned in the cache" ); } @@ -251,10 +232,10 @@ mod tests { ); assert!(cache.admits(u32::MAX as u64), "exactly u32::MAX is fine"); - cache.insert(parquet(2), batch(8), over_u32); + cache.insert(hash(2), batch(8), over_u32); cache.run_pending(); assert!( - cache.get(&parquet(2)).is_none(), + cache.get(&hash(2)).is_none(), "over-u32 weight never stored" ); } @@ -262,14 +243,14 @@ mod tests { #[test] fn over_budget_insert_is_noop() { let cache = SegmentCache::new(100); - cache.insert(parquet(3), batch(8), 200); + cache.insert(hash(3), batch(8), 200); cache.run_pending(); - assert!(cache.get(&parquet(3)).is_none(), "over-budget never stored"); + assert!(cache.get(&hash(3)).is_none(), "over-budget never stored"); let disabled = SegmentCache::disabled(); - disabled.insert(parquet(4), batch(8), 1); + disabled.insert(hash(4), batch(8), 1); disabled.run_pending(); - assert!(disabled.get(&parquet(4)).is_none(), "disabled never stores"); + assert!(disabled.get(&hash(4)).is_none(), "disabled never stores"); } #[test] @@ -280,7 +261,7 @@ mod tests { // moka's W-TinyLFU choice, not Penca's contract). let cache = SegmentCache::new(100); for i in 0..5 { - cache.insert(parquet(i), batch(10), 40); + cache.insert(hash(i), batch(10), 40); } cache.run_pending(); assert!( @@ -290,36 +271,13 @@ mod tests { ); } - /// `content_hash` digests the batch before the writer encodes it, so one - /// hash can name a Parquet file and a Lance file after an - /// `OBJECT_STORAGE_FORMAT` flip. Their file-native decodes are different - /// artifacts, so the format has to separate them. - #[test] - fn same_hash_under_two_formats_does_not_share_an_entry() { - let cache = SegmentCache::new(1_000); - let shared = Uuid::from_u128(6); - let as_parquet = SegmentCacheKey::new(shared, Format::Parquet.as_wire_code()); - let as_lance = SegmentCacheKey::new(shared, Format::Lance.as_wire_code()); - cache.insert(as_parquet, batch(4), 40); - cache.run_pending(); - - assert!( - cache.get(&as_lance).is_none(), - "a Lance row must not be served the Parquet decode" - ); - assert!( - cache.get(&as_parquet).is_some(), - "its own format still hits" - ); - } - #[test] fn hit_returns_arc_clone_same_buffers() { let cache = SegmentCache::new(1_000); let original = batch(16); - cache.insert(parquet(5), original.clone(), 40); + cache.insert(hash(5), original.clone(), 40); cache.run_pending(); - let hit = cache.get(&parquet(5)).expect("cached"); + let hit = cache.get(&hash(5)).expect("cached"); // Same backing column buffer — a hit is a refcount bump, no copy. assert_eq!( original.column(0).to_data().buffers()[0].as_ptr(), diff --git a/crates/penca-dl/src/driver.rs b/crates/penca-dl/src/driver.rs index 8dc5c2c..7089950 100644 --- a/crates/penca-dl/src/driver.rs +++ b/crates/penca-dl/src/driver.rs @@ -24,7 +24,7 @@ use penca_storage_cold::{COMMIT_SEQ_NUM_COLUMN, ColdStorageError}; use tracing::Instrument as _; use uuid::Uuid; -use crate::cache::{SegmentCache, SegmentCacheKey}; +use crate::cache::SegmentCache; use crate::provider::{build_persist_session, build_snapshot_session}; use crate::schema::LogSchemas; use crate::session_template::derive_cold_session; @@ -225,7 +225,7 @@ async fn decode_and_cache_native( uri: &str, offset: Option, length: Option, - key: SegmentCacheKey, + content_hash: Uuid, weight: u64, ) -> Result, DlError> { let batch = reader @@ -233,7 +233,7 @@ async fn decode_and_cache_native( .await .map_err(ColdStorageError::from)?; let batch = Arc::new(batch); - cache.insert(key, Arc::clone(&batch), weight); + cache.insert(content_hash, Arc::clone(&batch), weight); tracing::debug!(rows = batch.num_rows(), "segment decoded and cached"); Ok(batch) } @@ -314,9 +314,8 @@ async fn read_cached_snapshot_segment_unshaped( ) -> Result { let span = tracing::Span::current(); let code = segment.format.as_wire_code(); - let key = SegmentCacheKey::new(segment.content_hash, code); - if let Some(full) = cache.get(&key) { + if let Some(full) = cache.get(&segment.content_hash) { span.record("cache", "hit"); tracing::debug!(rows = full.num_rows(), "snapshot segment cache hit"); return Ok(CachedSegment::Native(full)); @@ -338,7 +337,7 @@ async fn read_cached_snapshot_segment_unshaped( &segment.uri, Some(segment.offset), Some(segment.length), - key, + segment.content_hash, weight, ) .await?, @@ -451,9 +450,7 @@ pub(crate) async fn read_cached_persist_segment( let span = tracing::Span::current(); let code = segment.format.as_wire_code(); - let key = SegmentCacheKey::new(segment.content_hash, code); - - if let Some(full) = cache.get(&key) { + if let Some(full) = cache.get(&segment.content_hash) { span.record("cache", "hit"); tracing::debug!(rows = full.num_rows(), "persist segment cache hit"); return shape_native(&full, full_schema); @@ -475,7 +472,7 @@ pub(crate) async fn read_cached_persist_segment( &segment.uri, segment.offset, segment.length, - key, + segment.content_hash, weight, ) .await?; @@ -514,11 +511,10 @@ async fn read_cached_index_sidecar( ) -> Result { let span = tracing::Span::current(); let code = sidecar.format.as_wire_code(); - let key = SegmentCacheKey::new(sidecar.content_hash, code); // The sidecar's key schema is the indexed columns' native types; the // identity/name sidecars are the all-Utf8 special case. let schema = penca_format::index::segment_index_schema(key_types); - if let Some(batch) = cache.get(&key) { + if let Some(batch) = cache.get(&sidecar.content_hash) { span.record("cache", "hit"); return shape_native(&batch, &schema); } @@ -545,7 +541,7 @@ async fn read_cached_index_sidecar( &sidecar.object_uri, Some(sidecar.offset), Some(sidecar.length), - key, + sidecar.content_hash, weight, ) .await?; @@ -2040,12 +2036,7 @@ mod tests { "oversized segment is never cached — both accesses re-read storage" ); assert!( - cache - .get(&SegmentCacheKey::new( - seg.content_hash, - seg.format.as_wire_code() - )) - .is_none(), + cache.get(&seg.content_hash).is_none(), "oversized segment not stored" ); } @@ -2067,14 +2058,7 @@ mod tests { // moka evicted one of {a,b} to honor the budget (we don't assert which // — that is moka's W-TinyLFU choice). Re-reading the evicted key must // hit storage again. - let a = segment("a", 150); - let evicted = if cache - .get(&SegmentCacheKey::new( - a.content_hash, - a.format.as_wire_code(), - )) - .is_none() - { + let evicted = if cache.get(&segment("a", 150).content_hash).is_none() { "a" } else { "b" diff --git a/crates/penca-format/src/reader/mod.rs b/crates/penca-format/src/reader/mod.rs index eb1c3b4..9c55ed0 100644 --- a/crates/penca-format/src/reader/mod.rs +++ b/crates/penca-format/src/reader/mod.rs @@ -63,9 +63,11 @@ pub trait FormatReader: Send + Sync { /// /// "The file's own types" means the schema the format engine embedded and /// returns, not the table's stored `arrow_schema` — nothing here casts, so - /// the types are the encoder's answer rather than the catalog's. That is why - /// `SegmentCache` keys on the format alongside the content hash; see the - /// open question in `docs/design-decisions.md`. + /// the types are the encoder's answer rather than the catalog's. Both + /// encoders are expected to give the same answer, which is what lets + /// `SegmentCache` key on the content hash alone; CHA-548 is the coverage + /// that asserts it, and the open question in `docs/design-decisions.md` is + /// what would make it structural. /// /// Shape the result to a caller's schema afterwards with /// [`shape_to_schema`] — the same tail `read_segment` runs internally. diff --git a/docs/design-decisions.md b/docs/design-decisions.md index f55582f..271c155 100644 --- a/docs/design-decisions.md +++ b/docs/design-decisions.md @@ -188,8 +188,8 @@ shape. Every cold-artifact metadata row — persist segments, snapshot segments, and segment index sidecars — 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 -`(content_hash, format)`. +(`penca_core::digest::segment_content_hash`). `SegmentCache` is keyed by that +hash alone. **The problem.** Reference copies mint a new row uuid over an unchanged `(object_uri, offset, length)`: `insert_carried_snapshot_segments` and @@ -209,14 +209,25 @@ It also makes dedup insensitive to encoding choices, so two independently writte segments holding the same rows share an entry even if the writer picked different row-group boundaries or dictionary encodings. -**Why `format` is the other half of the key.** Digesting the batch pre-encode is -exactly what lets one hash name files in two formats, once +**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. -The cached *value*, meanwhile, is the file-native decode — a per-format artifact, -since a round-trip may widen a type or re-dictionary-encode. So the format joins -the cache key rather than the digest: the pair names one native decode, and -`content_hash` stays a pure function of the batch, which is what lets a reference -copy inherit it verbatim. +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 Parquet/Lance divergence +would not be a fact for the key to record; it would mean one format's round trip +is *wrong*, which is fixed in that format (or by dropping the offending type from +`CanonicalType`), not absorbed by a wider key. + +That contract is currently unasserted: +`crates/penca-format/tests/format_reader.rs` checks row and column counts through +the shaped path, nothing compares a decoded batch against the batch that was +written, and nothing compares the two formats against each other. CHA-548 adds +that coverage. Pairing the format into the key in the meantime was considered and +rejected — it would permanently give up a real cross-format share to pre-empt a +defect nobody has demonstrated, and if the defect did exist it would mask it, +turning the loud cross-format type mismatch that CHA-548 is meant to produce into +silence. **What the key deliberately does *not* do is separate a reference copy from its source.** Carry-forward and fork copy select `old.content_hash` verbatim @@ -301,10 +312,13 @@ type coercion happens anywhere in `penca-format`'s readers or writers, so the decoded types are whatever the encoder chose to write and the decoder chose to return. -That is why `format` is in the key: the file's embedded schema is a per-format -artifact, and `shape_to_schema` cannot absorb a divergence — `null_fill_to_schema` +That is what the cache key assumes away: the decoded types are the encoder's +answer, and one key for one hash trusts both encoders to give the same answer. +`shape_to_schema` would not absorb a divergence either — `null_fill_to_schema` takes present columns by name verbatim, and `RecordBatch::try_new` then validates -their types against the output schema, so a mismatch is a hard read error. +their types against the output schema, so a mismatch is a hard read error, not a +cast. CHA-548 tests the assumption; directing the decode from the stored schema +(below) would remove the need for it. **Why the scope is uniform across artifact classes.** Base segments and index sidecars both come out of object storage through the same `SegmentCache` and are @@ -324,9 +338,9 @@ never cache-read and never reference-copied. **Open question.** CHA-545 names checksum reuse 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 it, and Parquet may widen a type or -re-dictionary-encode on round-trip, so using it as a stored-file checksum needs -round-trip stability established first. +before the format writer encodes it, so verifying a stored file against it means +re-deriving the digest from a decode — sound only once round-trip identity is +established. CHA-548 is that gate too. **Open question.** The decode should be directed by the segment's *stored* write-time schema rather than by the file's embedded one. `__penca_system__.tables` @@ -336,9 +350,10 @@ definition at a point in time — and a segment row carries `branch_uuid`, written is an as-of read away. Doing that would make the decoded schema a function of metadata instead of of the encoder's round-trip behavior, let a file whose physical schema contradicts its declared one be rejected rather than silently -trusted, and make `format` droppable from the cache key — both formats would -decode to the same declared types, so `content_hash` would name one decode on its -own. It must be the segment's write-time schema and not the reader's: the cache -key is a property of the segment alone, so anything that varies per reader cannot -direct a shared decode. The cost is resolving that schema per segment, which is -plumbing through the read path rather than a change local to `penca-format`. +trusted, and turn the cross-format assumption above into a guarantee rather than +something CHA-548 has to keep testing — both formats would decode to the declared +types by construction. It must be the segment's write-time schema and not the +reader's: the cache key is a property of the segment alone, so anything that +varies per reader cannot direct a shared decode. The cost is resolving that +schema per segment, which is plumbing through the read path rather than a change +local to `penca-format`. diff --git a/docs/schema-reference.md b/docs/schema-reference.md index d6858b6..5cbdde3 100644 --- a/docs/schema-reference.md +++ b/docs/schema-reference.md @@ -378,7 +378,7 @@ the `log_kind` classification (CHA-218: only `upsert_log` and | `length` | int64 (set at compact time) | | `row_count` | int64 | | `format` | text (parquet, lance) | -| `content_hash` | UUID NOT NULL (CHA-545, segment-cache key with `format`) | +| `content_hash` | UUID NOT NULL (CHA-545, segment-cache key) | | `size_bytes` | int64 | | `metadata` | JSONB | | `statistics` | JSONB | @@ -482,7 +482,7 @@ completes). | `length` | int64 NOT NULL (row count of the range) | | `size_bytes` | int64 | | `format` | text (lance, parquet) | -| `content_hash` | UUID NOT NULL (CHA-545, segment-cache key with `format` — see table 14) | +| `content_hash` | UUID NOT NULL (CHA-545, segment-cache key — see table 14) | | `metadata` | JSON (format-specific, e.g., row group size) | | `statistics` | JSON (column stats: min/max for filterable columns) | | `row_count` | int64 | @@ -539,7 +539,7 @@ forward by reference with its base segment and participates in the ref-counted G | `offset` | int64 | | `length` | int64 | | `format` | text (lance, parquet) | -| `content_hash` | UUID NOT NULL (CHA-545, segment-cache key with `format` — see table 14) | +| `content_hash` | UUID NOT NULL (CHA-545, segment-cache key — see table 14) | | `size_bytes` | int64 | | `statistics` | bytes (indexed-key min/max bounds; binary, decoded in-planner by the CHA-454 seek in the `SnapshotTableProvider`) | | `written_at_micros` | int64 (micros, auto-generated) | From 2b57ac735e58aa39d73bc63aba36288067bd224f Mon Sep 17 00:00:00 2001 From: Nico Bautista Hobin Date: Fri, 31 Jul 2026 20:07:35 +0000 Subject: [PATCH 31/32] perf(cold): compact byte-view buffers before hashing a segment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. `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` the byte-view columns first, descending one level into a list child since `CanonicalType` admits views there and rejects deeper nesting. That bounds each digest to its own rows and makes it canonical for these types: two segments holding the same values now agree even when their parents laid those values out differently, which is the dedup the digest exists for. The contract loses its slice-invariance exception. CHA-545 --- crates/penca-core/src/digest.rs | 286 +++++++++++++++++++++++++------- 1 file changed, 227 insertions(+), 59 deletions(-) diff --git a/crates/penca-core/src/digest.rs b/crates/penca-core/src/digest.rs index a8a1f2a..9c24817 100644 --- a/crates/penca-core/src/digest.rs +++ b/crates/penca-core/src/digest.rs @@ -1,6 +1,13 @@ //! Content digest of a decoded cold segment (CHA-545). -use arrow::array::RecordBatch; +use std::sync::Arc; + +use arrow::array::{ + Array, ArrayRef, BinaryViewArray, FixedSizeListArray, GenericListArray, LargeListArray, + ListArray, OffsetSizeTrait, RecordBatch, StringViewArray, +}; +use arrow::buffer::OffsetBuffer; +use arrow::datatypes::{DataType, FieldRef}; use arrow::error::ArrowError; use arrow::ipc::MetadataVersion; use arrow::ipc::writer::{DictionaryHandling, IpcWriteOptions, StreamWriter}; @@ -36,28 +43,16 @@ const IPC_ALIGNMENT: usize = 8; /// reuse as a possible future use of this digest; that use requires /// establishing round-trip stability first, and this function does not. /// -/// **Slice-invariant except for view-encoded columns.** A batch from `slice()` -/// stays a view onto its parent's buffers, which physically hold bytes outside -/// the slice, but the IPC writer encodes only the logical range. Measured on -/// arrow-rs 57.3 across the supported set (see [`crate::types`]): primitives, -/// decimals, dates/times/timestamps, `Boolean` at a non-byte-aligned offset, -/// `Utf8`/`LargeUtf8`/`Binary`, and `List`/`LargeList`/`FixedSizeList` all hold. -/// -/// `Utf8View`/`BinaryView` do not, top-level or as a list child: the writer -/// truncates the *views* buffer but emits each variadic data buffer whole, so a -/// slice carries bytes belonging to rows it does not cover. The condition is a -/// property of the parent array, not of the sliced rows — a single value above -/// the 12-byte inline threshold anywhere in the parent gives the array data -/// buffers, and every slice of it then carries them in full. Invariance holds -/// only when *no* value in the parent is longer than that. `Dictionary` fails -/// the same way under `DictionaryHandling::Resend` but is rejected at the type -/// boundary and cannot reach here. -/// -/// Neither costs correctness, and neither costs the dedup this digest exists -/// for: a reference copy *inherits* the stored hash rather than recomputing it, -/// so fork and copy sharing is unaffected. What is lost is dedup between two -/// segments written independently whose identical rows happened to be sliced out -/// of differently-shaped parent batches. +/// **Slice-invariant.** A batch from `slice()` stays a view onto its parent's +/// buffers, which physically hold bytes outside the slice, but the IPC writer +/// encodes only the logical range. Measured on arrow-rs 57.3 across the +/// supported set (see [`crate::types`]): primitives, decimals, +/// dates/times/timestamps, `Boolean` at a non-byte-aligned offset, +/// `Utf8`/`LargeUtf8`/`Binary`, and `List`/`LargeList`/`FixedSizeList` all hold +/// unaided. `Utf8View`/`BinaryView` do not, and [`compact_view_buffers`] below +/// is what makes them. `Dictionary` fails the same way under +/// `DictionaryHandling::Resend` but is rejected at the type boundary and cannot +/// reach here. /// /// Changing the IPC options or the hash changes every future digest. That is /// safe — stored digests are opaque identity, never recomputed and compared @@ -69,15 +64,123 @@ pub fn segment_content_hash(batch: &RecordBatch) -> Result { // today, and a future flip to `Delta` would make a batch's digest // depend on what the writer had already emitted. .with_dictionary_handling(DictionaryHandling::Resend); + let compacted = compact_view_buffers(batch)?; let mut bytes = Vec::new(); - let mut writer = StreamWriter::try_new_with_options(&mut bytes, &batch.schema(), options)?; - writer.write(batch)?; + let mut writer = StreamWriter::try_new_with_options(&mut bytes, &compacted.schema(), options)?; + writer.write(&compacted)?; writer.finish()?; drop(writer); Ok(Uuid::from_u128(xxh3_128(&bytes))) } +/// Rebuild every byte-view column so its data buffers hold only the rows the +/// batch covers. +/// +/// The IPC writer truncates each buffer it can, with one exception: for +/// `Utf8View`/`BinaryView` it emits every variadic data buffer **whole**, since +/// proving no surviving view still points into a pruned buffer is expensive +/// (arrow-ipc 57.3 `writer::write_array_data` says so and points at `gc`). A +/// digest of a slice would otherwise encode the whole parent's string payload, +/// and both `compact` and the snapshot packer take one digest per slice of a +/// single `concat_batches` result — so a wave of `N` inputs would re-serialize +/// every input's bytes `N` times, on exactly the many-small-segments workload +/// compaction exists for. +/// +/// Compacting first bounds each digest to its own rows. It also makes the +/// digest *canonical* for these types rather than merely cheaper: `gc` lays the +/// referenced bytes out in row order in buffers sized by the values alone, so +/// two arrays holding the same values agree even when their parents laid those +/// values out differently — which is the dedup this digest exists for. +/// +/// [`CanonicalType`](crate::types::CanonicalType) admits a view at the top level +/// or as the child of a single-level list, and rejects nesting below that, so +/// one level of descent is total over the supported set. +fn compact_view_buffers(batch: &RecordBatch) -> Result { + if !batch + .schema() + .fields() + .iter() + .any(|f| holds_byte_view(f.data_type())) + { + return Ok(batch.clone()); + } + let columns = batch + .columns() + .iter() + .map(compact_column) + .collect::, _>>()?; + // `try_new` re-validates against the schema, which is what catches a + // compaction that changed a length or a type rather than just a layout. + RecordBatch::try_new(batch.schema(), columns) +} + +/// Whether a column of this type carries variadic data buffers the IPC writer +/// would emit whole. +fn holds_byte_view(dt: &DataType) -> bool { + match dt { + DataType::Utf8View | DataType::BinaryView => true, + DataType::List(child) | DataType::LargeList(child) | DataType::FixedSizeList(child, _) => { + matches!(child.data_type(), DataType::Utf8View | DataType::BinaryView) + } + _ => false, + } +} + +fn compact_column(array: &ArrayRef) -> Result { + match array.data_type() { + DataType::Utf8View => Ok(Arc::new(downcast::(array)?.gc())), + DataType::BinaryView => Ok(Arc::new(downcast::(array)?.gc())), + DataType::List(child) if holds_byte_view(array.data_type()) => { + compact_list(downcast::(array)?, child) + } + DataType::LargeList(child) if holds_byte_view(array.data_type()) => { + compact_list(downcast::(array)?, child) + } + DataType::FixedSizeList(child, size) if holds_byte_view(array.data_type()) => { + // Unlike the offset flavours, `FixedSizeListArray::slice` slices its + // child too, so `values()` is already this array's own range. + let list = downcast::(array)?; + Ok(Arc::new(FixedSizeListArray::try_new( + child.clone(), + *size, + compact_column(list.values())?, + list.nulls().cloned(), + )?)) + } + _ => Ok(Arc::clone(array)), + } +} + +/// `GenericListArray::slice` keeps the whole child and slices only the offsets, +/// so compacting the child as-is would compact rows this list does not cover. +/// Narrow it to the range the offsets span, then rebase them onto it. +fn compact_list( + list: &GenericListArray, + child: &FieldRef, +) -> Result { + let offsets = list.offsets(); + let start = offsets[0].as_usize(); + let end = offsets[list.len()].as_usize(); + let values = compact_column(&list.values().slice(start, end - start))?; + let rebased: Vec = offsets.iter().map(|o| *o - offsets[0]).collect(); + Ok(Arc::new(GenericListArray::::try_new( + child.clone(), + OffsetBuffer::new(rebased.into()), + values, + list.nulls().cloned(), + )?)) +} + +fn downcast(array: &ArrayRef) -> Result<&A, ArrowError> { + array.as_any().downcast_ref::().ok_or_else(|| { + ArrowError::InvalidArgumentError(format!( + "{} column is not the array kind its type declares", + array.data_type() + )) + }) +} + #[cfg(test)] mod tests { use std::sync::Arc; @@ -250,7 +353,7 @@ mod tests { ); } - /// The nested twin, and the reason `segment_content_hash` needs no + /// The nested twin, and the reason a `List` of a non-view child needs no /// compaction step. Slicing a `List` rebases the offsets buffer but leaves /// the *child* array whole — the guard below pins that it still holds all /// four rows' values, shared with the parent — and the IPC writer still @@ -291,45 +394,110 @@ mod tests { ); } - /// The one exception in the contract above, pinned so the contract cannot go - /// stale: arrow-rs 57.3 truncates a view array's *views* buffer on slice but - /// emits each variadic data buffer whole. - /// - /// A failure here means arrow started compacting those buffers — delete this - /// test and the exception paragraph it guards. + /// Above the 12-byte inline threshold, so the values live in a data buffer + /// rather than in the view word itself — which is the only case where a + /// view array has variadic buffers to carry. + const LONG: [&str; 3] = [ + "aaaaaaaaaaaaaaaaaaaa", + "bbbbbbbbbbbbbbbbbbbb", + "cccccccccccccccccccc", + ]; + + fn views(v: Vec<&str>) -> RecordBatch { + batch( + vec![Field::new("s", DataType::Utf8View, true)], + vec![Arc::new(StringViewArray::from(v))], + ) + } + + /// The type the IPC writer will *not* truncate: it emits each variadic data + /// buffer whole rather than prove no surviving view still points into a + /// pruned one, so a slice's encoding would otherwise carry bytes belonging + /// to rows it does not cover. [`compact_view_buffers`] is what closes that, + /// so the fixture guard asserting the slice really does still share the + /// parent's data buffer is what makes the equality below non-trivial. #[test] - fn sliced_view_column_is_the_documented_non_invariant_case() { - let view = |v: Vec<&str>| { + fn sliced_view_column_hashes_equal_to_an_independently_built_batch() { + let parent = views(LONG.to_vec()); + let sliced = parent.slice(1, 2); + + assert_eq!( + parent.column(0).to_data().buffers()[1].as_ptr(), + sliced.column(0).to_data().buffers()[1].as_ptr(), + "fixture must still share the parent's data buffer, or this proves nothing" + ); + assert_eq!( + hash(&sliced), + hash(&views(vec![LONG[1], LONG[2]])), + "a sliced view column hashes as its own logical rows" + ); + // One long value anywhere in the parent gives the array a data buffer, + // so a slice of only *short* rows carries it too — compaction has to be + // driven by the parent's layout, not by the sliced rows' lengths. + assert_eq!( + hash(&views(vec!["aa", "bb", LONG[0]]).slice(0, 2)), + hash(&views(vec!["aa", "bb"])), + "an all-inline slice of a buffer-carrying parent is invariant too" + ); + } + + /// Independently written segments holding the same rows must agree even when + /// their parents laid the bytes out differently — that agreement *is* the + /// dedup. Views make this a real risk rather than a tautology: the same + /// values reached through different builders can sit at different buffer + /// offsets, which `gc` normalizes away. + #[test] + fn view_column_hashes_equal_across_differently_laid_out_parents() { + assert_eq!( + hash(&views(vec![LONG[0], LONG[1]])), + hash(&views(vec![LONG[2], LONG[0], LONG[1]]).slice(1, 2)), + "same values, different parent layout, one hash" + ); + } + + /// The nested case. [`crate::types::CanonicalType`] admits a view as the + /// child of a single-level list, where slicing leaves *both* the child array + /// and its data buffers whole — so the compaction has to narrow the child to + /// the offsets' range before it can help. + #[test] + fn sliced_list_of_view_column_hashes_equal_to_an_independently_built_batch() { + fn list_of_views(rows: Vec>) -> RecordBatch { + let child = Arc::new(Field::new("item", DataType::Utf8View, true)); + let mut values: Vec<&str> = Vec::new(); + let mut offsets: Vec = vec![0]; + for row in &rows { + values.extend(row.iter().copied()); + offsets.push(values.len() as i32); + } + let arr = ListArray::try_new( + Arc::clone(&child), + OffsetBuffer::new(offsets.into()), + Arc::new(StringViewArray::from(values)), + None, + ) + .expect("valid fixture"); batch( - vec![Field::new("s", DataType::Utf8View, true)], - vec![Arc::new(StringViewArray::from(v))], + vec![Field::new("l", DataType::List(child), true)], + vec![Arc::new(arr)], ) - }; - // Above the 12-byte inline threshold, so the values live in a data - // buffer rather than in the view word itself. - let long = [ - "aaaaaaaaaaaaaaaaaaaa", - "bbbbbbbbbbbbbbbbbbbb", - "cccccccccccccccccccc", - ]; + } + + let parent = list_of_views(vec![ + vec![LONG[0]], + vec![LONG[1], LONG[2]], + vec![LONG[0], LONG[2]], + ]); + let sliced = parent.slice(1, 1); - assert_ne!( - hash(&view(long.to_vec()).slice(1, 2)), - hash(&view(vec![long[1], long[2]])), - "arrow now truncates variadic data buffers — update the contract above" - ); assert_eq!( - hash(&view(vec!["aa", "bb", "cc"]).slice(1, 2)), - hash(&view(vec!["bb", "cc"])), - "an all-inline parent has no data buffer at all, so it stays invariant" + sliced.column(0).to_data().child_data()[0].len(), + parent.column(0).to_data().child_data()[0].len(), + "fixture must keep the parent's whole child array, or this proves nothing" ); - // The narrow reading of the line above — "short rows are invariant" — - // is false: one long value anywhere in the parent gives the array a - // data buffer, and a slice of only short rows still carries it. - assert_ne!( - hash(&view(vec!["aa", "bb", &long[0]]).slice(0, 2)), - hash(&view(vec!["aa", "bb"])), - "the exception is a property of the parent array, not of the sliced rows" + assert_eq!( + hash(&sliced), + hash(&list_of_views(vec![vec![LONG[1], LONG[2]]])), + "a sliced list-of-view column hashes as its own logical rows" ); } } From a78f281aa9f533ceaaa6df2aae4bb3ced80444e3 Mon Sep 17 00:00:00 2001 From: Nico Bautista Hobin Date: Fri, 31 Jul 2026 20:19:48 +0000 Subject: [PATCH 32/32] fix(cold): keep the row count when compacting a zero-size fixed list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `FixedSizeListArray::try_new` re-derives the length from the null buffer when `size == 0`, which is zero for a null-free column — and `CanonicalType` accepts size 0. Rebuilding through it collapsed such a column to zero rows, so two segments of different row counts would digest identically: a false dedup match on the one value that is supposed to be identity. `try_new_with_length` carries the length across. Also covers the compaction branches the first pass left untested — `BinaryView`, `LargeList`, and `FixedSizeList`, the last with a guard pinning the `slice`-narrows-the-child assumption its arm rests on. CHA-545 --- crates/penca-core/src/digest.rs | 156 +++++++++++++++++++++++++++----- 1 file changed, 131 insertions(+), 25 deletions(-) diff --git a/crates/penca-core/src/digest.rs b/crates/penca-core/src/digest.rs index 9c24817..e9db1bd 100644 --- a/crates/penca-core/src/digest.rs +++ b/crates/penca-core/src/digest.rs @@ -141,11 +141,16 @@ fn compact_column(array: &ArrayRef) -> Result { // Unlike the offset flavours, `FixedSizeListArray::slice` slices its // child too, so `values()` is already this array's own range. let list = downcast::(array)?; - Ok(Arc::new(FixedSizeListArray::try_new( + // Carry the length rather than let `try_new` re-derive it: + // `CanonicalType` accepts size 0, and at size 0 `try_new` reads the + // length off the null buffer, so a null-free column would rebuild + // with zero rows. + Ok(Arc::new(FixedSizeListArray::try_new_with_length( child.clone(), *size, compact_column(list.values())?, list.nulls().cloned(), + list.len(), )?)) } _ => Ok(Arc::clone(array)), @@ -455,34 +460,54 @@ mod tests { ); } - /// The nested case. [`crate::types::CanonicalType`] admits a view as the - /// child of a single-level list, where slicing leaves *both* the child array - /// and its data buffers whole — so the compaction has to narrow the child to - /// the offsets' range before it can help. + /// The `BinaryView` twin of the arm above. Same buffer mechanics, separate + /// downcast — a swapped array type here would be caught by nothing else. #[test] - fn sliced_list_of_view_column_hashes_equal_to_an_independently_built_batch() { - fn list_of_views(rows: Vec>) -> RecordBatch { - let child = Arc::new(Field::new("item", DataType::Utf8View, true)); - let mut values: Vec<&str> = Vec::new(); - let mut offsets: Vec = vec![0]; - for row in &rows { - values.extend(row.iter().copied()); - offsets.push(values.len() as i32); - } - let arr = ListArray::try_new( - Arc::clone(&child), - OffsetBuffer::new(offsets.into()), - Arc::new(StringViewArray::from(values)), - None, - ) - .expect("valid fixture"); + fn sliced_binary_view_column_hashes_equal_to_an_independently_built_batch() { + let binaries = |v: Vec<&str>| { batch( - vec![Field::new("l", DataType::List(child), true)], - vec![Arc::new(arr)], + vec![Field::new("b", DataType::BinaryView, true)], + vec![Arc::new(BinaryViewArray::from_iter_values( + v.into_iter().map(str::as_bytes), + ))], ) + }; + assert_eq!( + hash(&binaries(LONG.to_vec()).slice(1, 2)), + hash(&binaries(vec![LONG[1], LONG[2]])), + "a sliced binary-view column hashes as its own logical rows" + ); + } + + /// A single-level list of views, in both offset widths. + /// [`crate::types::CanonicalType`] admits a view there, and slicing leaves + /// *both* the child array and its data buffers whole — so the compaction has + /// to narrow the child to the offsets' range before it can help. + fn list_of_views(rows: Vec>) -> RecordBatch { + let child = Arc::new(Field::new("item", DataType::Utf8View, true)); + let mut values: Vec<&str> = Vec::new(); + let mut offsets: Vec = vec![O::zero()]; + for row in &rows { + values.extend(row.iter().copied()); + offsets.push(O::from_usize(values.len()).expect("fixture fits the offset width")); } + let arr = GenericListArray::::try_new( + Arc::clone(&child), + OffsetBuffer::new(offsets.into()), + Arc::new(StringViewArray::from(values)), + None, + ) + .expect("valid fixture"); + let data_type = if O::IS_LARGE { + DataType::LargeList(child) + } else { + DataType::List(child) + }; + batch(vec![Field::new("l", data_type, true)], vec![Arc::new(arr)]) + } - let parent = list_of_views(vec![ + fn assert_sliced_list_of_views_is_invariant() { + let parent = list_of_views::(vec![ vec![LONG[0]], vec![LONG[1], LONG[2]], vec![LONG[0], LONG[2]], @@ -496,8 +521,89 @@ mod tests { ); assert_eq!( hash(&sliced), - hash(&list_of_views(vec![vec![LONG[1], LONG[2]]])), + hash(&list_of_views::(vec![vec![LONG[1], LONG[2]]])), "a sliced list-of-view column hashes as its own logical rows" ); } + + #[test] + fn sliced_list_of_view_column_hashes_equal_to_an_independently_built_batch() { + assert_sliced_list_of_views_is_invariant::(); + } + + #[test] + fn sliced_large_list_of_view_column_hashes_equal_to_an_independently_built_batch() { + assert_sliced_list_of_views_is_invariant::(); + } + + fn fixed_size_list_of_views(rows: Vec<[&str; 2]>) -> RecordBatch { + let child = Arc::new(Field::new("item", DataType::Utf8View, true)); + let values: Vec<&str> = rows.iter().flatten().copied().collect(); + let arr = FixedSizeListArray::try_new( + Arc::clone(&child), + 2, + Arc::new(StringViewArray::from(values)), + None, + ) + .expect("valid fixture"); + batch( + vec![Field::new("l", DataType::FixedSizeList(child, 2), true)], + vec![Arc::new(arr)], + ) + } + + /// The third nesting flavour, which takes a structurally different path: it + /// narrows nothing, resting entirely on `FixedSizeListArray::slice` slicing + /// its child. The guard below pins that — if it ever stopped holding, the + /// compaction would `gc` the parent's whole child and silently re-absorb the + /// neighbouring rows' bytes. + #[test] + fn sliced_fixed_size_list_of_view_column_hashes_equal_to_an_independently_built_batch() { + let parent = fixed_size_list_of_views(vec![ + [LONG[0], LONG[1]], + [LONG[1], LONG[2]], + [LONG[2], LONG[0]], + ]); + let sliced = parent.slice(1, 1); + + assert_eq!( + sliced.column(0).to_data().child_data()[0].len(), + 2, + "fixture assumes `slice` narrows the child to the sliced rows" + ); + assert_eq!( + hash(&sliced), + hash(&fixed_size_list_of_views(vec![[LONG[1], LONG[2]]])), + "a sliced fixed-size-list-of-view column hashes as its own logical rows" + ); + } + + /// At `size == 0` — which `CanonicalType` accepts — `FixedSizeListArray` + /// carries no values to derive a length from and `try_new` reads it off the + /// null buffer, so rebuilding a null-free column through it would yield zero + /// rows. Two segments of different row counts would then digest identically: + /// a false dedup match on the one value that is supposed to be identity. + #[test] + fn zero_size_fixed_size_list_of_view_keeps_its_row_count() { + let empty_rows = |rows: usize| { + let child = Arc::new(Field::new("item", DataType::Utf8View, true)); + let arr = FixedSizeListArray::try_new_with_length( + Arc::clone(&child), + 0, + Arc::new(StringViewArray::from(Vec::<&str>::new())), + None, + rows, + ) + .expect("valid fixture"); + batch( + vec![Field::new("l", DataType::FixedSizeList(child, 0), true)], + vec![Arc::new(arr)], + ) + }; + assert_ne!( + hash(&empty_rows(3)), + hash(&empty_rows(2)), + "row count must survive compaction even with no values to count" + ); + } }