From 7acfb795848704db88c171a6f3110752ce83e668 Mon Sep 17 00:00:00 2001 From: osobh Date: Sat, 26 Sep 2026 10:03:22 -0500 Subject: [PATCH] writer: order dense name indexes by hash, then name libhdf5 compares the name when two hashes are equal; the writer broke ties by insertion order, and libhdf5 could not find one of two names whose lookup3 hashes collide (k69209 / k155448). Test fails before the fix with h5py's KeyError. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 11 +++++ crates/clawhdf5-format/src/file_writer.rs | 43 ++++++++++++------- .../clawhdf5/tests/writer_groups_interop.rs | 39 +++++++++++++++++ 3 files changed, 77 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 29804fc..0e50755 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,17 @@ ## Unreleased +### Writer: large dense indexes (2026-09-26) +- **Links and attributes whose name hashes collide are found by name.** The + dense name indexes (a group's links: B-tree v2 type 5; an object's + attributes: type 8) are ordered by the name's lookup3 hash and, when two + hashes are equal, by the name itself, as libhdf5 compares them. The writer + broke ties by insertion order, so libhdf5 could not open one of two + colliding names (`"k69209"` and `"k155448"` share hash `0x3a0b13e6`; + collisions are likely from about 77 000 names). Regression test + `names_whose_hashes_collide_are_found_by_name` in + `crates/clawhdf5/tests/writer_groups_interop.rs`. + ### Concurrent reads (2026-09-26) - **Full reads of chunked datasets scale with threads again when rayon's pool has one thread.** Each full read handed its chunks to rayon to diff --git a/crates/clawhdf5-format/src/file_writer.rs b/crates/clawhdf5-format/src/file_writer.rs index 11345a1..80a296b 100644 --- a/crates/clawhdf5-format/src/file_writer.rs +++ b/crates/clawhdf5-format/src/file_writer.rs @@ -910,20 +910,27 @@ pub(crate) fn build_dense_attrs( let heap_id_length = heap.heap_id_length; let heap_ids = &heap.heap_ids; - // Build B-tree v2 type 8 records (17 bytes each) + // Build B-tree v2 type 8 records (17 bytes each), in the index's key + // order: libhdf5 compares the name hash, then — for names whose hashes + // collide — the names themselves (`strcmp`). let record_size: u16 = heap_id_length + 1 + 4 + 4; - let mut records: Vec<(u32, u32, Vec)> = Vec::with_capacity(attrs.len()); - for (i, heap_id) in heap_ids.iter().enumerate() { - let mut rec = Vec::with_capacity(record_size as usize); - rec.extend_from_slice(heap_id); - rec.push(0); // msg_flags - rec.extend_from_slice(&(i as u32).to_le_bytes()); // creation_order - rec.extend_from_slice(&name_hashes[i].to_le_bytes()); // hash - records.push((name_hashes[i], i as u32, rec)); - } - records.sort_by(|a, b| a.0.cmp(&b.0).then(a.1.cmp(&b.1))); - - let records: Vec> = records.into_iter().map(|(_, _, rec)| rec).collect(); + let mut order: Vec = (0..attrs.len()).collect(); + order.sort_by(|&a, &b| { + name_hashes[a] + .cmp(&name_hashes[b]) + .then_with(|| attrs[a].name.as_bytes().cmp(attrs[b].name.as_bytes())) + }); + let records: Vec> = order + .into_iter() + .map(|i| { + let mut rec = Vec::with_capacity(record_size as usize); + rec.extend_from_slice(&heap_ids[i]); + rec.push(0); // msg_flags + rec.extend_from_slice(&(i as u32).to_le_bytes()); // creation_order + rec.extend_from_slice(&name_hashes[i].to_le_bytes()); // hash + rec + }) + .collect(); let bthd_addr = btree_addr; let mut blob = heap.blob; blob.extend_from_slice(&single_leaf_v2_btree( @@ -1039,14 +1046,18 @@ pub(crate) fn build_dense_links( let heap = build_single_block_fractal_heap(&serialized, base_address, 32, 7)?; let heap_id_length = heap.heap_id_length; - // Type 5 records: hash(4) + heap_id. The B-tree's key is the name hash, - // so records are sorted by (hash, order). + // Type 5 records: hash(4) + heap_id. The B-tree's key is the name hash + // and, for names whose hashes collide, the name (libhdf5 compares them + // with `strcmp`): records out of that order are not found by name. let mut by_name: Vec<(u32, usize)> = links .iter() .enumerate() .map(|(i, l)| (crate::checksum::jenkins_lookup3(l.name.as_bytes()), i)) .collect(); - by_name.sort_unstable(); + by_name.sort_unstable_by(|&(ha, a), &(hb, b)| { + ha.cmp(&hb) + .then_with(|| links[a].name.as_bytes().cmp(links[b].name.as_bytes())) + }); let name_records: Vec> = by_name .iter() .map(|&(hash, i)| { diff --git a/crates/clawhdf5/tests/writer_groups_interop.rs b/crates/clawhdf5/tests/writer_groups_interop.rs index 97e3544..46d155b 100644 --- a/crates/clawhdf5/tests/writer_groups_interop.rs +++ b/crates/clawhdf5/tests/writer_groups_interop.rs @@ -627,6 +627,45 @@ fn non_ascii_names_are_utf8() { assert_eq!(f.dataset("größe/wert").unwrap().read_i32().unwrap(), [1]); } +#[test] +fn names_whose_hashes_collide_are_found_by_name() { + skip_if_no_python!(); + // "k69209" and "k155448" have the same lookup3 hash (0x3a0b13e6). The + // dense name indexes (links: type 5, attributes: type 8) are ordered by + // hash and then by name, and libhdf5's lookup relies on it. The writer + // broke ties by insertion order, so with "k69209" added first libhdf5 + // could not open "k155448" by name. + let dir = tempfile::tempdir().unwrap(); + let mut b = FileBuilder::new(); + let mut g = b.create_group("g"); + for (i, n) in ["k69209", "k155448"] + .into_iter() + .chain((0..10).map(|_| "")) + .enumerate() + { + let name = if n.is_empty() { format!("d{i}") } else { n.into() }; + g.create_dataset(&name).with_i32_data(&[i as i32]); + } + b.add_group(g.finish()); + let x = b.create_dataset("x"); + x.with_i32_data(&[0]); + x.set_attr("k69209", AttrValue::I64(1)); + x.set_attr("k155448", AttrValue::I64(2)); + for i in 0..10 { + x.set_attr(&format!("a{i}"), AttrValue::I64(10 + i)); + } + let path = write(&dir, "collide.h5", b); + let out = h5py( + &path, + "with h5py.File(path, 'r') as f:\n\ + \x20 g, a = f['g'], f['x'].attrs\n\ + \x20 print(json.dumps([int(g['k69209'][0]), int(g['k155448'][0]), 'k155448' in g,\n\ + \x20 int(a['k69209']), int(a['k155448']), 'k155448' in a]))", + ); + assert_eq!(out, "[0, 1, true, 1, 2, true]"); + h5dump_ok(&path); +} + #[test] fn a_group_attribute_set_again_takes_the_new_value() { skip_if_no_python!();