Fix HDF5 reads of absolute field paths beneath an array of structures

docs/adr/0002-absolute-field-path-resolution.md

@@ -0,0 +1,130 @@
+---
+status: accepted
+---
+
+# An absolute field path resolves by segment match against the open context chain
+
+`al_read_data(ctx, fieldpath, …)` documents `fieldpath` as "always relative to
+current Context, dataobject absolute path can be specified with a prepended
+'/'" (`include/al_lowlevel.h`, read-data block). An absolute field path is not
+a filesystem path and does not carry the IDS name:
+`/time_slice/constraints/j_phi/reconstructed` names the same stored datum as
+`reconstructed` read from the corresponding array-of-structures context.
+
+The HDF5 read path honoured only the relative half. `HDF5Reader::read_ND_Data`
+replaced every `/` with `&` (the tensorized separator) and then
+*unconditionally* prepended the current AOS context's tensorized path, so an
+already-rooted name was appended to the context it was meant to replace:
+
+```text
+requested   /time_slice/constraints/j_phi/reconstructed
+looked up   time_slice[]&constraints&j_phi[]&&time_slice&constraints&j_phi&reconstructed
+stored as   time_slice[]&constraints&j_phi[]&reconstructed
+```
+
+`H5Lexists` found nothing, `read_ND_Data` returned 0, and `al_plugin_read_data`
+filled the caller's buffer with `EMPTY_DOUBLE` while reporting `code == 0`
+(`Lowlevel::setDefaultValue`). Stored data was therefore indistinguishable from
+genuinely absent data — a silent wrong answer, not an error (issue #65). The
+same defect made a root field unreadable from the operation context itself,
+where the leading `&` was the only thing left to strip.
+
+We keep the documented meaning and implement it: an absolute field path is
+resolved **by path segment** against the AOS chain the caller currently has
+open, retaining the tensorized markers and the indices of every level the
+target still sits under, and only those.
+
+## Considered options
+
+**1. Segment-match against the open context chain (chosen).** Implemented in
+`HDF5Utils::resolveAbsoluteFieldPath`, called from `read_ND_Data`. An IDS group
+is a flat namespace of tensorized dataset names — the DD path with `/` → `&`,
+each array-of-structures segment suffixed `[]`, the AOS flattened into a
+leading index dimension — so resolving an absolute path means answering two
+questions at once, which is why one function answers both:
+
+- **which name to look up.** The stored name keeps the `[]` of every AOS the
+  field sits under, so the tensorized prefix of the matched levels is retained
+  and only the remaining segments are appended.
+- **which indices still apply.** Each retained `[]` level keeps its current
+  cursor; levels the target does not sit under are dropped, along with the
+  timed-AOS designation if the timed level is among them (the dataset then
+  carries no time dimension to select in).
+
+Both fall out of one walk: compare the absolute path's segments against the
+open chain's segments and cut at the last AOS boundary that still matches.
+Matching is per segment, never by string prefix — `time_slice_extra/…` shares a
+textual prefix with `time_slice[]` but no path prefix at all. This is the same
+rule ADR 0001 adopted for `al_delete_data`, for the same reason: segment-wise
+is what tells a real descendant from a same-prefix stranger.
+
+Worked examples, with `time_slice[]` → `constraints/j_phi[]` open at
+`(slice=1, constraint=2)`:
+
+| Requested | Resolved name | Indices applied |
+|---|---|---|
+| `/time_slice/constraints/j_phi/reconstructed` | `time_slice[]&constraints&j_phi[]&reconstructed` | `(1, 2)` |
+| `/time_slice/global_quantities/ip` | `time_slice[]&global_quantities&ip` | `(1)` |
+| `/vacuum_toroidal_field/r0` | `vacuum_toroidal_field&r0` | none |
+| `/time_slice/constraints/j_phi/reconstructed`, from the `time_slice` context | `time_slice[]&constraints&j_phi&reconstructed` | `(1)` → not found, absent |
+
+**2. Strip the leading `/` and use the rest verbatim.** Rejected. It resolves a
+root field correctly and nothing else: the result carries no `[]` for any AOS
+above the target, so every path through an array of structures — the case the
+issue is actually about — still misses. It also silently duplicates the path
+when the target is under the open context.
+
+**3. Skip the context prefix by string length.** Rejected. Cheaper than a
+segment walk and wrong twice over: it drops the `[]` markers along with the
+prefix, and a purely textual prefix matches same-prefix strangers.
+
+**4. Keep the child's whole cursor regardless of where the target sits.**
+Rejected. An ancestor-level dataset carries fewer tensorized dimensions than
+the child context has indices, so applying all of them either selects out of
+extent or reads the wrong element. This is the mirror image of option 2's
+failure and would be the more dangerous one, since it returns a number.
+
+**5. Leave the core alone; have the caller rewrite absolute to relative.**
+Rejected. The contract being broken is the core's own, stated in its public
+header, so every caller would have to carry the same workaround. It is also not
+generally possible: an absolute path may address an *ancestor* or a *root*
+field, which no relative spelling from the current context can name, and a
+blanket rewrite of the caller's argument would corrupt exactly those cases.
+
+## Scope
+
+The reproduction, diagnosis and fix are HDF5 **reads**. Writes,
+`al_delete_data`, `al_begin_arraystruct_action` and the other backends were not
+established as affected by this reproduction and are deliberately not changed
+here; a concrete finding about any of them belongs in its own issue. The
+regression suite writes through relative spellings only, so it never presumes
+an answer for the write side.
+
+The change is confined to path/selection computation: no DD-version knowledge,
+no schema lookup, no C ABI change. The version knowledge constraint in
+`NORTH_STAR.md` is the reason a "just look up whether that segment is an AOS"
+resolution is not on the table at all.
+
+## Consequences
+
+- **A target beyond an AOS the caller never opened is reported absent.** There
+  is no cursor for that level, and the core holds no schema that would even
+  tell it the segment is an array of structures. The resolved name carries no
+  `[]` for it, finds nothing, and travels the ordinary missing-data path. That
+  is the honest answer; inventing an index is how a wrong value gets returned
+  in place of no value. Pinned by
+  `Hdf5AbsoluteFieldPaths.TargetBeyondAnUnopenedAosIsReportedAbsent`.
+- **Ordinary missing-data semantics are unchanged.** An absolute path that
+  resolves but names nothing stored still returns `code == 0` with the type's
+  empty sentinel, exactly as the relative spelling does. The fix makes stored
+  data findable; it does not make absent data look present.
+- **Existence and dataset caches are keyed on the resolved name**
+  (`existing_data_sets`, `opened_data_sets`), so the two spellings of one
+  address share one cache entry and cannot disagree across repeated reads.
+- **A root timebase is now classified correctly.** `/time` from the operation
+  context previously resolved to `&time`, which matched the "time dataset
+  inside a dynamic AOS" suffix test rather than the homogeneous-timebase one.
+  It now resolves to `time` and is classified as the homogeneous timebase.
+- **Absolute and relative reads may still differ in *cost*.** Nothing here
+  changes the per-node `Backend` contract: an absolute read is resolved and
+  looked up one field at a time, like every other read.

src/hdf5/hdf5_reader.cpp

@@ -478,14 +478,32 @@ int HDF5Reader::read_ND_Data(Context *ctx, std::string &att_name, std::string &t
     if (gid == -1) // IDS does not exist in the file
         return 0;
 
+    // Whether the field is addressed from the dataobject root must be decided
+    // before the separators are normalized, while the leading '/' marker is
+    // still distinguishable (issue #65).
+    const bool absolute_field = HDF5Utils::isAbsoluteFieldPath(att_name);
+
     std::string &dataset_name = att_name;
     std::replace(dataset_name.begin(), dataset_name.end(), '/', '&'); // character '/' is not supported in datasets names
     std::replace(timebasename.begin(), timebasename.end(), '/', '&');
 
     hid_t dataset_id = -1;
     std::string tensorized_path = dataset_name;
+    // How many of the open AOS chain's levels the field lives under. Only an
+    // absolute path can sit above the current context and so retain fewer than
+    // all of them; a relative one is always read at the full current cursor.
+    size_t applicable_aos_levels = std::numeric_limits<size_t>::max();
 
-    if (ctx->getType() == CTX_ARRAYSTRUCT_TYPE)
+    if (absolute_field)
+    {
+        const std::vector<std::string> no_open_aos; // an operation context opens none
+        const std::vector<std::string> &tensorized_paths =
+            (ctx->getType() == CTX_ARRAYSTRUCT_TYPE)
+                ? tensorized_paths_per_context[static_cast<ArraystructContext *>(ctx)]
+                : no_open_aos;
+        tensorized_path = HDF5Utils::resolveAbsoluteFieldPath(dataset_name, tensorized_paths, &applicable_aos_levels);
+    }
+    else if (ctx->getType() == CTX_ARRAYSTRUCT_TYPE)
     {
         auto &tensorized_paths = tensorized_paths_per_context[static_cast<ArraystructContext *>(ctx)];
         tensorized_path = tensorized_paths.back() + "&" + dataset_name;
@@ -540,6 +558,18 @@ int HDF5Reader::read_ND_Data(Context *ctx, std::string &att_name, std::string &t
     std::vector<int> current_arrctx_indices;
     hdf5_utils.getAOSIndices(ctx, current_arrctx_indices, &timed_AOS_index); // getting current AOS indices
 
+    if (applicable_aos_levels < current_arrctx_indices.size())
+    {
+        // An absolute field path that stops short of the innermost open AOS
+        // keeps only the indices of the levels it still passes through -- the
+        // child's cursor must not be applied to an ancestor's dataset, which
+        // carries fewer tensorized dimensions (issue #65). The indices run
+        // outermost first, so the retained ones are the leading entries.
+        current_arrctx_indices.resize(applicable_aos_levels);
+        if (timed_AOS_index >= static_cast<int>(applicable_aos_levels))
+            timed_AOS_index = -1; // the timed AOS is not among them: the field is not tensorized on time
+    }
+
     bool is_dynamic = timebasename.compare("") != 0 ? true : false;
     bool isTimed = (timed_AOS_index != -1);
     int slice_index = -1;

src/hdf5/hdf5_utils.cpp

@@ -587,6 +587,120 @@ void HDF5Utils::setTensorizedPaths(ArraystructContext * ctx, std::vector < std::
     std::replace(tensorized_paths.back().begin(), tensorized_paths.back().end(), '/', '&');
 }
 
+// --- absolute field paths (issue #65) --------------------------------------
+//
+// al_read_data's contract (al_lowlevel.h) lets a caller address a field from
+// the dataobject root by prepending '/': it is not a filesystem path and does
+// not carry the IDS name, so "/time_slice/constraints/j_phi/reconstructed"
+// names the same datum as "reconstructed" read from the corresponding AOS
+// context.
+//
+// Resolving one answers two questions at once, which is why they are answered
+// here together rather than by trimming the string at the call site:
+//
+//   * which dataset name to look up. The stored name keeps the tensorized
+//     "[]" marker of every AOS the field sits under, so simply dropping the
+//     '/' ("time_slice&constraints&j_phi&reconstructed") never matches, and
+//     simply skipping the context prefix duplicates nothing but loses those
+//     markers.
+//   * which AOS indices still apply. A field that sits above the current
+//     context must not be read at the child's cursor.
+//
+// Both follow from one walk: compare the absolute path's segments against the
+// open context chain's segments and cut at the last AOS boundary that still
+// matches. Segments, never string prefixes -- "time_slice_extra/..." shares a
+// textual prefix with "time_slice[]" but no path prefix at all.
+//
+// Whatever follows the cut is appended verbatim. A path that leaves the open
+// chain -- a sibling branch, or one crossing an AOS the caller never opened --
+// therefore resolves to a name carrying no "[]" for that branch: it is looked
+// up, not found, and reported by the ordinary missing-data path. That is the
+// honest answer rather than a guess: the core holds no schema, so it cannot
+// know an unopened segment is an AOS, and would have no cursor for it if it
+// did.
+
+// Tensorized dataset names join their segments with '&' ('/' is not a legal
+// HDF5 link-name character), and mark each array of structures with a
+// trailing "[]" -- see setTensorizedPaths() above.
+static const char TENSORIZED_SEPARATOR = '&';
+static const char *AOS_MARKER = "[]";
+
+static bool isAOSSegment(const std::string & segment)
+{
+    const size_t marker_length = strlen(AOS_MARKER);
+    return segment.size() >= marker_length &&
+           segment.compare(segment.size() - marker_length, marker_length, AOS_MARKER) == 0;
+}
+
+static std::string stripAOSMarker(const std::string & segment)
+{
+    return isAOSSegment(segment) ? segment.substr(0, segment.size() - strlen(AOS_MARKER)) : segment;
+}
+
+static std::vector < std::string > splitTensorizedSegments(const std::string & name)
+{
+    std::vector < std::string > segments;
+    size_t start = 0;
+    while (true) {
+        const size_t end = name.find(TENSORIZED_SEPARATOR, start);
+        if (end == std::string::npos) {
+            segments.push_back(name.substr(start));
+            return segments;
+        }
+        segments.push_back(name.substr(start, end - start));
+        start = end + 1;
+    }
+}
+
+bool HDF5Utils::isAbsoluteFieldPath(const std::string & field)
+{
+    return !field.empty() && field[0] == '/';
+}
+
+std::string HDF5Utils::resolveAbsoluteFieldPath(const std::string & absolute_dataset_name,
+                                                const std::vector < std::string > &tensorized_paths,
+                                                size_t * applicable_aos_levels)
+{
+    *applicable_aos_levels = 0;
+
+    // The '/' separators (including the leading absolute marker) have already
+    // been normalized to '&' by the caller, so the path and the context chain
+    // are compared in one spelling.
+    std::string rooted_name = absolute_dataset_name;
+    if (!rooted_name.empty() && rooted_name[0] == TENSORIZED_SEPARATOR)
+        rooted_name.erase(0, 1);
+
+    if (tensorized_paths.empty())     // no AOS is open: the marker was all there was to strip
+        return rooted_name;
+
+    const std::vector < std::string > context_segments = splitTensorizedSegments(tensorized_paths.back());
+    const std::vector < std::string > target_segments = splitTensorizedSegments(rooted_name);
+
+    size_t matched_segments = 0;      // target segments consumed at the last matching AOS boundary
+    size_t levels = 0;                // AOS levels retained at that boundary
+    size_t levels_walked = 0;
+    for (size_t i = 0; i < context_segments.size() && i < target_segments.size(); i++) {
+        if (stripAOSMarker(context_segments[i]) != target_segments[i])
+            break;
+        if (isAOSSegment(context_segments[i])) {
+            levels_walked++;
+            matched_segments = i + 1;
+            levels = levels_walked;
+        }
+    }
+    *applicable_aos_levels = levels;
+
+    // tensorized_paths[levels - 1] is that boundary's own tensorized path:
+    // every segment up to and including its "[]" marker.
+    std::string resolved_name = (levels == 0) ? std::string() : tensorized_paths[levels - 1];
+    for (size_t i = matched_segments; i < target_segments.size(); i++) {
+        if (!resolved_name.empty())
+            resolved_name += TENSORIZED_SEPARATOR;
+        resolved_name += target_segments[i];
+    }
+    return resolved_name;
+}
+
 
 hid_t HDF5Utils::createOrOpenHDF5Group(const std::string & path, const hid_t & parent_loc_id)
 {

src/hdf5/hdf5_utils.h

@@ -79,6 +79,18 @@ class HDF5Utils {
     void getAOSIndices(Context * ctx, std::vector < int >&indices, int *timedAOS_index);
     int getAOSIndicesSize(Context * ctx);
     void setTensorizedPaths(ArraystructContext * ctx, std::vector < std::string > &tensorized_paths);
+
+    // Absolute field paths (issue #65). isAbsoluteFieldPath() must be asked
+    // before '/' separators are normalized to '&'; resolveAbsoluteFieldPath()
+    // takes the already-normalized name, the tensorized paths of the open AOS
+    // chain (innermost last, as setTensorizedPaths builds them) and reports,
+    // through applicable_aos_levels, how many of those levels the resolved
+    // dataset still lives under -- i.e. how many of the caller's current AOS
+    // indices apply to it. See the block comment in hdf5_utils.cpp.
+    static bool isAbsoluteFieldPath(const std::string & field);
+    static std::string resolveAbsoluteFieldPath(const std::string & absolute_dataset_name,
+                                                const std::vector < std::string > &tensorized_paths,
+                                                size_t * applicable_aos_levels);
     void showStatus(hid_t file_id);
     enum Files_paths_strategies { FULL_MDSPLUS_STRATEGY = 1, MODIFIED_MDSPLUS_STRATEGY = 2, FREE_PATH_STRATEGY = 3};
     void setDefaultOptions(size_t *read_cache, size_t *write_cache, bool *readBuffering, bool *writeBuffering);

tests/contract/CMakeLists.txt

@@ -70,6 +70,7 @@ add_executable(contract_tests
   test_plugins.cpp
   test_backend_factory.cpp
   test_hdf5_version_drift.cpp
+  test_hdf5_absolute_paths.cpp
   real_dd_path_catalog.cpp
   test_real_dd_path_catalog.cpp
   test_structured_data.cpp