From 7d4784237a863aa4cf2b34f9ff1b6206db1b42a9 Mon Sep 17 00:00:00 2001 From: Sang Woo Kim Date: Thu, 13 Aug 2026 10:52:23 +0900 Subject: [PATCH 01/11] writer: validate chunk geometry at every dataset create validate_chunk_geometry mirrors H5D__chunk_construct: matching rank, no zero chunk dims, chunk within any fixed max dim whose current size is nonzero. The EA and compressed-vlen creators checked nothing, letting appends land rows at the chunk stride. SWMR tiled creates thereby lose chunk-larger-than-frame acceptance, geometry libhdf5 refuses to create. --- CHANGELOG.md | 18 ++++ src/dataset.rs | 26 ++++++ src/io/writer.rs | 217 ++++++++++++++++++++++++++++++++++------------- 3 files changed, 202 insertions(+), 59 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 252855c..eb6e4f2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,23 @@ # Changelog +## Unreleased + +### Fixed + +- Chunk geometry is validated at every dataset create, the rule libhdf5 + applies in `H5D__chunk_construct`: the chunk rank must match the dataspace, + no chunk dimension may be zero, and a chunk dimension may not exceed a + fixed maximum dimension unless that dimension's current size is zero. The + extensible-array and compressed-vlen creators previously accepted any + geometry, and a chunk wider than a fixed dimension made appends land rows + at the chunk stride — `[1, 2, 3, 4]` read back as `[1, 2, 0, 0]`. + + **Behavior change:** `SwmrWriter::create_streaming_dataset_tiled` no longer + accepts a chunk tile larger than the frame. libhdf5 refuses to create that + geometry, so no libhdf5-based writer (including the NDFileHDF5 tiling + controls the API mirrors) can produce such a file; previously the frame was + zero-padded up to the tile. + ## 0.4.1 ### Added diff --git a/src/dataset.rs b/src/dataset.rs index 6e0dddd..9f6b806 100644 --- a/src/dataset.rs +++ b/src/dataset.rs @@ -5321,4 +5321,30 @@ mod tests { "20 buffered updates left {size} bytes, one collection per update" ); } + + /// Regression: a chunk wider than a fixed max dimension used to be + /// accepted, and appends then packed rows at the chunk stride — writing + /// [1, 2, 3, 4] and reading back [1, 2, 0, 0]. libhdf5 rejects the + /// geometry at create (`H5D__chunk_construct`); so do we now. + #[test] + fn builder_rejects_a_chunk_wider_than_a_fixed_max_dimension() { + let path = temp_path("builder_chunk_wider_than_max"); + let file = H5File::create(&path).unwrap(); + let err = match file + .new_dataset::() + .shape([0, 2]) + .chunk(&[2, 4]) + .max_shape(&[None, Some(2)]) + .create("v5") + { + Ok(_) => panic!("create accepted a chunk wider than the fixed max dimension"), + Err(e) => e, + }; + assert!( + err.to_string().contains("maximum dimension size"), + "unexpected error: {err}" + ); + file.close().unwrap(); + std::fs::remove_file(&path).ok(); + } } diff --git a/src/io/writer.rs b/src/io/writer.rs index 2ee3b07..529dff1 100644 --- a/src/io/writer.rs +++ b/src/io/writer.rs @@ -385,6 +385,43 @@ impl ChunkGeometry { } } +/// Validate caller-supplied chunk geometry at dataset definition, the rule +/// libhdf5 applies in `H5D__chunk_construct` (H5Dchunk.c): the chunk rank +/// must match the dataspace rank, no chunk dimension may be zero, and a +/// chunk dimension may not exceed a fixed maximum dimension — except in a +/// dimension whose current size is zero, which libhdf5 exempts. +fn validate_chunk_geometry(dims: &[u64], max_dims: &[u64], chunk_dims: &[u64]) -> IoResult<()> { + let ndims = dims.len(); + if chunk_dims.len() != ndims { + return Err(crate::io::IoError::InvalidState(format!( + "chunk shape has {} dimensions but the dataspace has {}", + chunk_dims.len(), + ndims + ))); + } + if max_dims.len() != ndims { + return Err(crate::io::IoError::InvalidState(format!( + "maximum shape has {} dimensions but the dataspace has {}", + max_dims.len(), + ndims + ))); + } + for d in 0..ndims { + if chunk_dims[d] == 0 { + return Err(crate::io::IoError::InvalidState(format!( + "chunk dimension {d} is zero" + ))); + } + if dims[d] != 0 && max_dims[d] != u64::MAX && max_dims[d] < chunk_dims[d] { + return Err(crate::io::IoError::InvalidState(format!( + "chunk dimension {} is {} but the maximum dimension size is {}", + d, chunk_dims[d], max_dims[d] + ))); + } + } + Ok(()) +} + /// Runtime metadata for a fixed-array-indexed chunked dataset. pub struct FixedArrayDatasetInfo { /// Chunk dimension sizes. @@ -1614,6 +1651,7 @@ impl Hdf5Writer { // push so the two are atomic (see `create_lock`). let _create = self.create_lock.lock(); self.ensure_unique_dataset_name(name)?; + validate_chunk_geometry(dims, max_dims, chunk_dims)?; let earray_params = EarrayParams::default_params(); let ndblk_addrs = compute_ndblk_addrs(earray_params.sup_blk_min_data_ptrs)?; let nsblk_addrs = compute_nsblk_addrs( @@ -2471,6 +2509,7 @@ impl Hdf5Writer { use crate::format::messages::datatype::DatatypeMessage; let num_strings = strings.len() as u64; + validate_chunk_geometry(&[num_strings], &[num_strings], &[chunk_size as u64])?; // Build a global heap collection with all strings let mut gcol = GlobalHeapCollection::new(); @@ -3602,23 +3641,11 @@ impl Hdf5Writer { // push so the two are atomic (see `create_lock`). let _create = self.create_lock.lock(); self.ensure_unique_dataset_name(name)?; - // Compute total number of chunks. `chunk_dims` is caller-supplied; - // validate it before any indexing or division. + // A fixed-array index means a fixed shape: max dims are the dims. + validate_chunk_geometry(dims, dims, chunk_dims)?; let ndims = dims.len(); - if chunk_dims.len() != ndims { - return Err(crate::io::IoError::InvalidState(format!( - "chunk shape has {} dimensions but the dataspace has {}", - chunk_dims.len(), - ndims - ))); - } let mut num_chunks: u64 = 1; for d in 0..ndims { - if chunk_dims[d] == 0 { - return Err(crate::io::IoError::InvalidState(format!( - "chunk dimension {d} is zero" - ))); - } num_chunks = num_chunks .checked_mul(dims[d].div_ceil(chunk_dims[d])) .ok_or_else(|| { @@ -3703,21 +3730,11 @@ impl Hdf5Writer { // push so the two are atomic (see `create_lock`). let _create = self.create_lock.lock(); self.ensure_unique_dataset_name(name)?; + // A fixed-array index means a fixed shape: max dims are the dims. + validate_chunk_geometry(dims, dims, chunk_dims)?; let ndims = dims.len(); - if chunk_dims.len() != ndims { - return Err(crate::io::IoError::InvalidState(format!( - "chunk shape has {} dimensions but the dataspace has {}", - chunk_dims.len(), - ndims - ))); - } let mut num_chunks: u64 = 1; for d in 0..ndims { - if chunk_dims[d] == 0 { - return Err(crate::io::IoError::InvalidState(format!( - "chunk dimension {d} is zero" - ))); - } num_chunks = num_chunks .checked_mul(dims[d].div_ceil(chunk_dims[d])) .ok_or_else(|| { @@ -3843,14 +3860,8 @@ impl Hdf5Writer { // push so the two are atomic (see `create_lock`). let _create = self.create_lock.lock(); self.ensure_unique_dataset_name(name)?; + validate_chunk_geometry(dims, max_dims, chunk_dims)?; let ndims = dims.len(); - if chunk_dims.len() != ndims { - return Err(crate::io::IoError::InvalidState(format!( - "chunk shape has {} dimensions but the dataspace has {}", - chunk_dims.len(), - ndims - ))); - } // The filtered record's size field is as wide as libhdf5 will // recompute it from the uncompressed chunk size, exactly as the @@ -3937,6 +3948,7 @@ impl Hdf5Writer { chunk_dims: &[u64], compression_level: u32, ) -> IoResult { + validate_chunk_geometry(dims, max_dims, chunk_dims)?; let element_size = datatype.element_size() as u64; let chunk_bytes: u64 = chunk_dims.iter().product::() * element_size; let chunk_size_len = compute_chunk_size_len(chunk_bytes); @@ -4040,6 +4052,7 @@ impl Hdf5Writer { // push so the two are atomic (see `create_lock`). let _create = self.create_lock.lock(); self.ensure_unique_dataset_name(name)?; + validate_chunk_geometry(dims, max_dims, chunk_dims)?; let element_size = datatype.element_size() as u64; let chunk_bytes: u64 = chunk_dims.iter().product::() * element_size; let chunk_size_len = compute_chunk_size_len(chunk_bytes); @@ -6358,8 +6371,14 @@ mod tests { std::fs::remove_file(&path).ok(); } + /// A chunk tile larger than the frame is geometry libhdf5 refuses to + /// create (`H5D__chunk_construct`: chunk must not exceed a fixed maximum + /// dimension), so no libhdf5-based writer — including the NDFileHDF5 + /// tiling controls this API mirrors — can produce such a file. Until + /// 0.4.1 we accepted it and zero-padded the frame up to the tile; now + /// the create is rejected like every other creator's. #[test] - fn swmr_writer_tiled_chunk_larger_than_frame() { + fn swmr_writer_tiled_chunk_larger_than_frame_is_rejected() { use crate::io::swmr::SwmrWriter; use std::sync::atomic::{AtomicU64, Ordering}; static COUNTER: AtomicU64 = AtomicU64::new(0); @@ -6370,34 +6389,15 @@ mod tests { n )); - // Chunk tile larger than the frame: a 1x1 chunk grid, but the frame - // must still be zero-padded up to the full chunk size. let mut swmr = SwmrWriter::create(&path).unwrap(); - let idx = swmr + let err = swmr .create_streaming_dataset_tiled("det", DatatypeMessage::u16_type(), &[3, 3], &[8, 8]) - .unwrap(); - swmr.start_swmr().unwrap(); - for frame in 0..2u16 { - let data: Vec = (0..9).map(|i| frame * 10 + i).collect(); - let raw: Vec = data.iter().flat_map(|v| v.to_le_bytes()).collect(); - swmr.append_frame(idx, &raw).unwrap(); - } - swmr.flush().unwrap(); + .unwrap_err(); + assert!( + err.to_string().contains("maximum dimension size"), + "unexpected error: {err}" + ); swmr.close().unwrap(); - - let mut reader = Hdf5Reader::open(&path).unwrap(); - assert_eq!(reader.dataset_shape("det").unwrap(), vec![2, 3, 3]); - let raw = reader.read_dataset_raw("det").unwrap(); - let values: Vec = raw - .chunks(2) - .map(|c| u16::from_le_bytes(c.try_into().unwrap())) - .collect(); - assert_eq!(values.len(), 18); - for frame in 0..2u16 { - for i in 0..9usize { - assert_eq!(values[frame as usize * 9 + i], frame * 10 + i as u16); - } - } std::fs::remove_file(&path).ok(); } @@ -6610,4 +6610,103 @@ mod tests { std::fs::remove_file(&path).ok(); } + + /// libhdf5 (`H5D__chunk_construct`) rejects a chunk dimension that + /// exceeds a fixed maximum dimension. Before this check, such a dataset + /// was created and appends landed rows at the chunk stride instead of + /// the row stride, reading back [1, 2, 0, 0] for [1, 2, 3, 4]. + #[test] + fn create_rejects_a_chunk_wider_than_a_fixed_max_dimension() { + let path = temp_path("chunk_wider_than_max"); + + let writer = Hdf5Writer::create(&path).unwrap(); + let err = writer + .create_chunked_dataset( + "data", + DatatypeMessage::f64_type(), + &[0, 2], + &[u64::MAX, 2], + &[2, 4], + ) + .unwrap_err(); + assert!( + err.to_string().contains("maximum dimension size"), + "unexpected error: {err}" + ); + + // The fixed-array creators derive the maximum from the fixed dims. + let err = writer + .create_fixed_array_dataset("fa", DatatypeMessage::f64_type(), &[3], &[5]) + .unwrap_err(); + assert!( + err.to_string().contains("maximum dimension size"), + "unexpected error: {err}" + ); + + writer.close().unwrap(); + std::fs::remove_file(&path).ok(); + } + + /// libhdf5 exempts a dimension whose *current* size is zero from the + /// chunk-vs-maximum check (`curr_dims[u] &&` in `H5D__chunk_construct`), + /// and rejects a zero chunk dimension on every path. + #[test] + fn create_mirrors_the_libhdf5_chunk_geometry_exemptions() { + let path = temp_path("chunk_geometry_exemptions"); + + let writer = Hdf5Writer::create(&path).unwrap(); + // dims[1] == 0: chunk 4 > max 2 is allowed, as libhdf5 allows it. + writer + .create_chunked_dataset( + "exempt", + DatatypeMessage::f64_type(), + &[0, 0], + &[u64::MAX, 2], + &[2, 4], + ) + .unwrap(); + + let err = writer + .create_chunked_dataset("zero", DatatypeMessage::f64_type(), &[0], &[u64::MAX], &[0]) + .unwrap_err(); + assert!( + err.to_string().contains("chunk dimension 0 is zero"), + "unexpected error: {err}" + ); + + writer.close().unwrap(); + std::fs::remove_file(&path).ok(); + } + + /// The compressed vlen creator sizes its chunked layout from a + /// caller-supplied chunk size; it goes through the same geometry + /// validation as every other creator (empty inputs are exempt because + /// their current size is zero). + #[test] + #[cfg(feature = "deflate")] + fn compressed_vlen_create_validates_its_chunk_size() { + use crate::format::messages::filter::FilterPipeline; + let path = temp_path("vlen_compressed_chunk"); + + let writer = Hdf5Writer::create(&path).unwrap(); + let err = writer + .create_vlen_string_dataset_compressed( + "texts", + &["a", "b", "c"], + 100, + FilterPipeline::deflate(6), + ) + .unwrap_err(); + assert!( + err.to_string().contains("maximum dimension size"), + "unexpected error: {err}" + ); + + writer + .create_vlen_string_dataset_compressed("empty", &[], 16, FilterPipeline::deflate(6)) + .unwrap(); + + writer.close().unwrap(); + std::fs::remove_file(&path).ok(); + } } From 68be584281246e114406baec1771beeb9f74f69f Mon Sep 17 00:00:00 2001 From: Sang Woo Kim Date: Thu, 13 Aug 2026 11:02:11 +0900 Subject: [PATCH 02/11] writer: route append chunk writes through write_slice_chunked MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit write_append_frames replaces append_frames_into_chunk, expressing an append as the hyperslab it is. The old owner required the EA index — fixed-array and v2 B-tree appends buffered fine and lost the rows when close() failed — and packed rows at the frame stride, corrupting any dataset whose chunk row differs from the frame row. --- CHANGELOG.md | 11 +++ src/dataset.rs | 151 +++++++++++++++++++++++++-------- src/io/writer.rs | 214 +++++++++++++++++++++++++---------------------- 3 files changed, 244 insertions(+), 132 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index eb6e4f2..c5574b5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,17 @@ controls the API mirrors) can produce such a file; previously the frame was zero-padded up to the tile. +- Appends work on every chunk index and chunk shape. The append paths' + chunk writes required the extensible-array index, so appending to a + fixed-array or v2 B-tree dataset buffered fine and then failed at + `close()` with "not a chunked dataset", losing the buffered rows. They + also packed rows at the frame stride, so a chunk row narrower than the + frame — legal, libhdf5-creatable geometry — corrupted the first chunk + and errored on the second. Append writes now go through the same + index-generic hyperslab engine as `write_slice`, which also makes + appends to reopened 0.4.0 files with a wider-than-row chunk land + correctly instead of reading back `[1, 2, 0, 0]` for `[1, 2, 3, 4]`. + ## 0.4.1 ### Added diff --git a/src/dataset.rs b/src/dataset.rs index 9f6b806..10d2715 100644 --- a/src/dataset.rs +++ b/src/dataset.rs @@ -1663,39 +1663,37 @@ impl H5Dataset { combined.extend_from_slice(raw); let total_frames = buffered_frames + n_new_frames; - let total_bytes = combined.len(); - - // Base chunk index: account for buffered frames - let base_dim0 = current_dim0 - buffered_frames; - let mut byte_pos = 0usize; - let mut frame_pos = 0usize; - - while frame_pos < total_frames { - let abs_frame = base_dim0 + frame_pos; - let chunk_idx = abs_frame / chunk_dim0; - let remaining_frames = total_frames - frame_pos; - let frames_to_fill = chunk_dim0 - (abs_frame % chunk_dim0); - - if remaining_frames >= frames_to_fill { - // These frames complete the chunk's remaining span. - let end = byte_pos + frames_to_fill * frame_bytes; - let offset_in_chunk = (abs_frame % chunk_dim0) * frame_bytes; - writer.append_frames_into_chunk( - ds_index, - chunk_idx as u64, - offset_in_chunk, - &combined[byte_pos..end], - )?; - byte_pos = end; - frame_pos += frames_to_fill; - } else { - // Partial chunk — buffer for next append - let ds = writer.ds(ds_index); - let mut m = ds.lock(); - m.append_buffer = combined[byte_pos..total_bytes].to_vec(); - m.append_buffered_frames = remaining_frames as u64; - frame_pos = total_frames; - } + + // First appended row: account for buffered frames. + let base_dim0 = current_dim0.checked_sub(buffered_frames).ok_or_else(|| { + Hdf5Error::InvalidState(format!( + "the append buffer holds {buffered_frames} frames but the \ + dataset extent is {current_dim0}" + )) + })?; + + // Rows up to the last chunk boundary are written now; the + // tail that does not complete a chunk goes back in the + // buffer for the next append (or the flush at close). The + // boundary can precede `base_dim0` — a reopened file's + // flushed partial chunk leaves the base mid-chunk — in + // which case everything is tail. + let last_boundary = ((base_dim0 + total_frames) / chunk_dim0) * chunk_dim0; + let write_frames = last_boundary.saturating_sub(base_dim0); + let tail_frames = total_frames - write_frames; + if write_frames > 0 { + writer.write_append_frames( + ds_index, + base_dim0 as u64, + write_frames as u64, + &combined[..write_frames * frame_bytes], + )?; + } + if tail_frames > 0 { + let ds = writer.ds(ds_index); + let mut m = ds.lock(); + m.append_buffer = combined[write_frames * frame_bytes..].to_vec(); + m.append_buffered_frames = tail_frames as u64; } // Extend dims to include all frames (buffered + new) @@ -5322,6 +5320,93 @@ mod tests { ); } + /// Regression: appends to a v2 B-tree indexed dataset (two unlimited + /// dimensions) buffered fine but close() failed "not a chunked dataset" + /// and lost the buffered rows — the append's chunk writes required the + /// extensible-array index. They now go through the index-generic + /// hyperslab engine. + #[test] + fn append_to_a_btree_v2_dataset_survives_close() { + let path = temp_path("append_bt2_close"); + { + let file = H5File::create(&path).unwrap(); + let ds = file + .new_dataset::() + .shape([0, 3]) + .chunk(&[4, 3]) + .max_shape(&[None, None]) + .create("d") + .unwrap(); + // One buffered row, then a batch that crosses the chunk + // boundary: 4 rows fill chunk band 0, one row stays buffered + // for the flush at close. + ds.append(&[1, 2, 3]).unwrap(); + ds.append(&(4..=15).collect::>()).unwrap(); + file.close().unwrap(); + } + { + let file = H5File::open(&path).unwrap(); + let ds = file.dataset("d").unwrap(); + assert_eq!(ds.shape(), vec![5, 3]); + assert_eq!( + ds.read_raw::().unwrap(), + (1..=15).collect::>() + ); + } + std::fs::remove_file(&path).ok(); + } + + /// A chunk row narrower than the frame row is legal geometry (libhdf5 + /// creates it); appended frames must be scattered across the row's + /// tiles at the chunk stride, not packed at the frame stride. + #[test] + fn append_scatters_frames_across_narrow_chunk_tiles() { + let path = temp_path("append_narrow_chunks"); + { + let file = H5File::create(&path).unwrap(); + let ds = file + .new_dataset::() + .shape([0, 8]) + .chunk(&[2, 4]) + .max_shape(&[None, Some(8)]) + .create("d") + .unwrap(); + // 3 rows of 8: rows 0..2 complete chunk band 0 (two tiles), + // row 2 is flushed partial at close. + ds.append(&(0..24).collect::>()).unwrap(); + file.close().unwrap(); + } + { + let file = H5File::open(&path).unwrap(); + let ds = file.dataset("d").unwrap(); + assert_eq!(ds.shape(), vec![3, 8]); + assert_eq!(ds.read_raw::().unwrap(), (0..24).collect::>()); + } + std::fs::remove_file(&path).ok(); + } + + /// A fixed-array dataset has no room to grow: appending must surface an + /// error naming the chunk grid, not lose rows silently. (Before the + /// index-generic append it failed as "not a chunked dataset".) + #[test] + fn append_to_a_full_fixed_array_dataset_errors() { + let path = temp_path("append_fa_errors"); + let file = H5File::create(&path).unwrap(); + let ds = file + .new_dataset::() + .shape([4, 3]) + .chunk(&[2, 3]) + .create("d") + .unwrap(); + let err = ds.append(&(0..6).collect::>()).unwrap_err(); + assert!( + err.to_string().contains("chunk grid"), + "unexpected error: {err}" + ); + file.close().unwrap(); + std::fs::remove_file(&path).ok(); + } + /// Regression: a chunk wider than a fixed max dimension used to be /// accepted, and appends then packed rows at the chunk stride — writing /// [1, 2, 3, 4] and reading back [1, 2, 0, 0]. libhdf5 rejects the diff --git a/src/io/writer.rs b/src/io/writer.rs index 529dff1..a70f9de 100644 --- a/src/io/writer.rs +++ b/src/io/writer.rs @@ -2738,35 +2738,34 @@ impl Hdf5Writer { combined.extend_from_slice(&raw); let total_frames = buffered_frames + n_new_frames; - let total_bytes = combined.len(); - let base_dim0 = current_dim0 - buffered_frames; - let mut byte_pos = 0usize; - let mut frame_pos = 0usize; - - while frame_pos < total_frames { - let abs_frame = base_dim0 + frame_pos; - let chunk_idx = abs_frame / chunk_dim0; - let remaining_frames = total_frames - frame_pos; - let frames_to_fill = chunk_dim0 - (abs_frame % chunk_dim0); - - if remaining_frames >= frames_to_fill { - let end = byte_pos + frames_to_fill * frame_bytes; - let offset_in_chunk = (abs_frame % chunk_dim0) * frame_bytes; - self.append_frames_into_chunk( - ds_index, - chunk_idx as u64, - offset_in_chunk, - &combined[byte_pos..end], - )?; - byte_pos = end; - frame_pos += frames_to_fill; - } else { - let ds = self.ds(ds_index); - let mut m = ds.lock(); - m.append_buffer = combined[byte_pos..total_bytes].to_vec(); - m.append_buffered_frames = remaining_frames as u64; - frame_pos = total_frames; - } + let base_dim0 = current_dim0.checked_sub(buffered_frames).ok_or_else(|| { + crate::io::IoError::InvalidState(format!( + "the append buffer holds {buffered_frames} frames but the dataset \ + extent is {current_dim0}" + )) + })?; + + // Rows up to the last chunk boundary are written now; the tail that + // does not complete a chunk goes back in the buffer for the next + // append (or the flush at close). The boundary can precede + // `base_dim0` — a reopened file's flushed partial chunk leaves the + // base mid-chunk — in which case everything is tail. + let last_boundary = ((base_dim0 + total_frames) / chunk_dim0) * chunk_dim0; + let write_frames = last_boundary.saturating_sub(base_dim0); + let tail_frames = total_frames - write_frames; + if write_frames > 0 { + self.write_append_frames( + ds_index, + base_dim0 as u64, + write_frames as u64, + &combined[..write_frames * frame_bytes], + )?; + } + if tail_frames > 0 { + let ds = self.ds(ds_index); + let mut m = ds.lock(); + m.append_buffer = combined[write_frames * frame_bytes..].to_vec(); + m.append_buffered_frames = tail_frames as u64; } // Extend dims @@ -3286,56 +3285,43 @@ impl Hdf5Writer { crate::format::messages::fill_value::tiled_fill(chunk_bytes, fv) } - /// Place `frames` in chunk `chunk_idx`, `offset_in_chunk` bytes from its - /// start, keeping every byte outside that span. + /// Write `n_frames` whole frames whose first row is `base_frame`, for + /// whichever chunk index the dataset uses and whatever its chunk shape. /// - /// The single owner of an append's chunk write. A span covering the whole - /// chunk is written straight through; a narrower one is read-modify-write, - /// because the bytes around it belong to frames appended earlier. Building - /// a fresh fill-value buffer for a chunk that already holds frames is what - /// erased them. A chunk the index does not reach has had no write land in - /// it, so the fill value *is* its content — the rule - /// [`write_slice`](Self::write_slice) already applies to a partially - /// covered chunk. - pub(crate) fn append_frames_into_chunk( + /// The single owner of an append's chunk writes. The frames are one + /// hyperslab — rows `base_frame .. base_frame + n_frames` over the full + /// row shape — so the write goes through + /// [`write_slice_chunked`](Self::write_slice_chunked), the same engine + /// `write_slice` uses: a chunk the span covers completely is written + /// straight through, a partial one is read-modify-write on top of what + /// is stored (or the fill value), and a chunk row narrower or wider + /// than the frame row is scattered at the chunk stride. The previous + /// owner required the extensible-array index and packed rows at the + /// frame stride, so appends to a fixed-array or v2 B-tree dataset + /// failed at close and lost the buffered rows. + pub(crate) fn write_append_frames( &self, ds_index: usize, - chunk_idx: u64, - offset_in_chunk: usize, + base_frame: u64, + n_frames: u64, frames: &[u8], ) -> IoResult<()> { - let chunk_bytes = { - let ds = self.ds(ds_index); - let m = ds.lock(); - let chunked = m - .chunked - .as_ref() - .ok_or_else(|| crate::io::IoError::InvalidState("not a chunked dataset".into()))?; - chunked.chunk_dims.iter().product::() as usize * m.datatype.element_size() as usize - }; - if offset_in_chunk + frames.len() > chunk_bytes { + if n_frames == 0 { + return Ok(()); + } + let geo = self.chunk_geometry(ds_index)?; + let mut starts = vec![0u64; geo.dims.len()]; + starts[0] = base_frame; + let mut counts = geo.dims.clone(); + counts[0] = n_frames; + let expected = counts.iter().product::() * geo.element_size; + if frames.len() as u64 != expected { return Err(crate::io::IoError::InvalidState(format!( - "{} bytes at offset {offset_in_chunk} do not fit chunk {chunk_idx}, \ - which is {chunk_bytes} bytes", + "{n_frames} frames at rows {base_frame}.. need {expected} bytes, got {}", frames.len() ))); } - if offset_in_chunk == 0 && frames.len() == chunk_bytes { - return self.write_chunk(ds_index, chunk_idx, frames); - } - let mut chunk_buf = match self.read_chunk_if_present(ds_index, chunk_idx)? { - Some(existing) if existing.len() == chunk_bytes => existing, - Some(existing) => { - return Err(crate::io::IoError::InvalidState(format!( - "stored chunk {chunk_idx} is {} bytes but the chunk shape needs \ - {chunk_bytes}", - existing.len() - ))) - } - None => self.new_chunk_buffer(ds_index, chunk_bytes), - }; - chunk_buf[offset_in_chunk..offset_in_chunk + frames.len()].copy_from_slice(frames); - self.write_chunk(ds_index, chunk_idx, &chunk_buf) + self.write_slice_chunked(ds_index, &starts, &counts, frames) } /// Read an already-written chunk's *decompressed* bytes when the chunk @@ -4944,48 +4930,37 @@ impl Hdf5Writer { // ------------------------------------------------------------------ /// Flush any partial append buffers into the chunks they belong to, - /// through [`append_frames_into_chunk`](Self::append_frames_into_chunk): - /// frames already in the chunk survive, and the rest of it reads back as - /// the dataset's fill value (zeros when none is defined). + /// through [`write_append_frames`](Self::write_append_frames): frames + /// already in the chunk survive, and the rest of it reads back as the + /// dataset's fill value (zeros when none is defined). fn flush_append_buffers(&mut self) -> IoResult<()> { for i in 0..self.dataset_count() { - // Snapshot everything needed under one brief slot guard, then drop - // it: `new_chunk_buffer` and `write_chunk` below re-lock the slot. - let (buf, buffered_frames, chunk_dims, es, dims) = { + // Snapshot everything needed under one brief slot guard, then + // drop it: `write_append_frames` below re-locks the slot. + let (buf, buffered_frames, dims) = { let ds = self.ds(i); let mut m = ds.lock(); if m.append_buffer.is_empty() { continue; } - let chunk_dims = if let Some(ref c) = m.chunked { - c.chunk_dims.clone() - } else if let Some(ref f) = m.fixed_array { - f.chunk_dims.clone() - } else if let Some(ref b) = m.btree_v2 { - b.chunk_dims.clone() - } else { + if m.chunked.is_none() && m.fixed_array.is_none() && m.btree_v2.is_none() { continue; - }; - let es = m.datatype.element_size() as usize; - let buffered_frames = m.append_buffered_frames as usize; + } + let buffered_frames = m.append_buffered_frames; let dims = m.dataspace.dims.clone(); let buf = std::mem::take(&mut m.append_buffer); m.append_buffered_frames = 0; - (buf, buffered_frames, chunk_dims, es, dims) + (buf, buffered_frames, dims) }; - let chunk_dim0 = chunk_dims[0] as usize; - let current_dim0 = dims[0] as usize; - let base_frame = current_dim0 - buffered_frames; - let chunk_idx = base_frame / chunk_dim0; - - let frame_bytes = if dims.len() > 1 { - dims[1..].iter().map(|&d| d as usize).product::() * es - } else { - es - }; - let offset_in_chunk = (base_frame % chunk_dim0) * frame_bytes; - self.append_frames_into_chunk(i, chunk_idx as u64, offset_in_chunk, &buf)?; + let base_frame = dims[0].checked_sub(buffered_frames).ok_or_else(|| { + crate::io::IoError::InvalidState(format!( + "the append buffer holds {buffered_frames} frames but the \ + dataset extent is {}", + dims[0] + )) + })?; + self.write_append_frames(i, base_frame, buffered_frames, &buf)?; } Ok(()) } @@ -6678,6 +6653,47 @@ mod tests { std::fs::remove_file(&path).ok(); } + /// A file written by 0.4.0 can carry a chunk row wider than the frame + /// row — create now rejects that geometry, but reopened files keep it. + /// Appends must scatter frames at the chunk stride, not pack them at + /// the frame stride (which read back `[1, 2, 0, 0]` for `[1, 2, 3, 4]`). + /// The wide shape is simulated by widening the registered chunk dims + /// after create, which also lands in the layout message at close. + #[test] + fn append_scatters_into_a_legacy_wider_than_row_chunk() { + let path = temp_path("legacy_wide_chunk_append"); + + let writer = Hdf5Writer::create(&path).unwrap(); + let idx = writer + .create_chunked_dataset( + "data", + DatatypeMessage::i32_type(), + &[0, 2], + &[u64::MAX, 2], + &[2, 2], + ) + .unwrap(); + writer.ds(idx).lock().chunked.as_mut().unwrap().chunk_dims = vec![2, 4]; + + let frames: Vec = [1i32, 2, 3, 4] + .iter() + .flat_map(|v| v.to_le_bytes()) + .collect(); + writer.write_append_frames(idx, 0, 2, &frames).unwrap(); + writer.extend_dataset(idx, &[2, 2]).unwrap(); + writer.close().unwrap(); + + let mut reader = Hdf5Reader::open(&path).unwrap(); + assert_eq!(reader.dataset_shape("data").unwrap(), vec![2, 2]); + let raw = reader.read_dataset_raw("data").unwrap(); + let values: Vec = raw + .chunks(4) + .map(|c| i32::from_le_bytes(c.try_into().unwrap())) + .collect(); + assert_eq!(values, vec![1, 2, 3, 4]); + std::fs::remove_file(&path).ok(); + } + /// The compressed vlen creator sizes its chunked layout from a /// caller-supplied chunk size; it goes through the same geometry /// validation as every other creator (empty inputs are exempt because From 3c64987431653ceb564be4dbe45f81c134d85991 Mon Sep 17 00:00:00 2001 From: Sang Woo Kim Date: Thu, 13 Aug 2026 11:10:06 +0900 Subject: [PATCH 03/11] writer: record the append buffer's base row and flush on conflict AppendBuffer stores {base, frames, bytes}; flush_append_buffer is the one buffer-to-chunks transition, and write_slice, write_full_image_chunked and write_vlen_strings_slice flush intersecting rows before writing. Deriving the base from dims[0] is what let the close-time flush revert a write_slice into the buffered tail and mis-place rows after extend; the vlen slice's patch-the-buffer path goes away with the derivation. --- CHANGELOG.md | 10 ++ src/dataset.rs | 109 +++++++++++++++----- src/io/writer.rs | 261 +++++++++++++++++++---------------------------- 3 files changed, 200 insertions(+), 180 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c5574b5..d8f8031 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,16 @@ controls the API mirrors) can produce such a file; previously the frame was zero-padded up to the tile. +- The append buffer records the absolute row its frames belong to, and + every operation that writes rows the buffer holds flushes it to the + chunks first. Before this, the buffer's position was derived from the + current extent, and two operations broke the derivation: a typed + `write_slice` into the buffered tail was silently overwritten by the + flush at close (write 99, read back 50), and an `extend` with buffered + appends made the flush land them at the extended end instead of where + they were appended. `write_vlen_strings_slice` drops its + patch-the-buffer path for the same flush-first rule. + - Appends work on every chunk index and chunk shape. The append paths' chunk writes required the extensible-array index, so appending to a fixed-array or v2 B-tree dataset buffered fine and then failed at diff --git a/src/dataset.rs b/src/dataset.rs index 10d2715..0ede8ae 100644 --- a/src/dataset.rs +++ b/src/dataset.rs @@ -976,6 +976,9 @@ impl H5Dataset { )) } }; + // A buffered append tail would flush over the image at close; hand + // it to the chunks first, the image below overwrites everything. + writer.flush_append_buffer(index)?; let chunk_dims = writer .dataset_chunk_dims(index) .ok_or_else(|| Hdf5Error::InvalidState("dataset has no chunk info".into()))? @@ -1650,28 +1653,25 @@ impl H5Dataset { std::slice::from_raw_parts(data.as_ptr() as *const u8, data.len() * es) }; - // Merge buffered data with new data. Scope the slot guard: the - // loop below calls `write_chunk`, which re-locks the same slot. - let (buffered_frames, mut combined) = { - let ds = writer.ds(ds_index); - let mut m = ds.lock(); - let buffered_frames = m.append_buffered_frames as usize; - let combined = std::mem::take(&mut m.append_buffer); - m.append_buffered_frames = 0; - (buffered_frames, combined) + // Merge the buffer with the new frames when it is the + // dataset's tail; a buffer left mid-extent (the extent moved + // past it) keeps its recorded place — flush it and start + // fresh at the current end. + let taken = { writer.ds(ds_index).lock().append.take() }; + let (base_dim0, buffered_frames, mut combined) = match taken { + Some(b) if b.base + b.frames == current_dim0 as u64 => { + (b.base as usize, b.frames as usize, b.bytes) + } + Some(b) => { + writer.write_append_frames(ds_index, b.base, b.frames, &b.bytes)?; + (current_dim0, 0, Vec::new()) + } + None => (current_dim0, 0, Vec::new()), }; combined.extend_from_slice(raw); let total_frames = buffered_frames + n_new_frames; - // First appended row: account for buffered frames. - let base_dim0 = current_dim0.checked_sub(buffered_frames).ok_or_else(|| { - Hdf5Error::InvalidState(format!( - "the append buffer holds {buffered_frames} frames but the \ - dataset extent is {current_dim0}" - )) - })?; - // Rows up to the last chunk boundary are written now; the // tail that does not complete a chunk goes back in the // buffer for the next append (or the flush at close). The @@ -1692,8 +1692,11 @@ impl H5Dataset { if tail_frames > 0 { let ds = writer.ds(ds_index); let mut m = ds.lock(); - m.append_buffer = combined[write_frames * frame_bytes..].to_vec(); - m.append_buffered_frames = tail_frames as u64; + m.append = Some(crate::io::writer::AppendBuffer { + base: (base_dim0 + write_frames) as u64, + frames: tail_frames as u64, + bytes: combined[write_frames * frame_bytes..].to_vec(), + }); } // Extend dims to include all frames (buffered + new) @@ -4995,11 +4998,11 @@ mod tests { std::fs::remove_file(&path).ok(); } - /// Elements the append buffer still holds are not on disk yet, so the - /// update has to land in the buffer or the flush at close would write the - /// pre-update reference over it. + /// Elements the append buffer still holds are not on disk yet; the + /// update flushes them to their chunks first, so the flush at close has + /// nothing left to write the pre-update reference over. #[test] - fn write_vlen_strings_slice_reaches_the_append_buffer() { + fn write_vlen_strings_slice_updates_buffered_elements() { let path = temp_path("vlen_slice_buffered"); { let file = H5File::create(&path).unwrap(); @@ -5320,6 +5323,66 @@ mod tests { ); } + /// Regression: a typed `write_slice` into rows the append buffer still + /// held wrote the chunks, and the flush at close wrote the stale buffered + /// rows back over it — write 99, read 50. The slice now flushes the + /// buffer first, making the chunks the single authority for those rows. + #[test] + fn write_slice_into_the_buffered_tail_survives_close() { + let path = temp_path("slice_into_buffered_tail"); + { + let file = H5File::create(&path).unwrap(); + let ds = file + .new_dataset::() + .shape([0]) + .chunk(&[4]) + .max_shape(&[None]) + .create("d") + .unwrap(); + // 6 rows: 4 land in chunk 0, rows 4 and 5 stay buffered. + ds.append(&[10, 11, 12, 13, 50, 51]).unwrap(); + ds.write_slice(&[4], &[1], &[99]).unwrap(); + file.close().unwrap(); + } + { + let file = H5File::open(&path).unwrap(); + let ds = file.dataset("d").unwrap(); + assert_eq!(ds.read_raw::().unwrap(), vec![10, 11, 12, 13, 99, 51]); + } + std::fs::remove_file(&path).ok(); + } + + /// Extending a dataset while appends sit in the buffer must not move + /// them: the buffer records the absolute row its frames belong to, so + /// the flush at close lands them there, and the grown region reads as + /// fill. + #[test] + fn extend_does_not_move_buffered_appends() { + let path = temp_path("extend_keeps_buffered_rows"); + { + let file = H5File::create(&path).unwrap(); + let ds = file + .new_dataset::() + .shape([0]) + .chunk(&[4]) + .max_shape(&[None]) + .create("d") + .unwrap(); + ds.append(&[10, 11, 12, 13, 50, 51]).unwrap(); // rows 4, 5 buffered + ds.extend(&[10]).unwrap(); + file.close().unwrap(); + } + { + let file = H5File::open(&path).unwrap(); + let ds = file.dataset("d").unwrap(); + assert_eq!( + ds.read_raw::().unwrap(), + vec![10, 11, 12, 13, 50, 51, 0, 0, 0, 0] + ); + } + std::fs::remove_file(&path).ok(); + } + /// Regression: appends to a v2 B-tree indexed dataset (two unlimited /// dimensions) buffered fine but close() failed "not a chunked dataset" /// and lost the buffered rows — the append's chunk writes required the diff --git a/src/io/writer.rs b/src/io/writer.rs index a70f9de..56ffc64 100644 --- a/src/io/writer.rs +++ b/src/io/writer.rs @@ -207,6 +207,22 @@ pub(crate) type DatasetRef = Shared>; /// [`DatasetRef`]. pub(crate) type GroupRef = Shared>; +/// Appended frames held back until they complete a chunk. +/// +/// The buffer is the sole authority for rows `base .. base + frames`: the +/// file's chunks do not hold them yet, and any operation that writes those +/// rows must go through [`Hdf5Writer::flush_append_buffer`] first. `base` is +/// recorded when the frames are buffered — never derived from the current +/// extent, which an `extend_dataset` can move independently. +pub struct AppendBuffer { + /// Absolute row of the first buffered frame. + pub base: u64, + /// Number of buffered frames. + pub frames: u64, + /// The frames' bytes, `frames` whole rows, row-major. + pub bytes: Vec, +} + /// Metadata for a dataset being written. /// /// The whole struct lives behind a per-dataset [`Slot`] (via [`DatasetRef`]). @@ -233,10 +249,8 @@ pub struct DatasetInfo { pub fixed_array: Option, /// B-tree v2 chunked storage info. pub btree_v2: Option, - /// Buffer for partially filled chunks during append. - pub append_buffer: Vec, - /// Number of frames accumulated in `append_buffer`. - pub append_buffered_frames: u64, + /// Appended frames not yet written to chunks, `None` when empty. + pub append: Option, /// Attributes attached to this dataset. pub attributes: Vec, /// File offset where the dataset object header was written (for SWMR in-place rewrites). @@ -852,8 +866,7 @@ impl Hdf5Writer { chunked: None, fixed_array: None, btree_v2: None, - append_buffer: Vec::new(), - append_buffered_frames: 0, + append: None, attributes: attrs, obj_header_written_addr: Some(*obj_addr), obj_header_encoded_size: 0, @@ -1621,8 +1634,7 @@ impl Hdf5Writer { chunked: None, fixed_array: None, btree_v2: None, - append_buffer: Vec::new(), - append_buffered_frames: 0, + append: None, attributes: Vec::new(), obj_header_written_addr: None, obj_header_encoded_size: 0, @@ -1727,8 +1739,7 @@ impl Hdf5Writer { filt_iblk: None, chunk_size_len: 0, }), - append_buffer: Vec::new(), - append_buffered_frames: 0, + append: None, }); Ok(idx) @@ -2194,6 +2205,10 @@ impl Hdf5Writer { drop(ds); if is_chunked { + // Rows the append buffer holds are not in the chunks yet; writing + // them there anyway would be undone when the buffer flushes at + // close. Hand them to the chunks first. + self.flush_append_buffer_if_intersecting(index, starts[0], starts[0] + counts[0])?; return self.write_slice_chunked(index, starts, counts, data); } if base_addr == UNDEF_ADDR { @@ -2413,8 +2428,7 @@ impl Hdf5Writer { chunked: None, fixed_array: None, btree_v2: None, - append_buffer: Vec::new(), - append_buffered_frames: 0, + append: None, }); Ok(idx) @@ -2486,8 +2500,7 @@ impl Hdf5Writer { chunked: None, fixed_array: None, btree_v2: None, - append_buffer: Vec::new(), - append_buffered_frames: 0, + append: None, }); Ok(idx) @@ -2624,8 +2637,7 @@ impl Hdf5Writer { filt_iblk: Some(filt_iblk), chunk_size_len, }), - append_buffer: Vec::new(), - append_buffered_frames: 0, + append: None, }); // Write chunks of vlen references with compression @@ -2725,25 +2737,23 @@ impl Hdf5Writer { let chunk_dim0 = chunk_dims[0] as usize; let frame_bytes = ref_size; - // Merge buffered data with new data. Scope the slot guard: the loop - // below calls `write_chunk`, which re-locks the same slot. - let (buffered_frames, mut combined) = { - let ds = self.ds(ds_index); - let mut m = ds.lock(); - let buffered_frames = m.append_buffered_frames as usize; - let combined = std::mem::take(&mut m.append_buffer); - m.append_buffered_frames = 0; - (buffered_frames, combined) + // Merge the buffer with the new frames when it is the dataset's tail; + // a buffer left mid-extent (the extent moved past it) keeps its + // recorded place — flush it and start fresh at the current end. + let taken = { self.ds(ds_index).lock().append.take() }; + let (base_dim0, buffered_frames, mut combined) = match taken { + Some(b) if b.base + b.frames == current_dim0 as u64 => { + (b.base as usize, b.frames as usize, b.bytes) + } + Some(b) => { + self.write_append_frames(ds_index, b.base, b.frames, &b.bytes)?; + (current_dim0, 0, Vec::new()) + } + None => (current_dim0, 0, Vec::new()), }; combined.extend_from_slice(&raw); let total_frames = buffered_frames + n_new_frames; - let base_dim0 = current_dim0.checked_sub(buffered_frames).ok_or_else(|| { - crate::io::IoError::InvalidState(format!( - "the append buffer holds {buffered_frames} frames but the dataset \ - extent is {current_dim0}" - )) - })?; // Rows up to the last chunk boundary are written now; the tail that // does not complete a chunk goes back in the buffer for the next @@ -2764,8 +2774,11 @@ impl Hdf5Writer { if tail_frames > 0 { let ds = self.ds(ds_index); let mut m = ds.lock(); - m.append_buffer = combined[write_frames * frame_bytes..].to_vec(); - m.append_buffered_frames = tail_frames as u64; + m.append = Some(AppendBuffer { + base: (base_dim0 + write_frames) as u64, + frames: tail_frames as u64, + bytes: combined[write_frames * frame_bytes..].to_vec(), + }); } // Extend dims @@ -2789,9 +2802,8 @@ impl Hdf5Writer { /// does: `H5T__vlen_disk_write` deletes the reference it read into the /// conversion background buffer before storing the new one. /// - /// Elements the append buffer still holds are patched in the buffer rather - /// than on disk, because that buffer — not the file — is their current - /// content until it is flushed. + /// Elements the append buffer still holds are flushed to their chunks + /// first, so the whole range is on disk and one write path covers it. pub fn write_vlen_strings_slice( &self, ds_index: usize, @@ -2812,7 +2824,7 @@ impl Hdf5Writer { // Snapshot what the write needs, then drop the guard: `write_slice` // below re-locks the same slot. - let (charset, dims, buffered_frames) = { + let (charset, dims) = { let ds = self.ds(ds_index); let m = ds.lock(); let charset = match m.datatype { @@ -2824,11 +2836,7 @@ impl Hdf5Writer { )) } }; - ( - charset, - m.dataspace.dims.clone(), - m.append_buffered_frames as usize, - ) + (charset, m.dataspace.dims.clone()) }; if dims.len() != 1 { @@ -2862,28 +2870,17 @@ impl Hdf5Writer { } let ref_size = vlen_reference_size(&self.ctx); - // The append buffer holds the tail of the dataset, so the range splits - // into an on-disk prefix and a buffered suffix. An append that is - // between publishing its buffered count and extending the dims (two - // separate lock acquisitions on its side) can make the count exceed - // the extent; report that instead of wrapping. - let buffer_base = dims[0].checked_sub(buffered_frames as u64).ok_or_else(|| { - crate::io::IoError::InvalidState(format!( - "the append buffer holds {buffered_frames} frames but the dataset has {} elements", - dims[0] - )) - })?; - let split = end.min(buffer_base).max(start); + // Elements the append buffer holds are not in the chunks yet: hand + // them to the chunks first so the whole range is on disk and the one + // write path below covers it. + self.flush_append_buffer_if_intersecting(ds_index, start, end)?; // The on-disk references about to be overwritten, read before anything // moves. libhdf5 reads the same bytes into the conversion background // buffer (`H5D__scatgath_write` gathers the file's current elements // when `need_bkg` is set) and hands them to `H5T__vlen_disk_write`, - // which deletes them before storing the new reference. The buffered - // suffix is not on disk yet; its references are read further down, - // under the same lock that patches them. - let mut superseded = - self.current_element_bytes(ds_index, start, split - start, ref_size)?; + // which deletes them before storing the new reference. + let superseded = self.current_element_bytes(ds_index, start, end - start, ref_size)?; let gcol_encoded = gcol.encode(&self.ctx); let gcol_addr = self.allocator.allocate(gcol_encoded.len() as u64); @@ -2899,41 +2896,7 @@ impl Hdf5Writer { )); } - if split > start { - let n = (split - start) as usize; - self.write_slice(ds_index, &[start], &[n as u64], &refs[..n * ref_size])?; - } - if end > split { - // One lock acquisition owns the buffered suffix: the offset is - // derived, the superseded references read and the new ones patched - // against the same buffer. The snapshot above can be stale under - // the `threadsafe` feature — a concurrent append may have flushed - // the buffer and moved its base between the two acquisitions, - // putting the suffix on disk where this patch no longer reaches - // it — so a moved base is reported, not applied. A base that is - // unchanged means nothing flushed (appends only push it up), and - // then the buffer can only have grown past our range. - let ds = self.ds(ds_index); - let mut m = ds.lock(); - m.dataspace.dims[0] - .checked_sub(m.append_buffered_frames) - .filter(|&b| b == buffer_base) - .ok_or_else(|| { - crate::io::IoError::InvalidState(format!( - "the append buffer moved while elements {split}..{end} were being replaced" - )) - })?; - let off = ((split - buffer_base) as usize) * ref_size; - let tail = &refs[(split - start) as usize * ref_size..]; - if off + tail.len() > m.append_buffer.len() { - return Err(crate::io::IoError::InvalidState(format!( - "buffered elements {split}..{end} run past the {}-byte append buffer", - m.append_buffer.len() - ))); - } - superseded.extend_from_slice(&m.append_buffer[off..off + tail.len()]); - m.append_buffer[off..off + tail.len()].copy_from_slice(tail); - } + self.write_slice(ds_index, &[start], &[strings.len() as u64], &refs)?; // Only now that the new references are in place: a failure above must // not leave the file naming objects this already freed. @@ -3324,6 +3287,42 @@ impl Hdf5Writer { self.write_slice_chunked(ds_index, &starts, &counts, frames) } + /// Write the dataset's append buffer (if any) into its chunks and clear + /// it. The single owner of the buffer-to-chunks transition: the flush at + /// close, an append meeting a non-contiguous buffer, and any operation + /// about to write rows the buffer holds all come through here. + pub(crate) fn flush_append_buffer(&self, ds_index: usize) -> IoResult<()> { + let taken = { self.ds(ds_index).lock().append.take() }; + match taken { + Some(b) => self.write_append_frames(ds_index, b.base, b.frames, &b.bytes), + None => Ok(()), + } + } + + /// Flush the append buffer when rows `start_row .. end_row` intersect + /// the buffered range — those rows' current content is the buffer, and + /// writing them on disk while the buffer still holds them would be + /// undone by the flush at close. + pub(crate) fn flush_append_buffer_if_intersecting( + &self, + ds_index: usize, + start_row: u64, + end_row: u64, + ) -> IoResult<()> { + let intersects = { + let ds = self.ds(ds_index); + let m = ds.lock(); + m.append + .as_ref() + .is_some_and(|b| start_row < b.base + b.frames && end_row > b.base) + }; + if intersects { + self.flush_append_buffer(ds_index) + } else { + Ok(()) + } + } + /// Read an already-written chunk's *decompressed* bytes when the chunk /// is allocated and resolvable from the in-memory extensible-array /// index. Handles index-block and data-block chunks, filtered and @@ -3689,8 +3688,7 @@ impl Hdf5Writer { fa_dblk, chunks_written: 0, }), - append_buffer: Vec::new(), - append_buffered_frames: 0, + append: None, }); Ok(idx) @@ -3781,8 +3779,7 @@ impl Hdf5Writer { fa_dblk, chunks_written: 0, }), - append_buffer: Vec::new(), - append_buffered_frames: 0, + append: None, }); Ok(idx) @@ -3914,8 +3911,7 @@ impl Hdf5Writer { index: bt2_index, chunks_written: 0, }), - append_buffer: Vec::new(), - append_buffered_frames: 0, + append: None, }); Ok(idx) @@ -4017,8 +4013,7 @@ impl Hdf5Writer { filt_iblk: Some(filt_iblk), chunk_size_len, }), - append_buffer: Vec::new(), - append_buffered_frames: 0, + append: None, }); Ok(idx) @@ -4116,8 +4111,7 @@ impl Hdf5Writer { filt_iblk: Some(filt_iblk), chunk_size_len, }), - append_buffer: Vec::new(), - append_buffered_frames: 0, + append: None, }); Ok(idx) } @@ -4629,10 +4623,9 @@ impl Hdf5Writer { new_dims.len() ))); } - // A pending append buffer is positioned relative to the current - // logical size; changing the extent underneath it would make - // `flush_append_buffers` write the chunk at the wrong index. - if m.append_buffered_frames > 0 { + // A shrink can cut into buffered rows, whose recorded base would + // then point past the extent; refuse rather than reconcile. + if m.append.is_some() { return Err(crate::io::IoError::InvalidState( "set_extent cannot run while the dataset has buffered appends; \ flush them first" @@ -4929,38 +4922,13 @@ impl Hdf5Writer { // Internal helpers // ------------------------------------------------------------------ - /// Flush any partial append buffers into the chunks they belong to, - /// through [`write_append_frames`](Self::write_append_frames): frames + /// Flush every dataset's append buffer into the chunks it belongs to, + /// through [`flush_append_buffer`](Self::flush_append_buffer): frames /// already in the chunk survive, and the rest of it reads back as the /// dataset's fill value (zeros when none is defined). fn flush_append_buffers(&mut self) -> IoResult<()> { for i in 0..self.dataset_count() { - // Snapshot everything needed under one brief slot guard, then - // drop it: `write_append_frames` below re-locks the slot. - let (buf, buffered_frames, dims) = { - let ds = self.ds(i); - let mut m = ds.lock(); - if m.append_buffer.is_empty() { - continue; - } - if m.chunked.is_none() && m.fixed_array.is_none() && m.btree_v2.is_none() { - continue; - } - let buffered_frames = m.append_buffered_frames; - let dims = m.dataspace.dims.clone(); - let buf = std::mem::take(&mut m.append_buffer); - m.append_buffered_frames = 0; - (buf, buffered_frames, dims) - }; - - let base_frame = dims[0].checked_sub(buffered_frames).ok_or_else(|| { - crate::io::IoError::InvalidState(format!( - "the append buffer holds {buffered_frames} frames but the \ - dataset extent is {}", - dims[0] - )) - })?; - self.write_append_frames(i, base_frame, buffered_frames, &buf)?; + self.flush_append_buffer(i)?; } Ok(()) } @@ -5338,27 +5306,6 @@ mod tests { )) } - /// A concurrent append publishes its buffered count and extends the dims - /// under two different lock acquisitions, so a slice update can observe a - /// count that exceeds the extent. That half-published state must come back - /// as an error, not wrap `dims[0] - buffered` around zero. - #[test] - fn vlen_slice_reports_a_half_published_append_count() { - let path = temp_path("vlen_slice_half_published"); - - let writer = Hdf5Writer::create(&path).unwrap(); - let idx = writer.create_vlen_string_dataset("d", &["a", "b"]).unwrap(); - writer.ds(idx).lock().append_buffered_frames = 3; // dims[0] == 2 - let err = writer.write_vlen_strings_slice(idx, 0, &["x"]).unwrap_err(); - assert!( - err.to_string().contains("append buffer holds 3 frames"), - "unexpected error: {err}" - ); - - writer.ds(idx).lock().append_buffered_frames = 0; - std::fs::remove_file(&path).ok(); - } - /// A corrupt file can declare a zero-length chunk dimension; the /// superseded-reference read must reject it the way `write_slice` does, /// not divide by it. From 5bbf3f8a217232ce44214dfdce8303a96617c3f1 Mon Sep 17 00:00:00 2001 From: Sang Woo Kim Date: Thu, 13 Aug 2026 11:12:27 +0900 Subject: [PATCH 04/11] writer: share one vlen charset rule between slice and append MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ensure_vlen_charset owns the check write_vlen_strings_slice carried inline; append_vlen_strings gains it plus a datatype gate — it accepted any chunked dataset and wrote vlen references over its elements as raw bytes. libhdf5 stores the bytes unvalidated (no cset check on its vlen write path); h5py raises on the mismatch, and so do we. --- CHANGELOG.md | 7 +++++ src/io/writer.rs | 80 ++++++++++++++++++++++++++++++++++++++++++------ 2 files changed, 78 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d8f8031..5bfeb28 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,13 @@ controls the API mirrors) can produce such a file; previously the frame was zero-padded up to the tile. +- `append_vlen_strings` now applies the same character-set rule as + `write_vlen_strings_slice` — non-ASCII strings are rejected when the + dataset declares ASCII, instead of being stored mislabeled (libhdf5 + stores the bytes unvalidated; h5py raises on the same mismatch) — and + refuses a dataset whose elements are not variable-length strings, which + it previously overwrote with vlen references as raw bytes. + - The append buffer records the absolute row its frames belong to, and every operation that writes rows the buffer holds flushes it to the chunks first. Before this, the buffer's position was derived from the diff --git a/src/io/writer.rs b/src/io/writer.rs index 56ffc64..173b94a 100644 --- a/src/io/writer.rs +++ b/src/io/writer.rs @@ -436,6 +436,23 @@ fn validate_chunk_geometry(dims: &[u64], max_dims: &[u64], chunk_dims: &[u64]) - Ok(()) } +/// Reject strings the dataset's declared character set cannot label. +/// +/// A Rust `&str` is always UTF-8, so only an ASCII declaration (charset 0) +/// can be violated. libhdf5 stores the bytes unvalidated — its vlen write +/// path has no cset check anywhere — which mislabels them for every reader +/// that trusts the declaration (h5py raises on the same mismatch). +fn ensure_vlen_charset(charset: u8, strings: &[&str]) -> IoResult<()> { + if charset == 0 { + if let Some((i, s)) = strings.iter().enumerate().find(|(_, s)| !s.is_ascii()) { + return Err(crate::io::IoError::InvalidState(format!( + "string {i} ({s:?}) is not ASCII, but the dataset's character set is" + ))); + } + } + Ok(()) +} + /// Runtime metadata for a fixed-array-indexed chunked dataset. pub struct FixedArrayDatasetInfo { /// Chunk dimension sizes. @@ -2696,11 +2713,28 @@ impl Hdf5Writer { /// references, and appends them as new chunks to the dataset. pub fn append_vlen_strings(&self, ds_index: usize, strings: &[&str]) -> IoResult<()> { use crate::format::global_heap::{encode_vlen_reference, GlobalHeapCollection}; + use crate::format::messages::datatype::DatatypeMessage; if strings.is_empty() { return Ok(()); } + // The elements about to be written are vlen references; any other + // element type would be overwritten with them as raw bytes. + let charset = { + let ds = self.ds(ds_index); + let m = ds.lock(); + match m.datatype { + DatatypeMessage::VarLenString { charset } => charset, + _ => { + return Err(crate::io::IoError::InvalidState( + "append_vlen_strings is only for variable-length string datasets".into(), + )) + } + } + }; + ensure_vlen_charset(charset, strings)?; + // Build a new global heap collection for this batch let mut gcol = GlobalHeapCollection::new(); let mut obj_indices = Vec::with_capacity(strings.len()); @@ -2852,15 +2886,7 @@ impl Hdf5Writer { dims[0] ))); } - // charset 0 is ASCII. A Rust `&str` is UTF-8, so anything non-ASCII - // would be stored as UTF-8 under a datatype that declares otherwise. - if charset == 0 { - if let Some((i, s)) = strings.iter().enumerate().find(|(_, s)| !s.is_ascii()) { - return Err(crate::io::IoError::InvalidState(format!( - "string {i} ({s:?}) is not ASCII, but the dataset's character set is" - ))); - } - } + ensure_vlen_charset(charset, strings)?; // One collection for the batch, as `append_vlen_strings` does. let mut gcol = GlobalHeapCollection::new(); @@ -5306,6 +5332,42 @@ mod tests { )) } + /// The charset rule is one owner shared by every vlen string writer: + /// appends into an ASCII-declared dataset reject non-ASCII strings the + /// same way the slice writer does, and a dataset whose elements are not + /// vlen references at all is refused instead of overwritten with them. + #[test] + fn append_vlen_strings_checks_the_datatype_and_charset() { + let path = temp_path("append_vlen_charset"); + + let writer = Hdf5Writer::create(&path).unwrap(); + let idx = writer + .create_appendable_vlen_string_dataset("d", 4, None) + .unwrap(); + writer.ds(idx).lock().datatype = DatatypeMessage::vlen_string_ascii(); + let err = writer + .append_vlen_strings(idx, &["ok", "안녕"]) + .unwrap_err(); + assert!( + err.to_string().contains("is not ASCII"), + "unexpected error: {err}" + ); + writer.append_vlen_strings(idx, &["ok", "fine"]).unwrap(); + + let nums = writer + .create_chunked_dataset("n", DatatypeMessage::i32_type(), &[0], &[u64::MAX], &[4]) + .unwrap(); + let err = writer.append_vlen_strings(nums, &["x"]).unwrap_err(); + assert!( + err.to_string() + .contains("only for variable-length string datasets"), + "unexpected error: {err}" + ); + + writer.close().unwrap(); + std::fs::remove_file(&path).ok(); + } + /// A corrupt file can declare a zero-length chunk dimension; the /// superseded-reference read must reject it the way `write_slice` does, /// not divide by it. From 374ff8da01811f6ddfe8a0d682a00a9aff54cb7a Mon Sep 17 00:00:00 2001 From: Sang Woo Kim Date: Thu, 13 Aug 2026 11:19:44 +0900 Subject: [PATCH 05/11] writer: require the create gate by type at every dataset creator The vlen creators and create_chunked_dataset_compressed skipped create_lock and the uniqueness check, so a second create under an existing name emitted a file with two same-named links. begin_create now returns a CreateGuard witness that push_dataset requires, making the check-then-push atomicity hold at every creator by construction. --- CHANGELOG.md | 7 + src/io/writer.rs | 640 +++++++++++++++++++++++++++-------------------- 2 files changed, 373 insertions(+), 274 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5bfeb28..9c88c7f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -46,6 +46,13 @@ appends to reopened 0.4.0 files with a wider-than-row chunk land correctly instead of reading back `[1, 2, 0, 0]` for `[1, 2, 3, 4]`. +- Every dataset creator checks the new name is unique before registering + the dataset. The vlen creators and `create_chunked_dataset_compressed` + skipped the check, so creating two datasets under one name silently + emitted an invalid file with two same-named links. The check-then-push + pair is now a witness type (`begin_create` → `push_dataset`), so a + creator cannot skip it. + ## 0.4.1 ### Added diff --git a/src/io/writer.rs b/src/io/writer.rs index 173b94a..b0b138a 100644 --- a/src/io/writer.rs +++ b/src/io/writer.rs @@ -184,6 +184,17 @@ impl Slot { } } +/// Proof that the create gate (`create_lock`) is held and the new dataset's +/// name passed the uniqueness check. Only [`Hdf5Writer::begin_create`] +/// constructs one and [`Hdf5Writer::push_dataset`] demands one, so a creator +/// cannot reach the dataset registry while skipping either step. +pub(crate) struct CreateGuard<'a> { + #[cfg(not(feature = "threadsafe"))] + _gate: std::cell::RefMut<'a, ()>, + #[cfg(feature = "threadsafe")] + _gate: std::sync::MutexGuard<'a, ()>, +} + /// Reference-counted shared pointer, feature-selected. The single-thread /// build uses `Rc` (no atomics); the `threadsafe` build uses `Arc` so a /// dataset/group slot can be cloned out of the registry and locked on its @@ -688,10 +699,22 @@ impl Hdf5Writer { Shared::clone(&self.groups.lock()[index]) } + /// Enter the create gate: take `create_lock` and check that `name` is not + /// already taken. The returned witness is what [`Self::push_dataset`] + /// requires, so the uniqueness check and the registry push are atomic + /// (see `create_lock`) at every creator by construction. + pub(crate) fn begin_create(&self, name: &str) -> IoResult> { + let gate = self.create_lock.lock(); + self.ensure_unique_dataset_name(name)?; + Ok(CreateGuard { _gate: gate }) + } + /// Push a freshly-built dataset into the registry and return its index. /// Takes the registry lock only for the push, so it does not block an /// in-flight write that already cloned its own [`DatasetRef`] out. - pub(crate) fn push_dataset(&self, info: DatasetInfo) -> usize { + /// The [`CreateGuard`] proves the caller entered through + /// [`Self::begin_create`] and still holds the gate. + pub(crate) fn push_dataset(&self, _create: &CreateGuard<'_>, info: DatasetInfo) -> usize { let mut reg = self.datasets.lock(); let idx = reg.len(); reg.push(Shared::new(Slot::new(info))); @@ -1616,10 +1639,7 @@ impl Hdf5Writer { datatype: DatatypeMessage, dims: &[u64], ) -> IoResult { - // Hold the create gate across the uniqueness check and the registry - // push so the two are atomic (see `create_lock`). - let _create = self.create_lock.lock(); - self.ensure_unique_dataset_name(name)?; + let create = self.begin_create(name)?; let total_elements: u64 = if dims.is_empty() { 1 } else { @@ -1641,24 +1661,27 @@ impl Hdf5Writer { DataspaceMessage::simple(dims) }; - let idx = self.push_dataset(DatasetInfo { - name: name.to_string(), - datatype, - dataspace, - obj_header_addr: 0, // set during finalize - data_addr, - data_size, - chunked: None, - fixed_array: None, - btree_v2: None, - append: None, - attributes: Vec::new(), - obj_header_written_addr: None, - obj_header_encoded_size: 0, - filter_pipeline: None, - deleted: false, - fill_value: None, - }); + let idx = self.push_dataset( + &create, + DatasetInfo { + name: name.to_string(), + datatype, + dataspace, + obj_header_addr: 0, // set during finalize + data_addr, + data_size, + chunked: None, + fixed_array: None, + btree_v2: None, + append: None, + attributes: Vec::new(), + obj_header_written_addr: None, + obj_header_encoded_size: 0, + filter_pipeline: None, + deleted: false, + fill_value: None, + }, + ); Ok(idx) } @@ -1676,10 +1699,7 @@ impl Hdf5Writer { max_dims: &[u64], chunk_dims: &[u64], ) -> IoResult { - // Hold the create gate across the uniqueness check and the registry - // push so the two are atomic (see `create_lock`). - let _create = self.create_lock.lock(); - self.ensure_unique_dataset_name(name)?; + let create = self.begin_create(name)?; validate_chunk_geometry(dims, max_dims, chunk_dims)?; let earray_params = EarrayParams::default_params(); let ndblk_addrs = compute_ndblk_addrs(earray_params.sup_blk_min_data_ptrs)?; @@ -1728,36 +1748,39 @@ impl Hdf5Writer { max_dims: Some(max_dims.to_vec()), }; - let idx = self.push_dataset(DatasetInfo { - name: name.to_string(), - datatype, - dataspace, - obj_header_addr: 0, - data_addr: UNDEF_ADDR, - data_size: 0, - attributes: Vec::new(), - obj_header_written_addr: None, - obj_header_encoded_size: 0, - filter_pipeline: None, - deleted: false, - fill_value: None, - fixed_array: None, - btree_v2: None, - chunked: Some(ChunkedDatasetInfo { - chunk_dims: chunk_dims.to_vec(), - max_dims: max_dims.to_vec(), - earray_params, - ea_header_addr, - ea_iblk_addr, - ndblk_addrs, - ea_header, - ea_iblk, - chunks_written: 0, - filt_iblk: None, - chunk_size_len: 0, - }), - append: None, - }); + let idx = self.push_dataset( + &create, + DatasetInfo { + name: name.to_string(), + datatype, + dataspace, + obj_header_addr: 0, + data_addr: UNDEF_ADDR, + data_size: 0, + attributes: Vec::new(), + obj_header_written_addr: None, + obj_header_encoded_size: 0, + filter_pipeline: None, + deleted: false, + fill_value: None, + fixed_array: None, + btree_v2: None, + chunked: Some(ChunkedDatasetInfo { + chunk_dims: chunk_dims.to_vec(), + max_dims: max_dims.to_vec(), + earray_params, + ea_header_addr, + ea_iblk_addr, + ndblk_addrs, + ea_header, + ea_iblk, + chunks_written: 0, + filt_iblk: None, + chunk_size_len: 0, + }), + append: None, + }, + ); Ok(idx) } @@ -2391,6 +2414,7 @@ impl Hdf5Writer { use crate::format::global_heap::{encode_vlen_reference, GlobalHeapCollection}; use crate::format::messages::datatype::DatatypeMessage; + let create = self.begin_create(name)?; let num_strings = strings.len() as u64; // Build a global heap collection with all strings @@ -2429,24 +2453,27 @@ impl Hdf5Writer { let dataspace = crate::format::messages::dataspace::DataspaceMessage::simple(&[num_strings]); - let idx = self.push_dataset(DatasetInfo { - name: name.to_string(), - datatype, - dataspace, - obj_header_addr: 0, - data_addr, - data_size: data_size as u64, - attributes: Vec::new(), - obj_header_written_addr: None, - obj_header_encoded_size: 0, - filter_pipeline: None, - deleted: false, - fill_value: None, - chunked: None, - fixed_array: None, - btree_v2: None, - append: None, - }); + let idx = self.push_dataset( + &create, + DatasetInfo { + name: name.to_string(), + datatype, + dataspace, + obj_header_addr: 0, + data_addr, + data_size: data_size as u64, + attributes: Vec::new(), + obj_header_written_addr: None, + obj_header_encoded_size: 0, + filter_pipeline: None, + deleted: false, + fill_value: None, + chunked: None, + fixed_array: None, + btree_v2: None, + append: None, + }, + ); Ok(idx) } @@ -2463,6 +2490,7 @@ impl Hdf5Writer { use crate::format::global_heap::{encode_vlen_reference, GlobalHeapCollection}; use crate::format::messages::datatype::DatatypeMessage; + let create = self.begin_create(name)?; let num_items = items.len() as u64; // Build a global heap collection with all byte arrays. @@ -2501,24 +2529,27 @@ impl Hdf5Writer { let datatype = DatatypeMessage::vlen_bytes(); let dataspace = crate::format::messages::dataspace::DataspaceMessage::simple(&[num_items]); - let idx = self.push_dataset(DatasetInfo { - name: name.to_string(), - datatype, - dataspace, - obj_header_addr: 0, - data_addr, - data_size: data_size as u64, - attributes: Vec::new(), - obj_header_written_addr: None, - obj_header_encoded_size: 0, - filter_pipeline: None, - deleted: false, - fill_value: None, - chunked: None, - fixed_array: None, - btree_v2: None, - append: None, - }); + let idx = self.push_dataset( + &create, + DatasetInfo { + name: name.to_string(), + datatype, + dataspace, + obj_header_addr: 0, + data_addr, + data_size: data_size as u64, + attributes: Vec::new(), + obj_header_written_addr: None, + obj_header_encoded_size: 0, + filter_pipeline: None, + deleted: false, + fill_value: None, + chunked: None, + fixed_array: None, + btree_v2: None, + append: None, + }, + ); Ok(idx) } @@ -2538,6 +2569,7 @@ impl Hdf5Writer { use crate::format::global_heap::{encode_vlen_reference, GlobalHeapCollection}; use crate::format::messages::datatype::DatatypeMessage; + let create = self.begin_create(name)?; let num_strings = strings.len() as u64; validate_chunk_geometry(&[num_strings], &[num_strings], &[chunk_size as u64])?; @@ -2626,36 +2658,39 @@ impl Hdf5Writer { nsblk_addrs, ); - let idx = self.push_dataset(DatasetInfo { - name: name.to_string(), - datatype, - dataspace, - obj_header_addr: 0, - data_addr: UNDEF_ADDR, - data_size: 0, - attributes: Vec::new(), - obj_header_written_addr: None, - obj_header_encoded_size: 0, - filter_pipeline: Some(pipeline), - deleted: false, - fill_value: None, - fixed_array: None, - btree_v2: None, - chunked: Some(ChunkedDatasetInfo { - chunk_dims: chunk_dims.clone(), - max_dims: max_dims.clone(), - earray_params, - ea_header_addr, - ea_iblk_addr, - ndblk_addrs, - ea_header, - ea_iblk, - chunks_written: 0, - filt_iblk: Some(filt_iblk), - chunk_size_len, - }), - append: None, - }); + let idx = self.push_dataset( + &create, + DatasetInfo { + name: name.to_string(), + datatype, + dataspace, + obj_header_addr: 0, + data_addr: UNDEF_ADDR, + data_size: 0, + attributes: Vec::new(), + obj_header_written_addr: None, + obj_header_encoded_size: 0, + filter_pipeline: Some(pipeline), + deleted: false, + fill_value: None, + fixed_array: None, + btree_v2: None, + chunked: Some(ChunkedDatasetInfo { + chunk_dims: chunk_dims.clone(), + max_dims: max_dims.clone(), + earray_params, + ea_header_addr, + ea_iblk_addr, + ndblk_addrs, + ea_header, + ea_iblk, + chunks_written: 0, + filt_iblk: Some(filt_iblk), + chunk_size_len, + }), + append: None, + }, + ); // Write chunks of vlen references with compression let chunk_byte_size = chunk_bytes as usize; @@ -3648,10 +3683,7 @@ impl Hdf5Writer { dims: &[u64], chunk_dims: &[u64], ) -> IoResult { - // Hold the create gate across the uniqueness check and the registry - // push so the two are atomic (see `create_lock`). - let _create = self.create_lock.lock(); - self.ensure_unique_dataset_name(name)?; + let create = self.begin_create(name)?; // A fixed-array index means a fixed shape: max dims are the dims. validate_chunk_geometry(dims, dims, chunk_dims)?; let ndims = dims.len(); @@ -3691,31 +3723,34 @@ impl Hdf5Writer { let dataspace = DataspaceMessage::simple(dims); - let idx = self.push_dataset(DatasetInfo { - name: name.to_string(), - datatype, - dataspace, - obj_header_addr: 0, - data_addr: UNDEF_ADDR, - data_size: 0, - attributes: Vec::new(), - obj_header_written_addr: None, - obj_header_encoded_size: 0, - filter_pipeline: None, - deleted: false, - fill_value: None, - chunked: None, - btree_v2: None, - fixed_array: Some(FixedArrayDatasetInfo { - chunk_dims: chunk_dims.to_vec(), - fa_header_addr, - fa_dblk_addr, - fa_header, - fa_dblk, - chunks_written: 0, - }), - append: None, - }); + let idx = self.push_dataset( + &create, + DatasetInfo { + name: name.to_string(), + datatype, + dataspace, + obj_header_addr: 0, + data_addr: UNDEF_ADDR, + data_size: 0, + attributes: Vec::new(), + obj_header_written_addr: None, + obj_header_encoded_size: 0, + filter_pipeline: None, + deleted: false, + fill_value: None, + chunked: None, + btree_v2: None, + fixed_array: Some(FixedArrayDatasetInfo { + chunk_dims: chunk_dims.to_vec(), + fa_header_addr, + fa_dblk_addr, + fa_header, + fa_dblk, + chunks_written: 0, + }), + append: None, + }, + ); Ok(idx) } @@ -3736,10 +3771,7 @@ impl Hdf5Writer { chunk_dims: &[u64], pipeline: FilterPipeline, ) -> IoResult { - // Hold the create gate across the uniqueness check and the registry - // push so the two are atomic (see `create_lock`). - let _create = self.create_lock.lock(); - self.ensure_unique_dataset_name(name)?; + let create = self.begin_create(name)?; // A fixed-array index means a fixed shape: max dims are the dims. validate_chunk_geometry(dims, dims, chunk_dims)?; let ndims = dims.len(); @@ -3782,31 +3814,34 @@ impl Hdf5Writer { let dataspace = DataspaceMessage::simple(dims); - let idx = self.push_dataset(DatasetInfo { - name: name.to_string(), - datatype, - dataspace, - obj_header_addr: 0, - data_addr: UNDEF_ADDR, - data_size: 0, - attributes: Vec::new(), - obj_header_written_addr: None, - obj_header_encoded_size: 0, - filter_pipeline: Some(pipeline), - deleted: false, - fill_value: None, - chunked: None, - btree_v2: None, - fixed_array: Some(FixedArrayDatasetInfo { - chunk_dims: chunk_dims.to_vec(), - fa_header_addr, - fa_dblk_addr, - fa_header, - fa_dblk, - chunks_written: 0, - }), - append: None, - }); + let idx = self.push_dataset( + &create, + DatasetInfo { + name: name.to_string(), + datatype, + dataspace, + obj_header_addr: 0, + data_addr: UNDEF_ADDR, + data_size: 0, + attributes: Vec::new(), + obj_header_written_addr: None, + obj_header_encoded_size: 0, + filter_pipeline: Some(pipeline), + deleted: false, + fill_value: None, + chunked: None, + btree_v2: None, + fixed_array: Some(FixedArrayDatasetInfo { + chunk_dims: chunk_dims.to_vec(), + fa_header_addr, + fa_dblk_addr, + fa_header, + fa_dblk, + chunks_written: 0, + }), + append: None, + }, + ); Ok(idx) } @@ -3865,10 +3900,7 @@ impl Hdf5Writer { compute_chunk_size_len, Bt2Header, BT2_NODE_SIZE, }; - // Hold the create gate across the uniqueness check and the registry - // push so the two are atomic (see `create_lock`). - let _create = self.create_lock.lock(); - self.ensure_unique_dataset_name(name)?; + let create = self.begin_create(name)?; validate_chunk_geometry(dims, max_dims, chunk_dims)?; let ndims = dims.len(); @@ -3914,31 +3946,34 @@ impl Hdf5Writer { max_dims: Some(max_dims.to_vec()), }; - let idx = self.push_dataset(DatasetInfo { - name: name.to_string(), - datatype, - dataspace, - obj_header_addr: 0, - data_addr: UNDEF_ADDR, - data_size: 0, - attributes: Vec::new(), - obj_header_written_addr: None, - obj_header_encoded_size: 0, - filter_pipeline: pipeline, - deleted: false, - fill_value: None, - chunked: None, - fixed_array: None, - btree_v2: Some(Bt2DatasetInfo { - chunk_dims: chunk_dims.to_vec(), - max_dims: max_dims.to_vec(), - bt2_header_addr, - node_addrs: Vec::new(), - index: bt2_index, - chunks_written: 0, - }), - append: None, - }); + let idx = self.push_dataset( + &create, + DatasetInfo { + name: name.to_string(), + datatype, + dataspace, + obj_header_addr: 0, + data_addr: UNDEF_ADDR, + data_size: 0, + attributes: Vec::new(), + obj_header_written_addr: None, + obj_header_encoded_size: 0, + filter_pipeline: pipeline, + deleted: false, + fill_value: None, + chunked: None, + fixed_array: None, + btree_v2: Some(Bt2DatasetInfo { + chunk_dims: chunk_dims.to_vec(), + max_dims: max_dims.to_vec(), + bt2_header_addr, + node_addrs: Vec::new(), + index: bt2_index, + chunks_written: 0, + }), + append: None, + }, + ); Ok(idx) } @@ -3956,6 +3991,7 @@ impl Hdf5Writer { chunk_dims: &[u64], compression_level: u32, ) -> IoResult { + let create = self.begin_create(name)?; validate_chunk_geometry(dims, max_dims, chunk_dims)?; let element_size = datatype.element_size() as u64; let chunk_bytes: u64 = chunk_dims.iter().product::() * element_size; @@ -4011,36 +4047,39 @@ impl Hdf5Writer { nsblk_addrs, ); - let idx = self.push_dataset(DatasetInfo { - name: name.to_string(), - datatype, - dataspace, - obj_header_addr: 0, - data_addr: UNDEF_ADDR, - data_size: 0, - attributes: Vec::new(), - obj_header_written_addr: None, - obj_header_encoded_size: 0, - filter_pipeline: Some(FilterPipeline::deflate(compression_level)), - deleted: false, - fill_value: None, - fixed_array: None, - btree_v2: None, - chunked: Some(ChunkedDatasetInfo { - chunk_dims: chunk_dims.to_vec(), - max_dims: max_dims.to_vec(), - earray_params, - ea_header_addr, - ea_iblk_addr, - ndblk_addrs, - ea_header, - ea_iblk, - chunks_written: 0, - filt_iblk: Some(filt_iblk), - chunk_size_len, - }), - append: None, - }); + let idx = self.push_dataset( + &create, + DatasetInfo { + name: name.to_string(), + datatype, + dataspace, + obj_header_addr: 0, + data_addr: UNDEF_ADDR, + data_size: 0, + attributes: Vec::new(), + obj_header_written_addr: None, + obj_header_encoded_size: 0, + filter_pipeline: Some(FilterPipeline::deflate(compression_level)), + deleted: false, + fill_value: None, + fixed_array: None, + btree_v2: None, + chunked: Some(ChunkedDatasetInfo { + chunk_dims: chunk_dims.to_vec(), + max_dims: max_dims.to_vec(), + earray_params, + ea_header_addr, + ea_iblk_addr, + ndblk_addrs, + ea_header, + ea_iblk, + chunks_written: 0, + filt_iblk: Some(filt_iblk), + chunk_size_len, + }), + append: None, + }, + ); Ok(idx) } @@ -4055,10 +4094,7 @@ impl Hdf5Writer { chunk_dims: &[u64], pipeline: FilterPipeline, ) -> IoResult { - // Hold the create gate across the uniqueness check and the registry - // push so the two are atomic (see `create_lock`). - let _create = self.create_lock.lock(); - self.ensure_unique_dataset_name(name)?; + let create = self.begin_create(name)?; validate_chunk_geometry(dims, max_dims, chunk_dims)?; let element_size = datatype.element_size() as u64; let chunk_bytes: u64 = chunk_dims.iter().product::() * element_size; @@ -4109,36 +4145,39 @@ impl Hdf5Writer { nsblk_addrs, ); - let idx = self.push_dataset(DatasetInfo { - name: name.to_string(), - datatype, - dataspace, - obj_header_addr: 0, - data_addr: UNDEF_ADDR, - data_size: 0, - attributes: Vec::new(), - obj_header_written_addr: None, - obj_header_encoded_size: 0, - filter_pipeline: Some(pipeline), - deleted: false, - fill_value: None, - fixed_array: None, - btree_v2: None, - chunked: Some(ChunkedDatasetInfo { - chunk_dims: chunk_dims.to_vec(), - max_dims: max_dims.to_vec(), - earray_params, - ea_header_addr, - ea_iblk_addr, - ndblk_addrs, - ea_header, - ea_iblk, - chunks_written: 0, - filt_iblk: Some(filt_iblk), - chunk_size_len, - }), - append: None, - }); + let idx = self.push_dataset( + &create, + DatasetInfo { + name: name.to_string(), + datatype, + dataspace, + obj_header_addr: 0, + data_addr: UNDEF_ADDR, + data_size: 0, + attributes: Vec::new(), + obj_header_written_addr: None, + obj_header_encoded_size: 0, + filter_pipeline: Some(pipeline), + deleted: false, + fill_value: None, + fixed_array: None, + btree_v2: None, + chunked: Some(ChunkedDatasetInfo { + chunk_dims: chunk_dims.to_vec(), + max_dims: max_dims.to_vec(), + earray_params, + ea_header_addr, + ea_iblk_addr, + ndblk_addrs, + ea_header, + ea_iblk, + chunks_written: 0, + filt_iblk: Some(filt_iblk), + chunk_size_len, + }), + append: None, + }, + ); Ok(idx) } @@ -5368,6 +5407,59 @@ mod tests { std::fs::remove_file(&path).ok(); } + /// Every creator must enter through `begin_create`; the four that used + /// to bypass it could push a second dataset under an existing name and + /// emit an invalid file with two same-named links. + #[test] + fn every_creator_rejects_an_existing_dataset_name() { + let path = temp_path("create_gate"); + + let writer = Hdf5Writer::create(&path).unwrap(); + writer + .create_dataset("d", DatatypeMessage::i32_type(), &[2]) + .unwrap(); + + let attempts: [(&str, IoResult); 4] = [ + ( + "vlen_string", + writer.create_vlen_string_dataset("d", &["x"]), + ), + ("vlen_bytes", writer.create_vlen_bytes_dataset("d", &[b"x"])), + ( + "vlen_string_compressed", + writer.create_vlen_string_dataset_compressed( + "d", + &["x"], + 1, + FilterPipeline::deflate(6), + ), + ), + ( + "chunked_compressed", + writer.create_chunked_dataset_compressed( + "d", + DatatypeMessage::i32_type(), + &[0], + &[u64::MAX], + &[4], + 6, + ), + ), + ]; + for (which, res) in attempts { + match res { + Ok(_) => panic!("{which} accepted a duplicate name"), + Err(e) => assert!( + e.to_string().contains("already exists"), + "{which}: unexpected error: {e}" + ), + } + } + + writer.close().unwrap(); + std::fs::remove_file(&path).ok(); + } + /// A corrupt file can declare a zero-length chunk dimension; the /// superseded-reference read must reject it the way `write_slice` does, /// not divide by it. From 3a7e6faec191032c2f87e6e365e7fa40f3db083e Mon Sep 17 00:00:00 2001 From: Sang Woo Kim Date: Thu, 13 Aug 2026 11:20:50 +0900 Subject: [PATCH 06/11] writer: read a heap collection once in release_vlen_references Collections are padded to the 4096-byte H5HG_MINALLOC floor and most sit exactly there, so the 64-byte header probe plus full-size read was two syscalls where one covers the whole image. Oversized collections still get a second read at their declared size. --- src/io/writer.rs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/io/writer.rs b/src/io/writer.rs index b0b138a..f1d8a67 100644 --- a/src/io/writer.rs +++ b/src/io/writer.rs @@ -3083,10 +3083,15 @@ impl Hdf5Writer { } for (addr, indices) in per_collection { - let head = self.handle.read_at_most(addr, 64)?; - let declared = GlobalHeapCollection::decode_size(&head, &self.ctx)?; - let image = self.handle.read_at(addr, declared)?; - let (mut gcol, _) = GlobalHeapCollection::decode(&image, &self.ctx)?; + // A collection is at least 4096 bytes (H5HG_MINALLOC) and most are + // exactly that, so one read usually covers the whole image; only + // an oversized collection needs a second read at its declared size. + let mut image = self.handle.read_at_most(addr, 4096)?; + let declared = GlobalHeapCollection::decode_size(&image, &self.ctx)?; + if declared > image.len() { + image = self.handle.read_at(addr, declared)?; + } + let (mut gcol, _) = GlobalHeapCollection::decode(&image[..declared], &self.ctx)?; let mut removed_any = false; for idx in indices { removed_any |= gcol.remove_object(idx); From 3260353982fd380145f73f9d6965c800308280b1 Mon Sep 17 00:00:00 2001 From: Sang Woo Kim Date: Thu, 13 Aug 2026 11:21:22 +0900 Subject: [PATCH 07/11] writer: record the verified libhdf5 comparison on release_vlen_references Verified against hdf5 eeba6ab8a5a: H5T__vlen_disk_delete skips seq_len==0 while H5VL__native_blob_put inserts even empty sequences, so libhdf5 strands those objects and the address rule here frees them; and the vlen path never calls H5HG_link (only H5Dvirtual.c does), so the shared-object exposure equals libhdf5's. --- src/io/writer.rs | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/src/io/writer.rs b/src/io/writer.rs index f1d8a67..97621f9 100644 --- a/src/io/writer.rs +++ b/src/io/writer.rs @@ -3046,9 +3046,21 @@ impl Hdf5Writer { /// existing size with the recovered bytes given to the free-space marker, /// and a collection that ends up empty returns its block to the allocator. /// A nil reference (address 0 or `UNDEF_ADDR`) names no object. The - /// sequence length does not decide: this crate's writers store even the - /// empty string as a real heap object, so a zero-length reference with a - /// defined address still holds one that must be released. + /// address decides, not the sequence length: this crate's writers store + /// even the empty string as a real heap object, so a zero-length reference + /// with a defined address still holds one that must be released. libhdf5 + /// diverges here against itself — `H5T__vlen_disk_delete` returns before + /// `H5HG_remove` when the sequence length is zero, yet its write path + /// (`H5VL__native_blob_put`) inserts a heap object even for an empty + /// sequence, stranding it forever. The address rule frees those objects. + /// + /// Heap objects carry no reference count on this path, matching libhdf5: + /// its vlen code never calls `H5HG_link` (only the virtual-dataset layer + /// does). Releasing the same reference twice is absorbed by the + /// missing-index check below, but a crafted file in which two elements + /// share one heap object would lose it for the survivor when either is + /// replaced — the same exposure the file has under libhdf5. This crate's + /// writers never share: each element write inserts its own object. /// /// Under SWMR nothing is freed and no collection is rewritten: a reader may /// be following those references, the same reason `place_chunk` keeps a From 662a1738888a060a1d660ddf8b47972417c8c716 Mon Sep 17 00:00:00 2001 From: Sang Woo Kim Date: Thu, 13 Aug 2026 11:42:11 +0900 Subject: [PATCH 08/11] writer/reader: index chunks by the maximum-extent grid MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit libhdf5 assigns a chunk its row-major slot in the chunk grid of the MAXIMUM extent (max_down_chunks; the fixed array is sized from max_nchunks), so a slot never moves when the dataset grows. Slots here came from the current extent, which only coincides while every dimension after the first sits at its maximum; other geometries wrote libhdf5-unreadable files and re-scrambled slots on extend. One owner (io::chunk_grid) now serves writer, reader and dataset API. With it: create_fixed_array_dataset_with_max sizes the array from the maximum (the builder no longer drops a finite max_shape), growth past the stored maximum — or with none stored — is rejected as H5Dset_extent does, and an unlimited non-leading dimension is rejected at create since without swizzling its chunks have no fixed slot. --- CHANGELOG.md | 27 +++ src/dataset.rs | 261 +++++++++++++---------- src/io/chunk_grid.rs | 236 +++++++++++++++++++++ src/io/mod.rs | 1 + src/io/reader.rs | 123 +++++------ src/io/writer.rs | 372 ++++++++++++++++----------------- tests/h5py_cross_validation.rs | 43 ++++ 7 files changed, 685 insertions(+), 378 deletions(-) create mode 100644 src/io/chunk_grid.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 9c88c7f..029a331 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,6 +53,33 @@ pair is now a witness type (`begin_create` → `push_dataset`), so a creator cannot skip it. +- Chunk slots are computed against the **maximum-extent** chunk grid, the + libhdf5 rule (`max_down_chunks` in H5Dfarray.c/H5Dearray.c/H5Dnone.c; + the fixed array is sized from `max_nchunks`). Slots were computed from + the *current* extent, which coincides only while every dimension after + the first sits at its maximum — any other geometry wrote files libhdf5 + reads differently, and extending re-scrambled the mapping. One owner + (`io::chunk_grid`) now serves the writer, the reader, and the dataset + API. Fallout fixed with it: + - The builder silently dropped a finite `max_shape` on the fixed-array + path (no unlimited dimension): the array was sized from the current + shape and the stored dataspace had no maximum, so the dataset could + never grow. `create_fixed_array_dataset_with_max` sizes the array + from the maximum's grid and such datasets now extend/append up to + their maximum. The fixed-shape creators keep their signatures and + now store `max_dims == dims` explicitly. + - **Behavior change:** growing a dataset past its stored maximum — or + growing one with *no* stored maximum at all — is rejected by + `extend_dataset`/`set_dataset_extent`, matching `H5Dset_extent` + (libhdf5 defaults maxdims to dims at creation). Previously the grow + succeeded and writes failed later, or scrambled chunk slots. + - **Behavior change:** creating an extensible-array dataset whose + unlimited dimension is not dimension 0 is rejected. Its chunks have + no fixed linear slot without libhdf5's swizzling (not implemented); + the geometry previously re-indexed — i.e. silently lost — chunks on + every extend. Reading such a file (libhdf5-written) now errors + instead of returning wrong data. + ## 0.4.1 ### Added diff --git a/src/dataset.rs b/src/dataset.rs index 0ede8ae..276c6c7 100644 --- a/src/dataset.rs +++ b/src/dataset.rs @@ -319,20 +319,13 @@ impl DatasetBuilder { // must use the fixed-array index — libhdf5 // rejects an extensible-array index here. A // compressed fixed-shape dataset uses a *filtered* - // fixed array (FA client id 1). - if wants_filter { - writer.create_fixed_array_dataset_with_pipeline( - &full_name, - datatype, - &dims_u64, - &chunk_u64, - explicit_pipeline(), - )? - } else { - writer.create_fixed_array_dataset( - &full_name, datatype, &dims_u64, &chunk_u64, - )? - } + // fixed array (FA client id 1). The maximum shape + // sizes the array, so a finite max above the + // current shape stays growable. + let pipeline = wants_filter.then(explicit_pipeline); + writer.create_fixed_array_dataset_with_max( + &full_name, datatype, &dims_u64, &max_u64, &chunk_u64, pipeline, + )? } else if let Some(pipeline) = self.custom_pipeline { writer.create_chunked_dataset_with_pipeline( &full_name, datatype, &dims_u64, &max_u64, &chunk_u64, pipeline, @@ -997,7 +990,11 @@ impl H5Dataset { } let total_chunks: u64 = grid.iter().product(); - // Decode a linear chunk index into row-major grid coordinates. + // Decode the iteration counter into row-major coordinates over the + // *current* image's chunk grid. This is only an odometer over the + // chunks the image spans — the slot a chunk is recorded under comes + // from the index grid (`Hdf5Writer::chunk_slot`), which the maximum + // extent decides. let coords_of = |linear: u64| -> Vec { let mut rem = linear; let mut coords = vec![0u64; rank]; @@ -1031,23 +1028,25 @@ impl H5Dataset { let mut start = 0u64; while start < total_chunks { let end = (start + BATCH_WINDOW).min(total_chunks); - let items: Vec<(u64, Vec, Vec)> = (start..end) - .map(|linear| { - let coords = coords_of(linear); + let items: Vec<(Vec, Vec)> = (start..end) + .map(|counter| { + let coords = coords_of(counter); let buf = Self::gather_chunk(bytes, &dims, &chunk_dims, &coords, element_size); - (linear, coords, buf) + (coords, buf) }) .collect(); if fixed_array { let pairs: Vec<(&[u64], &[u8])> = items .iter() - .map(|(_, c, d)| (c.as_slice(), d.as_slice())) + .map(|(c, d)| (c.as_slice(), d.as_slice())) .collect(); writer.write_chunks_fixed_array_batch(index, &pairs)?; } else { - let pairs: Vec<(u64, &[u8])> = - items.iter().map(|(l, _, d)| (*l, d.as_slice())).collect(); + let mut pairs: Vec<(u64, &[u8])> = Vec::with_capacity(items.len()); + for (c, d) in &items { + pairs.push((writer.chunk_slot(index, c)?, d.as_slice())); + } writer.write_chunks_batch(index, &pairs)?; } start = end; @@ -1165,42 +1164,9 @@ impl H5Dataset { match &*inner { H5FileInner::Writer(writer) => { if *fixed_array { - // Fixed-array dataset: convert the linear chunk - // index into row-major grid coordinates. - let chunk_dims = writer - .dataset_chunk_dims(*index) - .ok_or_else(|| { - Hdf5Error::InvalidState("dataset has no chunk info".into()) - })? - .to_vec(); - let dims = writer.dataset_dims(*index).to_vec(); - let mut grid = vec![0u64; dims.len()]; - for d in 0..dims.len() { - grid[d] = if chunk_dims[d] > 0 { - dims[d].div_ceil(chunk_dims[d]) - } else { - 1 - }; - } - // A zero-extent dimension yields a grid of 0 - // chunks — there is no chunk to write. - if grid.contains(&0) { - return Err(Hdf5Error::InvalidState( - "dataset has a zero-extent dimension and no chunks".into(), - )); - } - let mut rem = chunk_idx as u64; - let mut coords = vec![0u64; dims.len()]; - for d in (0..dims.len()).rev() { - coords[d] = rem % grid[d]; - rem /= grid[d]; - } - // A leftover means chunk_idx exceeded the grid. - if rem != 0 { - return Err(Hdf5Error::InvalidState(format!( - "chunk index {chunk_idx} is out of range for this dataset" - ))); - } + // Fixed-array dataset: decode the index-grid slot + // into row-major grid coordinates. + let coords = writer.chunk_coords_from_slot(*index, chunk_idx as u64)?; writer.write_chunk_fixed_array(*index, &coords, data)?; } else { writer.write_chunk(*index, chunk_idx as u64, data)?; @@ -1270,42 +1236,9 @@ impl H5Dataset { match &*inner { H5FileInner::Writer(writer) => { if *fixed_array { - // Fixed-array dataset: convert the linear chunk - // index into row-major grid coordinates. - let chunk_dims = writer - .dataset_chunk_dims(*index) - .ok_or_else(|| { - Hdf5Error::InvalidState("dataset has no chunk info".into()) - })? - .to_vec(); - let dims = writer.dataset_dims(*index).to_vec(); - let mut grid = vec![0u64; dims.len()]; - for d in 0..dims.len() { - grid[d] = if chunk_dims[d] > 0 { - dims[d].div_ceil(chunk_dims[d]) - } else { - 1 - }; - } - // A zero-extent dimension yields a grid of 0 - // chunks — there is no chunk to write. - if grid.contains(&0) { - return Err(Hdf5Error::InvalidState( - "dataset has a zero-extent dimension and no chunks".into(), - )); - } - let mut rem = chunk_idx as u64; - let mut coords = vec![0u64; dims.len()]; - for d in (0..dims.len()).rev() { - coords[d] = rem % grid[d]; - rem /= grid[d]; - } - // A leftover means chunk_idx exceeded the grid. - if rem != 0 { - return Err(Hdf5Error::InvalidState(format!( - "chunk index {chunk_idx} is out of range for this dataset" - ))); - } + // Fixed-array dataset: decode the index-grid slot + // into row-major grid coordinates. + let coords = writer.chunk_coords_from_slot(*index, chunk_idx as u64)?; writer.write_compressed_chunk_fixed_array( *index, &coords, @@ -1488,24 +1421,9 @@ impl H5Dataset { .write_compressed_chunk_btree_v2(*index, &coords, data, filter_mask)?, } } else { - // Extensible array: linearize the chunk-grid coordinates - // (row-major) into the array's chunk index. - let mut linear = 0u64; - for d in 0..dims.len() { - let grid = if chunk_dims[d] > 0 { - dims[d].div_ceil(chunk_dims[d]) - } else { - 1 - }; - linear = linear - .checked_mul(grid) - .and_then(|l| l.checked_add(coords[d])) - .ok_or_else(|| { - Hdf5Error::InvalidState( - "chunk coordinates overflow the array index".into(), - ) - })?; - } + // Extensible array: the chunk's index-grid slot (row-major + // against the maximum extent). + let linear = writer.chunk_slot(*index, &coords)?; match bytes { ChunkBytes::Unfiltered(data) => writer.write_chunk(*index, linear, data)?, ChunkBytes::Prefiltered { data, filter_mask } => { @@ -5470,6 +5388,125 @@ mod tests { std::fs::remove_file(&path).ok(); } + /// A finite max_shape above the current shape used to be dropped on the + /// fixed-array path: the array was sized from the current dims and the + /// stored dataspace had no maximum, so growth failed. The array is now + /// sized from the maximum's chunk grid (libhdf5 `max_nchunks`), so a + /// fixed-max dataset appends up to its maximum and roundtrips. + #[test] + fn fixed_array_with_a_larger_max_shape_grows_and_survives_close() { + let path = temp_path("fa_growable_dim0"); + { + let file = H5File::create(&path).unwrap(); + let ds = file + .new_dataset::() + .shape([4, 3]) + .chunk(&[2, 3]) + .max_shape(&[Some(10), Some(3)]) + .create("d") + .unwrap(); + ds.write_raw(&(0..12).collect::>()).unwrap(); + ds.append(&(12..18).collect::>()).unwrap(); + file.close().unwrap(); + } + { + let file = H5File::open(&path).unwrap(); + let ds = file.dataset("d").unwrap(); + assert_eq!(ds.shape(), vec![6, 3]); + assert_eq!(ds.read_raw::().unwrap(), (0..18).collect::>()); + } + std::fs::remove_file(&path).ok(); + } + + /// The multiplier-dimension boundary: growing a dimension other than 0 + /// changes the current chunk grid but not the index grid. Chunk slots + /// must come from the maximum's grid (libhdf5 `max_down_chunks`), or the + /// chunks written before the extend are looked up under different + /// indices after it. + #[test] + fn fixed_array_growable_inner_dimension_keeps_chunk_slots() { + let path = temp_path("fa_growable_dim1"); + { + let file = H5File::create(&path).unwrap(); + let ds = file + .new_dataset::() + .shape([4, 3]) + .chunk(&[2, 3]) + .max_shape(&[Some(4), Some(9)]) + .create("d") + .unwrap(); + ds.write_raw(&(0..12).collect::>()).unwrap(); + ds.extend(&[4, 6]).unwrap(); + ds.write_slice(&[0, 3], &[4, 3], &(12..24).collect::>()) + .unwrap(); + file.close().unwrap(); + } + { + let file = H5File::open(&path).unwrap(); + let ds = file.dataset("d").unwrap(); + assert_eq!(ds.shape(), vec![4, 6]); + // Row-major [4,6]: row r is [r*3 .. r*3+3) from the first write + // then [12 + r*3 ..) from the second. + let mut expect = Vec::new(); + for r in 0i32..4 { + expect.extend((r * 3)..(r * 3 + 3)); + expect.extend((12 + r * 3)..(12 + r * 3 + 3)); + } + assert_eq!(ds.read_raw::().unwrap(), expect); + } + std::fs::remove_file(&path).ok(); + } + + /// Growth boundaries: past the stored maximum is rejected, and a dataset + /// without a stored maximum is fixed at its extent (libhdf5 defaults + /// maxdims to dims at creation). + #[test] + fn extend_beyond_the_maximum_is_rejected() { + let path = temp_path("extend_beyond_max"); + let file = H5File::create(&path).unwrap(); + let ds = file + .new_dataset::() + .shape([4, 3]) + .chunk(&[2, 3]) + .max_shape(&[Some(6), Some(3)]) + .create("d") + .unwrap(); + ds.extend(&[6, 3]).unwrap(); + let err = ds.extend(&[8, 3]).unwrap_err(); + assert!( + err.to_string().contains("exceeds the maximum"), + "unexpected error: {err}" + ); + file.close().unwrap(); + std::fs::remove_file(&path).ok(); + } + + /// An unlimited dimension other than 0 has no fixed linear slot without + /// libhdf5's extensible-array swizzling, which is not implemented; + /// creating the geometry silently re-indexed chunks on every extend, so + /// it is rejected at create. + #[test] + fn builder_rejects_an_unlimited_inner_dimension() { + let path = temp_path("unlimited_inner_dim"); + let file = H5File::create(&path).unwrap(); + let err = match file + .new_dataset::() + .shape([4, 0]) + .chunk(&[2, 2]) + .max_shape(&[Some(4), None]) + .create("d") + { + Ok(_) => panic!("create accepted an unlimited inner dimension"), + Err(e) => e, + }; + assert!( + err.to_string().contains("not the first"), + "unexpected error: {err}" + ); + file.close().unwrap(); + std::fs::remove_file(&path).ok(); + } + /// Regression: a chunk wider than a fixed max dimension used to be /// accepted, and appends then packed rows at the chunk stride — writing /// [1, 2, 3, 4] and reading back [1, 2, 0, 0]. libhdf5 rejects the diff --git a/src/io/chunk_grid.rs b/src/io/chunk_grid.rs new file mode 100644 index 0000000..ad9ec90 --- /dev/null +++ b/src/io/chunk_grid.rs @@ -0,0 +1,236 @@ +//! Row-major chunk linear indexing against the maximum-extent grid. +//! +//! libhdf5 indexes a chunk by its row-major position in the chunk grid of the +//! *maximum* dataspace extent, never the current one: `H5D__chunk_set_info_real` +//! (H5Dchunk.c) computes `max_chunks[d] = ceil(max_dims[d] / chunk_dims[d])` +//! and every linearly-addressed index — fixed array (H5Dfarray.c), extensible +//! array (H5Dearray.c), implicit (H5Dnone.c) — feeds `max_down_chunks` to +//! `H5VM_array_offset_pre`. That makes a chunk's index permanent: extending +//! the dataset never re-addresses chunks already written. +//! +//! Row-major offsets never multiply by the slowest dimension's own extent, so +//! dimension 0 may grow — or be unlimited — without entering the arithmetic. +//! An *unlimited* dimension other than 0 has no finite multiplier; libhdf5 +//! handles it by swizzling that dimension to the slowest position +//! (H5Dearray.c), which this crate does not implement, so such coordinates +//! are rejected here and such dataspaces are rejected at dataset create. +//! +//! This module is the single owner of that arithmetic for the writer, the +//! reader, and the high-level dataset API. + +use crate::io::{IoError, IoResult}; + +/// Chunk count along each dimension of the *index* grid: the maximum extent +/// where one is declared (absent maximum means the shape is fixed, so the +/// current extent is the maximum), the current extent for an unlimited +/// dimension. Rejects a zero chunk dimension and a rank mismatch. +pub(crate) fn index_grid( + dims: &[u64], + max_dims: Option<&[u64]>, + chunk_dims: &[u64], +) -> IoResult> { + let ndims = dims.len(); + if chunk_dims.len() != ndims { + return Err(IoError::InvalidState(format!( + "dataset chunk shape has {} dimensions but the dataspace has {}", + chunk_dims.len(), + ndims + ))); + } + if let Some(max) = max_dims { + if max.len() != ndims { + return Err(IoError::InvalidState(format!( + "dataset maximum shape has {} dimensions but the dataspace has {}", + max.len(), + ndims + ))); + } + } + let mut grid = Vec::with_capacity(ndims); + for d in 0..ndims { + if chunk_dims[d] == 0 { + return Err(IoError::InvalidState(format!( + "chunk dimension {d} is zero" + ))); + } + let extent = match max_dims { + Some(max) if max[d] != u64::MAX => max[d], + _ => dims[d], + }; + grid.push(extent.div_ceil(chunk_dims[d])); + } + Ok(grid) +} + +/// Whether dimension `d`'s maximum extent is unlimited. +fn is_unlimited(max_dims: Option<&[u64]>, d: usize) -> bool { + max_dims.is_some_and(|m| m[d] == u64::MAX) +} + +/// Row-major linear index of the chunk at grid `coords` — the slot an +/// extensible or fixed array records the chunk under. +/// +/// Bounds every coordinate by the index grid (an unlimited dimension is +/// unbounded); an out-of-grid coordinate on a bounded dimension would +/// otherwise silently alias another chunk's slot. +pub(crate) fn linear_index( + dims: &[u64], + max_dims: Option<&[u64]>, + chunk_dims: &[u64], + coords: &[u64], +) -> IoResult { + let ndims = dims.len(); + if coords.len() != ndims { + return Err(IoError::InvalidState(format!( + "chunk_coords has {} entries but the dataset has {} dimensions", + coords.len(), + ndims + ))); + } + let grid = index_grid(dims, max_dims, chunk_dims)?; + let mut linear = 0u64; + for d in 0..ndims { + if is_unlimited(max_dims, d) { + if d != 0 { + return Err(IoError::InvalidState(format!( + "unlimited dimension {d} is not the first: its chunks have \ + no fixed linear index (extensible-array swizzling is not \ + supported)" + ))); + } + } else if coords[d] >= grid[d] { + return Err(IoError::InvalidState(format!( + "chunk coordinate {} in dimension {} is outside the chunk grid (0..{})", + coords[d], d, grid[d] + ))); + } + // The multiplier for coords[d] is the grid extent of the dimensions + // after it; dimension 0's own extent is never multiplied in, which is + // what lets it grow without re-indexing. + linear = if d == 0 { + coords[0] + } else { + linear + .checked_mul(grid[d]) + .and_then(|l| l.checked_add(coords[d])) + .ok_or_else(|| { + IoError::InvalidState("chunk coordinates overflow the array index".into()) + })? + }; + } + Ok(linear) +} + +/// Grid coordinates of the chunk at row-major `linear` — the inverse of +/// [`linear_index`]. Dimension 0 takes the leftover quotient, so an index +/// beyond the current extent (a slot written before a shrink, or one that +/// only becomes visible after an extend) still decodes to its true position. +pub(crate) fn coords_of( + dims: &[u64], + max_dims: Option<&[u64]>, + chunk_dims: &[u64], + linear: u64, +) -> IoResult> { + let ndims = dims.len(); + let grid = index_grid(dims, max_dims, chunk_dims)?; + let mut coords = vec![0u64; ndims]; + let mut rem = linear; + for d in (1..ndims).rev() { + if is_unlimited(max_dims, d) { + return Err(IoError::InvalidState(format!( + "unlimited dimension {d} is not the first: its chunks have \ + no fixed linear index (extensible-array swizzling is not \ + supported)" + ))); + } + if grid[d] == 0 { + return Err(IoError::InvalidState(format!( + "chunk grid is empty in dimension {d}" + ))); + } + coords[d] = rem % grid[d]; + rem /= grid[d]; + } + if ndims > 0 { + if !is_unlimited(max_dims, 0) && rem >= grid[0] { + return Err(IoError::InvalidState(format!( + "chunk index {linear} is outside the chunk grid" + ))); + } + coords[0] = rem; + } else if rem != 0 { + return Err(IoError::InvalidState(format!( + "chunk index {linear} is outside the chunk grid" + ))); + } + Ok(coords) +} + +#[cfg(test)] +mod tests { + use super::*; + + /// The multiplier for every coordinate comes from the maximum extent, + /// so an index assigned while the dataset was small stays valid after + /// an extend — the libhdf5 `max_down_chunks` rule. + #[test] + fn indices_come_from_the_maximum_grid() { + // dims [4,3], max [10,9], chunks [2,3]: max grid is [5,3]. + let dims = [4, 3]; + let max = [10, 9]; + let chunks = [2, 3]; + let li = |c: &[u64]| linear_index(&dims, Some(&max), &chunks, c).unwrap(); + assert_eq!(li(&[0, 0]), 0); + assert_eq!(li(&[0, 2]), 2); + assert_eq!(li(&[1, 0]), 3); // 3 columns of chunks in the MAX grid, not 1 + assert_eq!(li(&[4, 2]), 14); + assert_eq!( + coords_of(&dims, Some(&max), &chunks, 14).unwrap(), + vec![4, 2] + ); + } + + /// Without a stored maximum the shape is fixed, so the current extent is + /// the maximum and the grids coincide. + #[test] + fn absent_maximum_means_the_current_extent() { + let dims = [4, 4]; + let chunks = [2, 2]; + assert_eq!(linear_index(&dims, None, &chunks, &[1, 1]).unwrap(), 3); + assert_eq!(coords_of(&dims, None, &chunks, 3).unwrap(), vec![1, 1]); + let err = linear_index(&dims, None, &chunks, &[2, 0]).unwrap_err(); + assert!(err.to_string().contains("outside the chunk grid")); + } + + /// Dimension 0 never enters the multiplication, so an unlimited first + /// dimension is indexable and its coordinate is unbounded; decoding an + /// index beyond the current extent recovers the true position. + #[test] + fn unlimited_dimension_zero_is_unbounded() { + let dims = [4, 8]; + let max = [u64::MAX, 8]; + let chunks = [2, 4]; + assert_eq!( + linear_index(&dims, Some(&max), &chunks, &[100, 1]).unwrap(), + 201 + ); + assert_eq!( + coords_of(&dims, Some(&max), &chunks, 201).unwrap(), + vec![100, 1] + ); + } + + /// An unlimited dimension other than 0 has no finite multiplier; libhdf5 + /// swizzles it to the slowest position, which this crate does not + /// implement, so both directions reject it. + #[test] + fn unlimited_inner_dimension_is_rejected() { + let dims = [4, 0]; + let max = [4, u64::MAX]; + let chunks = [2, 2]; + let err = linear_index(&dims, Some(&max), &chunks, &[0, 0]).unwrap_err(); + assert!(err.to_string().contains("not the first")); + let err = coords_of(&dims, Some(&max), &chunks, 0).unwrap_err(); + assert!(err.to_string().contains("not the first")); + } +} diff --git a/src/io/mod.rs b/src/io/mod.rs index fca1985..7225082 100644 --- a/src/io/mod.rs +++ b/src/io/mod.rs @@ -5,6 +5,7 @@ //! and SWMR (Single Writer Multiple Reader) protocol support. pub mod allocator; +pub(crate) mod chunk_grid; pub mod file_handle; pub(crate) mod hyperslab; pub mod locking; diff --git a/src/io/reader.rs b/src/io/reader.rs index 61a5ec2..740d970 100644 --- a/src/io/reader.rs +++ b/src/io/reader.rs @@ -1607,50 +1607,41 @@ impl Hdf5Reader { return Ok(()); } - // Total chunk count across all dimensions. - let chunks_dim0: u64 = (0..dims.len()) - .map(|d| { - if chunk_dims[d] > 0 { - dims[d].div_ceil(chunk_dims[d]) - } else { - 0 - } - }) - .fold(1u64, |acc, n| acc.saturating_mul(n)); + // Total slot count of the index grid. The maximum extent + // decides the multipliers (libhdf5 max_down_chunks); an + // unlimited dimension 0 is bounded by the current extent for + // this read — a slot beyond it (written before a shrink) is + // not visible. + let max_dims = info.dataspace.max_dims.clone(); + let grid = + crate::io::chunk_grid::index_grid(&dims, max_dims.as_deref(), chunk_dims)?; + let chunks_total: u64 = grid.iter().fold(1u64, |acc, &n| acc.saturating_mul(n)); let chunk_entries = self.collect_ea_chunk_entries( index_address, params, &dims, + max_dims.as_deref(), chunk_dims, element_size, )?; - let n_chunks = std::cmp::min(chunks_dim0 as usize, chunk_entries.len()); - - // Chunks are placed N-dimensionally: the linear chunk index - // maps (row-major) to chunk-grid coordinates, so sub-frame - // chunks (a chunk smaller than a full frame) land correctly. - let ndims = dims.len(); - let chunks_per_dim: Vec = (0..ndims) - .map(|d| { - if chunk_dims[d] > 0 { - dims[d].div_ceil(chunk_dims[d]) - } else { - 0 - } - }) - .collect(); - let chunk_coords = |mut i: u64| -> Vec { - let mut c = vec![0u64; ndims]; - for d in (0..ndims).rev() { - if chunks_per_dim[d] > 0 { - c[d] = i % chunks_per_dim[d]; - i /= chunks_per_dim[d]; - } - } - c - }; + let n_chunks = std::cmp::min(chunks_total as usize, chunk_entries.len()); + + // Chunks are placed N-dimensionally: each slot decodes + // (row-major, against the index grid) to chunk-grid + // coordinates, so sub-frame chunks (a chunk smaller than a + // full frame) land correctly. + let mut slot_coords = Vec::with_capacity(n_chunks); + for i in 0..n_chunks as u64 { + slot_coords.push(crate::io::chunk_grid::coords_of( + &dims, + max_dims.as_deref(), + chunk_dims, + i, + )?); + } + let chunk_coords = |i: u64| -> &[u64] { &slot_coords[i as usize] }; // Build one read job per chunk (no I/O yet), then read + // decompress them together (in parallel where positioned reads @@ -1669,7 +1660,7 @@ impl Hdf5Reader { || nbytes == 0 || addr >= file_size || nbytes > file_size - || !target.overlaps(&chunk_coords(i as u64), chunk_dims) + || !target.overlaps(chunk_coords(i as u64), chunk_dims) { None } else { @@ -1688,7 +1679,7 @@ impl Hdf5Reader { .enumerate() .map(|(i, &(addr, nbytes, _))| { if addr == UNDEF_ADDR - || !target.overlaps(&chunk_coords(i as u64), chunk_dims) + || !target.overlaps(chunk_coords(i as u64), chunk_dims) { None } else { @@ -1714,7 +1705,7 @@ impl Hdf5Reader { output, &dims, chunk_dims, - &coords, + coords, element_size, ); } @@ -1892,25 +1883,22 @@ impl Hdf5Reader { } } - // Compute number of chunks per dimension. A zero chunk dimension - // from a malformed layout message would divide by zero. - if chunk_dims.contains(&0) { - return Err(crate::io::IoError::InvalidState( - "fixed-array layout has a zero chunk dimension".into(), - )); + // Index-grid slot -> chunk-grid coordinates (row-major, against the + // maximum extent — the array was sized from its chunk grid, so a slot + // beyond the current extent still decodes to its true position and + // then simply falls outside the read target). A zero chunk dimension + // from a malformed layout message is rejected inside. + let max_dims = info.dataspace.max_dims.clone(); + let mut slot_coords = Vec::with_capacity(chunk_entries.len()); + for i in 0..chunk_entries.len() as u64 { + slot_coords.push(crate::io::chunk_grid::coords_of( + &dims, + max_dims.as_deref(), + chunk_dims, + i, + )?); } - let chunks_per_dim: Vec = (0..ndims) - .map(|d| dims[d].div_ceil(chunk_dims[d])) - .collect(); - // Linear chunk index -> chunk-grid coordinates (row-major). - let chunk_coords = |mut i: u64| -> Vec { - let mut c = vec![0u64; ndims]; - for d in (0..ndims).rev() { - c[d] = i % chunks_per_dim[d]; - i /= chunks_per_dim[d]; - } - c - }; + let chunk_coords = |i: u64| -> &[u64] { &slot_coords[i as usize] }; // Build one read job per chunk (no I/O yet). Filtered chunks carry // their exact compressed size (read at-most, since a zero size means @@ -1922,7 +1910,7 @@ impl Hdf5Reader { .enumerate() .map(|(linear_idx, &(addr, comp_size, mask))| { if addr == UNDEF_ADDR - || !target.overlaps(&chunk_coords(linear_idx as u64), chunk_dims) + || !target.overlaps(chunk_coords(linear_idx as u64), chunk_dims) { None } else if pipeline.is_some() { @@ -1960,7 +1948,7 @@ impl Hdf5Reader { output, &dims, chunk_dims, - &coords, + coords, element_size, ); } @@ -2648,6 +2636,7 @@ impl Hdf5Reader { index_address: u64, params: &data_layout::EarrayParams, dims: &[u64], + max_dims: Option<&[u64]>, chunk_dims: &[u64], element_size: u64, ) -> IoResult> { @@ -2662,17 +2651,13 @@ impl Hdf5Reader { return Ok(vec![]); } - // Total chunk count across every dimension (sub-frame chunks make - // this larger than the dim-0 chunk count alone). - let chunks_dim0: usize = (0..dims.len()) - .map(|d| { - if chunk_dims[d] > 0 { - dims[d].div_ceil(chunk_dims[d]) as usize - } else { - 0 - } - }) - .fold(1usize, |acc, n| acc.saturating_mul(n)); + // Slot count of the index grid, bounding the collection walk. The + // maximum extent decides the multipliers (sub-frame chunks make this + // larger than the dim-0 chunk count alone); the unlimited dimension 0 + // is bounded by the current extent. + let chunks_dim0: usize = crate::io::chunk_grid::index_grid(dims, max_dims, chunk_dims)? + .iter() + .fold(1usize, |acc, &n| acc.saturating_mul(n as usize)); let geo = EaGeometry::new( params.idx_blk_elmts, params.data_blk_min_elmts, diff --git a/src/io/writer.rs b/src/io/writer.rs index 97621f9..832d605 100644 --- a/src/io/writer.rs +++ b/src/io/writer.rs @@ -345,68 +345,16 @@ impl ChunkGeometry { self.chunk_dims.iter().product::() * self.element_size } - /// Number of chunks spanning each dimension at the current extent. - fn grid(&self) -> Vec { - self.dims - .iter() - .zip(&self.chunk_dims) - .map(|(&d, &c)| if c > 0 { d.div_ceil(c) } else { 0 }) - .collect() - } - /// Row-major position of `coords` in the chunk grid — the linear index an - /// extensible or fixed array records the chunk under. - /// - /// The grid extents multiplied here come from the *current* dims, which is - /// how every chunk written so far was indexed. The bound a coordinate is - /// checked against comes from `max_dims`, so an unlimited dimension — the - /// one an extensible array exists to grow — is unbounded, while a fixed - /// dimension still rejects an out-of-grid coordinate that would otherwise - /// silently alias another chunk's slot. + /// extensible or fixed array records the chunk under, computed against + /// the maximum-extent grid by [`crate::io::chunk_grid::linear_index`]. fn linear_index(&self, coords: &[u64]) -> IoResult { - let ndims = self.dims.len(); - if coords.len() != ndims { - return Err(crate::io::IoError::InvalidState(format!( - "chunk_coords has {} entries but the dataset has {} dimensions", - coords.len(), - ndims - ))); - } - if self.chunk_dims.len() != ndims { - return Err(crate::io::IoError::InvalidState(format!( - "dataset chunk shape has {} dimensions but the dataspace has {}", - self.chunk_dims.len(), - ndims - ))); - } - let grid = self.grid(); - let mut linear = 0u64; - for d in 0..ndims { - if self.chunk_dims[d] == 0 { - return Err(crate::io::IoError::InvalidState(format!( - "chunk dimension {d} is zero" - ))); - } - let extent = self.max_dims.as_ref().map_or(self.dims[d], |m| m[d]); - if extent != u64::MAX { - let bound = extent.div_ceil(self.chunk_dims[d]); - if coords[d] >= bound { - return Err(crate::io::IoError::InvalidState(format!( - "chunk coordinate {} in dimension {} is outside the chunk grid (0..{})", - coords[d], d, bound - ))); - } - } - linear = linear - .checked_mul(grid[d]) - .and_then(|l| l.checked_add(coords[d])) - .ok_or_else(|| { - crate::io::IoError::InvalidState( - "chunk coordinates overflow the array index".into(), - ) - })?; - } - Ok(linear) + crate::io::chunk_grid::linear_index( + &self.dims, + self.max_dims.as_deref(), + &self.chunk_dims, + coords, + ) } } @@ -447,6 +395,24 @@ fn validate_chunk_geometry(dims: &[u64], max_dims: &[u64], chunk_dims: &[u64]) - Ok(()) } +/// An extensible-array index linearizes chunk coordinates, which requires +/// every unlimited dimension to be dimension 0: any later dimension is a +/// multiplier in the row-major index and must be finite. libhdf5 supports +/// other positions by swizzling the unlimited dimension to the slowest one +/// (H5Dearray.c), which this crate does not implement. +fn ensure_unlimited_is_leading(max_dims: &[u64]) -> IoResult<()> { + for (d, &m) in max_dims.iter().enumerate().skip(1) { + if m == u64::MAX { + return Err(crate::io::IoError::InvalidState(format!( + "unlimited dimension {d} is not the first: extensible-array \ + swizzling is not supported; reorder the dimensions so the \ + unlimited one comes first" + ))); + } + } + Ok(()) +} + /// Reject strings the dataset's declared character set cannot label. /// /// A Rust `&str` is always UTF-8, so only an ASCII declaration (charset 0) @@ -1701,6 +1667,7 @@ impl Hdf5Writer { ) -> IoResult { let create = self.begin_create(name)?; validate_chunk_geometry(dims, max_dims, chunk_dims)?; + ensure_unlimited_is_leading(max_dims)?; let earray_params = EarrayParams::default_params(); let ndblk_addrs = compute_ndblk_addrs(earray_params.sup_blk_min_data_ptrs)?; let nsblk_addrs = compute_nsblk_addrs( @@ -3583,10 +3550,16 @@ impl Hdf5Writer { chunk_coords: &[u64], ) -> IoResult>> { let geo = self.chunk_geometry(ds_index)?; - let linear = geo.linear_index(chunk_coords)?; + // Only the linearly-addressed indexes compute a slot; a v2 B-tree is + // keyed by the coordinates themselves (and may hold unlimited inner + // dimensions, which have no linear slot). match geo.kind { - ChunkIndexKind::ExtensibleArray => self.read_chunk_if_present(ds_index, linear), + ChunkIndexKind::ExtensibleArray => { + let linear = geo.linear_index(chunk_coords)?; + self.read_chunk_if_present(ds_index, linear) + } ChunkIndexKind::FixedArray => { + let linear = geo.linear_index(chunk_coords)?; let ds = self.ds(ds_index); let m = ds.lock(); let pipeline = m.filter_pipeline.clone(); @@ -3689,10 +3662,31 @@ impl Hdf5Writer { }) } - /// Define a chunked dataset indexed by a fixed array (no unlimited dimensions). - /// - /// `dims` and `max_dims` should be the same (all fixed). `chunk_dims` defines the - /// chunk shape. Returns the dataset index. + /// Index-grid slot of the chunk at grid `coords` (see + /// [`crate::io::chunk_grid`]). + pub(crate) fn chunk_slot(&self, ds_index: usize, coords: &[u64]) -> IoResult { + self.chunk_geometry(ds_index)?.linear_index(coords) + } + + /// Grid coordinates of the chunk recorded under index-grid slot `linear` + /// — the inverse of [`Self::chunk_slot`]. + pub(crate) fn chunk_coords_from_slot( + &self, + ds_index: usize, + linear: u64, + ) -> IoResult> { + let geo = self.chunk_geometry(ds_index)?; + crate::io::chunk_grid::coords_of( + &geo.dims, + geo.max_dims.as_deref(), + &geo.chunk_dims, + linear, + ) + } + + /// Define a chunked dataset indexed by a fixed array, fixed at its + /// current shape (`max_dims == dims`). `chunk_dims` defines the chunk + /// shape. Returns the dataset index. pub fn create_fixed_array_dataset( &self, name: &str, @@ -3700,80 +3694,11 @@ impl Hdf5Writer { dims: &[u64], chunk_dims: &[u64], ) -> IoResult { - let create = self.begin_create(name)?; - // A fixed-array index means a fixed shape: max dims are the dims. - validate_chunk_geometry(dims, dims, chunk_dims)?; - let ndims = dims.len(); - let mut num_chunks: u64 = 1; - for d in 0..ndims { - num_chunks = num_chunks - .checked_mul(dims[d].div_ceil(chunk_dims[d])) - .ok_or_else(|| { - crate::io::IoError::InvalidState("chunk count overflows u64".into()) - })?; - } - - // Create FA header - let mut fa_header = FixedArrayHeader::new_for_chunks(&self.ctx, num_chunks); - let hdr_encoded = fa_header.encode(&self.ctx); - let fa_header_addr = self.allocator.allocate(hdr_encoded.len() as u64); - - // Create FA data block. libhdf5 switches to a paged layout once - // num_elmts exceeds dblk_page_nelmts; both layouts allocate space - // for `num_chunks` chunk addresses up front, but the paged layout - // also reserves the page-init bitmap and a per-page checksum. - let fa_dblk = FixedArrayDataBlock::new_unfiltered(fa_header_addr, num_chunks as usize); - let dblk_size = fixed_array_dblk_disk_size(&self.ctx, &fa_header); - let fa_dblk_addr = self.allocator.allocate(dblk_size); - - // Update header with data block address - fa_header.data_blk_addr = fa_dblk_addr; - - // Write both. The data block content is finalized in `flush_dataset` - // once all chunk addresses are known; here we just reserve space and - // write the header so the file is structurally consistent. - let hdr_encoded = fa_header.encode(&self.ctx); - self.handle.write_at(fa_header_addr, &hdr_encoded)?; - let dblk_encoded = encode_fixed_array_dblk(&self.ctx, &fa_header, &fa_dblk); - debug_assert_eq!(dblk_encoded.len() as u64, dblk_size); - self.handle.write_at(fa_dblk_addr, &dblk_encoded)?; - - let dataspace = DataspaceMessage::simple(dims); - - let idx = self.push_dataset( - &create, - DatasetInfo { - name: name.to_string(), - datatype, - dataspace, - obj_header_addr: 0, - data_addr: UNDEF_ADDR, - data_size: 0, - attributes: Vec::new(), - obj_header_written_addr: None, - obj_header_encoded_size: 0, - filter_pipeline: None, - deleted: false, - fill_value: None, - chunked: None, - btree_v2: None, - fixed_array: Some(FixedArrayDatasetInfo { - chunk_dims: chunk_dims.to_vec(), - fa_header_addr, - fa_dblk_addr, - fa_header, - fa_dblk, - chunks_written: 0, - }), - append: None, - }, - ); - - Ok(idx) + self.create_fixed_array_dataset_with_max(name, datatype, dims, dims, chunk_dims, None) } - /// Define a fixed-shape (no unlimited dimension) compressed chunked dataset - /// indexed by a *filtered* Fixed Array. + /// Define a fixed-shape compressed chunked dataset indexed by a + /// *filtered* Fixed Array (`max_dims == dims`). /// /// Like `create_fixed_array_dataset`, but the FA header carries the filtered /// client id and a `chunk_size_len`-wide compressed-size field per chunk @@ -3787,49 +3712,95 @@ impl Hdf5Writer { dims: &[u64], chunk_dims: &[u64], pipeline: FilterPipeline, + ) -> IoResult { + self.create_fixed_array_dataset_with_max( + name, + datatype, + dims, + dims, + chunk_dims, + Some(pipeline), + ) + } + + /// Define a chunked dataset indexed by a fixed array, growable up to + /// `max_dims` (every maximum finite — libhdf5 picks this index exactly + /// when no dimension is unlimited). + /// + /// The array is sized for the chunk grid of the *maximum* extent, the + /// libhdf5 rule (`H5D__farray_idx_create` uses `max_nchunks`), so the + /// dataset can be extended to `max_dims` without re-indexing chunks. + pub fn create_fixed_array_dataset_with_max( + &self, + name: &str, + datatype: DatatypeMessage, + dims: &[u64], + max_dims: &[u64], + chunk_dims: &[u64], + pipeline: Option, ) -> IoResult { let create = self.begin_create(name)?; - // A fixed-array index means a fixed shape: max dims are the dims. - validate_chunk_geometry(dims, dims, chunk_dims)?; - let ndims = dims.len(); + validate_chunk_geometry(dims, max_dims, chunk_dims)?; + if max_dims.contains(&u64::MAX) { + return Err(crate::io::IoError::InvalidState( + "a fixed-array index requires a fixed maximum shape (no unlimited dimension)" + .into(), + )); + } let mut num_chunks: u64 = 1; - for d in 0..ndims { - num_chunks = num_chunks - .checked_mul(dims[d].div_ceil(chunk_dims[d])) - .ok_or_else(|| { - crate::io::IoError::InvalidState("chunk count overflows u64".into()) - })?; + for g in crate::io::chunk_grid::index_grid(dims, Some(max_dims), chunk_dims)? { + num_chunks = num_chunks.checked_mul(g).ok_or_else(|| { + crate::io::IoError::InvalidState("chunk count overflows u64".into()) + })?; } - // chunk_size_len is sized from the uncompressed chunk byte count, the - // same way the filtered Extensible Array path computes it: the - // compressed size never exceeds the uncompressed size meaningfully, so - // this width always holds the stored value. - let element_size = datatype.element_size() as u64; - let chunk_bytes: u64 = chunk_dims.iter().product::() * element_size; - let chunk_size_len = compute_chunk_size_len(chunk_bytes); - - // Create the filtered FA header. - let mut fa_header = - FixedArrayHeader::new_for_filtered_chunks(&self.ctx, num_chunks, chunk_size_len); + // Create the FA header. For a filtered FA, chunk_size_len is sized + // from the uncompressed chunk byte count, the same way the filtered + // Extensible Array path computes it: the compressed size never + // exceeds the uncompressed size meaningfully, so this width always + // holds the stored value. + let mut fa_header = if pipeline.is_some() { + let element_size = datatype.element_size() as u64; + let chunk_bytes: u64 = chunk_dims.iter().product::() * element_size; + let chunk_size_len = compute_chunk_size_len(chunk_bytes); + FixedArrayHeader::new_for_filtered_chunks(&self.ctx, num_chunks, chunk_size_len) + } else { + FixedArrayHeader::new_for_chunks(&self.ctx, num_chunks) + }; let hdr_encoded = fa_header.encode(&self.ctx); let fa_header_addr = self.allocator.allocate(hdr_encoded.len() as u64); - // Create the filtered FA data block; both flat and paged layouts - // reserve space for `num_chunks` filtered entries up front. - let fa_dblk = FixedArrayDataBlock::new_filtered(fa_header_addr, num_chunks as usize); + // Create the FA data block. libhdf5 switches to a paged layout once + // num_elmts exceeds dblk_page_nelmts; both layouts allocate space + // for `num_chunks` entries up front, but the paged layout also + // reserves the page-init bitmap and a per-page checksum. + let fa_dblk = if pipeline.is_some() { + FixedArrayDataBlock::new_filtered(fa_header_addr, num_chunks as usize) + } else { + FixedArrayDataBlock::new_unfiltered(fa_header_addr, num_chunks as usize) + }; let dblk_size = fixed_array_dblk_disk_size(&self.ctx, &fa_header); let fa_dblk_addr = self.allocator.allocate(dblk_size); + // Update header with data block address fa_header.data_blk_addr = fa_dblk_addr; + // Write both. The data block content is finalized in `flush_dataset` + // once all chunk addresses are known; here we just reserve space and + // write the header so the file is structurally consistent. let hdr_encoded = fa_header.encode(&self.ctx); self.handle.write_at(fa_header_addr, &hdr_encoded)?; let dblk_encoded = encode_fixed_array_dblk(&self.ctx, &fa_header, &fa_dblk); debug_assert_eq!(dblk_encoded.len() as u64, dblk_size); self.handle.write_at(fa_dblk_addr, &dblk_encoded)?; - let dataspace = DataspaceMessage::simple(dims); + // The maximum is stored even when it equals the dims: it is what + // `extend_dataset` checks growth against, and the FA capacity above + // is exactly its chunk grid. + let dataspace = DataspaceMessage { + dims: dims.to_vec(), + max_dims: Some(max_dims.to_vec()), + }; let idx = self.push_dataset( &create, @@ -3843,7 +3814,7 @@ impl Hdf5Writer { attributes: Vec::new(), obj_header_written_addr: None, obj_header_encoded_size: 0, - filter_pipeline: Some(pipeline), + filter_pipeline: pipeline, deleted: false, fill_value: None, chunked: None, @@ -4010,6 +3981,7 @@ impl Hdf5Writer { ) -> IoResult { let create = self.begin_create(name)?; validate_chunk_geometry(dims, max_dims, chunk_dims)?; + ensure_unlimited_is_leading(max_dims)?; let element_size = datatype.element_size() as u64; let chunk_bytes: u64 = chunk_dims.iter().product::() * element_size; let chunk_size_len = compute_chunk_size_len(chunk_bytes); @@ -4113,6 +4085,7 @@ impl Hdf5Writer { ) -> IoResult { let create = self.begin_create(name)?; validate_chunk_geometry(dims, max_dims, chunk_dims)?; + ensure_unlimited_is_leading(max_dims)?; let element_size = datatype.element_size() as u64; let chunk_bytes: u64 = chunk_dims.iter().product::() * element_size; let chunk_size_len = compute_chunk_size_len(chunk_bytes); @@ -4296,32 +4269,14 @@ impl Hdf5Writer { .as_ref() .ok_or_else(|| crate::io::IoError::InvalidState("not a fixed-array dataset".into()))?; - // Compute linear chunk index from multidimensional coordinates. - let dims = &m.dataspace.dims; - let chunk_dims = &fa.chunk_dims; - let ndims = dims.len(); - if chunk_coords.len() != ndims { - return Err(crate::io::IoError::InvalidState(format!( - "chunk_coords has {} entries but the dataset has {} dimensions", - chunk_coords.len(), - ndims - ))); - } - let mut linear_idx: u64 = 0; - let mut stride: u64 = 1; - for d in (0..ndims).rev() { - let n_chunks_in_dim = dims[d].div_ceil(chunk_dims[d]); - // Reject an out-of-grid coordinate: without this an inner - // dimension's overflow silently aliases a different chunk slot. - if chunk_coords[d] >= n_chunks_in_dim { - return Err(crate::io::IoError::InvalidState(format!( - "chunk coordinate {} in dimension {} is outside the chunk grid (0..{})", - chunk_coords[d], d, n_chunks_in_dim - ))); - } - linear_idx += chunk_coords[d] * stride; - stride *= n_chunks_in_dim; - } + // Linear chunk index in the maximum-extent grid — the slot the fixed + // array (sized from that grid at create) records the chunk under. + let linear_idx = crate::io::chunk_grid::linear_index( + &m.dataspace.dims, + m.dataspace.max_dims.as_deref(), + &fa.chunk_dims, + chunk_coords, + )?; // Update the fixed array data block. The slot is read before the bytes // are placed so a rewrite can stay where it is (see `place_chunk`). @@ -4667,13 +4622,22 @@ impl Hdf5Writer { "extend_dataset cannot shrink dimension {d} from {cur} to {new}" ))); } - if let Some(ref max) = m.dataspace.max_dims { - if new > max[d] { + // An absent maximum shape means the shape is fixed (libhdf5 + // defaults maxdims to dims at creation), so any growth exceeds it. + match m.dataspace.max_dims { + Some(ref max) if new > max[d] => { return Err(crate::io::IoError::InvalidState(format!( "extend_dataset dimension {d} ({new}) exceeds the maximum {}", max[d] ))); } + None if new > cur => { + return Err(crate::io::IoError::InvalidState(format!( + "extend_dataset dimension {d} ({new}) exceeds the maximum {cur}: \ + a dataset without a stored maximum shape is fixed at its extent" + ))); + } + _ => {} } } m.dataspace.dims = new_dims.to_vec(); @@ -4714,12 +4678,26 @@ impl Hdf5Writer { .into(), )); } - if let Some(ref max) = m.dataspace.max_dims { - for (d, (&new, &mx)) in new_dims.iter().zip(max).enumerate() { - if new > mx { - return Err(crate::io::IoError::InvalidState(format!( - "set_extent dimension {d} ({new}) exceeds the maximum {mx}" - ))); + // An absent maximum shape means the shape is fixed (libhdf5 defaults + // maxdims to dims at creation), so growth is bounded by the extent. + match m.dataspace.max_dims { + Some(ref max) => { + for (d, (&new, &mx)) in new_dims.iter().zip(max).enumerate() { + if new > mx { + return Err(crate::io::IoError::InvalidState(format!( + "set_extent dimension {d} ({new}) exceeds the maximum {mx}" + ))); + } + } + } + None => { + for (d, (&new, &cur)) in new_dims.iter().zip(&m.dataspace.dims).enumerate() { + if new > cur { + return Err(crate::io::IoError::InvalidState(format!( + "set_extent dimension {d} ({new}) exceeds the maximum {cur}: \ + a dataset without a stored maximum shape is fixed at its extent" + ))); + } } } } diff --git a/tests/h5py_cross_validation.rs b/tests/h5py_cross_validation.rs index 4ca9430..fd9943f 100644 --- a/tests/h5py_cross_validation.rs +++ b/tests/h5py_cross_validation.rs @@ -772,3 +772,46 @@ fn bt2_4_depth_two_tree_readable_by_h5py() { ); std::fs::remove_file(&path).ok(); } + +/// A fixed-array dataset with a finite maximum above its shape: the array is +/// sized from the maximum's chunk grid and slots are row-major in that grid +/// (libhdf5 `max_nchunks` / `max_down_chunks`), including a maximum that +/// grows a non-leading dimension — the case where the index grid and the +/// current-extent grid disagree. h5py must see the maxshape and read every +/// value written before and after the extends. +#[test] +fn fa_growable_max_shape_readable_by_h5py() { + let Some(py) = python() else { return }; + let path = tmp("fa_growable_max"); + { + let file = H5File::create(&path).unwrap(); + let ds = file + .new_dataset::() + .shape([4, 3]) + .chunk(&[2, 3]) + .max_shape(&[Some(6), Some(9)]) + .create("grid") + .unwrap(); + ds.write_raw(&(0..12).collect::>()).unwrap(); + ds.extend(&[6, 6]).unwrap(); + ds.write_slice(&[0, 3], &[6, 3], &(100..118).collect::>()) + .unwrap(); + ds.write_slice(&[4, 0], &[2, 3], &(200..206).collect::>()) + .unwrap(); + file.close().unwrap(); + } + read_back_with_h5py( + py, + &path, + "ds = f['grid']\n\ + assert ds.shape == (6, 6), ds.shape\n\ + assert ds.maxshape == (6, 9), ds.maxshape\n\ + assert ds.chunks == (2, 3), ds.chunks\n\ + expect = np.zeros((6, 6), dtype=np.int32)\n\ + expect[:4, :3] = np.arange(12).reshape(4, 3)\n\ + expect[:, 3:6] = np.arange(100, 118).reshape(6, 3)\n\ + expect[4:6, :3] = np.arange(200, 206).reshape(2, 3)\n\ + assert np.array_equal(ds[...], expect), ds[...]\n", + ); + std::fs::remove_file(&path).ok(); +} From 85cd64de60deb3c3b403cb634930646a797d606d Mon Sep 17 00:00:00 2001 From: Sang Woo Kim Date: Thu, 13 Aug 2026 12:00:41 +0900 Subject: [PATCH 09/11] writer: serialize whole same-dataset operations with a per-dataset op lock The dataset slot mutex serializes one acquisition, not one operation: an append is take-buffer, write chunks, re-buffer, extend, and two threads could interleave between those steps and lose rows. DatasetCell now pairs an op lock with the metadata slot; public write entries hold it for the whole operation and delegate to _inner variants, whose callers must hold it or own the writer via &mut. The single-thread RefCell panics on nested acquisition, so a missed split fails any test run. Falsified: the new concurrency test fails with the guard removed. --- CHANGELOG.md | 11 ++ docs/threadsafe-fine-grained-locking.md | 33 ++-- src/dataset.rs | 63 +++++-- src/io/writer.rs | 241 ++++++++++++++++++++++-- tests/threadsafe_same_dataset_append.rs | 194 +++++++++++++++++++ 5 files changed, 497 insertions(+), 45 deletions(-) create mode 100644 tests/threadsafe_same_dataset_append.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 029a331..3c1ea3b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,17 @@ ### Fixed +- Concurrent operations on the *same* dataset serialize wholly under the + `threadsafe` feature. Each dataset now carries an operation lock beside + its metadata slot; every public write entry (`write_chunk*`, + `write_slice`, `append*`, `extend`/`set_extent`, `flush`) holds it for + the operation's full duration. The per-slot mutex only serialized each + individual acquisition, so two threads appending to one dataset could + interleave between the buffer take, the chunk writes and the extend — + losing or doubling rows. Same-dataset concurrency was previously + documented as unsupported; it is now correct, and writes to different + datasets still never contend. + - Chunk geometry is validated at every dataset create, the rule libhdf5 applies in `H5D__chunk_construct`: the chunk rank must match the dataspace, no chunk dimension may be zero, and a chunk dimension may not exceed a diff --git a/docs/threadsafe-fine-grained-locking.md b/docs/threadsafe-fine-grained-locking.md index 7ce34cc..d6a3671 100644 --- a/docs/threadsafe-fine-grained-locking.md +++ b/docs/threadsafe-fine-grained-locking.md @@ -140,17 +140,28 @@ chunked dataset) becomes: lock the *one* target `DatasetSlot`, do the short allocate+write+record, unlock. Different datasets run fully in parallel. -Scope of the same-dataset guarantee: a *single* slot operation — one -`write_chunk` / `record_*` — serializes on that slot's `Mutex`, so it -cannot tear the chunk index. It does **not** make a multi-step sequence -atomic. `append` deliberately drops the slot between phases (read dims → -take `append_buffer` → compress + write each chunk → `extend_dataset`) so -compression and disk I/O run unlocked; two threads appending to the *same* -dataset can therefore interleave those phases and corrupt the -buffer/dimension accounting. The supported concurrency is across -**distinct** datasets (one writer per dataset, e.g. a rayon `par_iter` -over datasets). Concurrent writes to a *single* dataset are unsupported; -callers must serialize them externally. +Scope of the same-dataset guarantee: the metadata slot's `Mutex` +serializes a *single* acquisition — one `record_*`, one buffer take — so +it cannot tear the chunk index, but it does not make a multi-step +sequence atomic. `append` deliberately drops the slot between phases +(read dims → take `append_buffer` → compress + write each chunk → +`extend_dataset`) so compression and disk I/O run unlocked. Whole +operations are serialized one level up: each `DatasetCell` carries an +**op lock** beside its metadata slot, every public write entry +(`write_chunk*`, `write_slice`, `append*`, `extend`/`set_extent`, +`flush`) takes it for the operation's full duration and delegates to a +`_inner` variant, and multi-acquisition compositions in `dataset.rs` +take it around their whole sequence. `_inner` variants and the +`pub(crate)` helpers (`write_append_frames`, `flush_append_buffer*`, +`write_chunk_at_coords`) require the caller to hold it — or to hold the +writer exclusively via `&mut`, as close and the SWMR wrapper do. The op +lock is not reentrant; the single-thread build's `RefCell` panics on a +nested acquisition, so a missed entry/inner split fails in every test +run. Lock order: `create_lock → op → registry spine → metadata slot`, +never two datasets' op locks at once. Concurrent operations on the +*same* dataset are therefore supported and serialize wholly +(`tests/threadsafe_same_dataset_append.rs`); the parallelism win remains +across distinct datasets, whose op locks never contend. ### 4.4 `SharedInner` — drop the outer `Mutex` diff --git a/src/dataset.rs b/src/dataset.rs index 276c6c7..c3af66f 100644 --- a/src/dataset.rs +++ b/src/dataset.rs @@ -969,6 +969,11 @@ impl H5Dataset { )) } }; + // Whole-operation guard: the flush, the grid snapshot and the chunk + // writes below must not interleave with a concurrent same-dataset + // operation. + let cell = writer.ds(index); + let _op = cell.op.lock(); // A buffered append tail would flush over the image at close; hand // it to the chunks first, the image below overwrites everything. writer.flush_append_buffer(index)?; @@ -1013,7 +1018,7 @@ impl H5Dataset { let coords = coords_of(linear); let chunk_buf = Self::gather_chunk(bytes, &dims, &chunk_dims, &coords, element_size); - writer.write_chunk_btree_v2(index, &coords, &chunk_buf)?; + writer.write_chunk_btree_v2_inner(index, &coords, &chunk_buf)?; } } else { // Extensible array and fixed array both compress each chunk through @@ -1041,13 +1046,13 @@ impl H5Dataset { .iter() .map(|(c, d)| (c.as_slice(), d.as_slice())) .collect(); - writer.write_chunks_fixed_array_batch(index, &pairs)?; + writer.write_chunks_fixed_array_batch_inner(index, &pairs)?; } else { let mut pairs: Vec<(u64, &[u8])> = Vec::with_capacity(items.len()); for (c, d) in &items { pairs.push((writer.chunk_slot(index, c)?, d.as_slice())); } - writer.write_chunks_batch(index, &pairs)?; + writer.write_chunks_batch_inner(index, &pairs)?; } start = end; } @@ -1163,13 +1168,17 @@ impl H5Dataset { let inner = borrow_inner(&self.file_inner); match &*inner { H5FileInner::Writer(writer) => { + // One op: the slot decode and the write see the same + // extents. + let cell = writer.ds(*index); + let _op = cell.op.lock(); if *fixed_array { // Fixed-array dataset: decode the index-grid slot // into row-major grid coordinates. let coords = writer.chunk_coords_from_slot(*index, chunk_idx as u64)?; - writer.write_chunk_fixed_array(*index, &coords, data)?; + writer.write_chunk_fixed_array_inner(*index, &coords, data)?; } else { - writer.write_chunk(*index, chunk_idx as u64, data)?; + writer.write_chunk_inner(*index, chunk_idx as u64, data)?; } Ok(()) } @@ -1235,18 +1244,22 @@ impl H5Dataset { let inner = borrow_inner(&self.file_inner); match &*inner { H5FileInner::Writer(writer) => { + // One op: the slot decode and the write see the same + // extents. + let cell = writer.ds(*index); + let _op = cell.op.lock(); if *fixed_array { // Fixed-array dataset: decode the index-grid slot // into row-major grid coordinates. let coords = writer.chunk_coords_from_slot(*index, chunk_idx as u64)?; - writer.write_compressed_chunk_fixed_array( + writer.write_compressed_chunk_fixed_array_inner( *index, &coords, data, filter_mask, )?; } else { - writer.write_compressed_chunk( + writer.write_compressed_chunk_inner( *index, chunk_idx as u64, data, @@ -1356,6 +1369,11 @@ impl H5Dataset { )) } }; + // Whole-operation guard: the dims snapshot, the chunk write + // and the extend below must not interleave with a concurrent + // same-dataset operation. + let cell = writer.ds(*index); + let _op = cell.op.lock(); let chunk_dims = writer .dataset_chunk_dims(*index) .ok_or_else(|| Hdf5Error::InvalidState("dataset has no chunk info".into()))? @@ -1399,10 +1417,10 @@ impl H5Dataset { // Fixed-array (fixed-shape) dataset: no dimension growth. match bytes { ChunkBytes::Unfiltered(data) => { - writer.write_chunk_fixed_array(*index, &coords, data)? + writer.write_chunk_fixed_array_inner(*index, &coords, data)? } ChunkBytes::Prefiltered { data, filter_mask } => writer - .write_compressed_chunk_fixed_array( + .write_compressed_chunk_fixed_array_inner( *index, &coords, data, @@ -1415,25 +1433,31 @@ impl H5Dataset { if btree2 { match bytes { ChunkBytes::Unfiltered(data) => { - writer.write_chunk_btree_v2(*index, &coords, data)? + writer.write_chunk_btree_v2_inner(*index, &coords, data)? } ChunkBytes::Prefiltered { data, filter_mask } => writer - .write_compressed_chunk_btree_v2(*index, &coords, data, filter_mask)?, + .write_compressed_chunk_btree_v2_inner( + *index, + &coords, + data, + filter_mask, + )?, } } else { // Extensible array: the chunk's index-grid slot (row-major // against the maximum extent). let linear = writer.chunk_slot(*index, &coords)?; match bytes { - ChunkBytes::Unfiltered(data) => writer.write_chunk(*index, linear, data)?, - ChunkBytes::Prefiltered { data, filter_mask } => { - writer.write_compressed_chunk(*index, linear, data, filter_mask)? + ChunkBytes::Unfiltered(data) => { + writer.write_chunk_inner(*index, linear, data)? } + ChunkBytes::Prefiltered { data, filter_mask } => writer + .write_compressed_chunk_inner(*index, linear, data, filter_mask)?, } } if new_dims != dims { - writer.extend_dataset(*index, &new_dims)?; + writer.extend_dataset_inner(*index, &new_dims)?; } Ok(()) } @@ -1533,6 +1557,13 @@ impl H5Dataset { } }; + // Whole-operation guard: the buffer take, the frame writes, + // the re-buffer and the extend below are separate slot + // acquisitions that a concurrent same-dataset append must not + // interleave with. + let cell = writer.ds(ds_index); + let _op = cell.op.lock(); + let chunk_dims = writer .dataset_chunk_dims(ds_index) .ok_or_else(|| Hdf5Error::InvalidState("dataset has no chunk info".into()))? @@ -1621,7 +1652,7 @@ impl H5Dataset { let logical_dim0 = base_dim0 + total_frames; let mut new_dims: Vec = dims; new_dims[0] = logical_dim0 as u64; - writer.extend_dataset(ds_index, &new_dims)?; + writer.extend_dataset_inner(ds_index, &new_dims)?; Ok(()) } diff --git a/src/io/writer.rs b/src/io/writer.rs index 832d605..1c84ff8 100644 --- a/src/io/writer.rs +++ b/src/io/writer.rs @@ -206,13 +206,63 @@ pub(crate) type Shared = std::rc::Rc; #[cfg(feature = "threadsafe")] pub(crate) type Shared = std::sync::Arc; -/// A single dataset's metadata behind its own [`Slot`], reference-counted so -/// a writer can clone it out of the registry (releasing the registry lock) -/// and then lock just this one dataset. Two threads writing different -/// datasets take different `DatasetRef` locks and never contend; the same -/// dataset's writes serialize, which is required because one chunk index is -/// not concurrently mutable. -pub(crate) type DatasetRef = Shared>; +/// One dataset's cell in the registry: its metadata slot plus the operation +/// lock that serializes whole logical operations on it. Both live in one +/// allocation so they cannot fall out of step — every dataset has its op +/// lock by construction. +pub(crate) struct DatasetCell { + /// Serializes one *whole* logical operation on this dataset. + /// + /// The metadata slot below serializes each individual acquisition, but a + /// multi-acquisition operation — take the append buffer → write chunks → + /// re-buffer the tail → extend, or flush-then-overwrite in a slice write + /// — would interleave with a concurrent same-dataset operation *between* + /// its acquisitions under `threadsafe`. Public write entries take this + /// lock and delegate to `_inner` variants; `_inner` variants and the + /// `pub(crate)` write helpers require the caller to hold it (or to hold + /// the writer exclusively via `&mut`, as close and the SWMR wrapper do). + /// + /// Not reentrant: the single-thread build's `RefCell` panics instantly + /// on a nested acquisition, so a missed entry/inner split fails loudly + /// in every test run rather than deadlocking only under `threadsafe`. + /// + /// Lock order: `create_lock → op → registry spine → metadata slot`. An + /// op lock is never held across another dataset's op lock, and no + /// op-lock holder takes `create_lock`, so the order is acyclic. + pub(crate) op: Slot<()>, + info: Slot, +} + +impl DatasetCell { + pub(crate) fn new(info: DatasetInfo) -> Self { + DatasetCell { + op: Slot::new(()), + info: Slot::new(info), + } + } + + /// Borrow the metadata slot (a single acquisition; see [`Self::op`] for + /// whole-operation serialization). + #[cfg(not(feature = "threadsafe"))] + pub(crate) fn lock(&self) -> std::cell::RefMut<'_, DatasetInfo> { + self.info.lock() + } + + /// Lock the metadata slot (a single acquisition; see [`Self::op`] for + /// whole-operation serialization). + #[cfg(feature = "threadsafe")] + pub(crate) fn lock(&self) -> std::sync::MutexGuard<'_, DatasetInfo> { + self.info.lock() + } +} + +/// A single dataset's [`DatasetCell`], reference-counted so a writer can +/// clone it out of the registry (releasing the registry lock) and then lock +/// just this one dataset. Two threads writing different datasets take +/// different `DatasetRef` locks and never contend; the same dataset's writes +/// serialize, which is required because one chunk index is not concurrently +/// mutable. +pub(crate) type DatasetRef = Shared; /// A single group's metadata behind its own [`Slot`], reference-counted like /// [`DatasetRef`]. @@ -683,7 +733,7 @@ impl Hdf5Writer { pub(crate) fn push_dataset(&self, _create: &CreateGuard<'_>, info: DatasetInfo) -> usize { let mut reg = self.datasets.lock(); let idx = reg.len(); - reg.push(Shared::new(Slot::new(info))); + reg.push(Shared::new(DatasetCell::new(info))); idx } @@ -1090,7 +1140,7 @@ impl Hdf5Writer { // only the final hand-off needs the `Shared>` shape. let datasets = existing_datasets .into_iter() - .map(|i| Shared::new(Slot::new(i))) + .map(|i| Shared::new(DatasetCell::new(i))) .collect(); let groups = groups .into_iter() @@ -1759,6 +1809,7 @@ impl Hdf5Writer { /// creation time. pub fn write_dataset_raw(&self, index: usize, data: &[u8]) -> IoResult<()> { let ds = self.ds(index); + let _op = ds.op.lock(); let data_addr = { let g = ds.lock(); if g.chunked.is_some() { @@ -1792,6 +1843,19 @@ impl Hdf5Writer { /// /// `data` must be exactly chunk_size bytes (product of chunk_dims * element_size). pub fn write_chunk(&self, index: usize, chunk_idx: u64, data: &[u8]) -> IoResult<()> { + let ds = self.ds(index); + let _op = ds.op.lock(); + self.write_chunk_inner(index, chunk_idx, data) + } + + /// [`Self::write_chunk`] body; the caller holds the dataset's op lock or + /// the writer exclusively. + pub(crate) fn write_chunk_inner( + &self, + index: usize, + chunk_idx: u64, + data: &[u8], + ) -> IoResult<()> { let ds = self.ds(index); // Read the chunk geometry and filter pipeline under one brief lock, // then drop it: compression runs *outside* the lock, and @@ -2162,6 +2226,20 @@ impl Hdf5Writer { starts: &[u64], counts: &[u64], data: &[u8], + ) -> IoResult<()> { + let ds = self.ds(index); + let _op = ds.op.lock(); + self.write_slice_inner(index, starts, counts, data) + } + + /// [`Self::write_slice`] body; the caller holds the dataset's op lock or + /// the writer exclusively. + pub(crate) fn write_slice_inner( + &self, + index: usize, + starts: &[u64], + counts: &[u64], + data: &[u8], ) -> IoResult<()> { let ds_ref = self.ds(index); let ds = ds_ref.lock(); @@ -2721,6 +2799,12 @@ impl Hdf5Writer { return Ok(()); } + // Whole-operation guard: buffer take, frame writes, re-buffer and + // extend below are separate slot acquisitions that a concurrent + // same-dataset append must not interleave with. + let cell = self.ds(ds_index); + let _op = cell.op.lock(); + // The elements about to be written are vlen references; any other // element type would be overwritten with them as raw bytes. let charset = { @@ -2821,7 +2905,7 @@ impl Hdf5Writer { let logical_dim0 = base_dim0 + total_frames; let mut new_dims = dims; new_dims[0] = logical_dim0 as u64; - self.extend_dataset(ds_index, &new_dims)?; + self.extend_dataset_inner(ds_index, &new_dims)?; Ok(()) } @@ -2858,6 +2942,12 @@ impl Hdf5Writer { return Ok(()); } + // Whole-operation guard: the flush, the old-reference reads and the + // slice write below must not interleave with a concurrent + // same-dataset operation. + let cell = self.ds(ds_index); + let _op = cell.op.lock(); + // Snapshot what the write needs, then drop the guard: `write_slice` // below re-locks the same slot. let (charset, dims) = { @@ -2924,7 +3014,7 @@ impl Hdf5Writer { )); } - self.write_slice(ds_index, &[start], &[strings.len() as u64], &refs)?; + self.write_slice_inner(ds_index, &[start], &[strings.len() as u64], &refs)?; // Only now that the new references are in place: a failure above must // not leave the file naming objects this already freed. @@ -3307,6 +3397,8 @@ impl Hdf5Writer { /// owner required the extensible-array index and packed rows at the /// frame stride, so appends to a fixed-array or v2 B-tree dataset /// failed at close and lost the buffered rows. + /// + /// The caller holds the dataset's op lock or the writer exclusively. pub(crate) fn write_append_frames( &self, ds_index: usize, @@ -3336,6 +3428,9 @@ impl Hdf5Writer { /// it. The single owner of the buffer-to-chunks transition: the flush at /// close, an append meeting a non-contiguous buffer, and any operation /// about to write rows the buffer holds all come through here. + /// + /// The caller holds the dataset's op lock or the writer exclusively — + /// the take and the frame writes are separate acquisitions. pub(crate) fn flush_append_buffer(&self, ds_index: usize) -> IoResult<()> { let taken = { self.ds(ds_index).lock().append.take() }; match taken { @@ -3348,6 +3443,8 @@ impl Hdf5Writer { /// the buffered range — those rows' current content is the buffer, and /// writing them on disk while the buffer still holds them would be /// undone by the flush at close. + /// + /// The caller holds the dataset's op lock or the writer exclusively. pub(crate) fn flush_append_buffer_if_intersecting( &self, ds_index: usize, @@ -3615,6 +3712,8 @@ impl Hdf5Writer { /// [`read_chunk_at_coords`](Self::read_chunk_at_coords). Unlike the /// dataset-level `write_chunk_at`, this never grows the dataspace — a /// hyperslab write is bounded by the current extent by definition. + /// + /// The caller holds the dataset's op lock or the writer exclusively. pub(crate) fn write_chunk_at_coords( &self, ds_index: usize, @@ -3625,12 +3724,14 @@ impl Hdf5Writer { match geo.kind { ChunkIndexKind::ExtensibleArray => { let linear = geo.linear_index(chunk_coords)?; - self.write_chunk(ds_index, linear, data) + self.write_chunk_inner(ds_index, linear, data) } ChunkIndexKind::FixedArray => { - self.write_chunk_fixed_array(ds_index, chunk_coords, data) + self.write_chunk_fixed_array_inner(ds_index, chunk_coords, data) + } + ChunkIndexKind::BtreeV2 => { + self.write_chunk_btree_v2_inner(ds_index, chunk_coords, data) } - ChunkIndexKind::BtreeV2 => self.write_chunk_btree_v2(ds_index, chunk_coords, data), } } @@ -4181,6 +4282,19 @@ impl Hdf5Writer { index: usize, chunk_coords: &[u64], data: &[u8], + ) -> IoResult<()> { + let ds = self.ds(index); + let _op = ds.op.lock(); + self.write_chunk_fixed_array_inner(index, chunk_coords, data) + } + + /// [`Self::write_chunk_fixed_array`] body; the caller holds the dataset's + /// op lock or the writer exclusively. + pub(crate) fn write_chunk_fixed_array_inner( + &self, + index: usize, + chunk_coords: &[u64], + data: &[u8], ) -> IoResult<()> { // Read what we need under one brief slot guard, then compress // OUTSIDE the lock: `record_fixed_array_chunk` re-locks the same slot, @@ -4235,6 +4349,20 @@ impl Hdf5Writer { chunk_coords: &[u64], data: &[u8], filter_mask: u32, + ) -> IoResult<()> { + let ds = self.ds(index); + let _op = ds.op.lock(); + self.write_compressed_chunk_fixed_array_inner(index, chunk_coords, data, filter_mask) + } + + /// [`Self::write_compressed_chunk_fixed_array`] body; the caller holds + /// the dataset's op lock or the writer exclusively. + pub(crate) fn write_compressed_chunk_fixed_array_inner( + &self, + index: usize, + chunk_coords: &[u64], + data: &[u8], + filter_mask: u32, ) -> IoResult<()> { if self.ds(index).lock().filter_pipeline.is_none() { return Err(crate::io::IoError::InvalidState( @@ -4367,6 +4495,19 @@ impl Hdf5Writer { index: usize, chunk_coords: &[u64], data: &[u8], + ) -> IoResult<()> { + let ds = self.ds(index); + let _op = ds.op.lock(); + self.write_chunk_btree_v2_inner(index, chunk_coords, data) + } + + /// [`Self::write_chunk_btree_v2`] body; the caller holds the dataset's op + /// lock or the writer exclusively. + pub(crate) fn write_chunk_btree_v2_inner( + &self, + index: usize, + chunk_coords: &[u64], + data: &[u8], ) -> IoResult<()> { // Read what the write needs under a brief guard, then compress OUTSIDE // the lock — filtering a chunk must not hold the dataset slot. @@ -4419,6 +4560,20 @@ impl Hdf5Writer { chunk_coords: &[u64], data: &[u8], filter_mask: u32, + ) -> IoResult<()> { + let ds = self.ds(index); + let _op = ds.op.lock(); + self.write_compressed_chunk_btree_v2_inner(index, chunk_coords, data, filter_mask) + } + + /// [`Self::write_compressed_chunk_btree_v2`] body; the caller holds the + /// dataset's op lock or the writer exclusively. + pub(crate) fn write_compressed_chunk_btree_v2_inner( + &self, + index: usize, + chunk_coords: &[u64], + data: &[u8], + filter_mask: u32, ) -> IoResult<()> { if self.ds(index).lock().filter_pipeline.is_none() { return Err(crate::io::IoError::InvalidState( @@ -4503,6 +4658,18 @@ impl Hdf5Writer { /// /// `chunks` is a list of (chunk_idx, data) pairs for an EA-indexed dataset. pub fn write_chunks_batch(&self, ds_index: usize, chunks: &[(u64, &[u8])]) -> IoResult<()> { + let ds = self.ds(ds_index); + let _op = ds.op.lock(); + self.write_chunks_batch_inner(ds_index, chunks) + } + + /// [`Self::write_chunks_batch`] body; the caller holds the dataset's op + /// lock or the writer exclusively. + pub(crate) fn write_chunks_batch_inner( + &self, + ds_index: usize, + chunks: &[(u64, &[u8])], + ) -> IoResult<()> { #[cfg(feature = "parallel")] { // If filter pipeline is set, compress all chunks in parallel. @@ -4517,14 +4684,14 @@ impl Hdf5Writer { // compressed fully, so filter_mask = 0 is truthful. let compressed = filter::apply_filters_parallel(pipeline, &chunk_data)?; for ((idx, _), compressed_data) in chunks.iter().zip(compressed.iter()) { - self.write_compressed_chunk(ds_index, *idx, compressed_data, 0)?; + self.write_compressed_chunk_inner(ds_index, *idx, compressed_data, 0)?; } return Ok(()); } } // Fallback: sequential for (idx, data) in chunks { - self.write_chunk(ds_index, *idx, data)?; + self.write_chunk_inner(ds_index, *idx, data)?; } Ok(()) } @@ -4542,6 +4709,18 @@ impl Hdf5Writer { &self, ds_index: usize, chunks: &[(&[u64], &[u8])], + ) -> IoResult<()> { + let ds = self.ds(ds_index); + let _op = ds.op.lock(); + self.write_chunks_fixed_array_batch_inner(ds_index, chunks) + } + + /// [`Self::write_chunks_fixed_array_batch`] body; the caller holds the + /// dataset's op lock or the writer exclusively. + pub(crate) fn write_chunks_fixed_array_batch_inner( + &self, + ds_index: usize, + chunks: &[(&[u64], &[u8])], ) -> IoResult<()> { #[cfg(feature = "parallel")] { @@ -4561,9 +4740,10 @@ impl Hdf5Writer { return Ok(()); } } - // Fallback: sequential (write_chunk_fixed_array compresses per chunk). + // Fallback: sequential (write_chunk_fixed_array_inner compresses per + // chunk). for (coords, data) in chunks { - self.write_chunk_fixed_array(ds_index, coords, data)?; + self.write_chunk_fixed_array_inner(ds_index, coords, data)?; } Ok(()) } @@ -4586,6 +4766,20 @@ impl Hdf5Writer { chunk_idx: u64, compressed_data: &[u8], filter_mask: u32, + ) -> IoResult<()> { + let ds = self.ds(index); + let _op = ds.op.lock(); + self.write_compressed_chunk_inner(index, chunk_idx, compressed_data, filter_mask) + } + + /// [`Self::write_compressed_chunk`] body; the caller holds the dataset's + /// op lock or the writer exclusively. + pub(crate) fn write_compressed_chunk_inner( + &self, + index: usize, + chunk_idx: u64, + compressed_data: &[u8], + filter_mask: u32, ) -> IoResult<()> { if self.ds(index).lock().filter_pipeline.is_none() { return Err(crate::io::IoError::InvalidState( @@ -4599,6 +4793,14 @@ impl Hdf5Writer { /// Extend the dimensions of a chunked dataset. pub fn extend_dataset(&self, index: usize, new_dims: &[u64]) -> IoResult<()> { + let ds = self.ds(index); + let _op = ds.op.lock(); + self.extend_dataset_inner(index, new_dims) + } + + /// [`Self::extend_dataset`] body; the caller holds the dataset's op lock + /// or the writer exclusively. + pub(crate) fn extend_dataset_inner(&self, index: usize, new_dims: &[u64]) -> IoResult<()> { let ds = self.ds(index); let mut m = ds.lock(); let is_unindexed = m.chunked.is_none() && m.fixed_array.is_none() && m.btree_v2.is_none(); @@ -4655,6 +4857,7 @@ impl Hdf5Writer { /// corrected back to the true number of frames written. pub fn set_dataset_extent(&self, index: usize, new_dims: &[u64]) -> IoResult<()> { let ds = self.ds(index); + let _op = ds.op.lock(); let mut m = ds.lock(); let is_unindexed = m.chunked.is_none() && m.fixed_array.is_none() && m.btree_v2.is_none(); if is_unindexed { @@ -4710,6 +4913,8 @@ impl Hdf5Writer { /// Writes the index blocks and issues an `fdatasync` so the data is /// durable — the guarantee SWMR readers and standalone callers rely on. pub fn flush_dataset(&self, index: usize) -> IoResult<()> { + let ds = self.ds(index); + let _op = ds.op.lock(); self.flush_dataset_synced(index, true) } diff --git a/tests/threadsafe_same_dataset_append.rs b/tests/threadsafe_same_dataset_append.rs new file mode 100644 index 0000000..2b71e72 --- /dev/null +++ b/tests/threadsafe_same_dataset_append.rs @@ -0,0 +1,194 @@ +//! Concurrent same-dataset appends under the `threadsafe` writer. +//! +//! An append is several dataset-slot acquisitions: take the buffered tail, +//! write the chunk-aligned rows, re-buffer the new tail, extend the extent. +//! The per-slot mutex serializes each acquisition but not the operation, so +//! without the per-dataset op lock two appends interleave between +//! acquisitions — two calls take the same base row, rows are lost or +//! overwritten. These tests drive that exact schedule: N threads append to +//! *one* dataset, with a frame count that never divides the chunk so the +//! buffered tail is always in play. +//! +//! Every append call must land as a contiguous, in-order block of rows +//! (atomicity), every appended frame must appear exactly once (no lost or +//! doubled rows), and no frame may be torn. Order *between* calls is the +//! scheduler's. + +#![cfg(feature = "threadsafe")] + +use rust_hdf5::H5File; + +fn tmp(name: &str) -> std::path::PathBuf { + use std::sync::atomic::{AtomicU64, Ordering}; + static COUNTER: AtomicU64 = AtomicU64::new(0); + let n = COUNTER.fetch_add(1, Ordering::Relaxed); + std::env::temp_dir().join(format!( + "rust_hdf5_same_ds_{}_{}_{}.h5", + name, + std::process::id(), + n + )) +} + +/// Tag of frame `f` of append call `j` by thread `k`. Distinct per +/// (k, j, f) so a lost, doubled, or split append shows up as a wrong +/// multiset or a broken run on read-back. +fn tag(k: usize, j: usize, f: usize) -> i32 { + (k * 1_000_000 + j * 100 + f) as i32 +} + +#[test] +fn concurrent_same_dataset_appends_serialize_wholly() { + const N: usize = 4; // threads + const M: usize = 40; // append calls per thread + const FRAMES: usize = 3; // frames per call — never a multiple of CHUNK0 + const W: usize = 8; // row width + const CHUNK0: usize = 4; // chunk rows: every call leaves a buffered tail + const ITERS: usize = 10; // repeat to shake out schedules + + for iter in 0..ITERS { + let path = tmp(&format!("iter{iter}")); + { + let file = H5File::create(&path).unwrap(); + let ds = file + .new_dataset::() + .shape([0, W]) + .chunk(&[CHUNK0, W]) + .max_shape(&[None, Some(W)]) + .create("d") + .unwrap(); + + std::thread::scope(|s| { + for k in 0..N { + let ds = &ds; + s.spawn(move || { + for j in 0..M { + let data: Vec = (0..FRAMES) + .flat_map(|f| std::iter::repeat_n(tag(k, j, f), W)) + .collect(); + ds.append(&data) + .unwrap_or_else(|e| panic!("append k={k} j={j}: {e}")); + } + }); + } + }); + file.close().unwrap(); + } + + let file = H5File::open(&path).unwrap(); + let ds = file.dataset("d").unwrap(); + assert_eq!(ds.shape(), vec![N * M * FRAMES, W], "iter {iter}"); + let got = ds.read_raw::().unwrap(); + + // No torn frames: each row is uniform. + let rows: Vec = got + .chunks_exact(W) + .enumerate() + .map(|(r, row)| { + assert!( + row.iter().all(|&v| v == row[0]), + "iter {iter}: torn frame at row {r}: {row:?}" + ); + row[0] + }) + .collect(); + + // Atomicity: each call's frames form one contiguous in-order run. + // Walking runs also proves every frame appears exactly once — the + // walk consumes exactly FRAMES rows per (k, j) and the total length + // already matched. + let mut seen = std::collections::HashSet::new(); + let mut r = 0; + while r < rows.len() { + let first = rows[r]; + let (k, rem) = ((first / 1_000_000) as usize, first % 1_000_000); + let (j, f) = ((rem / 100) as usize, (rem % 100) as usize); + assert_eq!(f, 0, "iter {iter}: run at row {r} starts mid-call: {first}"); + for f in 0..FRAMES { + assert_eq!( + rows[r + f], + tag(k, j, f), + "iter {iter}: call (k={k}, j={j}) split at row {}", + r + f + ); + } + assert!( + seen.insert((k, j)), + "iter {iter}: call (k={k}, j={j}) appended twice" + ); + r += FRAMES; + } + assert_eq!(seen.len(), N * M, "iter {iter}"); + + std::fs::remove_file(&path).ok(); + } +} + +#[test] +fn concurrent_same_dataset_vlen_appends_serialize_wholly() { + const N: usize = 4; // threads + const M: usize = 30; // append calls per thread + const STRINGS: usize = 3; // strings per call + const CHUNK: usize = 4; // never a multiple of STRINGS: tail always buffered + const ITERS: usize = 10; + + for iter in 0..ITERS { + let path = tmp(&format!("vlen{iter}")); + { + let file = H5File::create(&path).unwrap(); + file.create_appendable_vlen_dataset("strs", CHUNK, None) + .unwrap(); + + std::thread::scope(|s| { + for k in 0..N { + let file = &file; + s.spawn(move || { + for j in 0..M { + let batch: Vec = + (0..STRINGS).map(|f| format!("{k}:{j}:{f}")).collect(); + let refs: Vec<&str> = batch.iter().map(String::as_str).collect(); + file.append_vlen_strings("strs", &refs) + .unwrap_or_else(|e| panic!("append k={k} j={j}: {e}")); + } + }); + } + }); + file.close().unwrap(); + } + + let file = H5File::open(&path).unwrap(); + let ds = file.dataset("strs").unwrap(); + let got = ds.read_vlen_strings().unwrap(); + assert_eq!(got.len(), N * M * STRINGS, "iter {iter}"); + + // Each call's strings must form one contiguous in-order run, and the + // runs must cover every (k, j) exactly once. + let mut seen = std::collections::HashSet::new(); + let mut r = 0; + while r < got.len() { + let parts: Vec = got[r].split(':').map(|p| p.parse().unwrap()).collect(); + let (k, j, f) = (parts[0], parts[1], parts[2]); + assert_eq!( + f, 0, + "iter {iter}: run at element {r} starts mid-call: {}", + got[r] + ); + for f in 0..STRINGS { + assert_eq!( + got[r + f], + format!("{k}:{j}:{f}"), + "iter {iter}: call (k={k}, j={j}) split at element {}", + r + f + ); + } + assert!( + seen.insert((k, j)), + "iter {iter}: call (k={k}, j={j}) appended twice" + ); + r += STRINGS; + } + assert_eq!(seen.len(), N * M, "iter {iter}"); + + std::fs::remove_file(&path).ok(); + } +} From 43185cc7e730728f3c2293a2ebab27e4ebe67145 Mon Sep 17 00:00:00 2001 From: Sang Woo Kim Date: Thu, 13 Aug 2026 13:18:09 +0900 Subject: [PATCH 10/11] tests: let RUST_HDF5_TEST_PYTHON override the pinned h5py interpreter The cross-check interpreter was pinned to one machine's mamba env, so the h5py suites silently skipped everywhere else. The env var selects a local interpreter (falling back to the pinned path), verified once through a OnceLock so the skip notice prints once per process. --- tests/h5py_cross_validation.rs | 23 +++++++++++++++-------- tests/threadsafe_concurrent_create.rs | 18 ++++++++++++------ tests/threadsafe_parallel_write.rs | 18 ++++++++++++------ 3 files changed, 39 insertions(+), 20 deletions(-) diff --git a/tests/h5py_cross_validation.rs b/tests/h5py_cross_validation.rs index fd9943f..7f0770c 100644 --- a/tests/h5py_cross_validation.rs +++ b/tests/h5py_cross_validation.rs @@ -1,8 +1,9 @@ //! Cross-validation against h5py / libhdf5. //! //! Each test writes a file with rust-hdf5's public API and reads it back with -//! h5py to confirm the bytes are standard-tool readable. The tests skip (pass) -//! when the pinned h5py interpreter is not present, so CI without h5py is green. +//! h5py to confirm the bytes are standard-tool readable. The interpreter comes +//! from `RUST_HDF5_TEST_PYTHON`, falling back to the pinned path; the tests +//! skip (pass) when neither is present, so CI without h5py is green. use rust_hdf5::types::VarLenUnicode; use rust_hdf5::H5File; @@ -10,12 +11,18 @@ use rust_hdf5::H5File; const TEST_PYTHON: &str = "/Users/stevek/mamba/envs/bs2026.1/bin/python"; fn python() -> Option<&'static str> { - if std::path::Path::new(TEST_PYTHON).exists() { - Some(TEST_PYTHON) - } else { - eprintln!("skipping h5py cross-check: {TEST_PYTHON} not present"); - None - } + static PY: std::sync::OnceLock> = std::sync::OnceLock::new(); + PY.get_or_init(|| { + let candidate = + std::env::var("RUST_HDF5_TEST_PYTHON").unwrap_or_else(|_| TEST_PYTHON.to_string()); + if std::path::Path::new(&candidate).exists() { + Some(candidate) + } else { + eprintln!("skipping h5py cross-check: {candidate} not present"); + None + } + }) + .as_deref() } fn tmp(name: &str) -> std::path::PathBuf { diff --git a/tests/threadsafe_concurrent_create.rs b/tests/threadsafe_concurrent_create.rs index d3b51a9..28946f6 100644 --- a/tests/threadsafe_concurrent_create.rs +++ b/tests/threadsafe_concurrent_create.rs @@ -28,12 +28,18 @@ use rust_hdf5::H5File; const TEST_PYTHON: &str = "/Users/stevek/mamba/envs/bs2026.1/bin/python"; fn python() -> Option<&'static str> { - if std::path::Path::new(TEST_PYTHON).exists() { - Some(TEST_PYTHON) - } else { - eprintln!("skipping h5py cross-check: {TEST_PYTHON} not present"); - None - } + static PY: std::sync::OnceLock> = std::sync::OnceLock::new(); + PY.get_or_init(|| { + let candidate = + std::env::var("RUST_HDF5_TEST_PYTHON").unwrap_or_else(|_| TEST_PYTHON.to_string()); + if std::path::Path::new(&candidate).exists() { + Some(candidate) + } else { + eprintln!("skipping h5py cross-check: {candidate} not present"); + None + } + }) + .as_deref() } fn tmp(name: &str) -> std::path::PathBuf { diff --git a/tests/threadsafe_parallel_write.rs b/tests/threadsafe_parallel_write.rs index 805de79..f27c214 100644 --- a/tests/threadsafe_parallel_write.rs +++ b/tests/threadsafe_parallel_write.rs @@ -29,12 +29,18 @@ use rust_hdf5::H5File; const TEST_PYTHON: &str = "/Users/stevek/mamba/envs/bs2026.1/bin/python"; fn python() -> Option<&'static str> { - if std::path::Path::new(TEST_PYTHON).exists() { - Some(TEST_PYTHON) - } else { - eprintln!("skipping h5py cross-check: {TEST_PYTHON} not present"); - None - } + static PY: std::sync::OnceLock> = std::sync::OnceLock::new(); + PY.get_or_init(|| { + let candidate = + std::env::var("RUST_HDF5_TEST_PYTHON").unwrap_or_else(|_| TEST_PYTHON.to_string()); + if std::path::Path::new(&candidate).exists() { + Some(candidate) + } else { + eprintln!("skipping h5py cross-check: {candidate} not present"); + None + } + }) + .as_deref() } fn tmp(name: &str) -> std::path::PathBuf { From 4e5f4fe6b3ceef6c40c212e15941aaad3f6e8c8c Mon Sep 17 00:00:00 2001 From: Sang Woo Kim Date: Thu, 13 Aug 2026 13:50:36 +0900 Subject: [PATCH 11/11] Write layout message v5 for filtered chunked datasets on opt-in (issue #8) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit H5File::set_libver_latest is H5Pset_libver_bounds(low=V200): filtered chunked datasets get a v5 layout whose indexes store chunk sizes in fixed 8-byte fields; chunks over 4 GiB force v5 unconditionally, and a reopened file keeps its on-disk version so append never downgrades it. The FA/BT2 in-memory chunk_size fields widen u32 -> u64 — encoding an 8-byte field from a u32 paniced, and v4 with a derived 8-byte width could already truncate. --- CHANGELOG.md | 16 ++ src/file.rs | 26 ++ src/format/chunk_index/btree_v2.rs | 17 +- src/format/chunk_index/fixed_array.rs | 13 +- src/format/messages/data_layout.rs | 60 ++++- src/io/reader.rs | 4 +- src/io/writer.rs | 364 +++++++++++++++++++++++--- tests/h5py_cross_validation.rs | 69 +++++ 8 files changed, 508 insertions(+), 61 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3c1ea3b..0b6b3b9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,22 @@ ## Unreleased +### Added + +- `H5File::set_libver_latest` opts datasets created after the call into the + latest file format, the equivalent of libhdf5's + `H5Pset_libver_bounds(low = H5F_LIBVER_V200)`: filtered chunked datasets + get a version-5 data layout message whose chunk indexes store on-disk + chunk sizes in fixed 8-byte fields, removing the overflow risk when a + filter expands a chunk. Off by default — a v5 file needs libhdf5 ≥ 2.0 + (h5py bundling hdf5 1.14 rejects it), while the default v4 stays readable + everywhere. Independent of the knob, a chunk larger than 4 GiB forces + version 5, matching libhdf5, because v4 cannot represent its size field. + The version read from an existing file is preserved on reopen, so + appending to a v5 file never silently downgrades it. In-memory chunk-size + fields in the fixed-array and v2-B-tree indexes widened from `u32` to + `u64` to carry the 8-byte field. (issue #8) + ### Fixed - Concurrent operations on the *same* dataset serialize wholly under the diff --git a/src/file.rs b/src/file.rs index cc980bf..cf175df 100644 --- a/src/file.rs +++ b/src/file.rs @@ -166,6 +166,32 @@ impl H5File { H5FileOptions::default() } + /// Opt in to the latest file format for datasets created after this call — + /// the equivalent of libhdf5's `H5Pset_libver_bounds(low = H5F_LIBVER_V200)`. + /// + /// With `latest` set, filtered chunked datasets get a version-5 data layout + /// message, whose chunk indexes store on-disk chunk sizes in fixed-width + /// (`sizeof_size`, i.e. 8-byte) fields instead of fields sized from the + /// uncompressed chunk size. That removes the overflow risk when a filter + /// *expands* a chunk, but the file is only readable by libhdf5 ≥ 2.0 + /// (h5py bundling hdf5 1.14 rejects it with "bad version number"). + /// + /// Off by default; unfiltered and contiguous datasets are unaffected. + /// Independent of this setting, a chunk larger than 4 GiB forces version 5 + /// because version 4 cannot represent its size field, matching libhdf5. + /// + /// Errors in read mode. + pub fn set_libver_latest(&self, latest: bool) -> Result<()> { + let mut inner = borrow_inner_mut(&self.inner); + match &mut *inner { + H5FileInner::Writer(writer) => { + writer.set_libver_latest(latest); + Ok(()) + } + _ => Err(Hdf5Error::InvalidState("cannot write in read mode".into())), + } + } + /// Return a handle to the root group. /// /// The root group can be used to create datasets and sub-groups. diff --git a/src/format/chunk_index/btree_v2.rs b/src/format/chunk_index/btree_v2.rs index 3eb46bb..1134c97 100644 --- a/src/format/chunk_index/btree_v2.rs +++ b/src/format/chunk_index/btree_v2.rs @@ -83,8 +83,11 @@ pub struct Bt2FilteredChunkRecord { pub scaled_offsets: Vec, /// File address of the chunk data. pub chunk_address: u64, - /// Size of the chunk after filtering (compressed size). - pub chunk_size: u32, + /// Size of the chunk after filtering (compressed size). `u64` because the + /// encoded field is `chunk_size_len` bytes wide — up to 8 under a + /// version-5 layout (or a v4 layout whose uncompressed chunk derives an + /// 8-byte width). + pub chunk_size: u64, /// Filter mask (bit i set = skip filter i). pub filter_mask: u32, } @@ -981,7 +984,7 @@ impl Bt2ChunkIndex { &mut self, scaled_offsets: Vec, chunk_address: u64, - chunk_size: u32, + chunk_size: u64, filter_mask: u32, ) { match self @@ -1068,7 +1071,7 @@ impl Bt2ChunkIndex { // filter mask, then the scaled offsets. buf.extend_from_slice(&rec.chunk_address.to_le_bytes()[..sa]); buf.extend_from_slice( - &(rec.chunk_size as u64).to_le_bytes()[..self.chunk_size_len as usize], + &rec.chunk_size.to_le_bytes()[..self.chunk_size_len as usize], ); buf.extend_from_slice(&rec.filter_mask.to_le_bytes()); for &offset in &rec.scaled_offsets { @@ -1193,7 +1196,7 @@ impl Bt2ChunkIndex { // mask, then the scaled offsets. let chunk_address = read_addr(&record_data[pos..], sa); pos += sa; - let chunk_size = read_size(&record_data[pos..], chunk_size_len) as u32; + let chunk_size = read_size(&record_data[pos..], chunk_size_len); pos += chunk_size_len; let filter_mask = u32::from_le_bytes([ record_data[pos], @@ -1815,7 +1818,7 @@ mod tests { // record_size = 8 + 2 + 4 + 16 = 30, so a leaf holds (2048-10)/30 = 67. let mut idx = Bt2ChunkIndex::new_filtered(2, 2); for i in 0..500u64 { - idx.insert_filtered(vec![i, 0], 0x10_000 + i * 0x100, (i % 400) as u32 + 1, 0); + idx.insert_filtered(vec![i, 0], 0x10_000 + i * 0x100, (i % 400) + 1, 0); } let tree = idx.build_tree(&ctx); assert!(tree.depth() >= 1); @@ -1828,7 +1831,7 @@ mod tests { for (i, r) in records.iter().enumerate() { assert_eq!(r.scaled_offsets, vec![i as u64, 0]); assert_eq!(r.chunk_address, 0x10_000 + i as u64 * 0x100); - assert_eq!(r.chunk_size, (i as u32 % 400) + 1); + assert_eq!(r.chunk_size, (i as u64 % 400) + 1); } } diff --git a/src/format/chunk_index/fixed_array.rs b/src/format/chunk_index/fixed_array.rs index 4eab0f5..fbbc28d 100644 --- a/src/format/chunk_index/fixed_array.rs +++ b/src/format/chunk_index/fixed_array.rs @@ -217,7 +217,10 @@ pub struct FixedArrayChunkElement { #[derive(Debug, Clone, PartialEq)] pub struct FixedArrayFilteredChunkElement { pub address: u64, - pub chunk_size: u32, + /// Stored (on-disk) chunk size. `u64` because the encoded field is + /// `chunk_size_len` bytes wide — up to 8 under a version-5 layout (or a + /// v4 layout whose uncompressed chunk derives an 8-byte width). + pub chunk_size: u64, pub filter_mask: u32, } @@ -448,7 +451,7 @@ impl FixedArrayDataBlock { for _ in 0..num_elmts { let address = read_addr(&buf[pos..], sa); pos += sa; - let chunk_size = read_size(&buf[pos..], chunk_size_len) as u32; + let chunk_size = read_size(&buf[pos..], chunk_size_len); pos += chunk_size_len; let filter_mask = u32::from_le_bytes([buf[pos], buf[pos + 1], buf[pos + 2], buf[pos + 3]]); @@ -609,7 +612,7 @@ pub fn encode_filtered_page( let mut buf = Vec::with_capacity(elems.len() * elem_size + 4); for e in elems { buf.extend_from_slice(&e.address.to_le_bytes()[..sa]); - buf.extend_from_slice(&(e.chunk_size as u64).to_le_bytes()[..chunk_size_len]); + buf.extend_from_slice(&e.chunk_size.to_le_bytes()[..chunk_size_len]); buf.extend_from_slice(&e.filter_mask.to_le_bytes()); } let cksum = checksum_metadata(&buf); @@ -705,7 +708,7 @@ pub fn decode_filtered_page( for _ in 0..nelmts { let address = read_addr(&page_buf[pos..], sa); pos += sa; - let chunk_size = read_size(&page_buf[pos..], chunk_size_len) as u32; + let chunk_size = read_size(&page_buf[pos..], chunk_size_len); pos += chunk_size_len; let filter_mask = u32::from_le_bytes([ page_buf[pos], @@ -940,7 +943,7 @@ mod tests { let mut buf = Vec::new(); for e in elems { buf.extend_from_slice(&e.address.to_le_bytes()[..sa]); - buf.extend_from_slice(&(e.chunk_size as u64).to_le_bytes()[..chunk_size_len]); + buf.extend_from_slice(&e.chunk_size.to_le_bytes()[..chunk_size_len]); buf.extend_from_slice(&e.filter_mask.to_le_bytes()); } let cksum = checksum_metadata(&buf); diff --git a/src/format/messages/data_layout.rs b/src/format/messages/data_layout.rs index 643fb97..144d82a 100644 --- a/src/format/messages/data_layout.rs +++ b/src/format/messages/data_layout.rs @@ -19,14 +19,17 @@ //! D 4-byte LE dimension sizes (chunk dims; last is the element size). //! The chunk index is always a version-1 B-tree. //! -//! Binary layout (version 4, chunked only): -//! Byte 0: version = 4 +//! Binary layout (versions 4 and 5, chunked only): +//! Byte 0: version = 4 or 5 //! Byte 1: layout class = 2 (chunked) //! flags(1) + ndims(1) + enc_bytes_per_dim(1) //! + dim_sizes(ndims * enc_bytes_per_dim, each LE) //! + index_type(1) //! + [for earray: 5 param bytes] //! + index_address(sizeof_addr) +//! +//! Version 5 (libhdf5 2.0) differs from version 4 only in the version byte; +//! see [`VERSION_5`] for its effect on filtered chunk indexes. use crate::format::bytes::{read_le_addr as read_addr, read_le_uint as read_size}; use crate::format::{FormatContext, FormatError, FormatResult, UNDEF_ADDR}; @@ -149,8 +152,16 @@ pub enum DataLayoutMessage { /// Address of the version-1 B-tree that indexes the chunks. b_tree_address: u64, }, - /// Version 4 chunked storage. + /// Version 4 chunked storage. Version 5 shares this exact wire format — + /// only the version byte differs — so both decode into this variant. ChunkedV4 { + /// Message version byte: 4 or 5. Version 5 (libhdf5 2.0, + /// `H5O_LAYOUT_VERSION_5`) declares that the chunk index encodes + /// filtered-chunk sizes in a fixed `sizeof_size`-byte field instead + /// of the width derived from the chunk byte count, so a filter may + /// expand a chunk without overflowing the field. Readers older than + /// libhdf5 2.0 reject version 5. + version: u8, flags: u8, /// Chunk dimension sizes. chunk_dims: Vec, @@ -203,11 +214,13 @@ impl DataLayoutMessage { /// For example, for a 2D dataset with chunk=(1,4) and element_size=8, /// pass chunk_dims = [1, 4, 8]. pub fn chunked_v4_earray( + version: u8, chunk_dims: Vec, earray_params: EarrayParams, index_address: u64, ) -> Self { Self::ChunkedV4 { + version, flags: 0, chunk_dims, index_type: ChunkIndexType::ExtensibleArray, @@ -222,11 +235,13 @@ impl DataLayoutMessage { /// /// `chunk_dims` should include the trailing element-size dimension. pub fn chunked_v4_farray( + version: u8, chunk_dims: Vec, farray_params: FixedArrayParams, index_address: u64, ) -> Self { Self::ChunkedV4 { + version, flags: 0, chunk_dims, index_type: ChunkIndexType::FixedArray, @@ -240,8 +255,9 @@ impl DataLayoutMessage { /// Version 4 chunked layout with B-tree v2 index. /// /// `chunk_dims` should include the trailing element-size dimension. - pub fn chunked_v4_btree_v2(chunk_dims: Vec, index_address: u64) -> Self { + pub fn chunked_v4_btree_v2(version: u8, chunk_dims: Vec, index_address: u64) -> Self { Self::ChunkedV4 { + version, flags: 0, chunk_dims, index_type: ChunkIndexType::BTreeV2, @@ -257,6 +273,7 @@ impl DataLayoutMessage { /// `chunk_dims` should include the trailing element-size dimension. pub fn chunked_v4_single(chunk_dims: Vec, index_address: u64) -> Self { Self::ChunkedV4 { + version: VERSION_4, flags: 0, chunk_dims, index_type: ChunkIndexType::SingleChunk, @@ -307,6 +324,7 @@ impl DataLayoutMessage { buf } Self::ChunkedV4 { + version, flags, chunk_dims, index_type, @@ -323,8 +341,9 @@ impl DataLayoutMessage { let max_dim = chunk_dims.iter().copied().max().unwrap_or(1); let enc_bytes = enc_bytes_for_value(max_dim); + debug_assert!(matches!(*version, VERSION_4 | VERSION_5)); let mut buf = Vec::with_capacity(64); - buf.push(VERSION_4); + buf.push(*version); buf.push(CLASS_CHUNKED); buf.push(*flags); buf.push(ndims); @@ -674,6 +693,7 @@ impl DataLayoutMessage { Ok(( Self::ChunkedV4 { + version: buf[0], flags, chunk_dims, index_type, @@ -852,7 +872,7 @@ mod tests { #[test] fn roundtrip_chunked_v4_earray() { let params = EarrayParams::default_params(); - let msg = DataLayoutMessage::chunked_v4_earray(vec![1, 256, 256], params, 0x2000); + let msg = DataLayoutMessage::chunked_v4_earray(4, vec![1, 256, 256], params, 0x2000); let encoded = msg.encode(&ctx8()); assert_eq!(encoded[0], 4); // version 4 assert_eq!(encoded[1], 2); // class chunked @@ -864,13 +884,34 @@ mod tests { #[test] fn roundtrip_chunked_v4_earray_ctx4() { let params = EarrayParams::default_params(); - let msg = DataLayoutMessage::chunked_v4_earray(vec![1, 128], params, 0x1000); + let msg = DataLayoutMessage::chunked_v4_earray(4, vec![1, 128], params, 0x1000); let encoded = msg.encode(&ctx4()); let (decoded, consumed) = DataLayoutMessage::decode(&encoded, &ctx4()).unwrap(); assert_eq!(consumed, encoded.len()); assert_eq!(decoded, msg); } + /// A version-5 layout differs from v4 only in the version byte; the body + /// encodes identically and the version must survive the round trip (a + /// reopen that dropped it would silently downgrade the file to v4 while + /// its filtered index keeps 8-byte size fields). + #[test] + fn roundtrip_chunked_v5_earray() { + let params = EarrayParams::default_params(); + let v5 = DataLayoutMessage::chunked_v4_earray(5, vec![1, 256, 256], params.clone(), 0x2000); + let encoded = v5.encode(&ctx8()); + assert_eq!(encoded[0], 5); // version 5 + assert_eq!(encoded[1], 2); // class chunked + let (decoded, consumed) = DataLayoutMessage::decode(&encoded, &ctx8()).unwrap(); + assert_eq!(consumed, encoded.len()); + assert_eq!(decoded, v5); + + // Same message at v4: only byte 0 differs. + let v4 = DataLayoutMessage::chunked_v4_earray(4, vec![1, 256, 256], params, 0x2000); + let encoded_v4 = v4.encode(&ctx8()); + assert_eq!(encoded[1..], encoded_v4[1..]); + } + #[test] fn roundtrip_chunked_v4_single() { let msg = DataLayoutMessage::chunked_v4_single(vec![100, 200], 0x3000); @@ -888,6 +929,7 @@ mod tests { fn roundtrip_chunked_v4_single_filtered() { for ctx in [ctx8(), ctx4()] { let msg = DataLayoutMessage::ChunkedV4 { + version: 4, flags: 0x02, chunk_dims: vec![100, 200, 4], index_type: ChunkIndexType::SingleChunk, @@ -921,7 +963,7 @@ mod tests { fn chunked_v4_enc_bytes() { // chunk dims [1, 256, 256]: max=256, needs 2 bytes let params = EarrayParams::default_params(); - let msg = DataLayoutMessage::chunked_v4_earray(vec![1, 256, 256], params, 0x2000); + let msg = DataLayoutMessage::chunked_v4_earray(4, vec![1, 256, 256], params, 0x2000); let encoded = msg.encode(&ctx8()); // version(1) + class(1) + flags(1) + ndims(1) + enc_bytes(1) // + 3*2 dim bytes + index_type(1) + 5 earray params + 8 addr = 25 @@ -1000,7 +1042,7 @@ mod tests { fn chunked_v4_large_dims() { // Large dims requiring 4 bytes each let params = EarrayParams::default_params(); - let msg = DataLayoutMessage::chunked_v4_earray(vec![1, 65536], params, 0x4000); + let msg = DataLayoutMessage::chunked_v4_earray(4, vec![1, 65536], params, 0x4000); let encoded = msg.encode(&ctx8()); assert_eq!(encoded[4], 3); // enc_bytes_per_dim = 3 (65536 = 0x10000, needs 3 bytes) } diff --git a/src/io/reader.rs b/src/io/reader.rs index 740d970..2955375 100644 --- a/src/io/reader.rs +++ b/src/io/reader.rs @@ -1845,7 +1845,7 @@ impl Hdf5Reader { let elems = decode_filtered_page(&page_buf, &self.ctx, page_nelmts, chunk_size_len)?; for e in elems { - chunk_entries.push((e.address, e.chunk_size as u64, e.filter_mask)); + chunk_entries.push((e.address, e.chunk_size, e.filter_mask)); } } else { let addrs = decode_unfiltered_page(&page_buf, &self.ctx, page_nelmts)?; @@ -1872,7 +1872,7 @@ impl Hdf5Reader { chunk_size_len, )?; for e in &fa_dblk.filtered_elements { - chunk_entries.push((e.address, e.chunk_size as u64, e.filter_mask)); + chunk_entries.push((e.address, e.chunk_size, e.filter_mask)); } } else { let fa_dblk = diff --git a/src/io/writer.rs b/src/io/writer.rs index 1c84ff8..3d9a2e9 100644 --- a/src/io/writer.rs +++ b/src/io/writer.rs @@ -326,6 +326,12 @@ pub struct DatasetInfo { /// means default zero-fill; `Some` is emitted as a `fill_defined = 2` /// fill-value message in the dataset object header. pub fill_value: Option>, + /// Layout message version for chunked storage: 4, or 5 when the chunk + /// index encodes stored chunk sizes in a fixed `sizeof_size` field + /// (libhdf5 2.0). Chosen at create by `Hdf5Writer::chunk_layout_version`, + /// preserved from the file on reopen, and emitted verbatim at finalize. + /// Contiguous datasets ignore it. + pub layout_version: u8, } /// Runtime metadata for a chunked dataset. @@ -614,6 +620,14 @@ pub struct Hdf5Writer { /// outermost lock a create acquires (create_lock → spine → slot), and no /// write path takes it, so it cannot deadlock with the registry locks. pub(crate) create_lock: Slot<()>, + /// Target the libhdf5 2.0 file format (`H5Pset_libver_bounds` with low + /// bound `H5F_LIBVER_V200`): filtered chunked datasets created while + /// this is set get layout message version 5, whose chunk indexes encode + /// stored chunk sizes in a fixed `sizeof_size` field, so an expanding + /// filter cannot overflow the size field. Off by default — version-5 + /// files are rejected by libhdf5 before 2.0, including the 1.14-based + /// h5py wheels. + libver_latest: bool, closed: bool, /// Set once `finalize_for_swmr` has published a readable file. /// @@ -675,6 +689,7 @@ impl Hdf5Writer { hard_links: Slot::new(Vec::new()), root_attributes: Slot::new(Vec::new()), create_lock: Slot::new(()), + libver_latest: false, closed: false, swmr_active: false, root_group_addr: None, @@ -682,6 +697,43 @@ impl Hdf5Writer { }) } + /// Target the libhdf5 2.0 file format for datasets created after this + /// call: filtered chunked datasets get layout message version 5, whose + /// chunk indexes store chunk sizes in a fixed `sizeof_size`-byte field + /// with no overflow limit (see [`Self::chunk_layout_version`]). Off by + /// default, because readers older than libhdf5 2.0 — including the + /// 1.14-based h5py wheels — reject version 5. + pub fn set_libver_latest(&mut self, latest: bool) { + self.libver_latest = latest; + } + + /// Layout message version for a new chunked dataset — the + /// `H5D__chunk_set_info` rule (H5Dchunk.c): version 5 is *required* for + /// a chunk over 4 GiB (pre-2.0 readers cannot handle one even though the + /// v4 wire format could express it) and *preferred* for filtered chunks + /// when the file targets the 2.0 format; everything else stays at + /// version 4, which every 1.10+ reader accepts. + fn chunk_layout_version(&self, filtered: bool, chunk_bytes: u64) -> u8 { + if chunk_bytes > u32::MAX as u64 || (filtered && self.libver_latest) { + 5 + } else { + 4 + } + } + + /// Width of the stored-chunk-size field in a filtered chunk index: + /// version 5 uses the fixed `sizeof_size`; version 4 derives it from the + /// uncompressed chunk byte count (one spare byte included), the + /// `H5D_*_COMPUTE_CHUNK_SIZE_LEN` rule shared by the extensible-array, + /// fixed-array and v2-B-tree indexes. + fn chunk_size_len_for(&self, layout_version: u8, chunk_bytes: u64) -> u8 { + if layout_version >= 5 { + self.ctx.sizeof_size + } else { + compute_chunk_size_len(chunk_bytes) + } + } + /// Provide public access to the format context. pub fn ctx(&self) -> &FormatContext { &self.ctx @@ -929,6 +981,14 @@ impl Hdf5Writer { filter_pipeline: fp, deleted: false, fill_value, + // Preserve the on-disk layout version so finalize re-encodes + // what it read: a v5 file reopened and appended to must not be + // silently downgraded to v4 (the filtered indexes keep their + // 8-byte size fields, which v4 readers would mis-derive). + layout_version: match &dl { + DataLayoutMessage::ChunkedV4 { version, .. } => *version, + _ => 4, + }, }; // Reconstruct storage-specific metadata @@ -1156,6 +1216,7 @@ impl Hdf5Writer { hard_links: Slot::new(Vec::new()), root_attributes: Slot::new(root_attributes), create_lock: Slot::new(()), + libver_latest: false, closed: false, swmr_active: false, root_group_addr: None, @@ -1696,6 +1757,7 @@ impl Hdf5Writer { filter_pipeline: None, deleted: false, fill_value: None, + layout_version: 4, }, ); @@ -1718,6 +1780,8 @@ impl Hdf5Writer { let create = self.begin_create(name)?; validate_chunk_geometry(dims, max_dims, chunk_dims)?; ensure_unlimited_is_leading(max_dims)?; + let chunk_bytes = chunk_dims.iter().product::() * datatype.element_size() as u64; + let layout_version = self.chunk_layout_version(false, chunk_bytes); let earray_params = EarrayParams::default_params(); let ndblk_addrs = compute_ndblk_addrs(earray_params.sup_blk_min_data_ptrs)?; let nsblk_addrs = compute_nsblk_addrs( @@ -1780,6 +1844,7 @@ impl Hdf5Writer { filter_pipeline: None, deleted: false, fill_value: None, + layout_version, fixed_array: None, btree_v2: None, chunked: Some(ChunkedDatasetInfo { @@ -2513,6 +2578,7 @@ impl Hdf5Writer { filter_pipeline: None, deleted: false, fill_value: None, + layout_version: 4, chunked: None, fixed_array: None, btree_v2: None, @@ -2589,6 +2655,7 @@ impl Hdf5Writer { filter_pipeline: None, deleted: false, fill_value: None, + layout_version: 4, chunked: None, fixed_array: None, btree_v2: None, @@ -2652,7 +2719,8 @@ impl Hdf5Writer { let dims: Vec = vec![num_strings]; let max_dims: Vec = vec![num_strings]; let chunk_bytes = chunk_size as u64 * element_size; - let chunk_size_len = compute_chunk_size_len(chunk_bytes); + let layout_version = self.chunk_layout_version(true, chunk_bytes); + let chunk_size_len = self.chunk_size_len_for(layout_version, chunk_bytes); let earray_params = EarrayParams::default_params(); let ndblk_addrs = compute_ndblk_addrs(earray_params.sup_blk_min_data_ptrs)?; @@ -2718,6 +2786,7 @@ impl Hdf5Writer { filter_pipeline: Some(pipeline), deleted: false, fill_value: None, + layout_version, fixed_array: None, btree_v2: None, chunked: Some(ChunkedDatasetInfo { @@ -3664,7 +3733,7 @@ impl Hdf5Writer { let lidx = linear as usize; let (addr, nbytes, mask) = if pipeline.is_some() { match fa.fa_dblk.filtered_elements.get(lidx) { - Some(e) => (e.address, e.chunk_size as u64, e.filter_mask), + Some(e) => (e.address, e.chunk_size, e.filter_mask), None => return Ok(None), } } else { @@ -3687,7 +3756,7 @@ impl Hdf5Writer { let found = if bt2.index.filtered { bt2.index .lookup_filtered(chunk_coords) - .map(|r| (r.chunk_address, r.chunk_size as u64, r.filter_mask)) + .map(|r| (r.chunk_address, r.chunk_size, r.filter_mask)) } else { bt2.index .lookup(chunk_coords) @@ -3855,15 +3924,15 @@ impl Hdf5Writer { })?; } + let chunk_bytes: u64 = chunk_dims.iter().product::() * datatype.element_size() as u64; + let layout_version = self.chunk_layout_version(pipeline.is_some(), chunk_bytes); + // Create the FA header. For a filtered FA, chunk_size_len is sized - // from the uncompressed chunk byte count, the same way the filtered - // Extensible Array path computes it: the compressed size never - // exceeds the uncompressed size meaningfully, so this width always - // holds the stored value. + // the same way the filtered Extensible Array path computes it: + // derived from the uncompressed chunk byte count under layout v4, + // the fixed `sizeof_size` under layout v5. let mut fa_header = if pipeline.is_some() { - let element_size = datatype.element_size() as u64; - let chunk_bytes: u64 = chunk_dims.iter().product::() * element_size; - let chunk_size_len = compute_chunk_size_len(chunk_bytes); + let chunk_size_len = self.chunk_size_len_for(layout_version, chunk_bytes); FixedArrayHeader::new_for_filtered_chunks(&self.ctx, num_chunks, chunk_size_len) } else { FixedArrayHeader::new_for_chunks(&self.ctx, num_chunks) @@ -3918,6 +3987,7 @@ impl Hdf5Writer { filter_pipeline: pipeline, deleted: false, fill_value: None, + layout_version, chunked: None, btree_v2: None, fixed_array: Some(FixedArrayDatasetInfo { @@ -3985,22 +4055,21 @@ impl Hdf5Writer { chunk_dims: &[u64], pipeline: Option, ) -> IoResult { - use crate::format::chunk_index::btree_v2::{ - compute_chunk_size_len, Bt2Header, BT2_NODE_SIZE, - }; + use crate::format::chunk_index::btree_v2::{Bt2Header, BT2_NODE_SIZE}; let create = self.begin_create(name)?; validate_chunk_geometry(dims, max_dims, chunk_dims)?; let ndims = dims.len(); + let chunk_bytes: u64 = chunk_dims.iter().product::() * datatype.element_size() as u64; + let layout_version = self.chunk_layout_version(pipeline.is_some(), chunk_bytes); // The filtered record's size field is as wide as libhdf5 will - // recompute it from the uncompressed chunk size, exactly as the + // recompute it — from the uncompressed chunk size under layout v4, + // the fixed `sizeof_size` under layout v5 — exactly as the // extensible- and fixed-array filtered paths size theirs. let bt2_index = match pipeline { Some(_) => { - let chunk_bytes: u64 = - chunk_dims.iter().product::() * datatype.element_size() as u64; - let len = compute_chunk_size_len(chunk_bytes); + let len = self.chunk_size_len_for(layout_version, chunk_bytes); Bt2ChunkIndex::new_filtered(ndims, len) } None => Bt2ChunkIndex::new_unfiltered(ndims), @@ -4050,6 +4119,7 @@ impl Hdf5Writer { filter_pipeline: pipeline, deleted: false, fill_value: None, + layout_version, chunked: None, fixed_array: None, btree_v2: Some(Bt2DatasetInfo { @@ -4085,7 +4155,8 @@ impl Hdf5Writer { ensure_unlimited_is_leading(max_dims)?; let element_size = datatype.element_size() as u64; let chunk_bytes: u64 = chunk_dims.iter().product::() * element_size; - let chunk_size_len = compute_chunk_size_len(chunk_bytes); + let layout_version = self.chunk_layout_version(true, chunk_bytes); + let chunk_size_len = self.chunk_size_len_for(layout_version, chunk_bytes); let earray_params = EarrayParams::default_params(); let ndblk_addrs = compute_ndblk_addrs(earray_params.sup_blk_min_data_ptrs)?; @@ -4152,6 +4223,7 @@ impl Hdf5Writer { filter_pipeline: Some(FilterPipeline::deflate(compression_level)), deleted: false, fill_value: None, + layout_version, fixed_array: None, btree_v2: None, chunked: Some(ChunkedDatasetInfo { @@ -4189,7 +4261,8 @@ impl Hdf5Writer { ensure_unlimited_is_leading(max_dims)?; let element_size = datatype.element_size() as u64; let chunk_bytes: u64 = chunk_dims.iter().product::() * element_size; - let chunk_size_len = compute_chunk_size_len(chunk_bytes); + let layout_version = self.chunk_layout_version(true, chunk_bytes); + let chunk_size_len = self.chunk_size_len_for(layout_version, chunk_bytes); let earray_params = EarrayParams::default_params(); let ndblk_addrs = compute_ndblk_addrs(earray_params.sup_blk_min_data_ptrs)?; @@ -4251,6 +4324,7 @@ impl Hdf5Writer { filter_pipeline: Some(pipeline), deleted: false, fill_value: None, + layout_version, fixed_array: None, btree_v2: None, chunked: Some(ChunkedDatasetInfo { @@ -4414,11 +4488,6 @@ impl Hdf5Writer { // Filtered FA: store address + stored size + filter mask. A // non-zero mask bit means "filter i was skipped for this chunk". let stored_size = final_bytes.len(); - if stored_size > u32::MAX as usize { - return Err(crate::io::IoError::InvalidState(format!( - "compressed chunk size {stored_size} exceeds u32::MAX" - ))); - } // The stored size is encoded in the FA header's `chunk_size_len`-byte // field; libhdf5 errors if it does not fit (H5D_CHUNK_ENCODE_SIZE_CHECK) // rather than truncating silently. element_size = sizeof_addr + @@ -4438,14 +4507,12 @@ impl Hdf5Writer { } if lidx < fa.fa_dblk.filtered_elements.len() { let old = &fa.fa_dblk.filtered_elements[lidx]; - let chunk_addr = self.place_chunk( - Some((old.address, old.chunk_size as u64)), - stored_size as u64, - ); + let chunk_addr = + self.place_chunk(Some((old.address, old.chunk_size)), stored_size as u64); self.handle.write_at(chunk_addr, final_bytes)?; fa.fa_dblk.filtered_elements[lidx] = FixedArrayFilteredChunkElement { address: chunk_addr, - chunk_size: stored_size as u32, + chunk_size: stored_size as u64, filter_mask, }; fa.chunks_written += 1; @@ -4629,7 +4696,7 @@ impl Hdf5Writer { let old = if bt2.index.filtered { bt2.index .lookup_filtered(chunk_coords) - .map(|r| (r.chunk_address, r.chunk_size as u64)) + .map(|r| (r.chunk_address, r.chunk_size)) } else { bt2.index .lookup(chunk_coords) @@ -4640,12 +4707,8 @@ impl Hdf5Writer { let bt2 = m.btree_v2.as_mut().unwrap(); if bt2.index.filtered { - bt2.index.insert_filtered( - chunk_coords.to_vec(), - chunk_addr, - stored_len as u32, - filter_mask, - ); + bt2.index + .insert_filtered(chunk_coords.to_vec(), chunk_addr, stored_len, filter_mask); } else { bt2.index.insert(chunk_coords.to_vec(), chunk_addr); } @@ -5350,6 +5413,7 @@ impl Hdf5Writer { let mut layout_dims = chunked.chunk_dims.clone(); layout_dims.push(m.datatype.element_size() as u64); DataLayoutMessage::chunked_v4_earray( + m.layout_version, layout_dims, chunked.earray_params.clone(), chunked.ea_header_addr, @@ -5358,6 +5422,7 @@ impl Hdf5Writer { let mut layout_dims = fa.chunk_dims.clone(); layout_dims.push(m.datatype.element_size() as u64); DataLayoutMessage::chunked_v4_farray( + m.layout_version, layout_dims, FixedArrayParams::default_params(), fa.fa_header_addr, @@ -5365,7 +5430,11 @@ impl Hdf5Writer { } else if let Some(ref bt2) = m.btree_v2 { let mut layout_dims = bt2.chunk_dims.clone(); layout_dims.push(m.datatype.element_size() as u64); - DataLayoutMessage::chunked_v4_btree_v2(layout_dims, bt2.bt2_header_addr) + DataLayoutMessage::chunked_v4_btree_v2( + m.layout_version, + layout_dims, + bt2.bt2_header_addr, + ) } else { DataLayoutMessage::contiguous(m.data_addr, m.data_size) }; @@ -6198,7 +6267,7 @@ mod tests { let mut paged_dblk = FixedArrayDataBlock::new_filtered(0x1000, 2500); for (i, e) in paged_dblk.filtered_elements.iter_mut().enumerate() { e.address = 0x10000 + (i as u64) * 0x100; - e.chunk_size = (i % 200) as u32; + e.chunk_size = (i % 200) as u64; } let encoded = encode_fixed_array_dblk(&ctx, &paged, &paged_dblk); assert_eq!( @@ -7026,4 +7095,223 @@ mod tests { writer.close().unwrap(); std::fs::remove_file(&path).ok(); } + + /// `set_libver_latest` moves *filtered* chunked datasets to layout v5 with + /// fixed 8-byte chunk-size fields; unfiltered chunked and pre-opt-in + /// datasets keep v4 with the derived width, matching libhdf5's + /// `version_perf` rule (only the filtered index arms bump to 5). + #[cfg(feature = "deflate")] + #[test] + fn libver_latest_selects_v5_for_filtered_chunks_only() { + let path = temp_path("libver_v5_select"); + + let mut writer = Hdf5Writer::create(&path).unwrap(); + let before = writer + .create_chunked_dataset_compressed( + "d4", + DatatypeMessage::i32_type(), + &[0], + &[u64::MAX], + &[16], + 4, + ) + .unwrap(); + writer.set_libver_latest(true); + let ea5 = writer + .create_chunked_dataset_compressed( + "ea5", + DatatypeMessage::i32_type(), + &[0], + &[u64::MAX], + &[16], + 4, + ) + .unwrap(); + let plain = writer + .create_chunked_dataset( + "plain", + DatatypeMessage::i32_type(), + &[0], + &[u64::MAX], + &[16], + ) + .unwrap(); + let fa5 = writer + .create_fixed_array_dataset_with_pipeline( + "fa5", + DatatypeMessage::i32_type(), + &[4, 6], + &[2, 3], + FilterPipeline::deflate(6), + ) + .unwrap(); + let bt5 = writer + .create_btree_v2_dataset_with_pipeline( + "bt5", + DatatypeMessage::i32_type(), + &[0, 0], + &[u64::MAX, u64::MAX], + &[2, 3], + FilterPipeline::deflate(6), + ) + .unwrap(); + + { + let d4 = writer.ds(before); + let d4 = d4.lock(); + assert_eq!(d4.layout_version, 4); + assert_eq!( + d4.chunked.as_ref().unwrap().chunk_size_len, + compute_chunk_size_len(16 * 4) + ); + let e5 = writer.ds(ea5); + let e5 = e5.lock(); + assert_eq!(e5.layout_version, 5); + assert_eq!(e5.chunked.as_ref().unwrap().chunk_size_len, 8); + assert_eq!(writer.ds(plain).lock().layout_version, 4); + assert_eq!(writer.ds(fa5).lock().layout_version, 5); + assert_eq!(writer.ds(bt5).lock().layout_version, 5); + } + + // Write through the FA and BT2 v5 indexes so their 8-byte chunk-size + // fields are exercised end to end, not just selected. + for (coords, vals) in [ + ([0u64, 0], [0i32, 1, 2, 6, 7, 8]), + ([0, 1], [3, 4, 5, 9, 10, 11]), + ([1, 0], [12, 13, 14, 18, 19, 20]), + ([1, 1], [15, 16, 17, 21, 22, 23]), + ] { + let bytes: Vec = vals.iter().flat_map(|v| v.to_le_bytes()).collect(); + writer + .write_chunk_fixed_array(fa5, &coords, &bytes) + .unwrap(); + writer.write_chunk_btree_v2(bt5, &coords, &bytes).unwrap(); + } + writer.extend_dataset(bt5, &[4, 6]).unwrap(); + writer.close().unwrap(); + + let mut reader = Hdf5Reader::open(&path).unwrap(); + for name in ["fa5", "bt5"] { + let raw = reader.read_dataset_raw(name).unwrap(); + let values: Vec = raw + .chunks(4) + .map(|c| i32::from_le_bytes(c.try_into().unwrap())) + .collect(); + assert_eq!(values, (0..24).collect::>(), "dataset {name}"); + } + + std::fs::remove_file(&path).ok(); + } + + /// A v5 file reopened for append must stay v5: the decode → `DatasetInfo` + /// → finalize path carries the version through, so the re-encoded layout + /// message matches the 8-byte size fields the filtered index was built + /// with. A silent v4 downgrade here would make libhdf5 derive a narrower + /// field width than the index uses. + #[cfg(feature = "deflate")] + #[test] + fn v5_layout_survives_reopen_and_append() { + let path = temp_path("libver_v5_reopen"); + let chunk: usize = 8; + + let mut writer = Hdf5Writer::create(&path).unwrap(); + writer.set_libver_latest(true); + let idx = writer + .create_chunked_dataset_compressed( + "d", + DatatypeMessage::i32_type(), + &[0], + &[u64::MAX], + &[chunk as u64], + 4, + ) + .unwrap(); + for c in 0..2u64 { + let data: Vec = (0..chunk as i32) + .flat_map(|i| (c as i32 * chunk as i32 + i).to_le_bytes()) + .collect(); + writer.write_chunk(idx, c, &data).unwrap(); + } + writer.extend_dataset(idx, &[2 * chunk as u64]).unwrap(); + writer.close().unwrap(); + + // Reopen: the decoded layout version must be preserved, and appends + // must keep working against the 8-byte-size-field index. + let writer = Hdf5Writer::open_append(&path).unwrap(); + assert_eq!(writer.ds(0).lock().layout_version, 5); + for c in 2..4u64 { + let data: Vec = (0..chunk as i32) + .flat_map(|i| (c as i32 * chunk as i32 + i).to_le_bytes()) + .collect(); + writer.write_chunk(0, c, &data).unwrap(); + } + writer.extend_dataset(0, &[4 * chunk as u64]).unwrap(); + writer.close().unwrap(); + + // Still v5 after the second finalize, and fully readable. + let writer = Hdf5Writer::open_append(&path).unwrap(); + assert_eq!(writer.ds(0).lock().layout_version, 5); + writer.close().unwrap(); + + let mut reader = Hdf5Reader::open(&path).unwrap(); + let raw = reader.read_dataset_raw("d").unwrap(); + let values: Vec = raw + .chunks(4) + .map(|c| i32::from_le_bytes(c.try_into().unwrap())) + .collect(); + assert_eq!(values, (0..4 * chunk as i32).collect::>()); + + std::fs::remove_file(&path).ok(); + } + + /// A chunk strictly larger than `u32::MAX` bytes forces layout v5 with no + /// opt-in — v4's size field cannot represent it — while a chunk of exactly + /// `u32::MAX` bytes stays v4, matching libhdf5's `version_req` boundary + /// (`> 0xffffffff`, filtered or not). + #[test] + fn oversized_chunk_forces_v5_without_opt_in() { + let path = temp_path("libver_4gib_force"); + + let writer = Hdf5Writer::create(&path).unwrap(); + let at_limit = writer + .create_chunked_dataset_compressed( + "at_limit", + DatatypeMessage::u8_type(), + &[0], + &[u64::MAX], + &[u32::MAX as u64], + 4, + ) + .unwrap(); + let over = writer + .create_chunked_dataset_compressed( + "over", + DatatypeMessage::u8_type(), + &[0], + &[u64::MAX], + &[u32::MAX as u64 + 1], + 4, + ) + .unwrap(); + let over_unfiltered = writer + .create_chunked_dataset( + "over_plain", + DatatypeMessage::u8_type(), + &[0], + &[u64::MAX], + &[u32::MAX as u64 + 1], + ) + .unwrap(); + + assert_eq!(writer.ds(at_limit).lock().layout_version, 4); + { + let ds = writer.ds(over); + let ds = ds.lock(); + assert_eq!(ds.layout_version, 5); + assert_eq!(ds.chunked.as_ref().unwrap().chunk_size_len, 8); + } + assert_eq!(writer.ds(over_unfiltered).lock().layout_version, 5); + writer.close().unwrap(); + std::fs::remove_file(&path).ok(); + } } diff --git a/tests/h5py_cross_validation.rs b/tests/h5py_cross_validation.rs index 7f0770c..eb8f5c8 100644 --- a/tests/h5py_cross_validation.rs +++ b/tests/h5py_cross_validation.rs @@ -822,3 +822,72 @@ fn fa_growable_max_shape_readable_by_h5py() { ); std::fs::remove_file(&path).ok(); } + +/// Issue #8: `set_libver_latest(true)` writes a version-5 data layout message +/// for filtered chunked datasets. Two identical files differing only in the +/// knob prove both directions: the default file stays h5py-readable (v4), and +/// hdf5 < 2.0 rejects the opt-in file — the rejection is the on-disk proof +/// that a genuine v5 message was written, not a v4 one with wider index +/// fields. Under hdf5 >= 2.0 the v5 file must instead read back exactly. +/// Either way, rust-hdf5's own reader must read the v5 file. +#[cfg(feature = "deflate")] +#[test] +fn libver_latest_v5_layout_write_and_hdf5_1x_rejection() { + let Some(py) = python() else { return }; + let data: Vec = (0..35).collect(); // 7 x 5 row-major + let path_v4 = tmp("layout_default_v4"); + let path_v5 = tmp("layout_optin_v5"); + for (path, latest) in [(&path_v4, false), (&path_v5, true)] { + let file = H5File::create(path).unwrap(); + file.set_libver_latest(latest).unwrap(); + let ds = file + .new_dataset::() + .shape([7, 5]) + .chunk(&[3, 2]) + .deflate(4) + .create("grid") + .unwrap(); + ds.write_raw(&data).unwrap(); + file.close().unwrap(); + } + + // rust-hdf5's reader handles both versions. + for path in [&path_v4, &path_v5] { + let file = H5File::open(path).unwrap(); + let got = file.dataset("grid").unwrap().read_raw::().unwrap(); + assert_eq!(got, data, "rust read-back of {}", path.display()); + } + + // Positive control: the default file is plain v4 and h5py-readable. + read_back_with_h5py( + py, + &path_v4, + "ds = f['grid']\n\ + assert ds.compression == 'gzip', ds.compression\n\ + assert np.array_equal(ds[...], np.arange(35).reshape(7, 5)), ds[...]\n", + ); + + // The v5 file: rejected below hdf5 2.0, readable at 2.0+. + let script = format!( + "import h5py, numpy as np, sys\n\ + v2 = h5py.version.hdf5_version_tuple >= (2, 0, 0)\n\ + try:\n\ + \x20 f = h5py.File(r'{}', 'r')\n\ + \x20 v = f['grid'][...]\n\ + except Exception:\n\ + \x20 assert not v2, 'hdf5 >= 2.0 must read a v5 layout'\n\ + \x20 sys.exit(0)\n\ + assert v2, 'hdf5 < 2.0 read the opt-in file, so it is not v5 on disk'\n\ + assert np.array_equal(v, np.arange(35).reshape(7, 5)), v\n", + path_v5.display() + ); + let status = std::process::Command::new(py) + .arg("-c") + .arg(&script) + .status() + .expect("failed to spawn python"); + assert!(status.success(), "v5 layout check failed for {path_v5:?}"); + + std::fs::remove_file(&path_v4).ok(); + std::fs::remove_file(&path_v5).ok(); +}