diff --git a/CHANGELOG.md b/CHANGELOG.md index d37b2cf..b34d1db 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,38 @@ ## Unreleased +### Correctness: resizing chunked datasets with no recorded maximum (2026-09-27) +- **`FileEditor::resize` scrambled the values of a chunked dataset whose + dataspace records no maximum dimensions when it shrank it** (fixed + 2026-09-27). The editor shipped on main in PR #18 (a4c2ace) and reached + Python as `Dataset.resize` in `'r+'` files; no release has it. clawhdf5's + writer stores such a dataspace for every chunked dataset created without + a `maxshape`, with a Fixed Array (or Single Chunk) chunk index. The + editor patched only the current dimensions, and with no maximum + recorded the maximum is the current dimensions — which is also what the + Fixed Array linearises chunks by — so a shrink moved every chunk after + the first row: h5py, h5dump and our reader all read wrong values + without complaint (20x20, chunks 6x6, resized to 15x15: row 6 read + `0 0 0 0 0 0 120 ...`). After a shrink the dataset could not grow back + either (`3 exceeds the maximum 0`). libhdf5 itself never writes such a + dataspace (`H5S_set_extent_simple` records the maximum, equal to the + dimensions when none is given); reading one, `H5S_extent_get_dims` + reports the current dimensions as the maximum and `H5S_set_extent` + checks against no maximum at all, so libhdf5's own `H5Dset_extent` + scrambles such a file the same way (and lets it grow past its Fixed + Array). The editor now records the maximum libhdf5 would have written + — the dimensions before the first resize, the ones the index was built + with — then changes the current ones (the dataspace message grows by one + length per dimension and moves in the header when it has to). The + dataset then shrinks, grows back to that extent and refuses more, as + one libhdf5 wrote would. The writer (`FileBuilder`) now records the + maximum of every chunked dataset too, as libhdf5 does, so h5py can + resize what it writes (8 more bytes per dimension). Tests: + `crates/clawhdf5/tests/edit_resize_interop.rs` (a 2.7.0-written fixture, + new `FileBuilder` files and h5py files through shrinks, zero extents + and growth, checked against a model with our reader and h5py; h5py + resizing a `FileBuilder` file) and `test_edit.py`'s numpy-model checks. + ### Python bindings: in-place editing (2026-09-27) - **`clawhdf5.File(path, 'r+')`** (and `'a'` on an existing file) opens a file for editing through `clawhdf5::FileEditor`, holding its exclusive diff --git a/crates/clawhdf5-format/src/file_writer.rs b/crates/clawhdf5-format/src/file_writer.rs index 1a1af83..c64e1cf 100644 --- a/crates/clawhdf5-format/src/file_writer.rs +++ b/crates/clawhdf5-format/src/file_writer.rs @@ -130,6 +130,22 @@ pub(crate) fn build_chunked_dataset_oh( fill_message: &[u8], refcount: u32, ) -> Result, FormatError> { + // libhdf5 records every simple dataspace's maximum dimensions (the + // dimensions themselves when none are given, `H5S_set_extent_simple`). + // Without them libhdf5 takes the maximum to be the current dimensions, + // so a resize by libhdf5 (h5py's `Dataset.resize`) would also change + // the maximum a Fixed Array chunk index is laid out by, and move every + // chunk already written. + let recorded; + let ds = if ds.space_type == DataspaceType::Simple && ds.max_dimensions.is_none() { + recorded = Dataspace { + max_dimensions: Some(ds.dimensions.clone()), + ..ds.clone() + }; + &recorded + } else { + ds + }; let mut w = ObjectHeaderWriter::new(); w.add_message_with_flags(MessageType::Datatype, dt.serialize(), 0x01); w.add_message(MessageType::Dataspace, ds.serialize(LENGTH_SIZE)); diff --git a/crates/clawhdf5-py/tests/test_edit.py b/crates/clawhdf5-py/tests/test_edit.py index 99090e2..a2fd83d 100644 --- a/crates/clawhdf5-py/tests/test_edit.py +++ b/crates/clawhdf5-py/tests/test_edit.py @@ -427,13 +427,89 @@ def test_random_edits_match_h5py(h5py, tmp_path, source, seed): key = _random_key(rng, shape) sel_shape, fancy = _selection_shape(key, shape) op = ("set", name, key, _random_value(rng, sel_shape, fancy, dtype)) + before = None + if op[0] == "resize": + with h5py.File(ours_path, "r", locking=False) as o: + before, fill = o[op[1]][()], o[op[1]].fillvalue if edit_both(h5py, theirs, ours_path, ours, op) is not None: refused += 1 + elif before is not None: + # Independently of h5py (which a wrong index layout fools + # the same way): the kept elements keep their values. + want = resized_model(before, op[2], fill) + np.testing.assert_array_equal(ours[op[1]][()], want, err_msg=repr(op)) + with h5py.File(ours_path, "r", locking=False) as o: + np.testing.assert_array_equal(o[op[1]][()], want, err_msg=repr(op)) assert_ours_reads_like_h5py(h5py, ours, ours_path, "end") assert refused < 30 h5dump_reads(ours_path, base) +def resized_model(before, shape, fill): + """`before` resized to `shape` as HDF5 resizes: elements inside both + extents keep their values, the others read as the fill value.""" + out = np.full(shape, fill, dtype=before.dtype) + common = tuple(slice(0, min(a, b)) for a, b in zip(before.shape, shape)) + out[common] = before[common] + return out + + +RESIZES = [(15, 15), (3, 2), (20, 20), (1, 1), (1, 0), (0, 0), (7, 20), (20, 13), (20, 20)] + + +def _check_resizes(h5py, path, name, orig, maxshape=(20, 20), base=None): + """Resize `name` through RESIZES in 'r+', each checked against a numpy + model with clawhdf5 and h5py and the file with `h5rs check`.""" + model = orig + with clawhdf5.File(path, "r+") as f: + ds = f[name] + for shape in RESIZES: + ds.resize(shape) + model = resized_model(model, shape, 0) + np.testing.assert_array_equal(ds[()], model, err_msg=f"{name} {shape}") + with h5py.File(path, "r", locking=False) as t: + np.testing.assert_array_equal(t[name][()], model, err_msg=f"h5py: {name} {shape}") + assert t[name].maxshape == maxshape + h5rs = os.environ.get("CLAWHDF5_H5RS") + if h5rs: # h5dump would wait for the editor's lock + r = subprocess.run([h5rs, "check", "--data", path], capture_output=True, text=True) + assert r.returncode == 0, f"{name} {shape}: " + (r.stdout + r.stderr)[-2000:] + with pytest.raises(ValueError): + ds.resize((maxshape[0] + 1, 20)) + # Written values survive a shrink. + ds[...] = orig + ds.resize((15, 15)) + with h5py.File(path, "r") as t: + np.testing.assert_array_equal(t[name][()], orig[:15, :15]) + h5dump_reads(path, base) + + +@pytest.mark.parametrize("source", SOURCES) +def test_resizes_keep_values(h5py, tmp_path, source): + """Shrinking, zero extents and growing back keep the values a numpy + model keeps, on every source (on clawhdf5's own files a shrink once + moved every chunk: h5py read the same wrong values).""" + _, ours, base = _make(h5py, tmp_path, source) + with clawhdf5.File(ours, "r+") as f: + f["chunk_gzip"].resize((20, 20)) + maxshape = (20, 20) if source == "clawhdf5" else (40, 40) + _check_resizes(h5py, ours, "chunk_gzip", np.arange(400, dtype="u2", "i8", "f8"] diff --git a/crates/clawhdf5/src/edit/mod.rs b/crates/clawhdf5/src/edit/mod.rs index a5a3c1b..feb1d2b 100644 --- a/crates/clawhdf5/src/edit/mod.rs +++ b/crates/clawhdf5/src/edit/mod.rs @@ -877,7 +877,7 @@ impl FileEditor { /// the fill value (`H5D__chunk_prune_by_extent`). pub fn resize(&mut self, path: &str, shape: &[u64]) -> Result<(), Error> { self.edit(|f, img| { - let t = Target::load(f, path)?; + let mut t = Target::load(f, path)?; let dims = t.dims().to_vec(); if shape.len() != dims.len() { return Err(Error::InvalidArgument(format!( @@ -889,7 +889,12 @@ impl FileEditor { if shape == dims.as_slice() { return Ok(()); } - let max = t.ds.max_dimensions.clone().unwrap_or_else(|| dims.clone()); + // No maximum recorded means the current dimensions (see below). + let record_max = t.ds.max_dimensions.is_none(); + let max = + t.ds.max_dimensions + .get_or_insert_with(|| dims.clone()) + .clone(); for d in 0..dims.len() { if shape[d] > max[d] { return Err(Error::InvalidArgument(format!( @@ -928,7 +933,36 @@ impl FileEditor { } put_uint(&mut dims_bytes[d * ls..], n, img.ls); } - hdr.patch(img, i, first, &dims_bytes)?; + if !record_max { + hdr.patch(img, i, first, &dims_bytes)?; + } else { + // No maximum recorded (clawhdf5's writer, for a dataset + // created without a maxshape). libhdf5 never writes such a + // dataspace: `H5S_set_extent_simple` records the maximum, + // equal to the dimensions when none is given. Reading one, + // libhdf5 takes the maximum to be the *current* dimensions + // (`H5S_extent_get_dims`), so changing them would also + // change the maximum the chunk index was built with — the + // Fixed Array linearises chunks by it — and move every + // existing chunk. Record the maximum libhdf5 would have + // written, the dimensions before this resize, so the index + // keeps its layout and the dataset can grow back to them. + let body_len = first + dims.len() * ls; + if body.len() < body_len || body[2] & !0x01 != 0 { + return Err(Error::Unsupported("dataspace message layout".into())); + } + let mut new_body = body[..first].to_vec(); + new_body[2] |= 0x01; + new_body.extend_from_slice(&dims_bytes); + let at = new_body.len(); + new_body.resize(at + dims.len() * ls, 0); + for (d, &n) in dims.iter().enumerate() { + put_uint(&mut new_body[at + d * ls..], n, img.ls); + } + let (flags, corder) = (hdr.msgs[i].flags, hdr.msgs[i].corder); + hdr.delete(img, i)?; + hdr.insert(img, MSG_DATASPACE, flags, &new_body, corder)?; + } let fill = fill_info(img, &hdr)?; hdr.finish(img)?; let expand = shape.iter().zip(&dims).any(|(n, o)| n > o); diff --git a/crates/clawhdf5/tests/edit_resize_interop.rs b/crates/clawhdf5/tests/edit_resize_interop.rs new file mode 100644 index 0000000..83b4dae --- /dev/null +++ b/crates/clawhdf5/tests/edit_resize_interop.rs @@ -0,0 +1,287 @@ +//! `FileEditor::resize` on chunked datasets whose dataspace records no +//! maximum dimensions, as clawhdf5's writer stored a dataset created without +//! a `maxshape` up to 2.7.0 (`fixtures/chunked_no_maxshape_v2_7_0.h5`). libhdf5 never writes such a dataspace (`H5S_set_extent_simple` +//! always records the maximum, equal to the dimensions when none is given), +//! and its Fixed Array chunk index linearises chunks by the maximum +//! dimensions. The editor therefore records the maximum libhdf5 would have +//! written (the dimensions the index was built with) before it changes the +//! current ones, so existing chunks stay where the index put them and the +//! dataset can grow back to its original extent. +//! +//! The writer now records the maximum too, so h5py can resize what it writes. +//! +//! Checked against a model of the expected values, with our reader and with +//! h5py (`CLAWHDF5_PYTHON`; skipped without it unless +//! `CLAWHDF5_REQUIRE_INTEROP=1`), on files clawhdf5 (old and new) and h5py +//! wrote. + +use std::path::{Path, PathBuf}; +use std::process::Command; + +use clawhdf5::{Error, File, FileBuilder, FileEditor}; + +fn python() -> String { + std::env::var("CLAWHDF5_PYTHON").unwrap_or_else(|_| "python3".to_string()) +} + +fn h5py_ok() -> bool { + let ok = Command::new(python()) + .args(["-c", "import h5py, numpy"]) + .output() + .is_ok_and(|o| o.status.success()); + if !ok { + assert!( + !std::env::var("CLAWHDF5_REQUIRE_INTEROP").is_ok_and(|v| v == "1"), + "CLAWHDF5_REQUIRE_INTEROP=1 but h5py/numpy is not available" + ); + eprintln!("SKIP (h5py part): h5py/numpy not available"); + } + ok +} + +fn py(script: &str) -> String { + let o = Command::new(python()) + .args(["-c", script]) + .output() + .expect("run python"); + assert!( + o.status.success(), + "python failed:\n{script}\nSTDERR: {}", + String::from_utf8_lossy(&o.stderr) + ); + String::from_utf8_lossy(&o.stdout).trim().to_string() +} + +/// Row-major values of a 2-D model after resizing `data` (shape `old`) to +/// `new`: kept elements keep their values, new ones are 0 (the fill value). +fn resized(data: &[f32], old: [u64; 2], new: [u64; 2]) -> Vec { + let mut out = vec![0f32; (new[0] * new[1]) as usize]; + for r in 0..old[0].min(new[0]) { + for c in 0..old[1].min(new[1]) { + out[(r * new[1] + c) as usize] = data[(r * old[1] + c) as usize]; + } + } + out +} + +/// Our reader and (when available) h5py read `expect` at `shape`. +fn check(path: &Path, name: &str, shape: [u64; 2], expect: &[f32], with_h5py: bool) { + let f = File::open(path).unwrap(); + let d = f.dataset(name).unwrap(); + assert_eq!(d.shape().unwrap(), shape); + assert_eq!( + d.read_f32().unwrap(), + expect, + "{name}: our reader at {shape:?}" + ); + if with_h5py { + let got = py(&format!( + "import h5py, numpy as np\n\ + with h5py.File({p:?}, 'r') as f:\n\ + \x20 d = f[{name:?}][()]\n\ + print(d.shape, ','.join(repr(float(x)) for x in d.ravel()))", + p = path.to_str().unwrap() + )); + let want = format!( + "({}, {}) {}", + shape[0], + shape[1], + expect + .iter() + .map(|x| format!("{:?}", f64::from(*x))) + .collect::>() + .join(",") + ); + assert_eq!(got, want.trim(), "{name}: h5py at {shape:?}"); + } +} + +/// Resize `name` (20 x 20, values 0..400) through a sequence of shrinks, +/// zero extents and growth back, checking every step. +fn run(path: &Path, name: &str, with_h5py: bool) { + let orig: Vec = (0..400).map(|i| i as f32).collect(); + let mut shape = [20u64, 20]; + let mut data = orig.clone(); + check(path, name, shape, &data, with_h5py); + for next in [ + [15, 15], + [3, 2], + [20, 20], + [1, 1], + [1, 0], + [0, 0], + [7, 20], + [20, 13], + [20, 20], + ] { + let mut ed = FileEditor::open(path).unwrap(); + ed.resize(name, &next).unwrap(); + drop(ed); + data = resized(&data, shape, next); + shape = next; + check(path, name, shape, &data, with_h5py); + } + // The maximum is the extent the dataset was created with. + let mut ed = FileEditor::open(path).unwrap(); + assert!(matches!( + ed.resize(name, &[21, 20]), + Err(Error::InvalidArgument(_)) + )); + drop(ed); + let f = File::open(path).unwrap(); + assert_eq!( + f.dataset(name).unwrap().max_dimensions().unwrap(), + Some(vec![20, 20]) + ); + drop(f); + // A shrink keeps the values it keeps. + let mut ed = FileEditor::open(path).unwrap(); + let vals: Vec = orig.iter().map(|v| v + 0.5).collect(); + ed.write_values(name, &clawhdf5::Selection::All, &vals) + .unwrap(); + ed.resize(name, &[15, 15]).unwrap(); + drop(ed); + check( + path, + name, + [15, 15], + &resized(&vals, [20, 20], [15, 15]), + with_h5py, + ); +} + +fn fixture(dir: &Path, name: &str) -> PathBuf { + let path = dir.join(name); + std::fs::copy( + Path::new(env!("CARGO_MANIFEST_DIR")) + .join("tests/fixtures") + .join(name), + &path, + ) + .unwrap(); + path +} + +/// Files clawhdf5 2.7.0 wrote: a Fixed Array index (Single Chunk for `s`) +/// and a dataspace with no maximum. Shrinking scrambled the values +/// (released in 2.7.0's `FileEditor`, PR #18). +#[test] +fn resize_without_stored_maxshape_keeps_values() { + let with_h5py = h5py_ok(); + for name in ["d", "z"] { + let dir = tempfile::tempdir().unwrap(); + let path = fixture(dir.path(), "chunked_no_maxshape_v2_7_0.h5"); + run(&path, name, with_h5py); + } + // A single-chunk dataset and an empty one keep their extents as maxima. + let dir = tempfile::tempdir().unwrap(); + let path = fixture(dir.path(), "chunked_no_maxshape_v2_7_0.h5"); + let mut ed = FileEditor::open(&path).unwrap(); + ed.resize("s", &[2, 4]).unwrap(); + ed.resize("s", &[3, 4]).unwrap(); + assert!(matches!( + ed.resize("s", &[4, 4]), + Err(Error::InvalidArgument(_)) + )); + assert!(matches!( + ed.resize("e", &[1, 5]), + Err(Error::InvalidArgument(_)) + )); + ed.resize("e", &[0, 3]).unwrap(); + drop(ed); + let f = File::open(&path).unwrap(); + let s = f.dataset("s").unwrap(); + assert_eq!(s.max_dimensions().unwrap(), Some(vec![3, 4])); + let mut want: Vec = (0..12).collect(); + want[8..].fill(0); + assert_eq!(s.read_i32().unwrap(), want); + assert_eq!( + f.dataset("e").unwrap().max_dimensions().unwrap(), + Some(vec![0, 5]) + ); +} + +fn written(path: &Path, deflate: bool) { + let data: Vec = (0..400).map(|i| i as f32).collect(); + let mut b = FileBuilder::new(); + let d = b + .create_dataset("d") + .with_f32_data(&data) + .with_shape(&[20, 20]) + .with_chunks(&[6, 6]); + if deflate { + d.with_deflate(4); + } + b.write(path).unwrap(); +} + +/// clawhdf5's writer now records the maximum, as libhdf5 does. +#[test] +fn resize_file_written_without_maxshape_keeps_values() { + let with_h5py = h5py_ok(); + for deflate in [false, true] { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("cw.h5"); + written(&path, deflate); + let f = File::open(&path).unwrap(); + assert_eq!( + f.dataset("d").unwrap().max_dimensions().unwrap(), + Some(vec![20, 20]) + ); + drop(f); + run(&path, "d", with_h5py); + } +} + +/// h5py resizing a file clawhdf5 wrote without a maxshape keeps its values +/// (it scrambled them while the writer recorded no maximum). +#[test] +fn h5py_resizes_what_clawhdf5_writes() { + if !h5py_ok() { + return; + } + for deflate in [false, true] { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("cw.h5"); + written(&path, deflate); + let out = py(&format!( + "import h5py, numpy as np\n\ + exp = np.arange(400, dtype='f4').reshape(20, 20)\n\ + with h5py.File({p:?}, 'r+') as f:\n\ + \x20 f['d'].resize((15, 15))\n\ + \x20 ok = np.array_equal(f['d'][()], exp[:15, :15])\n\ + \x20 f['d'].resize((20, 20))\n\ + \x20 back = f['d'][()]\n\ + want = np.zeros((20, 20), 'f4'); want[:15, :15] = exp[:15, :15]\n\ + print(ok and np.array_equal(back, want))", + p = path.to_str().unwrap() + )); + assert_eq!(out, "True"); + let mut want = vec![0f32; 400]; + for r in 0..15 { + for c in 0..15 { + want[r * 20 + c] = (r * 20 + c) as f32; + } + } + check(&path, "d", [20, 20], &want, false); + } +} + +/// h5py's files record the maximum; the same sequence must hold. +#[test] +fn resize_h5py_file_without_maxshape_keeps_values() { + if !h5py_ok() { + return; + } + for libver in ["earliest", "v110", "latest"] { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("hp.h5"); + py(&format!( + "import h5py, numpy as np\n\ + with h5py.File({p:?}, 'w', libver=({libver:?}, 'latest')) as f:\n\ + \x20 f.create_dataset('d', data=np.arange(400, dtype='f4').reshape(20, 20), chunks=(6, 6))", + p = path.to_str().unwrap() + )); + run(&path, "d", true); + } +} diff --git a/crates/clawhdf5/tests/fixtures/chunked_no_maxshape_v2_7_0.h5 b/crates/clawhdf5/tests/fixtures/chunked_no_maxshape_v2_7_0.h5 new file mode 100644 index 0000000..7ecdfe7 Binary files /dev/null and b/crates/clawhdf5/tests/fixtures/chunked_no_maxshape_v2_7_0.h5 differ diff --git a/crates/clawhdf5/tests/plugin_filters_interop.rs b/crates/clawhdf5/tests/plugin_filters_interop.rs index f157ff5..9b41b2a 100644 --- a/crates/clawhdf5/tests/plugin_filters_interop.rs +++ b/crates/clawhdf5/tests/plugin_filters_interop.rs @@ -966,7 +966,10 @@ fn skipped_optional_filters_are_masked_as_libhdf5_masks_them() { /// Files whose chunks all compress are written exactly as before optional /// filters could be skipped: every mask is 0 and nothing else changed. The -/// hashes are of the files the writer produced before that change. +/// hashes are of the files the writer produced before that change, except +/// that a chunked dataset without a maxshape now records its maximum +/// dimensions (8 bytes per dimension; `lzf_fixed`, `lzf_single`, and +/// `blosc_fixed`). #[cfg(feature = "lzf")] #[test] fn files_whose_chunks_all_compress_are_unchanged() { @@ -985,7 +988,7 @@ fn files_whose_chunks_all_compress_are_unchanged() { .with_chunks(&[500]) .with_lzf(); }, - (3965, 449169442), + (3973, 452644487), ), ( "lzf_ea_noshuffle", @@ -1017,7 +1020,7 @@ fn files_whose_chunks_all_compress_are_unchanged() { .with_chunks(&[3000]) .with_lzf(); }, - (546, 690805477), + (554, 1394027497), ), ]; #[cfg(feature = "blosc")] @@ -1029,7 +1032,7 @@ fn files_whose_chunks_all_compress_are_unchanged() { .with_chunks(&[1024]) .with_blosc(BloscCodec::Lz4, 5, BloscShuffle::Byte); }, - (2776, 4278611376), + (2784, 1180133244), )); for (name, build, want) in &cases { let mut fb = clawhdf5::FileBuilder::new(); diff --git a/docs/known-issues.md b/docs/known-issues.md index e33b80c..690bb04 100644 --- a/docs/known-issues.md +++ b/docs/known-issues.md @@ -7,6 +7,33 @@ deleting it. --- +## Shrinking a chunked dataset with no recorded maximum scrambled it + +**Status:** fixed 2026-09-27, before any release (`FileEditor::resize` +shipped on main in PR #18, a4c2ace). Files the writer produced before the +fix still lack the maximum; see *Existing files*. + +clawhdf5's writer stored no maximum dimensions for a chunked dataset +created without a `maxshape` (libhdf5 always stores one, equal to the +dimensions when none is given). With none recorded, the maximum is the +current dimensions (`H5S_extent_get_dims`), and a Fixed Array chunk index +places chunks by the maximum. `FileEditor::resize` changed only the +current dimensions, so a shrink moved every existing chunk and every +reader returned wrong values; after a shrink the dataset could not grow +back. Found by the review of the Python editing work. + +**Fix:** before the first resize of such a dataset the editor records the +maximum libhdf5 would have written (the dimensions the index was built +with); the writer now records it for every chunked dataset. **Test:** +`crates/clawhdf5/tests/edit_resize_interop.rs`. **Existing files:** +datasets written before the fix have no recorded maximum. The fixed +editor handles them; **libhdf5 (h5py `Dataset.resize`, `H5Dset_extent`) +does not** — it scrambles them the same way, and lets them grow past +their Fixed Array. Resize them once with a fixed `FileEditor` (a resize +to the same shape changes nothing; shrink and grow back) before letting +libhdf5 resize them. A dataset already shrunk by the unfixed +editor holds misplaced chunks; rewrite it from a good copy. + ## Fletcher-32 checksums disagreed with libhdf5 on about 1 chunk in 32768 **Status:** fixed 2026-09-26, after v2.7.0. **Every release (v2.1.0 to