Add test for attributes#77
Conversation
jblomer
left a comment
jblomer
left a comment
There was a problem hiding this comment.
In principle looks good to me. Since we are at it, I think we should make the test slightly more interesting and write three entries, with one attribute attached to the first entry and another attribute attached to the second and third entry.
And then, for a corner case, also test an attribute for an empty RNTuple.
| os << "}\n"; | ||
| return; | ||
| } catch (const runtime_error &e) { | ||
| cout << "Skipped structure/attributes/read.C - " << e.what() << endl; |
There was a problem hiding this comment.
Should we handle the case of missing structure.attributes.root more explicitly? That is the only error that I think is permissable, all others should make the test fail.
There was a problem hiding this comment.
AFAICT the code hasn't changed, a missing structure.attributes.root is not handled explicitly...
There was a problem hiding this comment.
Explicit error handling for missing file was added.
undefined-panda
force-pushed
the
attributes-test
branch
3 times, most recently
from
0fa2cd4 to
047cee5
Compare
| } else { | ||
| os << " }\n"; | ||
| } | ||
| std::cout << " attr = " << *attr << " (valid for range [" << *range.GetFirst() << ", " << *range.GetLast() << "])\n"; |
| // same logic as is read_structure.hxx, with additional special part for | ||
| // attributes |
There was a problem hiding this comment.
Actually, the latest code doesn't print the main entry anymore. Probably we don't need this, but then we should update / remove this comment.
There was a problem hiding this comment.
Comment updated
| auto Int32 = | ||
| reader->GetModel().GetDefaultEntry().GetPtr<std::int32_t>("Int32"); |
| // list of values for attr with its range | ||
| os << " {\n"; | ||
| os << " \"Attr\": " << "[\n"; | ||
| first = true; |
|
|
||
| ## Fields | ||
|
|
||
| * `Int{32}` |
There was a problem hiding this comment.
| * `Int{32}` | |
| * `Int32` |
| os << "}\n"; | ||
| return; | ||
| } catch (const runtime_error &e) { | ||
| cout << "Skipped structure/attributes/read.C - " << e.what() << endl; |
There was a problem hiding this comment.
AFAICT the code hasn't changed, a missing structure.attributes.root is not handled explicitly...
| 1. Attribute set name | ||
| 2. Attribute set description | ||
| 3. List of attribute entries with value and regular field range |
There was a problem hiding this comment.
These are not (RNTuple) entries. What you want to describe here is that there is one attribute entry spanning one main entry, and the second attribute entry spanning two main entries.
| ## Fields | ||
|
|
||
| * `Int{32}` | ||
| * `Attr` |
There was a problem hiding this comment.
In the code it's "attr" (lower-case). Maybe you can also make it explicit that Int32 is the field in the main RNTuple, and attr is the field in the attribute model
undefined-panda
force-pushed
the
attributes-test
branch
from
e84cbd2 to
93792a4
Compare
3 participants
Skip write.C for versions that don't support attributes (everything before 6.40) and read.C if binary for attributes doesn't exist.