[ntuple] Map integral types to fixed width equivalents #16039

hahnjo · 2024-07-16T17:03:44Z

This already used to work for some types, such as int, which happen to have the same underlying storage as a fixed width type. This change adds a general RIntegralTypeMap with specializations for standard integer types. As a result, RNTuple gains support for (unsigned) long long on Linux, which is now mapped to [u]int64_t.

github-actions · 2024-07-16T18:46:30Z

Test Results

13 files 13 suites 2d 20h 45m 59s ⏱️
2 677 tests 2 677 ✅ 0 💤 0 ❌
32 708 runs 32 708 ✅ 0 💤 0 ❌

Results for commit a764b19.

♻️ This comment has been updated with latest results.

hahnjo · 2024-07-19T09:36:27Z

(no changes yet, just to test that the incremental CI is now properly fixed for PRs)

hahnjo · 2024-07-22T10:00:35Z

Seems to work, all tests pass --> ready for review 😃

jblomer

In principle LGTM. Only question about the use of RIntegralField in the visitor.

jblomer · 2024-07-23T15:28:20Z

tree/ntuple/v7/inc/ROOT/RFieldVisitor.hxx

-   virtual void VisitInt16Field(const RField<std::int16_t> &field) { VisitField(field); }
-   virtual void VisitInt32Field(const RField<std::int32_t> &field) { VisitField(field); }
-   virtual void VisitInt64Field(const RField<std::int64_t> &field) { VisitField(field); }
+   virtual void VisitInt8Field(const RIntegralField<std::int8_t> &field) { VisitField(field); }


Shouldn't we pass an RField<integer type> here and for the others? RIntegralField is an implementation details, isn't it?

So I agreed yesterday evening, but when looking into it I remembered why I decided to pass RIntegralField: the solution proposed in this PR builds on a surjective, but not injective function that maps any integral type (the T in RField<T>) onto one of the (fewer) fixed width integer types (the T in RIntegralField, mapped via RIntegralTypeMap). We could continue to pass RFields, but we wouldn't know which of RField<long>, RField<long long> and RField<std::int64_t> to implement in the interface (Linux maps typedef long int64_t; while macOS does typedef long long int64_t;).

In practical terms it would also not be nice to implement because AcceptVisitor is currently a part of RIntegralField, where *this doesn't have type RField<T>. We would either have to add dynamic casting in there, or move AcceptVisitor to the templated RField<T>. That in turn would again require specializations because the name of the Visit function depends on the type...

jblomer · 2024-07-23T15:29:51Z

gui/browsable/src/RFieldProvider.hxx

@@ -80,7 +83,7 @@ class RFieldProvider : public RProvider {
      }

      template<typename T>
-      void FillHistogram(const RField<T> &field)
+      void FillHistogramImpl(const ROOT::Experimental::RFieldBase &field, ROOT::Experimental::RNTupleView<T, false> &view)


We should not need to change this, I think.

Based on my other comment, I think we are forced to change this... 😐

Ok, let's not forget the follow-up from your comment to remove the tainting of the ROOT namespace

It was the only type translation for the "signed" modifier. If this is needed, it should probably be handled programmatically in GetNormalizedTypeName().

In order to map standard integer types to fixed width equivalents, we want to introduce a C++ type mapping which requires one level of template indirection. For the moment, RField<T> just inherits from RIntegralType<T> for integral types T. Co-authored-by: Florine de Geus <[email protected]>

This already used to work for some types, such as int, which happen to have the same underlying storage as a fixed width type. This change adds a general RIntegralTypeMap with specializations for standard integer types. As a result, RNTuple gains support for (unsigned) long long on Linux, which is now mapped to [u]int64_t. Co-authored-by: Florine de Geus <[email protected]>

Remove or change entries from type translation map and explicitly instantiate RField<T> for standard integer types, which will be mapped to RIntegralType using RIntegralTypeMap since the last commit. Co-authored-by: Florine de Geus <[email protected]>

This is important because the call to RColumn::Map[V] must use the fixed width integer type, but the returned pointer type must match the (unmapped) RField type. Test this via RNTupleView's of standard integer types.

hahnjo · 2024-07-24T09:06:24Z

I only did two minor changes to the existing commits, as discussed yesterday evening:

diff --git a/tree/ntuple/v7/inc/ROOT/RField.hxx b/tree/ntuple/v7/inc/ROOT/RField.hxx
index 7c9ccebeaa..411ef5733c 100644
--- a/tree/ntuple/v7/inc/ROOT/RField.hxx
+++ b/tree/ntuple/v7/inc/ROOT/RField.hxx
@@ -2179,9 +2179,8 @@ public:
 
 template <typename T>
 class RIntegralField {
-   // Cannot say static_assert(false) because not all compilers implement CWG2518 yet...
-   static_assert(std::is_integral_v<T>, "RIntegralField requires integral type");
-   static_assert(!std::is_integral_v<T>, "unsupported integral type");
+   // Instantiating this base template definition should never happen and is an error!
+   RIntegralField() = delete;
 };
 
 template <>
@@ -2573,6 +2572,8 @@ template <typename T>
 class RField<T, typename std::enable_if<std::is_integral_v<T>>::type> final
    : public RIntegralField<typename Internal::RIntegralTypeMap<T>::type> {
    using MappedType = typename Internal::RIntegralTypeMap<T>::type;
+   static_assert(sizeof(T) == sizeof(MappedType), "invalid size of mapped type");
+   static_assert(std::is_signed_v<T> == std::is_signed_v<MappedType>, "invalid signedness of mapped type");
 
 public:
    RField(std::string_view name) : RIntegralField<MappedType>(name) {}

Eventually, after implementing the changes, I decided to hold off moving some member functions to the templated RField specialization: It didn't work for the Map[V] functions because RColumn::Map[V] has to be called with the fixed width integer type after mapping. Instead the newly added last commit reinterpret_casts the pointer since we can guarantee that the mapped type has identical storage layout. This solves the problem with RNTupleViews, discussed on Mattermost. I still plan to come back to possible deduplications and simplifications in a follow-up PR (now #16101).

With the addition of `RIntegralTypeMap` (PR root-project#16039), most of the type name deduction for type-erased fields was moved from the type translation map to `RField`'s template specialization. Upon creation of type-erased STL map(-like) types, originally only the type translation map was used. This now can cause issues where the map's item types can be non-normalized. By first explicitly creating fields for the items and using the type names from those fields, this issue is resolved.

With the addition of `RIntegralTypeMap` (PR root-project#16039), most of the type name deduction for type-erased fields was moved from the type translation map to `RField`'s template specialization. Upon creation of type-erased STL map(-like) types, originally only the type translation map was used. This now can cause issues where the map's item types may be non-normalized. By using the type names of the subfields of the `std::pair` item field, we ensure the correct inner type names are used.

With the addition of `RIntegralTypeMap` (PR #16039), most of the type name deduction for type-erased fields was moved from the type translation map to `RField`'s template specialization. Upon creation of type-erased STL map(-like) types, originally only the type translation map was used. This now can cause issues where the map's item types may be non-normalized. By using the type names of the subfields of the `std::pair` item field, we ensure the correct inner type names are used.

With the addition of `RIntegralTypeMap` (PR root-project#16039), most of the type name deduction for type-erased fields was moved from the type translation map to `RField`'s template specialization. Upon creation of type-erased STL map(-like) types, originally only the type translation map was used. This now can cause issues where the map's item types may be non-normalized. By using the type names of the subfields of the `std::pair` item field, we ensure the correct inner type names are used.

hahnjo added the in:RNTuple label Jul 16, 2024

hahnjo requested review from jblomer, silverweed, vepadulano and enirolf July 16, 2024 17:03

hahnjo self-assigned this Jul 16, 2024

hahnjo force-pushed the ntuple-RIntegralField branch from e245189 to d573122 Compare July 19, 2024 09:36

hahnjo force-pushed the ntuple-RIntegralField branch from d573122 to 1049206 Compare July 22, 2024 08:42

hahnjo marked this pull request as ready for review July 22, 2024 10:00

hahnjo requested review from bellenot and linev as code owners July 22, 2024 10:00

hahnjo force-pushed the ntuple-RIntegralField branch 2 times, most recently from 0f885e1 to d607160 Compare July 23, 2024 14:18

jblomer reviewed Jul 23, 2024

View reviewed changes

hahnjo and others added 7 commits July 24, 2024 09:27

[ntuple] Remove "signed char" normalization

19340d2

It was the only type translation for the "signed" modifier. If this is needed, it should probably be handled programmatically in GetNormalizedTypeName().

[ntuple] Extend RField testing with integral types

278eabd

[ntuple] Add RField testing of standard integer types

f314130

[ntuple] Fix Map[V] return type for mapped integral types

a764b19

This is important because the call to RColumn::Map[V] must use the fixed width integer type, but the returned pointer type must match the (unmapped) RField type. Test this via RNTupleView's of standard integer types.

hahnjo force-pushed the ntuple-RIntegralField branch from d607160 to a764b19 Compare July 24, 2024 09:05

This was referenced Jul 24, 2024

[ntuple] Deduplicate code in RField.hxx #16101

Merged

[ntuple] Split RField.hxx header #16103

Merged

jblomer approved these changes Jul 24, 2024

View reviewed changes

jblomer mentioned this pull request Jul 24, 2024

[ntuple] Improve normalization of platform-specific primitives and typedefs #7875

Closed

jblomer mentioned this pull request Jul 24, 2024

[ntuple] Check the usage of Long_t in RField (size of the type might differ between 32- and 64-bit platforms) #11616

Open

1 task

hahnjo merged commit 08b97d1 into root-project:master Jul 25, 2024
17 of 18 checks passed

hahnjo deleted the ntuple-RIntegralField branch July 25, 2024 05:37

jblomer mentioned this pull request Jul 25, 2024

[ntuple] Add support for (unsigned) long long fields #15838

Closed

enirolf mentioned this pull request Aug 12, 2024

[ntuple] Fix item type names for STL map fields #16216

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ntuple] Map integral types to fixed width equivalents #16039

[ntuple] Map integral types to fixed width equivalents #16039

hahnjo commented Jul 16, 2024 •

edited

Loading

github-actions bot commented Jul 16, 2024 •

edited

Loading

hahnjo commented Jul 19, 2024

hahnjo commented Jul 22, 2024

jblomer left a comment

jblomer Jul 23, 2024

hahnjo Jul 24, 2024

jblomer Jul 23, 2024

hahnjo Jul 24, 2024

jblomer Jul 24, 2024

hahnjo commented Jul 24, 2024 •

edited

Loading

[ntuple] Map integral types to fixed width equivalents #16039

[ntuple] Map integral types to fixed width equivalents #16039

Conversation

hahnjo commented Jul 16, 2024 • edited Loading

github-actions bot commented Jul 16, 2024 • edited Loading

Test Results

hahnjo commented Jul 19, 2024

hahnjo commented Jul 22, 2024

jblomer left a comment

Choose a reason for hiding this comment

jblomer Jul 23, 2024

Choose a reason for hiding this comment

hahnjo Jul 24, 2024

Choose a reason for hiding this comment

jblomer Jul 23, 2024

Choose a reason for hiding this comment

hahnjo Jul 24, 2024

Choose a reason for hiding this comment

jblomer Jul 24, 2024

Choose a reason for hiding this comment

hahnjo commented Jul 24, 2024 • edited Loading

hahnjo commented Jul 16, 2024 •

edited

Loading

github-actions bot commented Jul 16, 2024 •

edited

Loading

hahnjo commented Jul 24, 2024 •

edited

Loading