[ntuple] Binary format RC3 #16645

jblomer · 2024-10-09T13:06:55Z

Version leading into the 1.0 format. Final cleanups and removal of backward compatibility code for previous experimental anchors.

github-actions · 2024-10-09T19:02:09Z

Test Results

18 files 18 suites 3d 18h 36m 23s ⏱️
2 699 tests 2 697 ✅ 1 💤 1 ❌
46 129 runs 46 128 ✅ 0 💤 1 ❌

For more details on these failures, see this check.

Results for commit 2a90c5f.

♻️ This comment has been updated with latest results.

hahnjo

two quick questions regarding RNTuple::Streamer()

hahnjo · 2024-10-10T10:59:15Z

tree/ntuple/v7/src/RNTuple.cxx

-      auto classVersion = buf.ReadVersion(&offClassBuf, &bcnt);
-      if (classVersion < 4)
-         throw RException(R__FAIL("unsupported RNTuple pre-release"));
+      buf.ReadVersion(&offClassBuf, &bcnt);


Do we need this? My understanding is that it was required for the custom streaming of class version 4. I didn't check how this interferes with the checksum below, but I believe that offCkData takes care of skipping the class version already.

Related question, how does the reading fail after the PR when reading old (unsupported) RNtuple files?

It fails to open à la "ROOT::Experimental::RNTuple is not an RNTuple". It's like trying to open an RNTuple from a TTree or and TH1F key.

Do we need this? My understanding is that it was required for the custom streaming of class version 4. I didn't check how this interferes with the checksum below, but I believe that offCkData takes care of skipping the class version already.

I thought we need this to get the byte count (bcnt).. @silverweed ?

Ah... I believe we can reverse the operations: first read the "final" ReadClassBuffer, then compute the checksum and compare it to the value on disk.

This is what I had in mind: hahnjo@3649c92 (feel free to just pick into this PR, or we can do it as follow-up) It compares the checksum only after reading the class, but this is likely fine

Now submitted as #16956

hahnjo · 2024-10-10T11:00:21Z

tree/ntuple/v7/src/RNTuple.cxx

+      // Rewind the version bytes, as ReadClassBuffer needs to read the version again.
+      buf.SetBufferOffset(offClassBuf);


see above; maybe we can remove this?

pcanal · 2024-10-10T14:59:35Z

tree/ntuple/v7/src/RNTuple.cxx

+      buf.SetBufferOffset(offClassBuf);
+      buf.ReadClassBuffer(RNTuple::Class(), this);


I missed this the first time around, we should be using:

virtual Int_t ReadClassBuffer(const TClass *cl, void *pointer, Int_t version, UInt_t start, UInt_t count, const TClass *onfile_class = nullptr) = 0;

Suggested change

buf.SetBufferOffset(offClassBuf);

buf.ReadClassBuffer(RNTuple::Class(), this);

buf.ReadClassBuffer(RNTuple::Class(), this, classVersion, bcnt);

rather than do the version reading twice.

jblomer · 2024-10-11T14:39:35Z

Draft for now. We don't want to merge it before CHEP in order to not impact benchmarks done on master.

Make the on-disk type name of the cardinality field spell `ROOT::RNTupleCardinality<T>` instead of `ROOT::Experimental::RNTupleCardinality<T>`.

pcanal · 2024-10-31T13:36:12Z

tree/dataframe/inc/ROOT/RNTupleDS.hxx

-namespace Experimental {
+class RNTuple;

+namespace Experimental {


Why are those still in Experimental? Should they be in Internal and/or Detail?

As discussed, at this point only type names that are part of the on-disk format move out of experimental.

pcanal · 2024-10-31T13:43:05Z

tree/ntuple/v7/src/RNTupleMerger.cxx

 #include <vector>

 using namespace ROOT::Experimental;
 using namespace ROOT::Experimental::Internal;


Suggested change

using namespace ROOT::Experimental::Internal;

using namespace ROOT;

using namespace ROOT::Experimental::Internal;

(and keeping the rest of the code as it was).
seems to match 'better' the spirit of this file coding style.

I think the idea of using namespace ROOT::Experimental::Internal was avoiding long boilerplate strings throughout the file. However, I find the ROOT:: prefix short and concise and I'd have a preference of writing ROOT::RNTuple rather than RNTuple (similar to std::string over string).

jblomer · 2024-11-01T04:30:05Z

Note: clang-format is fine with the changes except that it doesn't understand string literals.

hahnjo

LGTM

enirolf

Two small comments, LGTM otherwise!

tree/ntuple/v7/doc/BinaryFormatSpecification.md

enirolf · 2024-11-04T09:13:03Z

tree/ntuple/v7/src/RMiniFile.cxx

   offset = key.GetSeekKey() + key.fKeyLen;

-   constexpr size_t kMinNTupleSize = 70; // size of a RTFNTuple version 4 (min supported version)
+   constexpr size_t kMinNTupleSize = 78; // size of a RTFNTuple version 2 (min supported version)


Is there a particular reason RTFNuple::GetSizePlusChecksum is not used here instead?

I believe the idea is that the anchor size can grow in the future, so we have to maintain kMinNTupleSize independently of the RTFNuple structure

But that contradicts the comment next to it then, no?

Hm not sure, for me it's quite clear:

For future versions 3, 4, 5, ... RTFNTuple gains more members that have a default initialization. That will increase RTFNuple::GetSizePlusChecksum

This exact line will forever stay like this because it was the size of RTFNTuple version 2, when we declared RNTuple 1.0.0.0, and we will always be able to read it back.

I'll left the hard-coded 78 but added a static_assert, which makes it clear that this number is currently identical to RTFNuple::GetSizePlusChecksum(). If we grow the anchor in the future, the static assert will remind us of this spot.

Moves ROOT::Experimental::RNTuple to ROOT::RNTuple. Resets class version to 2 (general first class version recommendation for new classes). Removes previous compatibility code.

For historical reasons, the current column flags are 0x08 and 0x10. Reset to 0x01 and 0x02 for format RC3.

Make sure that we can easily extend the field meta-data by putting all the mandatory components first and the optional components last.

There is no (foreseen) use of a version range for the extra type info. Its only current purpose, storing streamer info records, does not need the version at all. A possible future use is storing enum constant names, but that would also be bound to a specific type version and not to a version range. If needed in a future version, a type version range can be added then.

pcanal · 2024-11-04T18:28:43Z

tree/ntuple/v7/src/RMiniFile.cxx

-   constexpr size_t kMinNTupleSize = 70; // size of a RTFNTuple version 4 (min supported version)
+   // size of a RTFNTuple version 2 (min supported version); future anchor versions can grow.
+   constexpr size_t kMinNTupleSize = 78;
+   static_assert(kMinNTupleSize == RTFNTuple::GetSizePlusChecksum());


< static_assert(kMinNTupleSize == RTFNTuple::GetSizePlusChecksum());

static_assert(kMinNTupleSize <= RTFNTuple::GetSizePlusChecksum());

Moelf · 2024-11-13T18:07:18Z

tree/ntuple/v7/doc/BinaryFormatSpecification.md

-| 0x08 |   32 | Real32       | IEEE-754 single precision float                                                               |
-| 0x09 |   16 | Real16       | IEEE-754 half precision float                                                                 |
-| 0x16 |   64 | Int64        | Two's complement, little-endian 8-byte signed integer                                         |
+| 0x00 |    1 | Bit          | Boolean value                                                                                 |


why does this start at 0x00 instead of 0x01 now?

That was part of the format cleanup. It seemed unclear why we should not not start at 0.

jblomer added the in:RNTuple label Oct 9, 2024

jblomer requested review from enirolf, hahnjo and silverweed October 9, 2024 13:06

jblomer self-assigned this Oct 9, 2024

jblomer requested review from bellenot, linev, martamaja10, pcanal and vepadulano as code owners October 9, 2024 13:06

jblomer marked this pull request as draft October 9, 2024 13:07

jblomer force-pushed the ntuple-rc3 branch from 1e8e53f to 757b9c9 Compare October 9, 2024 15:10

hahnjo reviewed Oct 10, 2024

View reviewed changes

jblomer force-pushed the ntuple-rc3 branch 3 times, most recently from 5b2f516 to 23e17c9 Compare October 10, 2024 14:27

pcanal reviewed Oct 10, 2024

View reviewed changes

jblomer force-pushed the ntuple-rc3 branch from 23e17c9 to 4528827 Compare October 11, 2024 08:33

jblomer marked this pull request as ready for review October 11, 2024 08:33

jblomer requested a review from couet as a code owner October 11, 2024 08:33

jblomer marked this pull request as draft October 11, 2024 09:05

jblomer force-pushed the ntuple-rc3 branch from 4528827 to 1922d09 Compare October 11, 2024 09:10

jblomer marked this pull request as ready for review October 11, 2024 09:42

jblomer force-pushed the ntuple-rc3 branch from 1922d09 to 0c93b6f Compare October 11, 2024 12:17

jblomer requested review from hahnjo and pcanal October 11, 2024 12:21

jblomer marked this pull request as draft October 11, 2024 14:39

jblomer force-pushed the ntuple-rc3 branch from 0c93b6f to c5ff9eb Compare October 12, 2024 09:32

jblomer force-pushed the ntuple-rc3 branch 2 times, most recently from 43cefee to 3397df9 Compare October 31, 2024 08:42

jblomer added 2 commits October 31, 2024 13:27

[ntuple] bump anchor version to RC3

54fa7e9

[ntuple] Fix RNTupleCardinality on-disk type name

08dd336

Make the on-disk type name of the cardinality field spell `ROOT::RNTupleCardinality<T>` instead of `ROOT::Experimental::RNTupleCardinality<T>`.

jblomer force-pushed the ntuple-rc3 branch from 3397df9 to aef0fb6 Compare October 31, 2024 13:03

jblomer marked this pull request as ready for review October 31, 2024 13:15

pcanal reviewed Oct 31, 2024

View reviewed changes

jblomer force-pushed the ntuple-rc3 branch 3 times, most recently from 5289735 to 294f9a6 Compare November 1, 2024 04:11

jblomer requested review from hahnjo and pcanal November 1, 2024 04:11

hahnjo approved these changes Nov 4, 2024

View reviewed changes

enirolf approved these changes Nov 4, 2024

View reviewed changes

jblomer force-pushed the ntuple-rc3 branch from f004a54 to 89e3f7b Compare November 4, 2024 10:41

jblomer added 7 commits November 4, 2024 11:45

[ntuple] move anchor out of experimental namespace

fb32cb1

Moves ROOT::Experimental::RNTuple to ROOT::RNTuple. Resets class version to 2 (general first class version recommendation for new classes). Removes previous compatibility code.

[ntuple] remove column groups

c5887c1

[ntuple] reset on-disk column flags

8528ac3

For historical reasons, the current column flags are 0x08 and 0x10. Reset to 0x01 and 0x02 for format RC3.

[ntuple] field s11n: optional bits last

c211a5e

Make sure that we can easily extend the field meta-data by putting all the mandatory components first and the optional components last.

[ntuple] reorder column constants

4fed802

[ntuple] update links to input files to RC3

2a90c5f

jblomer force-pushed the ntuple-rc3 branch from 89e3f7b to 2a90c5f Compare November 4, 2024 10:45

jblomer merged commit d2a8b3c into root-project:master Nov 4, 2024
18 of 21 checks passed

jblomer deleted the ntuple-rc3 branch November 4, 2024 14:00

pcanal reviewed Nov 4, 2024

View reviewed changes

Moelf mentioned this pull request Nov 13, 2024

[RNTuple] v1.0.0.0 JuliaHEP/UnROOT.jl#366

Open

Moelf reviewed Nov 13, 2024

View reviewed changes

		// Rewind the version bytes, as ReadClassBuffer needs to read the version again.
		buf.SetBufferOffset(offClassBuf);

		buf.SetBufferOffset(offClassBuf);
		buf.ReadClassBuffer(RNTuple::Class(), this);

	using namespace ROOT::Experimental::Internal;
	using namespace ROOT;
	using namespace ROOT::Experimental::Internal;

[ntuple] Binary format RC3 #16645

[ntuple] Binary format RC3 #16645

Uh oh!

Conversation

jblomer commented Oct 9, 2024

Uh oh!

github-actions bot commented Oct 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

hahnjo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jblomer Oct 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jblomer commented Oct 11, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pcanal Oct 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jblomer commented Nov 1, 2024

Uh oh!

hahnjo left a comment

Choose a reason for hiding this comment

Uh oh!

enirolf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

github-actions bot commented Oct 9, 2024 •

edited

Loading

jblomer Oct 10, 2024 •

edited

Loading

pcanal Oct 31, 2024 •

edited

Loading