From 090d873cefb2da27543ecd06ec07d18da34cf464 Mon Sep 17 00:00:00 2001 From: Jonas Hahnfeld Date: Tue, 7 Jan 2025 16:50:04 +0100 Subject: [PATCH 1/3] [ntuple] Correct exception message in RNTupleParallelWriter --- tree/ntuple/v7/src/RNTupleParallelWriter.cxx | 2 +- tree/ntuple/v7/test/ntuple_parallel_writer.cxx | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tree/ntuple/v7/src/RNTupleParallelWriter.cxx b/tree/ntuple/v7/src/RNTupleParallelWriter.cxx index 8abd00b90e939..9ce6c1b81ae7a 100644 --- a/tree/ntuple/v7/src/RNTupleParallelWriter.cxx +++ b/tree/ntuple/v7/src/RNTupleParallelWriter.cxx @@ -118,7 +118,7 @@ ROOT::Experimental::RNTupleParallelWriter::RNTupleParallelWriter(std::unique_ptr : fSink(std::move(sink)), fModel(std::move(model)), fMetrics("RNTupleParallelWriter") { if (fModel->GetRegisteredSubfields().size() > 0) { - throw RException(R__FAIL("cannot create an RNTupleWriter from a model with registered subfields")); + throw RException(R__FAIL("cannot create an RNTupleParallelWriter from a model with registered subfields")); } fModel->Freeze(); fSink->Init(*fModel.get()); diff --git a/tree/ntuple/v7/test/ntuple_parallel_writer.cxx b/tree/ntuple/v7/test/ntuple_parallel_writer.cxx index 0619b97e5c31f..99b0329245726 100644 --- a/tree/ntuple/v7/test/ntuple_parallel_writer.cxx +++ b/tree/ntuple/v7/test/ntuple_parallel_writer.cxx @@ -289,7 +289,7 @@ TEST(RNTupleParallelWriter, ForbidModelWithSubfields) FAIL() << "should not able to create a writer using a model with registered subfields"; } catch (const ROOT::RException &err) { EXPECT_THAT(err.what(), - testing::HasSubstr("cannot create an RNTupleWriter from a model with registered subfields")); + testing::HasSubstr("cannot create an RNTupleParallelWriter from a model with registered subfields")); } } From d7ce86e3483047135eede7af47c43747a0a4946b Mon Sep 17 00:00:00 2001 From: Jonas Hahnfeld Date: Tue, 7 Jan 2025 16:51:18 +0100 Subject: [PATCH 2/3] [ntuple] Remove outdated comment We require buffered writing with the RNTupleParallelWriter because of its better scalability and less fragmented output files, see also https://github.com/root-project/root/pull/14939. --- tree/ntuple/v7/src/RNTupleParallelWriter.cxx | 3 --- 1 file changed, 3 deletions(-) diff --git a/tree/ntuple/v7/src/RNTupleParallelWriter.cxx b/tree/ntuple/v7/src/RNTupleParallelWriter.cxx index 9ce6c1b81ae7a..143ef581357a4 100644 --- a/tree/ntuple/v7/src/RNTupleParallelWriter.cxx +++ b/tree/ntuple/v7/src/RNTupleParallelWriter.cxx @@ -182,9 +182,6 @@ std::shared_ptr ROOT::Experimental::RNTu std::lock_guard g(fMutex); auto model = fModel->Clone(); - - // TODO: Think about honoring RNTupleWriteOptions::SetUseBufferedWrite(false); this requires synchronization on every - // call to CommitPage() *and* preparing multiple cluster descriptors in parallel! auto sink = std::make_unique(std::make_unique(*fSink, fSinkMutex)); // Cannot use std::make_shared because the constructor of RNTupleFillContext is private. Also it would mean that the From 0d55b49fcfcba45a52395ad267f3927d3b35ab0d Mon Sep 17 00:00:00 2001 From: Jonas Hahnfeld Date: Tue, 7 Jan 2025 16:59:38 +0100 Subject: [PATCH 3/3] [ntuple] Forbid RNTupleParallelWriter with a non-binary TFile --- tree/ntuple/v7/src/RNTupleParallelWriter.cxx | 12 ++++++++++++ tree/ntuple/v7/test/ntuple_parallel_writer.cxx | 10 ++++++++++ 2 files changed, 22 insertions(+) diff --git a/tree/ntuple/v7/src/RNTupleParallelWriter.cxx b/tree/ntuple/v7/src/RNTupleParallelWriter.cxx index 143ef581357a4..98344e4b5a965 100644 --- a/tree/ntuple/v7/src/RNTupleParallelWriter.cxx +++ b/tree/ntuple/v7/src/RNTupleParallelWriter.cxx @@ -21,7 +21,9 @@ #include #include +#include #include +#include namespace { @@ -168,6 +170,16 @@ std::unique_ptr ROOT::Experimental::RNTupleParallelWriter::Append(std::unique_ptr model, std::string_view ntupleName, TDirectory &fileOrDirectory, const RNTupleWriteOptions &options) { + auto file = fileOrDirectory.GetFile(); + if (!file) { + throw RException( + R__FAIL("RNTupleParallelWriter only supports writing to a ROOT file. Cannot write into a directory " + "that is not backed by a file")); + } + if (!file->IsBinary()) { + throw RException(R__FAIL("RNTupleParallelWriter only supports writing to a ROOT file. Cannot write into " + + std::string(file->GetName()))); + } if (!options.GetUseBufferedWrite()) { throw RException(R__FAIL("parallel writing requires buffering")); } diff --git a/tree/ntuple/v7/test/ntuple_parallel_writer.cxx b/tree/ntuple/v7/test/ntuple_parallel_writer.cxx index 99b0329245726..b59233344db58 100644 --- a/tree/ntuple/v7/test/ntuple_parallel_writer.cxx +++ b/tree/ntuple/v7/test/ntuple_parallel_writer.cxx @@ -293,6 +293,16 @@ TEST(RNTupleParallelWriter, ForbidModelWithSubfields) } } +TEST(RNTupleParallelWriter, ForbidNonRootTFiles) +{ + FileRaii fileGuard("test_ntuple_parallel_forbid_xml.xml"); + + auto model = RNTupleModel::Create(); + auto file = std::unique_ptr(TFile::Open(fileGuard.GetPath().c_str(), "RECREATE")); + // Opening an XML TFile should fail + EXPECT_THROW(RNTupleParallelWriter::Append(std::move(model), "ntpl", *file), ROOT::RException); +} + TEST(RNTupleFillContext, FlushColumns) { FileRaii fileGuard("test_ntuple_context_flush.root");