[rfile] Introduce RFile first prototype #19958

silverweed · 2025-09-23T11:22:54Z

This PR adds a first version of the new RFile prototype, implementing basic Open/Append/Recreate/Get/Put methods.

For a sneak peek of the final shape of the prototype, see here

Checklist:

tested changes locally
updated the docs (if necessary)

hageboeck

Thanks a lot for the work!

Seeing the code sparked a lot of questions. Many of those might be either addressed in later PRs and some might be irrelevant, so feel free to "Resolve" those.

.github/workflows/root-ci.yml

hageboeck · 2025-09-23T12:14:43Z

io/io/inc/ROOT/RFile.hxx

After yesterday's discussion, is it wise to put this in io/v7? Version numbers in file names or configs will just create headaches. How about putting it in io/rfile, where it can remain also in the future?

In the end I just put it in io/io to avoid CMake headaches. Since it's always built anyway it doesn't make much of a difference in my opinion.

hageboeck · 2025-09-23T12:16:13Z

io/io/inc/ROOT/RFile.hxx

+
+} // namespace Internal
+
+class RFile final {


final will prevent users from inheriting and possibly overriding. Is this the intention?

A larger docstring for the class documentation would be desirable, but it

can be added later

and could be placed in the .cxx, so you don't have to recompile everything when updating the docs.

final will prevent users from inheriting and possibly overriding. Is this the intention?

Considering we're not defining any virtual method (including the destructor) I don't see how the user could effectively inherit this class anyway. If we change our mind later we can just remove the final, but I consider it a good default.

hageboeck · 2025-09-23T12:18:55Z

io/io/inc/ROOT/RFile.hxx

+   /// The caller should immediately wrap it into a unique_ptr of the type described by `type`.
+   [[nodiscard]] void *GetUntyped(std::string_view path, const TClass &type) const;
+
+   /// Writes `obj` to file, without taking its ownership.


Maybe this makes it clearer?

Suggested change

/// Writes `obj` to file, without taking its ownership.

/// Writes a copy of `obj` to file, without taking its ownership.

An explanation of type and flags would be great, too.

Sorry, I see now that this is private, so the request has low priority.

Not sure about the "a copy of" part; the object is not really copied in memory, it's just serialized into the file. At least, that's the mental model that the both us and the user should have, regardless of how TFile actually implements it

hageboeck · 2025-09-23T12:21:55Z

io/io/inc/ROOT/RFile.hxx

+      return std::unique_ptr<T>(static_cast<T *>(obj));
+   }
+
+   /// Puts an object into the file.


Suggested change

/// Puts an object into the file.

/// Puts a copy of an object into the file.

hageboeck · 2025-09-23T13:18:13Z

io/v7/rfile/test/rfile.cxx

@@ -0,0 +1,397 @@
+#include "gtest/gtest.h"
+#include "gmock/gmock.h"


Is it using gmock? I might have removed gmock from the automatically linked CMake targets, so you might have to add it if required.

It is apparently needed by testing::HasSubstr

hageboeck · 2025-09-23T13:21:32Z

io/v7/rfile/test/rfile.cxx

+   EXPECT_FALSE(file->Get<TH1F>("hist"));
+   EXPECT_TRUE(file->Get<TH1>("hist"));
+
+   // We do NOT want to globally register RFiles ever.


Maybe yes if the browser is supposed to show a list of open files. But if we decide that that's what it should do, one should maybe do this in a different list with weak pointers or so ...

hageboeck · 2025-09-23T13:33:52Z

io/v7/rfile/test/rfile.cxx

+      auto hist = file->Get<TH1D>("hist");
+      ASSERT_TRUE(hist);
+      EXPECT_EQ(static_cast<int>(hist->GetEntries()), 10);
+


Does this hold?

Suggested change

EXPECT_EQ(*file->Get<TH1D>("hist;2"), *hist);

I don't think the equality operator is defined for TH1..

hageboeck · 2025-09-23T13:39:17Z

io/v7/rfile/test/rfile.cxx

+   constexpr const char *kFileName = "http://root.cern/files/RNTuple.root";
+
+   auto file = RFile::Open(kFileName);
+   auto ntuple = file->Get<ROOT::RNTuple>("Contributors");


Can this gracefully fail with GTEST_SKIP() if one is offline?

I don't know...how can we reliably detect whether we're offline? Do we do this in other tests?

hageboeck · 2025-09-23T13:44:44Z

io/v7/rfile/test/rfile.cxx

+            TH1D hist(histName, histTitle, 100, -10 * (i + 1), 10 * (i + 1));
+            file->Put(histPath, hist);


I'm not sure this test increases coverage. Also, if it was covering a new case, wouldn't you at least have to check that the objects can be read / listed?

github-actions · 2025-09-23T17:28:34Z

Test Results

21 files 21 suites 3d 17h 56m 43s ⏱️
3 680 tests 3 679 ✅ 0 💤 1 ❌
75 460 runs 75 454 ✅ 5 💤 1 ❌

For more details on these failures, see this check.

Results for commit 6cbc229.

♻️ This comment has been updated with latest results.

hahnjo

Two small comments, but in general I'll leave review to others...

hahnjo · 2025-09-25T11:38:18Z

io/io/test/CMakeLists.txt

  ROOT_ADD_GTEST(RIoUring RIoUring.cxx LIBRARIES RIO)
 endif()

+ROOT_ADD_GTEST(rfile rfile.cxx LIBRARIES RIO Hist ROOTNTuple)


In principle, ROOTNTuple is a higher level in terms of layering than RIO, not sure if it's a good idea to depend on it (even for a test)...

Isn't that the case also for Hist?

io/io/inc/ROOT/RFile.hxx

silverweed requested review from jblomer, hahnjo, vepadulano, hageboeck and enirolf September 23, 2025 11:22

silverweed self-assigned this Sep 23, 2025

silverweed requested review from bellenot and pcanal as code owners September 23, 2025 11:22

silverweed added the in:I/O label Sep 23, 2025

silverweed requested a review from dpiparo as a code owner September 23, 2025 12:07

hageboeck reviewed Sep 23, 2025

View reviewed changes

silverweed force-pushed the rfile_01_1 branch 3 times, most recently from 978733c to 7430133 Compare September 24, 2025 11:41

hahnjo reviewed Sep 25, 2025

View reviewed changes

silverweed force-pushed the rfile_01_1 branch from 7430133 to 868ac3e Compare September 25, 2025 13:13

[rfile] Introduce RFile first prototype

6cbc229

silverweed force-pushed the rfile_01_1 branch from 868ac3e to 6cbc229 Compare September 25, 2025 13:14

silverweed requested a review from hageboeck September 25, 2025 13:15

	/// Writes `obj` to file, without taking its ownership.
	/// Writes a copy of `obj` to file, without taking its ownership.

	/// Puts an object into the file.
	/// Puts a copy of an object into the file.

		@@ -0,0 +1,397 @@
		#include "gtest/gtest.h"
		#include "gmock/gmock.h"

		TH1D hist(histName, histTitle, 100, -10 * (i + 1), 10 * (i + 1));
		file->Put(histPath, hist);

[rfile] Introduce RFile first prototype #19958

Are you sure you want to change the base?

[rfile] Introduce RFile first prototype #19958

Uh oh!

Conversation

silverweed commented Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist:

Uh oh!

hageboeck 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!

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!

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!

github-actions bot commented Sep 23, 2025 • 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!

Uh oh!

Uh oh!

silverweed commented Sep 23, 2025 •

edited

Loading

github-actions bot commented Sep 23, 2025 •

edited

Loading