Skip to content

[ntuple] Add GetView<void> with type name string or std::type_info argument #18185

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Apr 11, 2025

Conversation

enirolf
Copy link
Contributor

@enirolf enirolf commented Mar 28, 2025

Enables passing a type name string to GetView<void> to signal that the pointer has a different underlying type than the on-disk field's type. The field used by RNTupleView will be constructed with this type, in turn benefitting from RNTuples built-in type- and bounds-checking.

To illustrate further, assuming myInt is a std::uint64_t on-disk:

std::int32_t myInt;
void* myIntPtr = &myInt;
auto myIntView = reader->GetView<void>("myInt", myIntPtr);
myIntView(0);
doSomething(myInt);

GetView<void> will currently always reconstruct the field used for loading the values from the on-disk descriptor. However, in the example above this can lead to undefined behavior when the actual values on disk don't fit in std::int32_t. The change proposed in this PR enables passing the type name string to GetView<void>, which is then used for the construction of the field instead, leveraging RNTuple's internal type- and bounds-checking:

std::int32_t myInt;
void* myIntPtr = &myInt;
auto myIntView = reader->GetView<void>("myInt", myIntPtr, "std::int32_t");
myIntView(0);
doSomething(myInt);

This feature was requested by and discussed with ATLAS.

@enirolf enirolf self-assigned this Mar 28, 2025
@enirolf enirolf requested a review from jblomer as a code owner March 28, 2025 10:21
Copy link
Contributor

@silverweed silverweed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just one question that probably doesn't belong to this PR

Copy link

github-actions bot commented Mar 28, 2025

Test Results

    17 files      17 suites   4d 22h 16m 20s ⏱️
 2 688 tests  2 688 ✅ 0 💤 0 ❌
45 074 runs  45 074 ✅ 0 💤 0 ❌

Results for commit 1d32f22.

♻️ This comment has been updated with latest results.

Copy link
Member

@hahnjo hahnjo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG, thanks for the clear implementation!

@enirolf enirolf force-pushed the ntuple-view-typename branch from 6f2228b to bb4d08e Compare April 2, 2025 08:11
@enirolf enirolf changed the title [ntuple] Add GetView<void> with type name string argument [ntuple] Add GetView<void> with type name string or std::type_info argument Apr 2, 2025
@enirolf enirolf force-pushed the ntuple-view-typename branch from bb4d08e to 51503fd Compare April 2, 2025 08:12
@enirolf enirolf requested review from hahnjo, silverweed and pcanal April 2, 2025 08:12
Copy link
Contributor

@jblomer jblomer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many thanks! LGTM!

Copy link
Member

@vepadulano vepadulano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! But see comment before merging

@enirolf enirolf force-pushed the ntuple-view-typename branch 2 times, most recently from 7ed3e89 to dec2acd Compare April 4, 2025 13:22
@hahnjo hahnjo force-pushed the ntuple-view-typename branch from dec2acd to ddebfca Compare April 8, 2025 13:42
@hahnjo
Copy link
Member

hahnjo commented Apr 8, 2025

As discussed on Friday, I removed the shared_ptr overload and un-templated GetView instead of the static_assert that T = void

@hahnjo hahnjo self-requested a review April 8, 2025 13:43
@hahnjo hahnjo added this to the 6.36.00 milestone Apr 8, 2025
Copy link
Member

@vepadulano vepadulano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed as discussed, LGTM

enirolf and others added 5 commits April 10, 2025 17:40
Enables passing a type name string to `GetView<void>` to signal that the
pointer has a different underlying type than the on-disk field's type.
The field used by `RNTupleView` will be constructed with this type, in
turn benefitting from RNTuples built-in type- and bounds-checking.
...which in turn calls the overload that takes a type name string.
@hahnjo hahnjo force-pushed the ntuple-view-typename branch from ddebfca to 1d32f22 Compare April 10, 2025 16:00
@hahnjo hahnjo merged commit 4e6fa0d into root-project:master Apr 11, 2025
21 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants