From 483f8389fe3b7912a3a49e160bd426cd1ad79761 Mon Sep 17 00:00:00 2001 From: Sang Woo Kim Date: Thu, 13 Aug 2026 08:13:05 +0900 Subject: [PATCH 1/9] dataset: read a whole string dataset with read_strings The fixed-string width is a per-dataset property, so a caller slicing read_raw_bytes by hand has to hard-code one and gets the next file wrong. The padding rule and character set the datatype declares are honoured here; read_strings_lossy is the variant for producers that mislabel the charset. Closes #5, requested by @janosh. --- CHANGELOG.md | 14 ++ src/dataset.rs | 431 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 445 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index ffd6d29..d721024 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,19 @@ # Changelog +## Unreleased + +### Added + +- `H5Dataset::read_strings` and `read_strings_lossy` decode a whole string + dataset in one call, at whatever width the file declares (issue #5). Reading + a fixed-string column previously meant `read_raw_bytes` plus hand-written + slicing, and the width is a per-dataset property, so the caller had to + hard-code one and get it wrong on the next file. Both string datatypes go + through the same call, the padding rule decides where each value ends, and + the character set is enforced — `read_strings` fails on bytes that do not + decode, `read_strings_lossy` substitutes U+FFFD for producers that mislabel + the character set. + ## 0.4.0 ### Changed diff --git a/src/dataset.rs b/src/dataset.rs index c1152ef..bb1f171 100644 --- a/src/dataset.rs +++ b/src/dataset.rs @@ -490,6 +490,57 @@ enum ChunkBytes<'a> { Prefiltered { data: &'a [u8], filter_mask: u32 }, } +/// Strip a fixed-string element's padding, leaving the bytes that carry the +/// value. +/// +/// The three padding rules are the HDF5 datatype message's: null-terminated +/// stops at the first NUL and says nothing about the bytes after it, +/// null-padded and space-padded fill the tail with that byte. `index` names +/// the element in the error a reserved padding rule produces. +fn trim_fixed_string(elem: &[u8], padding: u8, index: usize) -> Result<&[u8]> { + let end = match padding { + // Null-terminated. + 0 => elem.iter().position(|&b| b == 0).unwrap_or(elem.len()), + // Null-padded / space-padded: the tail of that byte is padding. + 1 => elem.iter().rposition(|&b| b != 0).map_or(0, |i| i + 1), + 2 => elem.iter().rposition(|&b| b != b' ').map_or(0, |i| i + 1), + other => { + return Err(Hdf5Error::InvalidState(format!( + "string {index} uses padding rule {other}, which the format reserves" + ))) + } + }; + Ok(&elem[..end]) +} + +/// Decode one string element's bytes under the datatype's character set. +/// +/// `lossy` replaces what it cannot decode with U+FFFD instead of failing; +/// `index` names the element in the error otherwise. +fn decode_string(bytes: &[u8], charset: u8, lossy: bool, index: usize) -> Result { + if lossy { + return Ok(String::from_utf8_lossy(bytes).into_owned()); + } + match charset { + // ASCII. Bytes are 7-bit, which makes them UTF-8 as well. + 0 => match bytes.iter().position(|&b| b >= 0x80) { + None => Ok(String::from_utf8_lossy(bytes).into_owned()), + Some(at) => Err(Hdf5Error::InvalidState(format!( + "string {index} declares the ASCII character set but byte {at} is {:#04x}", + bytes[at] + ))), + }, + 1 => String::from_utf8(bytes.to_vec()).map_err(|e| { + Hdf5Error::InvalidState(format!( + "string {index} declares UTF-8 but is not valid UTF-8: {e}" + )) + }), + other => Err(Hdf5Error::InvalidState(format!( + "string {index} uses character set {other}, which the format reserves" + ))), + } +} + impl H5Dataset { /// Create a reader-mode dataset handle (called internally by `H5File::dataset`). pub(crate) fn new_reader( @@ -1899,6 +1950,81 @@ impl H5Dataset { } } + /// Read a string dataset, fixed-width or variable-length, as one `String` + /// per element. + /// + /// The width of a `FixedString` dataset is whatever the file says, so a + /// 24-byte label column and a 100-byte one are read by the same call. The + /// padding rule the datatype declares decides where each element ends — + /// null-terminated (0), null-padded (1) or space-padded (2) — and its + /// character set decides how the remaining bytes are decoded: ASCII (0) + /// requires 7-bit bytes, UTF-8 (1) requires valid UTF-8. An element that + /// violates either is an error naming the element, not a silent + /// substitution; [`read_strings_lossy`](Self::read_strings_lossy) is the + /// call that accepts such a file, replacing what it cannot decode. + /// + /// ```no_run + /// # use rust_hdf5::H5File; + /// let file = H5File::open("labels.h5").unwrap(); + /// let labels = file.dataset("names").unwrap().read_strings().unwrap(); + /// ``` + pub fn read_strings(&self) -> Result> { + self.read_strings_inner(false) + } + + /// [`read_strings`](Self::read_strings), but bytes that do not decode + /// under the dataset's character set become U+FFFD instead of an error. + /// + /// Producers do mislabel the character set — a file that declares ASCII + /// while storing Latin-1 or UTF-8 bytes reads here and not there. + pub fn read_strings_lossy(&self) -> Result> { + self.read_strings_inner(true) + } + + /// The single owner of string decoding for both string datatypes: the + /// element bytes are found differently, the padding and character-set + /// rules that turn them into a `String` are the same. + fn read_strings_inner(&self, lossy: bool) -> Result> { + if matches!(self.info, DatasetInfo::Writer { .. }) { + return Err(Hdf5Error::InvalidState( + "cannot read strings from a dataset in write mode".into(), + )); + } + match self.datatype()? { + DatatypeMessage::VarLenString { charset } => self + .read_vlen_bytes()? + .iter() + .enumerate() + .map(|(i, bytes)| decode_string(bytes, charset, lossy, i)) + .collect(), + DatatypeMessage::FixedString { + size, + padding, + charset, + } => { + let width = size as usize; + if width == 0 { + // A corrupt file can declare it; `chunks_exact(0)` panics. + return Err(Hdf5Error::InvalidState( + "fixed-string datatype has zero width".into(), + )); + } + // `read_raw_bytes` returns `product(dims) * width` bytes, so + // `chunks_exact` leaves no remainder. + let raw = self.read_raw_bytes()?; + raw.chunks_exact(width) + .enumerate() + .map(|(i, elem)| { + decode_string(trim_fixed_string(elem, padding, i)?, charset, lossy, i) + }) + .collect() + } + other => Err(Hdf5Error::InvalidState(format!( + "read_strings is only for string datasets, this one is {other:?}" + ))), + } + } + /// Read the entire dataset as a typed vector. /// /// The raw bytes are read from the file and reinterpreted as `T`. The @@ -4458,4 +4584,309 @@ mod tests { ); std::fs::remove_file(&path).ok(); } + + // ---- issue #5: runtime-width fixed-string reading ---------------------- + + use crate::format::messages::datatype::DatatypeMessage; + + /// Build a 1-D fixed-string dataset of `width` bytes per element from raw + /// element images, optionally chunked and deflated. + fn write_fixed_string_dataset( + path: &std::path::Path, + dt: DatatypeMessage, + width: usize, + elems: &[&[u8]], + compressed: bool, + ) { + let mut raw = Vec::with_capacity(elems.len() * width); + for e in elems { + assert!(e.len() <= width); + raw.extend_from_slice(e); + raw.resize(raw.len() + (width - e.len()), 0); + } + let file = H5File::create(path).unwrap(); + let mut b = file.new_dataset::().datatype(dt).shape([elems.len()]); + if compressed { + b = b.chunk(&[2]).deflate(6); + } + let ds = b.create("labels").unwrap(); + ds.write_raw_bytes(&raw).unwrap(); + file.close().unwrap(); + } + + /// The width is whatever the file says, so one call reads a 24-byte label + /// column and a 100-byte one. Producers like VASP pick it per dataset. + #[test] + fn read_strings_handles_any_fixed_width() { + for width in [4usize, 24, 100] { + let path = temp_path(&format!("fixed_str_{width}")); + write_fixed_string_dataset( + &path, + DatatypeMessage::fixed_string(width as u32), + width, + &[b"ab", b"cde", b""], + false, + ); + let file = H5File::open(&path).unwrap(); + let got = file.dataset("labels").unwrap().read_strings().unwrap(); + assert_eq!(got, vec!["ab", "cde", ""], "width {width}"); + std::fs::remove_file(&path).ok(); + } + } + + /// Each padding rule decides where the value ends. Null-terminated stops at + /// the first NUL and ignores the bytes after it; the two pad rules strip a + /// tail of that byte and keep everything before it. + #[test] + fn read_strings_honors_every_padding_rule() { + // "ab" then a NUL then trailing junk a null-terminated read must drop + // and a null-padded read must keep. + let elem: &[u8] = b"ab\0X\0\0"; + for (padding, want) in [(0u8, "ab"), (1, "ab\0X")] { + let path = temp_path(&format!("fixed_pad_{padding}")); + write_fixed_string_dataset( + &path, + DatatypeMessage::FixedString { + size: 6, + padding, + charset: 0, + }, + 6, + &[elem], + false, + ); + let file = H5File::open(&path).unwrap(); + let got = file.dataset("labels").unwrap().read_strings().unwrap(); + assert_eq!(got, vec![want.to_string()], "padding {padding}"); + std::fs::remove_file(&path).ok(); + } + // Space-padded keeps interior spaces and strips only the tail. + let path = temp_path("fixed_pad_2"); + write_fixed_string_dataset( + &path, + DatatypeMessage::FixedString { + size: 8, + padding: 2, + charset: 0, + }, + 8, + &[b"a b "], + false, + ); + let file = H5File::open(&path).unwrap(); + assert_eq!( + file.dataset("labels").unwrap().read_strings().unwrap(), + vec!["a b".to_string()] + ); + std::fs::remove_file(&path).ok(); + } + + /// A reserved padding or character-set code is an error naming the element, + /// not a guess. + #[test] + fn read_strings_rejects_reserved_datatype_codes() { + for (padding, charset, want) in [(3u8, 0u8, "padding rule 3"), (0, 7, "character set 7")] { + let path = temp_path(&format!("fixed_reserved_{padding}_{charset}")); + write_fixed_string_dataset( + &path, + DatatypeMessage::FixedString { + size: 4, + padding, + charset, + }, + 4, + &[b"ab"], + false, + ); + let file = H5File::open(&path).unwrap(); + let err = file + .dataset("labels") + .unwrap() + .read_strings() + .unwrap_err() + .to_string(); + assert!(err.contains(want), "got: {err}"); + std::fs::remove_file(&path).ok(); + } + } + + /// The declared character set is enforced: a byte that cannot be decoded is + /// an error naming the element, and the lossy call is what accepts the file + /// instead of a silent substitution here. + #[test] + fn read_strings_enforces_the_character_set_and_lossy_does_not() { + // Latin-1 "é" (0xE9) in a dataset that declares ASCII, and a lone 0xFF + // in one that declares UTF-8. + for (charset, bytes, want) in [ + (0u8, b"caf\xe9".as_slice(), "ASCII character set"), + (1, b"a\xff".as_slice(), "not valid UTF-8"), + ] { + let path = temp_path(&format!("fixed_charset_{charset}")); + write_fixed_string_dataset( + &path, + DatatypeMessage::FixedString { + size: 6, + padding: 1, + charset, + }, + 6, + &[b"ok", bytes], + false, + ); + let file = H5File::open(&path).unwrap(); + let ds = file.dataset("labels").unwrap(); + let err = ds.read_strings().unwrap_err().to_string(); + assert!(err.contains(want) && err.contains("string 1"), "got: {err}"); + let lossy = ds.read_strings_lossy().unwrap(); + assert_eq!(lossy[0], "ok"); + assert_eq!( + lossy[1].chars().next().unwrap(), + if charset == 0 { 'c' } else { 'a' } + ); + std::fs::remove_file(&path).ok(); + } + } + + /// Valid multi-byte UTF-8 survives, and the trailing NUL padding does not + /// split a character. + #[test] + fn read_strings_reads_utf8_fixed_strings() { + let path = temp_path("fixed_utf8"); + write_fixed_string_dataset( + &path, + DatatypeMessage::fixed_string_utf8(12), + 12, + &["héllo".as_bytes(), "안녕".as_bytes()], + false, + ); + let file = H5File::open(&path).unwrap(); + assert_eq!( + file.dataset("labels").unwrap().read_strings().unwrap(), + vec!["héllo".to_string(), "안녕".to_string()] + ); + std::fs::remove_file(&path).ok(); + } + + /// The decode sits on the decoded raw-data path, so a chunked and deflated + /// dataset reads the same as a contiguous one. + #[cfg(feature = "deflate")] + #[test] + fn read_strings_reads_a_compressed_fixed_string_dataset() { + let path = temp_path("fixed_str_deflate"); + write_fixed_string_dataset( + &path, + DatatypeMessage::fixed_string(16), + 16, + &[b"alpha", b"beta", b"gamma", b"delta", b"epsilon"], + true, + ); + let file = H5File::open(&path).unwrap(); + assert_eq!( + file.dataset("labels").unwrap().read_strings().unwrap(), + vec!["alpha", "beta", "gamma", "delta", "epsilon"] + ); + std::fs::remove_file(&path).ok(); + } + + /// One call covers both string datatypes, so a caller need not branch on + /// which one the file used. + #[test] + fn read_strings_also_reads_variable_length_strings() { + let path = temp_path("read_strings_vlen"); + { + let file = H5File::create(&path).unwrap(); + file.write_vlen_strings("names", &["alpha", "", "안녕"]) + .unwrap(); + file.close().unwrap(); + } + let file = H5File::open(&path).unwrap(); + assert_eq!( + file.dataset("names").unwrap().read_strings().unwrap(), + vec!["alpha".to_string(), String::new(), "안녕".to_string()] + ); + std::fs::remove_file(&path).ok(); + } + + /// A file declaring a zero-width fixed string is an error, not the panic + /// `chunks_exact(0)` would raise. Nothing in this crate writes one, so the + /// test patches the width in the encoded datatype message down to zero and + /// re-stamps the object header's checksum over the result. + #[test] + fn read_strings_rejects_a_zero_width_fixed_string_dataset() { + use crate::format::checksum::checksum_metadata; + use crate::format::object_header::OHDR_SIGNATURE; + + let path = temp_path("fixed_str_zero_width"); + write_fixed_string_dataset( + &path, + DatatypeMessage::fixed_string(37), + 37, + &[b"ab", b"cd"], + false, + ); + + // Version 1 string datatype: class|version, padding|charset, two + // reserved bytes, then the width as a little-endian u32. The width is + // 37 so the eight bytes occur once in the file. + let mut bytes = std::fs::read(&path).unwrap(); + let needle = [0x13u8, 0, 0, 0, 37, 0, 0, 0]; + let at = bytes + .windows(needle.len()) + .position(|w| w == needle) + .expect("encoded fixed-string datatype message"); + assert!( + !bytes[at + 1..].windows(needle.len()).any(|w| w == needle), + "the datatype message pattern is not unique in the file" + ); + + // The enclosing v2 object header ends in a checksum over everything + // from its signature onwards; find the offset where the stored value + // still agrees, so the patched header can be re-stamped there. + let ohdr = bytes[..at] + .windows(4) + .rposition(|w| w == OHDR_SIGNATURE) + .expect("enclosing object header"); + let cksum_at = (at + needle.len()..bytes.len() - 4) + .find(|&e| { + u32::from_le_bytes(bytes[e..e + 4].try_into().unwrap()) + == checksum_metadata(&bytes[ohdr..e]) + }) + .expect("object header checksum"); + + bytes[at + 4..at + 8].copy_from_slice(&0u32.to_le_bytes()); + let fixed = checksum_metadata(&bytes[ohdr..cksum_at]); + bytes[cksum_at..cksum_at + 4].copy_from_slice(&fixed.to_le_bytes()); + std::fs::write(&path, &bytes).unwrap(); + + let file = H5File::open(&path).unwrap(); + let err = file + .dataset("labels") + .unwrap() + .read_strings() + .unwrap_err() + .to_string(); + assert!(err.contains("zero width"), "got: {err}"); + std::fs::remove_file(&path).ok(); + } + + /// A non-string dataset is an error, not an attempt to reinterpret bytes. + #[test] + fn read_strings_rejects_a_non_string_dataset() { + let path = temp_path("read_strings_numeric"); + { + let file = H5File::create(&path).unwrap(); + let ds = file.new_dataset::().shape([3]).create("nums").unwrap(); + ds.write_raw(&[1i32, 2, 3]).unwrap(); + file.close().unwrap(); + } + let file = H5File::open(&path).unwrap(); + let err = file + .dataset("nums") + .unwrap() + .read_strings() + .unwrap_err() + .to_string(); + assert!(err.contains("only for string datasets"), "got: {err}"); + std::fs::remove_file(&path).ok(); + } } From 25324e32b535945fc7883a5717a9c9c2f9aa023d Mon Sep 17 00:00:00 2001 From: Sang Woo Kim Date: Thu, 13 Aug 2026 08:13:45 +0900 Subject: [PATCH 2/9] dataset: replace vlen strings at an offset, not only whole or appended Elements the append buffer still holds are patched in the buffer, not on disk, or the flush at close would write the pre-update reference back over them. The global heap objects the replaced references pointed at stay in the file unreachable; libhdf5 does not reclaim them either. Closes #6, requested by @janosh. --- CHANGELOG.md | 9 ++ src/dataset.rs | 239 +++++++++++++++++++++++++++++++++++++++++++++++ src/io/writer.rs | 129 +++++++++++++++++++++++++ 3 files changed, 377 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d721024..e467984 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,15 @@ decode, `read_strings_lossy` substitutes U+FFFD for producers that mislabel the character set. +- `H5Dataset::write_vlen_strings_slice` replaces variable-length strings at an + arbitrary offset (issue #6). Vlen datasets could only be created whole or + appended to, so correcting one entry meant rewriting the dataset. The new + strings go into one global-heap collection and their references are written + over the range; elements still held in the append buffer are patched there, + so the flush at close does not write the pre-update reference back over + them. The heap objects the replaced references pointed at stay in the file + as unreachable bytes — libhdf5 does not reclaim them either. + ## 0.4.0 ### Changed diff --git a/src/dataset.rs b/src/dataset.rs index bb1f171..d56dc13 100644 --- a/src/dataset.rs +++ b/src/dataset.rs @@ -1910,6 +1910,47 @@ impl H5Dataset { } } + /// Replace elements `start .. start + strings.len()` of a 1-D + /// variable-length string dataset. + /// + /// The extent and every element outside the range are left alone, and the + /// cost is the new strings plus the chunks holding their references — not + /// the column. The dataset's character set is enforced: a non-ASCII + /// replacement in a dataset that declares ASCII is rejected rather than + /// stored under a datatype that misdescribes it. + /// + /// The objects the replaced references pointed at stay in their global + /// heap collections, unreachable. Nothing in this library frees global + /// heap space, so a column updated many times grows the file; rewriting + /// the dataset is what reclaims it. + /// + /// ```no_run + /// # use rust_hdf5::H5File; + /// let file = H5File::open_rw("meta.h5").unwrap(); + /// let ds = file.dataset_writer("notes").unwrap(); + /// ds.write_vlen_strings_slice(42, &["replacement"]).unwrap(); + /// file.close().unwrap(); + /// ``` + pub fn write_vlen_strings_slice(&self, start: usize, strings: &[&str]) -> Result<()> { + match &self.info { + DatasetInfo::Writer { index, .. } => { + let inner = borrow_inner(&self.file_inner); + match &*inner { + H5FileInner::Writer(writer) => { + writer.write_vlen_strings_slice(*index, start as u64, strings)?; + Ok(()) + } + _ => Err(Hdf5Error::InvalidState( + "file is no longer in write mode".into(), + )), + } + } + DatasetInfo::Reader { .. } => { + Err(Hdf5Error::InvalidState("cannot write in read mode".into())) + } + } + } + /// Read variable-length strings from a dataset. /// /// This handles h5py-style vlen string datasets that store strings @@ -4889,4 +4930,202 @@ mod tests { assert!(err.contains("only for string datasets"), "got: {err}"); std::fs::remove_file(&path).ok(); } + + // ---- issue #6: random updates to vlen string datasets ------------------ + + /// One element changes; the extent and every other element stay as they + /// were, on a contiguous vlen dataset. + #[test] + fn write_vlen_strings_slice_replaces_one_element() { + let path = temp_path("vlen_slice_contig"); + { + let file = H5File::create(&path).unwrap(); + file.write_vlen_strings("notes", &["a", "b", "c", "d"]) + .unwrap(); + file.close().unwrap(); + } + { + let file = H5File::open_rw(&path).unwrap(); + file.dataset_writer("notes") + .unwrap() + .write_vlen_strings_slice(1, &["replacement"]) + .unwrap(); + file.close().unwrap(); + } + let file = H5File::open(&path).unwrap(); + let ds = file.dataset("notes").unwrap(); + assert_eq!(ds.shape(), vec![4]); + assert_eq!( + ds.read_vlen_strings().unwrap(), + vec!["a", "replacement", "c", "d"] + ); + std::fs::remove_file(&path).ok(); + } + + /// The same on an appendable chunked dataset, across a reopen, over a range + /// that spans a chunk boundary. + #[test] + fn write_vlen_strings_slice_spans_chunks_after_reopen() { + let path = temp_path("vlen_slice_chunked"); + { + let file = H5File::create(&path).unwrap(); + file.create_appendable_vlen_dataset("notes", 2, None) + .unwrap(); + let all: Vec = (0..6).map(|i| format!("v{i}")).collect(); + let refs: Vec<&str> = all.iter().map(|s| s.as_str()).collect(); + file.append_vlen_strings("notes", &refs).unwrap(); + file.close().unwrap(); + } + { + // Elements 1..4 cross the 2-element chunk boundary twice. + let file = H5File::open_rw(&path).unwrap(); + file.dataset_writer("notes") + .unwrap() + .write_vlen_strings_slice(1, &["x", "y", "z"]) + .unwrap(); + file.close().unwrap(); + } + let file = H5File::open(&path).unwrap(); + let ds = file.dataset("notes").unwrap(); + assert_eq!(ds.shape(), vec![6]); + assert_eq!( + ds.read_vlen_strings().unwrap(), + vec!["v0", "x", "y", "z", "v4", "v5"] + ); + 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. + #[test] + fn write_vlen_strings_slice_reaches_the_append_buffer() { + let path = temp_path("vlen_slice_buffered"); + { + let file = H5File::create(&path).unwrap(); + file.create_appendable_vlen_dataset("notes", 4, None) + .unwrap(); + // 3 of a 4-element chunk: all three stay in the append buffer. + file.append_vlen_strings("notes", &["a", "b", "c"]).unwrap(); + file.dataset_writer("notes") + .unwrap() + .write_vlen_strings_slice(1, &["patched"]) + .unwrap(); + file.close().unwrap(); + } + let file = H5File::open(&path).unwrap(); + assert_eq!( + file.dataset("notes").unwrap().read_vlen_strings().unwrap(), + vec!["a", "patched", "c"] + ); + std::fs::remove_file(&path).ok(); + } + + /// A range past the end is rejected before anything is written, and an + /// empty batch costs the file nothing — without the early return it would + /// still allocate and write an empty global-heap collection. + #[test] + fn write_vlen_strings_slice_checks_its_range() { + let build = |name: &str, empty_call: bool| { + let path = temp_path(name); + let file = H5File::create(&path).unwrap(); + file.write_vlen_strings("notes", &["a", "b"]).unwrap(); + let ds = file.dataset_writer("notes").unwrap(); + let err = ds + .write_vlen_strings_slice(1, &["x", "y"]) + .unwrap_err() + .to_string(); + assert!( + err.contains("outside the dataset's 2 elements"), + "got: {err}" + ); + if empty_call { + ds.write_vlen_strings_slice(0, &[]).unwrap(); + } + file.close().unwrap(); + path + }; + + let with_empty = build("vlen_slice_range", true); + let control = build("vlen_slice_range_control", false); + assert_eq!( + std::fs::metadata(&with_empty).unwrap().len(), + std::fs::metadata(&control).unwrap().len(), + "the rejected and empty calls must leave the file untouched" + ); + + let file = H5File::open(&with_empty).unwrap(); + assert_eq!( + file.dataset("notes").unwrap().read_vlen_strings().unwrap(), + vec!["a", "b"] + ); + std::fs::remove_file(&with_empty).ok(); + std::fs::remove_file(&control).ok(); + } + + /// The element offset is one-dimensional, so a multi-dimensional dataset is + /// rejected rather than silently indexed along the first axis. + #[test] + fn write_vlen_strings_slice_rejects_a_multidimensional_dataset() { + let path = temp_path("vlen_slice_2d"); + let file = H5File::create(&path).unwrap(); + let ds = file + .new_dataset::() + .datatype(DatatypeMessage::vlen_string_utf8()) + .shape([2, 3]) + .create("grid") + .unwrap(); + let err = ds + .write_vlen_strings_slice(0, &["x"]) + .unwrap_err() + .to_string(); + assert!(err.contains("1-dimension datasets"), "got: {err}"); + file.close().unwrap(); + std::fs::remove_file(&path).ok(); + } + + /// A `&str` is UTF-8, so writing a non-ASCII one into a dataset that + /// declares the ASCII character set would mislabel the bytes. + #[test] + fn write_vlen_strings_slice_enforces_the_ascii_character_set() { + let path = temp_path("vlen_slice_ascii"); + let file = H5File::create(&path).unwrap(); + let ds = file + .new_dataset::() + .datatype(DatatypeMessage::vlen_string_ascii()) + .shape([3]) + .create("notes") + .unwrap(); + let err = ds + .write_vlen_strings_slice(0, &["ok", "안녕"]) + .unwrap_err() + .to_string(); + assert!( + err.contains("string 1") && err.contains("is not ASCII"), + "got: {err}" + ); + ds.write_vlen_strings_slice(0, &["ok", "fine"]).unwrap(); + file.close().unwrap(); + std::fs::remove_file(&path).ok(); + } + + /// A numeric dataset is rejected: its elements are not vlen references and + /// writing one would corrupt the column. + #[test] + fn write_vlen_strings_slice_rejects_a_non_vlen_dataset() { + let path = temp_path("vlen_slice_numeric"); + let file = H5File::create(&path).unwrap(); + let ds = file.new_dataset::().shape([3]).create("nums").unwrap(); + ds.write_raw(&[1i32, 2, 3]).unwrap(); + let err = ds + .write_vlen_strings_slice(0, &["x"]) + .unwrap_err() + .to_string(); + assert!( + err.contains("only for variable-length string datasets"), + "got: {err}" + ); + file.close().unwrap(); + std::fs::remove_file(&path).ok(); + } } diff --git a/src/io/writer.rs b/src/io/writer.rs index de5b34f..f3c4014 100644 --- a/src/io/writer.rs +++ b/src/io/writer.rs @@ -2739,6 +2739,135 @@ impl Hdf5Writer { Ok(()) } + /// Replace elements `start .. start + strings.len()` of a 1-D + /// variable-length string dataset, leaving its extent and every other + /// element alone. + /// + /// The replacements go into a fresh global heap collection and only the + /// vlen references of the named elements are rewritten, so the cost is the + /// new strings plus the chunks those references live in — not the column. + /// The objects the old references pointed at stay in their collections, + /// unreachable: the global heap has no free path here, the same as for a + /// chunk a rewrite relocates under SWMR. + /// + /// 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. + pub fn write_vlen_strings_slice( + &self, + ds_index: usize, + start: u64, + strings: &[&str], + ) -> IoResult<()> { + use crate::format::global_heap::{ + encode_vlen_reference, vlen_reference_size, GlobalHeapCollection, + }; + use crate::format::messages::datatype::DatatypeMessage; + + // An empty batch must not reach the heap write below: an empty + // collection still encodes to the 4096-byte `H5HG_MINALLOC` minimum, + // so the file would grow by a block nothing references. + if strings.is_empty() { + return Ok(()); + } + + // Snapshot what the write needs, then drop the guard: `write_slice` + // below re-locks the same slot. + let (charset, dims, buffered_frames) = { + let ds = self.ds(ds_index); + let m = ds.lock(); + let charset = match m.datatype { + DatatypeMessage::VarLenString { charset } => charset, + _ => { + return Err(crate::io::IoError::InvalidState( + "write_vlen_strings_slice is only for variable-length string datasets" + .into(), + )) + } + }; + ( + charset, + m.dataspace.dims.clone(), + m.append_buffered_frames as usize, + ) + }; + + if dims.len() != 1 { + return Err(crate::io::IoError::InvalidState(format!( + "write_vlen_strings_slice is only for 1-dimension datasets, this one has {}", + dims.len() + ))); + } + let end = start + strings.len() as u64; + if end > dims[0] { + return Err(crate::io::IoError::InvalidState(format!( + "elements {start}..{end} are outside the dataset's {} elements", + 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" + ))); + } + } + + // One collection for the batch, as `append_vlen_strings` does. + let mut gcol = GlobalHeapCollection::new(); + let mut obj_indices = Vec::with_capacity(strings.len()); + for s in strings { + obj_indices.push(gcol.add_object(s.as_bytes().to_vec())?); + } + let gcol_encoded = gcol.encode(&self.ctx); + let gcol_addr = self.allocator.allocate(gcol_encoded.len() as u64); + self.handle.write_at(gcol_addr, &gcol_encoded)?; + + let ref_size = vlen_reference_size(&self.ctx); + let mut refs = Vec::with_capacity(strings.len() * ref_size); + for (i, &obj_idx) in obj_indices.iter().enumerate() { + refs.extend_from_slice(&encode_vlen_reference( + strings[i].len() as u32, + gcol_addr, + obj_idx as u32, + &self.ctx, + )); + } + + // The append buffer holds the tail of the dataset, so the range splits + // into an on-disk prefix and a buffered suffix. + let buffer_base = dims[0] - buffered_frames as u64; + let split = end.min(buffer_base).max(start); + + 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 { + let ds = self.ds(ds_index); + let mut m = ds.lock(); + let off = ((split - buffer_base) as usize) * ref_size; + let tail = &refs[(split - start) as usize * ref_size..]; + // `append_buffer.len() == append_buffered_frames * ref_size` holds + // for a 1-D vlen dataset, so the range is in bounds for the + // snapshot taken above. It can still be stale under the + // `thread-safe` feature, where another thread may have flushed the + // buffer between the two lock acquisitions; report that rather than + // panicking on the slice. + 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() + ))); + } + m.append_buffer[off..off + tail.len()].copy_from_slice(tail); + } + + Ok(()) + } + /// Add an attribute to a dataset. /// /// The attribute will be written as a message in the dataset's object From e7c657569c4b7941fbe4da8ac53b918a505d04fd Mon Sep 17 00:00:00 2001 From: Sang Woo Kim Date: Thu, 13 Aug 2026 08:39:27 +0900 Subject: [PATCH 3/9] writer: free the heap object a replaced vlen element gave up MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The docs claimed libhdf5 leaves it too; it does not — H5T__vlen_disk_write deletes the reference it read into the conversion background buffer before storing the new one. Each update here stranded a 4096-byte collection (H5HG_MINALLOC), so a column updated in a loop grew without bound. Superseded references are now read first, from the append buffer when the element has not been flushed, and their objects removed. The collection is rewritten at its existing size so allocate and free stay symmetric; SWMR suppresses the release, as for a relocated chunk. Prompted by @janosh's ask in #6 to document the superseded-object behavior. --- CHANGELOG.md | 14 ++- src/dataset.rs | 171 +++++++++++++++++++++++++++++++++++- src/format/global_heap.rs | 177 ++++++++++++++++++++++++++++++++++---- src/io/writer.rs | 165 +++++++++++++++++++++++++++++++++-- 4 files changed, 495 insertions(+), 32 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e467984..f6964e3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,8 +20,18 @@ strings go into one global-heap collection and their references are written over the range; elements still held in the append buffer are patched there, so the flush at close does not write the pre-update reference back over - them. The heap objects the replaced references pointed at stay in the file - as unreachable bytes — libhdf5 does not reclaim them either. + them. + +- Superseded global heap objects are now freed. Overwriting a vlen element + reads the reference it replaces first and removes the object it names, as + libhdf5 does (`H5T__vlen_disk_write` → `H5T__vlen_disk_delete` → + `H5HG_remove`): the collection is rewritten at its existing size with the + recovered bytes given to its free-space marker, and one left with no objects + returns its block to the allocator. Without this, each update stranded a + 4096-byte collection (`H5HG_MINALLOC`), so a column updated in a loop grew + the file without bound. Under SWMR the release is suppressed, since a reader + may still be following those references — the same rule a relocated chunk's + old block follows. ## 0.4.0 diff --git a/src/dataset.rs b/src/dataset.rs index d56dc13..e8a190c 100644 --- a/src/dataset.rs +++ b/src/dataset.rs @@ -1919,10 +1919,12 @@ impl H5Dataset { /// replacement in a dataset that declares ASCII is rejected rather than /// stored under a datatype that misdescribes it. /// - /// The objects the replaced references pointed at stay in their global - /// heap collections, unreachable. Nothing in this library frees global - /// heap space, so a column updated many times grows the file; rewriting - /// the dataset is what reclaims it. + /// The global heap objects the replaced references pointed at are freed — + /// the same reclaim libhdf5 performs on an overwrite — so updating one + /// element repeatedly reuses space rather than growing the file. A + /// collection emptied by the update returns its block to the allocator. + /// Under SWMR nothing is freed, because a reader may still be following + /// those references. /// /// ```no_run /// # use rust_hdf5::H5File; @@ -5128,4 +5130,165 @@ mod tests { file.close().unwrap(); std::fs::remove_file(&path).ok(); } + + // ---- superseded global heap objects (libhdf5 H5HG_remove parity) ------- + + /// Repeatedly replacing the same element must not grow the file per + /// update: the collection each update supersedes is freed and the next + /// update's collection lands in that block. Without the release every + /// update costs another `H5HG_MINALLOC` (4096) bytes. + #[test] + fn write_vlen_strings_slice_reuses_the_freed_heap_block() { + let size_after = |updates: usize| { + let path = temp_path(&format!("vlen_slice_heap_reuse_{updates}")); + let file = H5File::create(&path).unwrap(); + file.write_vlen_strings("notes", &["a", "b"]).unwrap(); + let ds = file.dataset_writer("notes").unwrap(); + for i in 0..updates { + ds.write_vlen_strings_slice(0, &[&format!("update {i}")]) + .unwrap(); + } + file.close().unwrap(); + let n = std::fs::metadata(&path).unwrap().len(); + let read = H5File::open(&path).unwrap(); + assert_eq!( + read.dataset("notes").unwrap().read_vlen_strings().unwrap(), + vec![format!("update {}", updates - 1), "b".to_string()] + ); + drop(read); + std::fs::remove_file(&path).ok(); + n + }; + + // The allocator settles once a freed block is available to reuse, so + // every count past that produces the same file. + let settled = size_after(3); + assert_eq!(size_after(20), settled, "20 updates against 3"); + assert_eq!(size_after(50), settled, "50 updates against 3"); + } + + /// The elements the update does not name keep their strings, so freeing + /// the superseded objects must not disturb the collection's survivors. + #[test] + fn write_vlen_strings_slice_keeps_the_untouched_strings_readable() { + let path = temp_path("vlen_slice_heap_survivors"); + { + let file = H5File::create(&path).unwrap(); + file.write_vlen_strings("notes", &["a", "b", "c", "d"]) + .unwrap(); + let ds = file.dataset_writer("notes").unwrap(); + // Two updates inside the one collection the create wrote, so the + // second reads a collection the first already rewrote. + ds.write_vlen_strings_slice(1, &["B"]).unwrap(); + ds.write_vlen_strings_slice(3, &["D"]).unwrap(); + file.close().unwrap(); + } + let file = H5File::open(&path).unwrap(); + assert_eq!( + file.dataset("notes").unwrap().read_vlen_strings().unwrap(), + vec!["a", "B", "c", "D"] + ); + std::fs::remove_file(&path).ok(); + } + + /// Replacing every element of a chunked dataset empties the collection the + /// append wrote, and the file must still read back correctly after its + /// block goes to the allocator. + #[test] + fn write_vlen_strings_slice_frees_an_emptied_collection() { + let path = temp_path("vlen_slice_heap_emptied"); + { + let file = H5File::create(&path).unwrap(); + file.create_appendable_vlen_dataset("notes", 2, None) + .unwrap(); + file.append_vlen_strings("notes", &["p", "q", "r", "s"]) + .unwrap(); + file.close().unwrap(); + } + { + let file = H5File::open_rw(&path).unwrap(); + file.dataset_writer("notes") + .unwrap() + .write_vlen_strings_slice(0, &["w", "x", "y", "z"]) + .unwrap(); + file.close().unwrap(); + } + let file = H5File::open(&path).unwrap(); + assert_eq!( + file.dataset("notes").unwrap().read_vlen_strings().unwrap(), + vec!["w", "x", "y", "z"] + ); + std::fs::remove_file(&path).ok(); + } + + /// A collection larger than the 4096-byte minimum must keep its size when + /// an object leaves it. Re-encoding at the natural size instead shrinks + /// what the header declares, so the block's tail stops being part of the + /// collection and the eventual free returns less than was allocated — + /// stranding the difference on every cycle. + #[test] + fn write_vlen_strings_slice_keeps_an_oversized_collections_block_whole() { + let big = |tag: char| std::iter::repeat_n(tag, 2000).collect::(); + let size_after = |cycles: usize| { + let path = temp_path(&format!("vlen_slice_heap_big_{cycles}")); + let file = H5File::create(&path).unwrap(); + let seed: Vec = "abcd".chars().map(big).collect(); + let refs: Vec<&str> = seed.iter().map(|s| s.as_str()).collect(); + // Four 2000-byte strings do not fit the 4096-byte minimum, so this + // is one collection well above it. + file.write_vlen_strings("notes", &refs).unwrap(); + let ds = file.dataset_writer("notes").unwrap(); + for _ in 0..cycles { + // Partially empty the collection, then finish it off: the + // block is freed only after it has been rewritten once. + let head = big('x'); + ds.write_vlen_strings_slice(0, &[&head]).unwrap(); + let tail: Vec = "yzw".chars().map(big).collect(); + let tail_refs: Vec<&str> = tail.iter().map(|s| s.as_str()).collect(); + ds.write_vlen_strings_slice(1, &tail_refs).unwrap(); + } + file.close().unwrap(); + let n = std::fs::metadata(&path).unwrap().len(); + let read = H5File::open(&path).unwrap(); + assert_eq!( + read.dataset("notes").unwrap().read_vlen_strings().unwrap(), + vec![big('x'), big('y'), big('z'), big('w')] + ); + drop(read); + std::fs::remove_file(&path).ok(); + n + }; + + let settled = size_after(4); + assert_eq!(size_after(30), settled, "30 cycles against 4"); + } + + /// An element still in the append buffer has never been on disk, so its + /// superseded object has to be found in the buffer or it is stranded. + #[test] + fn write_vlen_strings_slice_releases_a_buffered_elements_object() { + let path = temp_path("vlen_slice_heap_buffered"); + let file = H5File::create(&path).unwrap(); + file.create_appendable_vlen_dataset("notes", 4, None) + .unwrap(); + file.append_vlen_strings("notes", &["a", "b", "c"]).unwrap(); + let ds = file.dataset_writer("notes").unwrap(); + for i in 0..20 { + ds.write_vlen_strings_slice(1, &[&format!("patch {i}")]) + .unwrap(); + } + file.close().unwrap(); + + let file = H5File::open(&path).unwrap(); + assert_eq!( + file.dataset("notes").unwrap().read_vlen_strings().unwrap(), + vec!["a", "patch 19", "c"] + ); + let size = std::fs::metadata(&path).unwrap().len(); + std::fs::remove_file(&path).ok(); + assert!( + size < 20 * 4096, + "20 buffered updates left {size} bytes, one collection per update" + ); + } } diff --git a/src/format/global_heap.rs b/src/format/global_heap.rs index 65bfd88..f2a1aee 100644 --- a/src/format/global_heap.rs +++ b/src/format/global_heap.rs @@ -83,6 +83,33 @@ impl GlobalHeapCollection { .map(|o| o.data.as_slice()) } + /// Drop the object with this 1-based index, reporting whether it was + /// there. + /// + /// The surviving objects keep their indices, so the vlen references that + /// name them stay valid; only their offsets within the collection move. + /// This is what libhdf5's `H5HG_remove` does — it compacts the objects and + /// gives the recovered bytes to the free-space marker — and re-encoding at + /// the collection's existing size ([`encode_at_size`](Self::encode_at_size)) + /// reproduces that layout. Removing an index that is already gone is not + /// an error: two elements of one dataset may name the same object, so a + /// range update can reach it twice (libhdf5 tolerates the same case, see + /// its HDFFV-10635 note). + pub fn remove_object(&mut self, index: u16) -> bool { + match self.objects.iter().position(|o| o.index == index) { + Some(at) => { + self.objects.remove(at); + true + } + None => false, + } + } + + /// True when the collection holds no objects, so its block is free. + pub fn is_empty(&self) -> bool { + self.objects.is_empty() + } + /// Encode the collection into a byte vector. /// /// The encoded blob includes the GCOL header and all heap objects, @@ -90,24 +117,41 @@ impl GlobalHeapCollection { /// The total size is padded to at least 4096 bytes (H5HG_MINALLOC) /// for compatibility with the HDF5 C library. pub fn encode(&self, ctx: &FormatContext) -> Vec { - let ss = ctx.sizeof_size as usize; + // Unwrap: the size asked for is the one `encode_at_size` computes as + // the minimum, so it always fits. + self.encode_at_size(ctx, self.encoded_size(ctx)).unwrap() + } - // libhdf5 (H5HGpkg.h) 8-byte-aligns both the collection header and - // every object header (H5HG_ALIGN). For ss == 8 the raw sizes are - // already multiples of 8, so this is a no-op there and only matters - // for files with 4-byte lengths. - let header_size = pad_to_8(4 + 1 + 3 + ss); // GCOL + version + reserved + collection_size - let objhdr_size = pad_to_8(2 + 2 + 4 + ss); // index + ref_count + reserved + size - let mut objects_size: usize = 0; - for obj in &self.objects { - objects_size += objhdr_size + pad_to_8(obj.data.len()); - } - // Free-space marker carries the same (aligned) object header. - let free_marker_size = objhdr_size; - let content_size = header_size + objects_size + free_marker_size; + /// The smallest collection size that holds these objects — what + /// [`encode`](Self::encode) uses. + pub fn encoded_size(&self, ctx: &FormatContext) -> usize { + let (header_size, objhdr_size, objects_size) = self.layout(ctx); + (header_size + objects_size + objhdr_size).max(GCOL_MIN_SIZE) + } - // HDF5 C library requires collection_size >= 4096 (H5HG_MINALLOC) - let collection_size = content_size.max(GCOL_MIN_SIZE); + /// Encode the collection into a block of exactly `collection_size` bytes, + /// with everything not used by an object given to the free-space marker. + /// + /// A collection is rewritten in place after [`remove_object`](Self::remove_object), + /// and its block does not shrink — libhdf5's `H5HG_remove` keeps the + /// collection's size and grows its free space instead, and shortening the + /// image would leave the tail of the previous, longer one on disk. Fails + /// if the objects do not fit in `collection_size`. + pub fn encode_at_size( + &self, + ctx: &FormatContext, + collection_size: usize, + ) -> FormatResult> { + let ss = ctx.sizeof_size as usize; + let (header_size, objhdr_size, objects_size) = self.layout(ctx); + + // The free-space marker's own header has to fit after the objects. + let content_size = header_size + objects_size + objhdr_size; + if collection_size < content_size { + return Err(FormatError::InvalidData(format!( + "global heap collection needs {content_size} bytes but was given {collection_size}" + ))); + } // HDF5 convention: free marker size = collection_size - header - objects // (includes the free marker's own header in the "free space") let free_space = collection_size - header_size - objects_size; @@ -143,7 +187,49 @@ impl GlobalHeapCollection { buf.resize(collection_size, 0); debug_assert_eq!(buf.len(), collection_size); - buf + Ok(buf) + } + + /// Aligned header size, aligned object-header size, and the total the + /// objects occupy — the three numbers every size calculation here needs. + /// + /// libhdf5 (H5HGpkg.h) 8-byte-aligns both the collection header and every + /// object header (`H5HG_ALIGN`). For `ss == 8` the raw sizes are already + /// multiples of 8, so the alignment is a no-op there and only matters for + /// files with 4-byte lengths. + fn layout(&self, ctx: &FormatContext) -> (usize, usize, usize) { + let ss = ctx.sizeof_size as usize; + let header_size = pad_to_8(4 + 1 + 3 + ss); // GCOL + version + reserved + collection_size + let objhdr_size = pad_to_8(2 + 2 + 4 + ss); // index + ref_count + reserved + size + let objects_size = self + .objects + .iter() + .map(|obj| objhdr_size + pad_to_8(obj.data.len())) + .sum(); + (header_size, objhdr_size, objects_size) + } + + /// Read just the collection's declared total size from its header. + /// + /// [`decode`](Self::decode) needs the whole collection in `buf`, and the + /// size that says how much to read is in the header — so a caller reading + /// from a file asks here first, with only the header in hand. + pub fn decode_size(buf: &[u8], ctx: &FormatContext) -> FormatResult { + let ss = ctx.sizeof_size as usize; + let header_size = pad_to_8(4 + 1 + 3 + ss); + if buf.len() < header_size { + return Err(FormatError::BufferTooShort { + needed: header_size, + available: buf.len(), + }); + } + if buf[0..4] != GCOL_SIGNATURE { + return Err(FormatError::InvalidSignature); + } + if buf[4] != GCOL_VERSION { + return Err(FormatError::InvalidVersion(buf[4])); + } + Ok(read_size(&buf[8..], ss) as usize) } /// Decode a global heap collection from a byte buffer. @@ -459,4 +545,61 @@ mod tests { let (decoded, _) = GlobalHeapCollection::decode(&encoded, &ctx()).unwrap(); assert_eq!(decoded.get_object(1), Some([].as_slice())); } + + /// Removing an object keeps the other indices valid, so the vlen + /// references that name them do not have to be rewritten. + #[test] + fn remove_object_keeps_the_survivors_indices() { + let mut coll = GlobalHeapCollection::new(); + assert_eq!(coll.add_object(b"first".to_vec()).unwrap(), 1); + assert_eq!(coll.add_object(b"second".to_vec()).unwrap(), 2); + assert_eq!(coll.add_object(b"third".to_vec()).unwrap(), 3); + + assert!(coll.remove_object(2)); + // Already gone: two elements of one dataset may name the same object. + assert!(!coll.remove_object(2)); + + let encoded = coll.encode(&ctx()); + let (decoded, _) = GlobalHeapCollection::decode(&encoded, &ctx()).unwrap(); + assert_eq!(decoded.get_object(1), Some(b"first".as_slice())); + assert_eq!(decoded.get_object(2), None); + assert_eq!(decoded.get_object(3), Some(b"third".as_slice())); + + assert!(coll.remove_object(1) && coll.remove_object(3)); + assert!(coll.is_empty()); + } + + /// A collection above the 4096-byte minimum keeps its size when an object + /// leaves it, the way libhdf5's `H5HG_remove` gives the recovered bytes to + /// the free-space marker rather than shortening the collection. Re-encoding + /// at the natural size would declare a smaller collection than the block + /// the file allocated, so a later free would return less than was taken. + #[test] + fn encode_at_size_holds_an_oversized_collection_open() { + let mut coll = GlobalHeapCollection::new(); + for _ in 0..4 { + coll.add_object(vec![0xab; 2000]).unwrap(); + } + let block = coll.encode(&ctx()).len(); + assert!(block > GCOL_MIN_SIZE, "{block} is not above the minimum"); + + coll.remove_object(2); + assert!( + coll.encoded_size(&ctx()) < block, + "the test needs the natural size to have shrunk" + ); + + let held = coll.encode_at_size(&ctx(), block).unwrap(); + assert_eq!(held.len(), block); + assert_eq!( + GlobalHeapCollection::decode(&held, &ctx()).unwrap().1, + block, + "the collection must still declare the whole block" + ); + let (decoded, _) = GlobalHeapCollection::decode(&held, &ctx()).unwrap(); + assert_eq!(decoded.get_object(3), Some(vec![0xab; 2000].as_slice())); + + // Asking for less than the objects need is refused, not truncated. + assert!(coll.encode_at_size(&ctx(), 64).is_err()); + } } diff --git a/src/io/writer.rs b/src/io/writer.rs index f3c4014..f50a6c6 100644 --- a/src/io/writer.rs +++ b/src/io/writer.rs @@ -2746,9 +2746,10 @@ impl Hdf5Writer { /// The replacements go into a fresh global heap collection and only the /// vlen references of the named elements are rewritten, so the cost is the /// new strings plus the chunks those references live in — not the column. - /// The objects the old references pointed at stay in their collections, - /// unreachable: the global heap has no free path here, the same as for a - /// chunk a rewrite relocates under SWMR. + /// The objects the old references pointed at are freed, so repeated + /// updates reuse space instead of growing the file. This is what libhdf5 + /// 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 @@ -2821,11 +2822,34 @@ impl Hdf5Writer { for s in strings { obj_indices.push(gcol.add_object(s.as_bytes().to_vec())?); } + 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. + let buffer_base = dims[0] - buffered_frames as u64; + let split = end.min(buffer_base).max(start); + + // The 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, so its current references come from the buffer. + let mut superseded = + self.current_element_bytes(ds_index, start, split - start, ref_size)?; + if end > split { + let ds = self.ds(ds_index); + let m = ds.lock(); + let off = ((split - buffer_base) as usize) * ref_size; + let len = ((end - split) as usize) * ref_size; + superseded + .extend_from_slice(&m.append_buffer[off..(off + len).min(m.append_buffer.len())]); + } + let gcol_encoded = gcol.encode(&self.ctx); let gcol_addr = self.allocator.allocate(gcol_encoded.len() as u64); self.handle.write_at(gcol_addr, &gcol_encoded)?; - let ref_size = vlen_reference_size(&self.ctx); let mut refs = Vec::with_capacity(strings.len() * ref_size); for (i, &obj_idx) in obj_indices.iter().enumerate() { refs.extend_from_slice(&encode_vlen_reference( @@ -2836,11 +2860,6 @@ impl Hdf5Writer { )); } - // The append buffer holds the tail of the dataset, so the range splits - // into an on-disk prefix and a buffered suffix. - let buffer_base = dims[0] - buffered_frames as u64; - let split = end.min(buffer_base).max(start); - if split > start { let n = (split - start) as usize; self.write_slice(ds_index, &[start], &[n as u64], &refs[..n * ref_size])?; @@ -2865,6 +2884,134 @@ impl Hdf5Writer { m.append_buffer[off..off + tail.len()].copy_from_slice(tail); } + // Only now that the new references are in place: a failure above must + // not leave the file naming objects this already freed. + self.release_vlen_references(&superseded)?; + + Ok(()) + } + + /// The bytes elements `start .. start + count` of a 1-D dataset currently + /// hold, whichever layout stores them. + /// + /// Elements no write has reached yet read as zeros — for a vlen dataset + /// that is the nil reference, which names no heap object. + fn current_element_bytes( + &self, + ds_index: usize, + start: u64, + count: u64, + element_size: usize, + ) -> IoResult> { + let mut out = vec![0u8; count as usize * element_size]; + if count == 0 { + return Ok(out); + } + + let (is_chunked, data_addr) = { + let ds = self.ds(ds_index); + let m = ds.lock(); + ( + m.chunked.is_some() || m.fixed_array.is_some() || m.btree_v2.is_some(), + m.data_addr, + ) + }; + + if !is_chunked { + if data_addr != UNDEF_ADDR { + // `read_at_most`, not `read_at`: a contiguous dataset's block is + // reserved when it is created, so the file can still be shorter + // than the block until something writes it. What is missing has + // never been written, which is the zeros above. + let at = data_addr + start * element_size as u64; + let got = self.handle.read_at_most(at, out.len())?; + out[..got.len()].copy_from_slice(&got); + } + return Ok(out); + } + + let geo = self.chunk_geometry(ds_index)?; + let per_chunk = geo.chunk_dims[0]; + let end = start + count; + for c in (start / per_chunk)..=((end - 1) / per_chunk) { + let origin = c * per_chunk; + let lo = start.max(origin); + let hi = end.min(origin + per_chunk); + // A chunk with no block yet leaves this span as the zeros above. + let Some(chunk) = self.read_chunk_at_coords(ds_index, &[c])? else { + continue; + }; + let src = ((lo - origin) as usize) * element_size; + let dst = ((lo - start) as usize) * element_size; + let len = ((hi - lo) as usize) * element_size; + if src + len > chunk.len() { + return Err(crate::io::IoError::InvalidState(format!( + "chunk {c} is {} bytes, too short for elements {lo}..{hi}", + chunk.len() + ))); + } + out[dst..dst + len].copy_from_slice(&chunk[src..src + len]); + } + Ok(out) + } + + /// Free the global heap objects `refs` names, so replacing a vlen element + /// does not strand what it used to point at. + /// + /// This is libhdf5's `H5HG_remove` reached through `H5T__vlen_disk_delete`: + /// the object leaves its collection, the collection is rewritten at its + /// 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`) and a zero-length sequence + /// name no object, which is the `seq_len > 0` test libhdf5 makes. + /// + /// Under SWMR nothing is freed and no collection is rewritten: a reader may + /// be following those references, the same reason `place_chunk` keeps a + /// relocated chunk's old block. + fn release_vlen_references(&self, refs: &[u8]) -> IoResult<()> { + use crate::format::global_heap::{ + decode_vlen_reference, vlen_reference_size, GlobalHeapCollection, + }; + + if self.swmr_active { + return Ok(()); + } + let ref_size = vlen_reference_size(&self.ctx); + if ref_size == 0 || refs.len() < ref_size { + return Ok(()); + } + + // Group by collection so one holding several replaced objects is read, + // rewritten and judged empty exactly once. + let mut per_collection: std::collections::BTreeMap> = Default::default(); + for r in refs.chunks_exact(ref_size) { + let (seq_len, addr, obj_idx) = decode_vlen_reference(r, &self.ctx)?; + if seq_len == 0 || addr == 0 || addr == UNDEF_ADDR { + continue; + } + let Ok(idx) = u16::try_from(obj_idx) else { + return Err(crate::io::IoError::InvalidState(format!( + "global heap object index {obj_idx} does not fit the 16-bit on-disk field" + ))); + }; + per_collection.entry(addr).or_default().push(idx); + } + + 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)?; + for idx in indices { + gcol.remove_object(idx); + } + if gcol.is_empty() { + self.allocator.free(addr, declared as u64); + } else { + let rewritten = gcol.encode_at_size(&self.ctx, declared)?; + self.handle.write_at(addr, &rewritten)?; + } + } Ok(()) } From ebd1914884e36e43f40d7ec71b656dfb8af1407c Mon Sep 17 00:00:00 2001 From: Sang Woo Kim Date: Thu, 13 Aug 2026 09:48:51 +0900 Subject: [PATCH 4/9] writer: release the heap object behind an empty-string reference MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit release_vlen_references treated seq_len == 0 as nil, but this crate's writers store even the empty string as a real heap object, so superseding one stranded its collection forever — 4096 bytes per update. Whether a reference names an object is the address's call. --- src/dataset.rs | 30 ++++++++++++++++++++++++++++++ src/io/writer.rs | 10 ++++++---- 2 files changed, 36 insertions(+), 4 deletions(-) diff --git a/src/dataset.rs b/src/dataset.rs index e8a190c..6e0dddd 100644 --- a/src/dataset.rs +++ b/src/dataset.rs @@ -5167,6 +5167,36 @@ mod tests { assert_eq!(size_after(50), settled, "50 updates against 3"); } + /// An empty string is stored as a real heap object under a reference whose + /// sequence length is zero, so the release must go by the address, not the + /// length — a length test strands the object and its collection forever. + #[test] + fn write_vlen_strings_slice_frees_an_empty_strings_object() { + let size_after = |updates: usize| { + let path = temp_path(&format!("vlen_slice_empty_reuse_{updates}")); + let file = H5File::create(&path).unwrap(); + file.write_vlen_strings("notes", &["", "b"]).unwrap(); + let ds = file.dataset_writer("notes").unwrap(); + for _ in 0..updates { + ds.write_vlen_strings_slice(0, &[""]).unwrap(); + } + file.close().unwrap(); + let n = std::fs::metadata(&path).unwrap().len(); + let read = H5File::open(&path).unwrap(); + assert_eq!( + read.dataset("notes").unwrap().read_vlen_strings().unwrap(), + vec!["".to_string(), "b".to_string()] + ); + drop(read); + std::fs::remove_file(&path).ok(); + n + }; + + let settled = size_after(3); + assert_eq!(size_after(20), settled, "20 empty updates against 3"); + assert_eq!(size_after(50), settled, "50 empty updates against 3"); + } + /// The elements the update does not name keep their strings, so freeing /// the superseded objects must not disturb the collection's survivors. #[test] diff --git a/src/io/writer.rs b/src/io/writer.rs index f50a6c6..f4b439e 100644 --- a/src/io/writer.rs +++ b/src/io/writer.rs @@ -2962,8 +2962,10 @@ impl Hdf5Writer { /// the object leaves its collection, the collection is rewritten at its /// 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`) and a zero-length sequence - /// name no object, which is the `seq_len > 0` test libhdf5 makes. + /// 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. /// /// Under SWMR nothing is freed and no collection is rewritten: a reader may /// be following those references, the same reason `place_chunk` keeps a @@ -2985,8 +2987,8 @@ impl Hdf5Writer { // rewritten and judged empty exactly once. let mut per_collection: std::collections::BTreeMap> = Default::default(); for r in refs.chunks_exact(ref_size) { - let (seq_len, addr, obj_idx) = decode_vlen_reference(r, &self.ctx)?; - if seq_len == 0 || addr == 0 || addr == UNDEF_ADDR { + let (_seq_len, addr, obj_idx) = decode_vlen_reference(r, &self.ctx)?; + if addr == 0 || addr == UNDEF_ADDR { continue; } let Ok(idx) = u16::try_from(obj_idx) else { From 19c98746885454bb4ce42469dc4434957a90c138 Mon Sep 17 00:00:00 2001 From: Sang Woo Kim Date: Thu, 13 Aug 2026 09:51:02 +0900 Subject: [PATCH 5/9] writer: give the buffered suffix one lock acquisition, checked write_vlen_strings_slice read the superseded buffered references with offsets from an earlier lock acquisition; under threadsafe a flush between the two panicked the slice or silently truncated it. One acquisition now derives the offset, reads and patches, and a moved base or a half-published append count is reported, not applied. --- src/io/writer.rs | 78 ++++++++++++++++++++++++++++++++++-------------- 1 file changed, 56 insertions(+), 22 deletions(-) diff --git a/src/io/writer.rs b/src/io/writer.rs index f4b439e..8f572c7 100644 --- a/src/io/writer.rs +++ b/src/io/writer.rs @@ -2825,26 +2825,27 @@ 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. - let buffer_base = dims[0] - buffered_frames as u64; + // 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); - // The 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, so its current references come from the buffer. + // 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)?; - if end > split { - let ds = self.ds(ds_index); - let m = ds.lock(); - let off = ((split - buffer_base) as usize) * ref_size; - let len = ((end - split) as usize) * ref_size; - superseded - .extend_from_slice(&m.append_buffer[off..(off + len).min(m.append_buffer.len())]); - } let gcol_encoded = gcol.encode(&self.ctx); let gcol_addr = self.allocator.allocate(gcol_encoded.len() as u64); @@ -2865,22 +2866,34 @@ impl Hdf5Writer { 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..]; - // `append_buffer.len() == append_buffered_frames * ref_size` holds - // for a 1-D vlen dataset, so the range is in bounds for the - // snapshot taken above. It can still be stale under the - // `thread-safe` feature, where another thread may have flushed the - // buffer between the two lock acquisitions; report that rather than - // panicking on the slice. 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); } @@ -5319,6 +5332,27 @@ 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(); + } + #[test] fn create_empty_file() { let path = temp_path("empty"); From af1c7690e8cb22110957d5089af2655a3c7cea52 Mon Sep 17 00:00:00 2001 From: Sang Woo Kim Date: Thu, 13 Aug 2026 09:52:14 +0900 Subject: [PATCH 6/9] writer: reject a zero-length chunk dimension in the superseded read current_element_bytes divided by chunk_dims[0] with no guard, so a corrupt file that declares a zero chunk dimension panicked the vlen slice update instead of returning the InvalidState error write_slice already gives the same file. --- src/io/writer.rs | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/src/io/writer.rs b/src/io/writer.rs index 8f572c7..b1929bd 100644 --- a/src/io/writer.rs +++ b/src/io/writer.rs @@ -2945,6 +2945,14 @@ impl Hdf5Writer { let geo = self.chunk_geometry(ds_index)?; let per_chunk = geo.chunk_dims[0]; + // Only a corrupt or crafted file declares a zero-length chunk + // dimension; the divisions below must reject it the way + // `write_slice` does, not panic. + if per_chunk == 0 { + return Err(crate::io::IoError::InvalidState( + "chunk shape has a zero-length dimension".into(), + )); + } let end = start + count; for c in (start / per_chunk)..=((end - 1) / per_chunk) { let origin = c * per_chunk; @@ -5353,6 +5361,30 @@ mod tests { 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. + #[test] + fn vlen_slice_rejects_a_zero_chunk_dimension() { + let path = temp_path("vlen_slice_zero_chunk"); + + let writer = Hdf5Writer::create(&path).unwrap(); + let idx = writer + .create_appendable_vlen_string_dataset("d", 2, None) + .unwrap(); + writer.append_vlen_strings(idx, &["a", "b"]).unwrap(); + writer.ds(idx).lock().chunked.as_mut().unwrap().chunk_dims[0] = 0; + let err = writer.write_vlen_strings_slice(idx, 0, &["x"]).unwrap_err(); + assert!( + err.to_string().contains("zero-length dimension"), + "unexpected error: {err}" + ); + + writer.ds(idx).lock().chunked.as_mut().unwrap().chunk_dims[0] = 2; + writer.close().unwrap(); + std::fs::remove_file(&path).ok(); + } + #[test] fn create_empty_file() { let path = temp_path("empty"); From e1d4be1302be19b6def77ecfdd2f14c8173dd8c4 Mon Sep 17 00:00:00 2001 From: Sang Woo Kim Date: Thu, 13 Aug 2026 09:53:26 +0900 Subject: [PATCH 7/9] writer: refuse a vlen sequence the 32-bit length field cannot say MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every vlen write site cast the byte length with `as u32`, so a 4 GiB string stored all its bytes under a reference whose length wrapped — every read returned the low-32-bits prefix with no error. The new vlen_seq_len owns the conversion and errors instead. --- src/format/global_heap.rs | 22 ++++++++++++++++++++++ src/io/writer.rs | 15 ++++++++------- 2 files changed, 30 insertions(+), 7 deletions(-) diff --git a/src/format/global_heap.rs b/src/format/global_heap.rs index f2a1aee..5da23df 100644 --- a/src/format/global_heap.rs +++ b/src/format/global_heap.rs @@ -369,6 +369,20 @@ pub fn vlen_reference_size(ctx: &FormatContext) -> usize { 4 + ctx.sizeof_addr as usize + 4 } +/// The `u32` sequence length a vlen reference stores, or an error when the +/// data is longer than the on-disk field can say. +/// +/// The single owner of this conversion: a bare `as u32` at a write site +/// silently wraps a 4 GiB sequence to its low 32 bits — the heap object +/// keeps every byte, but each read returns the wrapped length. +pub fn vlen_seq_len(data_len: usize) -> FormatResult { + u32::try_from(data_len).map_err(|_| { + FormatError::InvalidData(format!( + "a {data_len}-byte vlen sequence does not fit the 32-bit length field" + )) + }) +} + /// Round `n` up to the next multiple of 8. fn pad_to_8(n: usize) -> usize { (n + 7) & !7 @@ -474,6 +488,14 @@ mod tests { assert_eq!(idx, 7); } + #[test] + fn vlen_seq_len_bounds() { + assert_eq!(vlen_seq_len(0).unwrap(), 0); + assert_eq!(vlen_seq_len(u32::MAX as usize).unwrap(), u32::MAX); + #[cfg(target_pointer_width = "64")] + assert!(vlen_seq_len(u32::MAX as usize + 1).is_err()); + } + #[test] fn vlen_reference_size_check() { assert_eq!(vlen_reference_size(&ctx()), 16); diff --git a/src/io/writer.rs b/src/io/writer.rs index b1929bd..bbe4f72 100644 --- a/src/io/writer.rs +++ b/src/io/writer.rs @@ -2341,7 +2341,7 @@ impl Hdf5Writer { let data_size = (num_strings as usize) * ref_size; let mut raw_data = Vec::with_capacity(data_size); for (i, &obj_idx) in obj_indices.iter().enumerate() { - let seq_len = strings[i].len() as u32; + let seq_len = crate::format::global_heap::vlen_seq_len(strings[i].len())?; raw_data.extend_from_slice(&encode_vlen_reference( seq_len, gcol_addr, @@ -2415,7 +2415,7 @@ impl Hdf5Writer { let mut raw_data = Vec::with_capacity(data_size); for (i, &obj_idx) in obj_indices.iter().enumerate() { // base is u8, so element count == byte count. - let seq_len = items[i].len() as u32; + let seq_len = crate::format::global_heap::vlen_seq_len(items[i].len())?; raw_data.extend_from_slice(&encode_vlen_reference( seq_len, gcol_addr, @@ -2490,7 +2490,7 @@ impl Hdf5Writer { let data_size = (num_strings as usize) * ref_size; let mut raw_data = Vec::with_capacity(data_size); for (i, &obj_idx) in obj_indices.iter().enumerate() { - let seq_len = strings[i].len() as u32; + let seq_len = crate::format::global_heap::vlen_seq_len(strings[i].len())?; raw_data.extend_from_slice(&encode_vlen_reference( seq_len, gcol_addr, @@ -2665,7 +2665,7 @@ impl Hdf5Writer { let ref_size = crate::format::global_heap::vlen_reference_size(&self.ctx); let mut raw = Vec::with_capacity(strings.len() * ref_size); for (i, &obj_idx) in obj_indices.iter().enumerate() { - let seq_len = strings[i].len() as u32; + let seq_len = crate::format::global_heap::vlen_seq_len(strings[i].len())?; raw.extend_from_slice(&encode_vlen_reference( seq_len, gcol_addr, @@ -2854,7 +2854,7 @@ impl Hdf5Writer { let mut refs = Vec::with_capacity(strings.len() * ref_size); for (i, &obj_idx) in obj_indices.iter().enumerate() { refs.extend_from_slice(&encode_vlen_reference( - strings[i].len() as u32, + crate::format::global_heap::vlen_seq_len(strings[i].len())?, gcol_addr, obj_idx as u32, &self.ctx, @@ -3107,7 +3107,8 @@ impl Hdf5Writer { let gcol_addr = self.allocator.allocate(gcol_encoded.len() as u64); self.handle.write_at(gcol_addr, &gcol_encoded)?; - let data = encode_vlen_reference(value.len() as u32, gcol_addr, obj_idx as u32, &self.ctx); + let seq_len = crate::format::global_heap::vlen_seq_len(value.len())?; + let data = encode_vlen_reference(seq_len, gcol_addr, obj_idx as u32, &self.ctx); Ok(AttributeMessage { name: name.to_string(), datatype: DatatypeMessage::vlen_string_utf8(), @@ -3152,7 +3153,7 @@ impl Hdf5Writer { let mut entries: Vec<(u32, u16)> = Vec::with_capacity(values.len()); for v in values { let obj_idx = gcol.add_object(v.as_bytes().to_vec())?; - entries.push((v.len() as u32, obj_idx)); + entries.push((crate::format::global_heap::vlen_seq_len(v.len())?, obj_idx)); } let gcol_encoded = gcol.encode(&self.ctx); let gcol_addr = self.allocator.allocate(gcol_encoded.len() as u64); From 3a6755080b337ebae8f6c72c0b58dac755aa819a Mon Sep 17 00:00:00 2001 From: Sang Woo Kim Date: Thu, 13 Aug 2026 09:55:06 +0900 Subject: [PATCH 8/9] writer: leave a collection no removal reached alone A stale or duplicate superseded reference can name an index its collection no longer holds. Rewriting anyway was not just wasted I/O: a 100%-full collection written by libhdf5 has no free-space marker, so encode_at_size cannot fit one at the declared size and the whole element update failed. --- src/io/writer.rs | 49 +++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 48 insertions(+), 1 deletion(-) diff --git a/src/io/writer.rs b/src/io/writer.rs index bbe4f72..2ee3b07 100644 --- a/src/io/writer.rs +++ b/src/io/writer.rs @@ -3025,8 +3025,17 @@ impl Hdf5Writer { let declared = GlobalHeapCollection::decode_size(&head, &self.ctx)?; let image = self.handle.read_at(addr, declared)?; let (mut gcol, _) = GlobalHeapCollection::decode(&image, &self.ctx)?; + let mut removed_any = false; for idx in indices { - gcol.remove_object(idx); + removed_any |= gcol.remove_object(idx); + } + // Every index already gone (a stale or duplicate reference): + // leave the image alone. Rewriting is not just wasted I/O — a + // 100%-full collection written by libhdf5 has no free-space + // marker, so re-encoding it at its declared size cannot fit one + // and the whole element update would fail. + if !removed_any { + continue; } if gcol.is_empty() { self.allocator.free(addr, declared as u64); @@ -5386,6 +5395,44 @@ mod tests { std::fs::remove_file(&path).ok(); } + /// A libhdf5-written collection can be 100% full — no free-space marker, + /// content exactly the declared size. When a stale reference names an + /// index that is not there, nothing is removed, and the collection must + /// be left alone: re-encoding it at its declared size cannot fit the + /// free-space marker and would fail the whole update. + #[test] + fn release_leaves_a_full_collection_it_removed_nothing_from() { + use crate::format::global_heap::encode_vlen_reference; + + let path = temp_path("release_full_gcol"); + let writer = Hdf5Writer::create(&path).unwrap(); + + // Hand-built full collection: 16-byte header + one 16+8-byte object, + // declared size exactly 40, no free-space marker. + let mut img = Vec::new(); + img.extend_from_slice(b"GCOL"); + img.push(1); + img.extend_from_slice(&[0u8; 3]); + img.extend_from_slice(&40u64.to_le_bytes()); + img.extend_from_slice(&1u16.to_le_bytes()); // object index 1 + img.extend_from_slice(&1u16.to_le_bytes()); // ref_count + img.extend_from_slice(&0u32.to_le_bytes()); // reserved + img.extend_from_slice(&8u64.to_le_bytes()); // data size + img.extend_from_slice(b"deadbeef"); + assert_eq!(img.len(), 40); + let addr = writer.allocator.allocate(img.len() as u64); + writer.handle.write_at(addr, &img).unwrap(); + + // The superseded reference names index 2, which the collection does + // not hold — a no-op removal. + let refs = encode_vlen_reference(3, addr, 2, &writer.ctx); + writer.release_vlen_references(&refs).unwrap(); + assert_eq!(writer.handle.read_at(addr, 40).unwrap(), img); + + writer.close().unwrap(); + std::fs::remove_file(&path).ok(); + } + #[test] fn create_empty_file() { let path = temp_path("empty"); From 957e2f95432e939b57650efd69956661a50eeba1 Mon Sep 17 00:00:00 2001 From: Sang Woo Kim Date: Thu, 13 Aug 2026 09:56:00 +0900 Subject: [PATCH 9/9] Release 0.4.1: string dataset reads and offset vlen string updates Patch rather than minor: read_strings, read_strings_lossy, and write_vlen_strings_slice are new API and nothing public changed shape. Both features were requested by @janosh (#5, #6); the heap reclaim came out of #6's ask to document the superseded-object behavior. --- CHANGELOG.md | 17 ++++++++++++++--- Cargo.toml | 2 +- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f6964e3..252855c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,11 +1,12 @@ # Changelog -## Unreleased +## 0.4.1 ### Added - `H5Dataset::read_strings` and `read_strings_lossy` decode a whole string - dataset in one call, at whatever width the file declares (issue #5). Reading + dataset in one call, at whatever width the file declares (issue #5, requested + by @janosh). Reading a fixed-string column previously meant `read_raw_bytes` plus hand-written slicing, and the width is a per-dataset property, so the caller had to hard-code one and get it wrong on the next file. Both string datatypes go @@ -15,7 +16,8 @@ the character set. - `H5Dataset::write_vlen_strings_slice` replaces variable-length strings at an - arbitrary offset (issue #6). Vlen datasets could only be created whole or + arbitrary offset (issue #6, requested by @janosh). Vlen datasets could only + be created whole or appended to, so correcting one entry meant rewriting the dataset. The new strings go into one global-heap collection and their references are written over the range; elements still held in the append buffer are patched there, @@ -33,6 +35,15 @@ may still be following those references — the same rule a relocated chunk's old block follows. +### Fixed + +- A vlen sequence of 4 GiB or more is now refused instead of silently + truncated. Every vlen write site cast the byte length to the 32-bit on-disk + length field with `as u32`, so the heap object stored all the bytes while + the reference recorded the wrapped length — reads returned the low-32-bits + prefix with no error. The conversion now has one owner (`vlen_seq_len`) + that errors. + ## 0.4.0 ### Changed diff --git a/Cargo.toml b/Cargo.toml index 859bd02..13335f6 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,7 +1,7 @@ [package] name = "rust-hdf5" description = "Pure Rust HDF5 library with full read/write and SWMR support" -version = "0.4.0" +version = "0.4.1" edition = "2021" rust-version = "1.89" license = "MIT"