Repository navigation
Fix HDF5 al_delete_data ignoring its path argument - #101
Open
olivhoenen wants to merge 4 commits into
Open
olivhoenen wants to merge 4 commits into
olivhoenen wants to merge 4 commits into
Conversation
`al_delete_data`'s `path` is accepted by the ABI and dropped by the HDF5 backend: `HDF5Backend::deleteData` never forwards it and `HDF5Writer:: deleteData` has no parameter to receive it, so any non-empty path removes the whole IDS occurrence and reports success. Characterize that before touching it. The existing tripwire only observed the sibling leaf; widen it to the two consequences issue #63 measured as well — the occurrence's own `<ids>.h5` is deleted, and the master file is left holding an external link to a file that no longer exists (observed through `al_get_occurrences`, which enumerates those links live). Confirmed red/green today: the three correct-contract assertions fail, the tripwire passes. (cherry picked from commit 5748ec5)
Decision: `path` means what the ABI has always said it means — a non-empty path deletes that node and its subtree only, an empty path addresses the whole DATAOBJECT. Recorded in docs/adr/0001-al-delete-data-path-semantics.md together with the per-backend matrix and why the cheaper alternative (rejecting a non-empty path as a caller error) was rejected: it is not what the header says, not what the other backends do, and it would fail every non-slice `put()` and `delete_data()` through imas-python, whose traversal issues one delete per DD leaf. `HDF5Writer::deleteSubtree` selects the matching datasets out of the IDS group's flat namespace of tensorized names — the mangled path, its `_SHAPE` companion, and anything continuing with `&` (a structure child) or `[]` (an AOS index). The continuation character is what keeps `time` from taking `time_slice[]&x` with it. Buffered writes are flushed before anything is unlinked, since HDF5DataSetHandler::close() frees write buffers without writing them. The group listing is cached across a run of deletes, and invalidated by anything that can add a dataset, so the HLI's leaf-by-leaf traversal is not quadratic in the size of the IDS. `deleteOccurrence` keeps the empty-path behavior and fixes two defects in it: the master file's external link is now removed with the file it points at (it used to be left dangling, so al_get_occurrences kept reporting an occurrence whose file was gone), and a closed-but-present IDS file is deleted instead of being silently reopened and leaked. Tests: DeleteMatrix's HDF5 leaf/structure cells are enabled, the HDF5 xfail pair retired, and the layout-specific consequences the backend-agnostic matrix cannot see are pinned in Hdf5Delete.* — which files and master-file links survive which granularity, same-prefix siblings, an absent path, and a run of successive leaf deletes all taking effect. Verified against the suite, the Debug leg, and ASan/UBSan. (cherry picked from commit c8e49ca)
The first cut matched dataset names by string prefix plus a continuation character. That is right for every path the HLI issues, but it matches nothing at all for a path that traverses an array of structures: `outer/inner` mangles to `outer&inner` while the dataset is named `outer[]&inner&leaf`. The call then deleted nothing and reported success — in miniature, the same "argument accepted, ignored, success reported" state this issue exists to remove. Confirmed by test before changing the rule. Match per segment instead: each segment of the requested path matches a dataset-name segment with or without its `[]` AOS suffix, and the name's remaining segments are its subtree. Segment-wise still tells a real child from a same-prefix stranger (`time` vs `time_slice[]&x`, `code` vs `code_name`), and it gives an AOS-crossing path the one meaning the ABI leaves available: `deleteData` receives an OperationContext, never an arraystruct one, so no element can be named and the path can only mean "that subtree in every element" — exactly what the tensorized layout holds. (cherry picked from commit 1c00c85)
Standards axis: - The group listing now validates itself against the group's live link count instead of relying on four hand-placed invalidateGroupMembers() calls on the write paths. Only a write can add a link, and a write can only add, so an unchanged count means an unchanged set of names — which removes the latent coupling where any future dataset-creating path had to remember to invalidate. - unlinkFromGroup() owns both the H5Ldelete loop and keeping the cache in step, so deleteSubtree no longer reassigns the writer's own vector through a returned reference. - The delete error builds its message as a std::string rather than through a fixed-size buffer: a dataset name is a caller-supplied DD path, which the src/hdf5 `char error_message[200]` idiom cannot bound. - Both H5 failures in the listing name their own function. - Hdf5Delete's tests all go through the fixture now; the free observation helper that duplicated three of its members (and skipped make_legacy_tree, unlike every other on-disk path in the file) is gone. Spec axis: - al_lowlevel.h's per-backend summary said memory does "node and subtree" where only the AOS case is characterized, and omitted UDA entirely. - The ADR records the two behaviour changes the issue did not itself measure — master-link removal on an empty-path delete, and deleting an IDS file that is closed but still on disk — rather than leaving them silent. The one substantive fidelity finding, that a path below an AOS silently matched nothing, was already fixed in the preceding commit; the review confirms it independently. (cherry picked from commit 0250431)
This was referenced Oct 14, 2026
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of the split of #85 (Tests and Benchmarks for regression) into smaller, single-purpose PRs that are easier to review.
Bug fix. On HDF5,
al_delete_datadropped itspathargument and always deleted the whole IDS occurrence. It also left a dangling link inmaster.h5, soal_get_occurrencesstill listed the deleted occurrence. Now a non-emptypathunlinks only that node's subtree, matched per path segment and across arrays of structures; an emptypathdeletes the occurrence and removes its master link. The semantics are specified inal_lowlevel.h/al_backend.hand in ADRdocs/adr/0001-al-delete-data-path-semantics.md. Includes the regression tests (the former xfail tripwire is replaced by correct-contract tests).Fork history: issue yohannmarguier#63, fork PR yohannmarguier#64.
Stack position: 8/9; depends on # (only for the test harness), independent of #100 and the benchmarks. Whichever of #100 and this PR merges second may need a small rebase in
test_structured_data.cpp.Authorship: these commits are @yohannmarguier's, cherry-picked unchanged from his fork with
git cherry-pick -x, so each one keeps its original author, date and message plus a(cherry picked from commit …)link back to the fork. Only conflicts with currentdevelopand with the new commit order were resolved.🤖 Generated with Claude Code