Repository navigation
fix(rntuple): preserve optional records when writing and reading (#1713) - #1779
sumitsaraswat362 wants to merge 1 commit into
Conversation
d0a3d20 to
4bb2ce2
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
|
ariostas
left a comment
There was a problem hiding this comment.
🤖 AI text below 🤖
Thanks for working on this! The uproot round trip works, but ROOT can't open the files this writes; details inline.
| raise AssertionError(msg) | ||
| if not subcall and "UntypedRecord" in typename: | ||
| typename = "" # empty types for anything that contains UntypedRecord | ||
| if typename.startswith("std::optional"): |
There was a problem hiding this comment.
🤖 AI text below 🤖
With this change, files with an optional record can no longer be opened by ROOT. Writing ak.Array([{"o": {"a": 1.0}}, {"o": None}]) (the case in the new test) records the field types as ['std::optional<>', '', 'double'], and ROOT 6.40.02 fails on it:
RException: no type name specified for field '_0'
The same data written from main (types ['', '', 'double']) opens in ROOT as a collection.
| if not subcall and "UntypedRecord" in typename: | ||
| typename = "" # empty types for anything that contains UntypedRecord | ||
| if typename.startswith("std::optional"): | ||
| typename = typename.replace("UntypedRecord", "") |
There was a problem hiding this comment.
🤖 AI text below 🤖
replace("UntypedRecord", "") removes the marker at every nesting level, not only where the optional wraps the record, so nested types get malformed C++ names written to disk:
ak.Array([{"o": (1.0, {"x": 2})}, {"o": None}])->std::optional<std::tuple<double,>>ak.Array([{"o": [{"a": 1.0}]}, {"o": None}])->std::optional<std::vector<>>
uproot reads these back only because the reader checks startswith("std::optional"), but ROOT rejects them and users would see them in RField.typename.
| @@ -114,7 +114,10 @@ def _cpp_typename(akform, subcall=False): | |||
| msg = "This should not have happened. Please report this issue to the Uproot developers." | |||
| raise AssertionError(msg) | |||
| if not subcall and "UntypedRecord" in typename: | |||
There was a problem hiding this comment.
🤖 AI text below 🤖
Neither option here gives a file that both ROOT and uproot read correctly: an empty type name makes uproot read None back as [] (#1713), while std::optional<...> around an untyped record makes the file unreadable by ROOT. I think this needs to be decided in the IndexedOptionForm branch rather than by post-processing the string, e.g. keep the ROOT-compatible type name and record the optional-ness in a way the reader can detect, or reject/warn on optional untyped records.
More generally, the "UntypedRecord" placeholder plus substring checks means every wrapper (std::array, std::variant, tuples, ...) needs its own patch-up. Having subcalls report "untyped" explicitly (e.g. returning None or a flag) and handling it in each container branch would avoid the global string replacement.
| import uproot | ||
|
|
||
|
|
||
| def test_optional_record_round_trip(tmp_path): |
There was a problem hiding this comment.
🤖 AI text below 🤖
These tests only round-trip through uproot, so they pass even though ROOT can't open the file. Could you add a ROOT read-back check guarded by pytest.importorskip("ROOT"), like tests/test_0705_rntuple_writing_metadata.py does? It would have caught the issue above.
|
|
||
|
|
||
| def test_optional_record_extend_and_dask(tmp_path): | ||
| pytest.importorskip("dask_awkward") |
There was a problem hiding this comment.
🤖 AI text below 🤖
Because importorskip("dask_awkward") is at the top, the update()/extend() round trip is skipped entirely when dask_awkward isn't installed. Could the extend check go in its own test, with the dask part separate?
It would also be good to cover the nested cases (optional tuple, optional list of records) and assert the on-disk type names.
4bb2ce2 to
89a2b7c
Compare
|
Thanks for catching the ROOT interoperability issue and pointing out the malformed template names in nested containers! I have refactored this to address all the points:
|
Resolves #1713.
Description
When writing an optional record such as
?{a: float64},_cascadentuple._cpp_typenamegeneratedstd::optional<UntypedRecord>. The subsequent checkif not subcall and "UntypedRecord" in typename: typename = ""erased the entire string to"", stripping thestd::optionalmarker from the collection field description on disk. As a consequence, reading the RNTuple fell back toListOffsetForm, silently convertingNoneinto empty lists[].This PR narrows the blank out in
_cascadentuple._cpp_typename:typename.startswith("std::optional"),"UntypedRecord"is removed from the template argument sostd::optional<>survives on the collection field record.RecordForm) with empty type names remain unaffected and retain emptytype_nameforadd_fieldscompatibility.self._all_fields[cfid].record.type_name.startswith("std::optional")successfully matches, reconstructingak.forms.IndexedOptionForm(idx_type, inner)whereinneris the childRecordForm.Tests
Added
tests/test_1713_rntuple_optional_records.pycovering:test_optional_record_round_trip: Full round trip, single field reading (nt["o"]), dotted subfield access (nt["o.a"]), and filtering.test_optional_record_with_multiple_fields: Multiple fields under an optional record.test_optional_record_extend_and_dask:extend()afterupdate()anduproot.daskcomputation.