diff --git a/CHANGELOG.md b/CHANGELOG.md index ffd6d29..252855c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,49 @@ # Changelog +## 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, 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 + 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. + +- `H5Dataset::write_vlen_strings_slice` replaces variable-length strings at an + 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, + so the flush at close does not write the pre-update reference back over + 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. + +### 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" diff --git a/src/dataset.rs b/src/dataset.rs index c1152ef..6e0dddd 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( @@ -1859,6 +1910,49 @@ 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 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; + /// 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 @@ -1899,6 +1993,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 +4627,698 @@ 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(); + } + + // ---- 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(); + } + + // ---- 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"); + } + + /// 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] + 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..5da23df 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. @@ -283,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 @@ -388,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); @@ -459,4 +567,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 de5b34f..2ee3b07 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, @@ -2739,6 +2739,314 @@ 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 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 + /// 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 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); + + // 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)?; + + 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 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( + crate::format::global_heap::vlen_seq_len(strings[i].len())?, + gcol_addr, + obj_idx as u32, + &self.ctx, + )); + } + + 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); + } + + // 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]; + // 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; + 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`) 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 + /// 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 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)?; + let mut removed_any = false; + for idx in indices { + 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); + } else { + let rewritten = gcol.encode_at_size(&self.ctx, declared)?; + self.handle.write_at(addr, &rewritten)?; + } + } + Ok(()) + } + /// Add an attribute to a dataset. /// /// The attribute will be written as a message in the dataset's object @@ -2808,7 +3116,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(), @@ -2853,7 +3162,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); @@ -5041,6 +5350,89 @@ 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. + #[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(); + } + + /// 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");