[ntuple] Attributes: add support for writing by silverweed · Pull Request #21289 · root-project/root

silverweed · 2026-02-16T10:32:29Z

This Pull request:

adds the RNTupleAttrSetWriter class and initial support for writing attributes. This involves touching, among other classes, the RNTupleWriter, RNTupleFillContext, RPageSink and RMiniFile.

Original reference PR (now quite outdated): #19153

Checklist:

tested changes locally
updated the docs (if necessary)

github-actions · 2026-02-16T15:17:53Z

Test Results

17 files 17 suites 2d 11h 58m 51s ⏱️
3 815 tests 3 815 ✅ 0 💤 0 ❌
57 969 runs 57 969 ✅ 0 💤 0 ❌

Results for commit 0e6734f.

♻️ This comment has been updated with latest results.

hahnjo

A first review... I think one high-level decision to make is where to build RNTupleAttrSetDescriptor. RNTupleFileWriter feels like the wrong place to do this.

hahnjo · 2026-02-25T12:53:00Z

tree/ntuple/inc/ROOT/RNTupleAttributes.hxx

+#ifndef ROOT7_RNTuple_Attributes
+#define ROOT7_RNTuple_Attributes


A while ago, we started having one header per class. I think we should keep this pattern and split the header RNTupleAttributes.hxx proposed here

We still have some exceptions where it makes sense (RNTupleDescriptor.hxx, RNTupleUtils.hxx, ...); I'm not sure that splitting this would bring much value - perhaps the splitting could be made at the reading vs writing level?

Yes, some classes are still "bundled" - agreed on the reading vs writing level. Probably that will need some more headers for classes shared between the two (?)

tree/ntuple/inc/ROOT/RNTupleAttributes.hxx

hahnjo · 2026-02-25T12:56:50Z

tree/ntuple/inc/ROOT/RNTupleAttributes.hxx

+inline const std::uint16_t kSchemaVersionMajor = 1;
+inline const std::uint16_t kSchemaVersionMinor = 0;


Should these move into the RNTuple class where we have the other constants such as kVersionEpoch?

Possibly; though these only concern attributes so it's not necessarily interesting for users of the RNTuple class. It's a matter of deciding whether we want to centralize these binary specs-related constants or not.
I have no strong opinion either way.

Thinking again about this, I'd not put these in RNTuple as that's only one specific anchor, whereas these constants are anchor-agnostic.
If anything they might live in the Serializer, but I'd rather keep them close to the other related constants (rangeStartFieldName etc).
Probably in a separate header that is only included by some cxx file and not directly exposed to users.

AFAICT we are already using the constants in RNTuple to initialize the RNTupleDescriptorBuilder...

That's true, but I'm not sure that was the best decision on our side. In retrospect, I'd probably have put them in some dedicated namespace (although I see the argument of using the RNTuple name to namespace them)...

tree/ntuple/inc/ROOT/RNTupleAttributes.hxx

tree/ntuple/test/ntuple_attributes.cxx

tree/ntuple/inc/ROOT/RNTupleAttributes.hxx

silverweed · 2026-02-25T14:36:28Z

Thanks for the thorough review @hahnjo! For the descriptor building I agree it feels weird to have it in the Writer; let's talk about it offline.

io/io/inc/ROOT/RFile.hxx

hahnjo · 2026-03-10T16:03:50Z

tree/ntuple/inc/ROOT/RNTupleAttributes.hxx

-   friend class ROOT::RNTupleFillContext;
+   friend class ROOT::RNTupleWriter;


The wrong friend declaration should probably be removed from the original commit...

hahnjo · 2026-03-10T16:03:56Z

tree/ntuple/inc/ROOT/RNTupleAttributes.hxx

-   friend class ROOT::RNTupleFillContext;
+   friend class ROOT::RNTupleWriter;


tree/ntuple/inc/ROOT/RNTupleAttributes.hxx

hahnjo · 2026-03-10T16:09:26Z

tree/ntuple/inc/ROOT/RNTupleFillContext.hxx

+   /// NOTE: this is a deque to keep pointers stable upon adding/removing, but have deterministic iteration at the
+   /// same time.


Do we need this stability guarantee?

hahnjo · 2026-03-10T16:10:13Z

tree/ntuple/src/RNTupleWriter.cxx

+   // NOTE: references into attrSets are guaranteed to be stable, so we can hand out the reference
+   // to the AttrSetWriter's shared_ptr into the handle.
+   return Experimental::RNTupleAttrSetWriterHandle{addedSet};


Do we need this guarantee? RNTupleAttrSetWriterHandle (currently) creates a std::weak_ptr, which precisely has the point that the shared_ptr doesn't need to stay around...

silverweed requested review from enirolf, hahnjo and pcanal February 16, 2026 10:32

silverweed self-assigned this Feb 16, 2026

silverweed requested review from bellenot and jblomer as code owners February 16, 2026 10:32

silverweed added the in:RNTuple label Feb 16, 2026

silverweed added the clean build Ask CI to do non-incremental build on PR label Feb 18, 2026

silverweed force-pushed the ntuple_attr_write branch 4 times, most recently from 16e0a45 to 9b598df Compare February 23, 2026 10:05

silverweed force-pushed the ntuple_attr_write branch from 9b598df to 505da84 Compare February 25, 2026 08:16

hahnjo requested changes Feb 25, 2026

View reviewed changes

silverweed force-pushed the ntuple_attr_write branch 8 times, most recently from 2d7c429 to b20c52b Compare March 4, 2026 15:03

silverweed requested a review from hahnjo March 5, 2026 09:07

hahnjo reviewed Mar 6, 2026

View reviewed changes

io/io/inc/ROOT/RFile.hxx Show resolved Hide resolved

silverweed added 4 commits March 10, 2026 10:26

[rfile] Add some more information in RKeyInfo

83d1acb

[ntuple] clang-format RPageStorageDaos

73ac8bd

[ntuple] Start introducing RNTupleAttrSetWriter

3887083

[ntuple] Add RNTupleAttrSetWriter::Begin/CommitRange()

1436923

silverweed added 6 commits March 10, 2026 10:26

[ntuple] Allow creating AttrSetWriters from RNTupleWriter

80171dd

[ntuple] Add basic test for creating RNTupleAttrSetWriter

13b5fec

[ntuple] Return anchor info from RMiniFileWriter::Commit()

899e794

[ntuple] Return anchor locator+length from sink's CommitDataset

a3f22ae

[ntuple] Add attribute committing

d8dd25b

[ntuple] Rename RNTupleAttributes to RNTupleAttrWriting and split it

0e6734f

silverweed force-pushed the ntuple_attr_write branch from b20c52b to 0e6734f Compare March 10, 2026 09:27

hahnjo reviewed Mar 10, 2026

View reviewed changes

		#ifndef ROOT7_RNTuple_Attributes
		#define ROOT7_RNTuple_Attributes

		inline const std::uint16_t kSchemaVersionMajor = 1;
		inline const std::uint16_t kSchemaVersionMinor = 0;

		friend class ROOT::RNTupleFillContext;
		friend class ROOT::RNTupleWriter;

		/// NOTE: this is a deque to keep pointers stable upon adding/removing, but have deterministic iteration at the
		/// same time.

Conversation

silverweed commented Feb 16, 2026

This Pull request:

Checklist:

Uh oh!

github-actions bot commented Feb 16, 2026 • 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!

silverweed Feb 25, 2026 • 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!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

silverweed commented Feb 25, 2026

Uh oh!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Feb 16, 2026 •

edited

Loading

silverweed Feb 25, 2026 •

edited

Loading