Propagate drop_variables into HDFParser subgroup recursion - #1058
Open
aladinor wants to merge 1 commit into
Open
Propagate drop_variables into HDFParser subgroup recursion#1058aladinor wants to merge 1 commit into
aladinor wants to merge 1 commit into
Conversation
_construct_manifest_group applied drop_variables only at the level the parser was called on: the recursive call for child groups omitted the parameter, so it reset to None at every level of descent. A dropped name nested one group deeper was still parsed, and if that dataset had a dtype zarr cannot represent (e.g. ODIM_H5's compound-dtype legend tables) the whole parse failed even though the user excluded it. The user-supplied names now propagate into the recursion, so a listed name is dropped wherever it appears in the walked hierarchy - matching the behaviour of the Zarr and Icechunk parsers' recursive walks. The per-group auto-detected non-coordinate dimension vars are kept separate and applied only to the current group's datasets, where they could previously never match a child group's name anyway. Closes zarr-developers#1057
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1058 +/- ##
=======================================
Coverage 90.39% 90.39%
=======================================
Files 37 37
Lines 2248 2248
=======================================
Hits 2032 2032
Misses 216 216
🚀 New features to boost your workflow:
|
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.
Closes #1057
Problem
HDFParser(drop_variables=[...])filtered variables only at the group the parser was called on._construct_manifest_group's recursive call for child groups omitted the parameter, so it reset toNoneat every level of descent — a dropped name nested one group deeper was still parsed. If that nested dataset has a dtype zarr cannot represent (e.g. a compound dtype with a vlen-string field), the whole parse failed even though the user explicitly excluded it. Real-world case: ODIM_H5 weather-radar volumes (FMI's open archive, files from 2026 on) carry compound-dtypequality*/legendtables under every sweep group, making any root-group parse impossible.Fix
The user-supplied
drop_variablesset now propagates into the subgroup recursion, so a listed name is dropped wherever it appears in the walked hierarchy — the semantics the parameter's docstring already implied, and the same behaviour as the Zarr/Icechunk parsers' recursive walks (construct_manifest_group_treeappliesskip_variablesat every level).One subtlety: the per-group auto-detected
non_coordinate_dimension_vars(the big-endian workaround set) are deliberately not propagated — they are level-specific, and leaking them into sibling subtrees could silently drop an identically named, perfectly valid variable. They are now applied only to the current group's datasets comprehension, where they could previously never match a child group's name anyway (they are always dataset names, and HDF5 group namespaces are unique).Tests
test_drop_variables_in_nested_groups— the regression: parsing the nested compound-dtype file fails without the drop, succeeds with it (newnested_compound_dtype_hdf5_urlfixture mirroring the issue MRE)test_drop_variables_applies_at_every_depth— a name present at two depths is dropped at bothtest_drop_variables_group_name_skips_subtree— a group name indrop_variablesskips the whole subtreetest_dimension_var_drops_stay_group_local— an auto-dropped dimension var in one group does not drop an identically named variable in a sibling groupmanifest_store_from_hdf_url(test helper) gained adrop_variablespassthrough so the new tests follow the module's existing pattern.