diff --git a/form/root_storage/root_tfile.cpp b/form/root_storage/root_tfile.cpp index 5d9185c7b..0b23d23e6 100644 --- a/form/root_storage/root_tfile.cpp +++ b/form/root_storage/root_tfile.cpp @@ -7,10 +7,14 @@ #include #include +#include #include #include +#include +#include #include +#include #include #include #include @@ -79,4 +83,44 @@ void root_tfile_imp::write_descriptor( } } +std::optional>> root_tfile_imp::read_descriptor() +{ + if (file_ == nullptr) { + throw std::runtime_error("root_tfile_imp::read_descriptor no file attached for " + name()); + } + + // Look the key up first so an absent descriptor is told apart from one that fails to read. + // GetKey returns the highest cycle and the key stays owned by the file. + std::string const object_name{descriptor_object_name}; + TKey* key = file_->GetKey(object_name.c_str()); + if (key == nullptr) { + return std::nullopt; + } + + // ReadObj hands ownership of the TList to the caller; SetOwner below extends it to the entries. + std::unique_ptr const object{key->ReadObj()}; + if (!object) { + throw std::runtime_error("root_tfile_imp::read_descriptor failed to read object '" + + object_name + "' from " + name()); + } + auto* list = dynamic_cast(object.get()); + if (list == nullptr) { + throw std::runtime_error("root_tfile_imp::read_descriptor object '" + object_name + "' in " + + name() + " is not a TList"); + } + list->SetOwner(true); + + std::vector> entries; + entries.reserve(static_cast(list->GetSize())); + for (TObject const* entry : *list) { + auto const* named = dynamic_cast(entry); + if (named == nullptr) { + throw std::runtime_error("root_tfile_imp::read_descriptor object '" + object_name + "' in " + + name() + " holds an entry that is not a TNamed"); + } + entries.emplace_back(named->GetName(), named->GetTitle()); + } + return entries; +} + std::shared_ptr root_tfile_imp::get_tfile() { return file_; } diff --git a/form/root_storage/root_tfile.hpp b/form/root_storage/root_tfile.hpp index 01b48a3d3..052cbb20e 100644 --- a/form/root_storage/root_tfile.hpp +++ b/form/root_storage/root_tfile.hpp @@ -6,6 +6,7 @@ #include "storage/storage_file.hpp" #include +#include #include #include #include @@ -25,6 +26,7 @@ namespace form::detail::experimental { void set_attribute(std::string const& key, std::string const& value) override; void write_descriptor(std::vector> const& entries) override; + std::optional>> read_descriptor() override; std::shared_ptr get_tfile(); diff --git a/form/storage/istorage.hpp b/form/storage/istorage.hpp index 1a935a110..6fcb0163d 100644 --- a/form/storage/istorage.hpp +++ b/form/storage/istorage.hpp @@ -4,12 +4,14 @@ #define FORM_STORAGE_ISTORAGE_HPP #include "core/placement.hpp" +#include "core/technology.hpp" #include "core/token.hpp" #include "form/config.hpp" #include #include #include +#include #include #include #include @@ -39,6 +41,14 @@ namespace form::detail::experimental { void const** data, std::type_info const& type, form::experimental::config::tech_setting_config const& settings) = 0; + + // Reads descriptor entries in their original order. + // Returns std::nullopt if no descriptor is present; throws if the file cannot be opened. + // The major technology selects the backend; the full id selects its settings. + virtual std::optional>> read_descriptor( + std::string const& file_name, + form::technology::id technology, + form::experimental::config::tech_setting_config const& settings) = 0; }; class i_storage_writer { @@ -73,6 +83,8 @@ namespace form::detail::experimental { virtual void write_descriptor( std::vector> const& entries) = 0; + + virtual std::optional>> read_descriptor() = 0; }; class i_storage_write_container { diff --git a/form/storage/storage_file.cpp b/form/storage/storage_file.cpp index 0f4c15ceb..652ef76ab 100644 --- a/form/storage/storage_file.cpp +++ b/form/storage/storage_file.cpp @@ -2,6 +2,7 @@ #include "storage_file.hpp" +#include #include #include #include @@ -26,7 +27,7 @@ void storage_file::write_descriptor(std::vector> const& storage_file::descriptor() const +std::optional>> storage_file::read_descriptor() { return descriptor_; } diff --git a/form/storage/storage_file.hpp b/form/storage/storage_file.hpp index 7a8c4eeb0..6cf094180 100644 --- a/form/storage/storage_file.hpp +++ b/form/storage/storage_file.hpp @@ -5,6 +5,7 @@ #include "istorage.hpp" +#include #include #include #include @@ -21,12 +22,12 @@ namespace form::detail::experimental { void set_attribute(std::string const& name, std::string const& value) override; void write_descriptor(std::vector> const& entries) override; - std::vector> const& descriptor() const; + std::optional>> read_descriptor() override; private: std::string name_; char mode_; - std::vector> descriptor_; + std::optional>> descriptor_; }; } // namespace form::detail::experimental diff --git a/form/storage/storage_reader.cpp b/form/storage/storage_reader.cpp index 42fb42b5e..699bc77d7 100644 --- a/form/storage/storage_reader.cpp +++ b/form/storage/storage_reader.cpp @@ -12,6 +12,7 @@ #include #include #include +#include #include #include #include @@ -61,6 +62,30 @@ namespace form::detail::experimental { } } +std::shared_ptr const& storage_reader::open_file( + std::string const& file_name, + form::technology::id technology, + form::experimental::config::tech_setting_config const& settings) +{ + if (auto const found = files_.find(file_name); found != files_.end()) { + return found->second; + } + // Cache the file only after all file settings have been applied successfully. + auto opened = create_file(technology, file_name, 'i'); + for (auto const& [key, value] : get_file_table(settings, technology, file_name)) { + opened->set_attribute(key, value); + } + return files_.emplace(file_name, std::move(opened)).first->second; +} + +std::optional>> storage_reader::read_descriptor( + std::string const& file_name, + form::technology::id technology, + form::experimental::config::tech_setting_config const& settings) +{ + return open_file(file_name, technology, settings)->read_descriptor(); +} + std::uint64_t storage_reader::get_index( token const& token, std::string const& id, @@ -82,20 +107,11 @@ void storage_reader::prime(token const& token, auto cont_key = std::make_pair(token.file_name(), token.container_name()); auto cont = read_containers_.find(cont_key); if (cont == read_containers_.end()) { - auto file = files_.find(token.file_name()); - if (file == files_.end()) { - file = - files_.insert({token.file_name(), create_file(token.technology(), token.file_name(), 'i')}) - .first; - for (auto const& [key, value] : - get_file_table(settings, token.technology(), token.file_name())) { - file->second->set_attribute(key, value); - } - } + auto const& file = open_file(token.file_name(), token.technology(), settings); cont = read_containers_ .insert({cont_key, create_read_container(token.technology(), token.container_name())}) .first; - cont->second->set_file(file->second); + cont->second->set_file(file); for (auto const& [key, value] : get_container_table(settings, token.technology(), token.container_name())) { cont->second->set_attribute(key, value); @@ -125,16 +141,7 @@ std::map const& storage_reader::index_map( auto cont = read_containers_.find(cont_key); if (cont == read_containers_.end()) { - auto file = files_.find(token.file_name()); - if (file == files_.end()) { - file = - files_.insert({token.file_name(), create_file(token.technology(), token.file_name(), 'i')}) - .first; - for (auto const& [key, value] : - get_file_table(settings, token.technology(), token.file_name())) { - file->second->set_attribute(key, value); - } - } + auto const& file = open_file(token.file_name(), token.technology(), settings); cont = read_containers_ .insert({cont_key, create_read_container(token.technology(), token.container_name())}) .first; @@ -142,7 +149,7 @@ std::map const& storage_reader::index_map( get_container_table(settings, token.technology(), token.container_name())) { cont->second->set_attribute(key, value); } - cont->second->set_file(file->second); + cont->second->set_file(file); } std::map index; @@ -172,20 +179,11 @@ void storage_reader::read_container(token const& token, auto cont_key = std::make_pair(token.file_name(), token.container_name()); auto cont = read_containers_.find(cont_key); if (cont == read_containers_.end()) { - auto file = files_.find(token.file_name()); - if (file == files_.end()) { - file = - files_.insert({token.file_name(), create_file(token.technology(), token.file_name(), 'i')}) - .first; - for (auto const& [key, value] : - get_file_table(settings, token.technology(), token.file_name())) { - file->second->set_attribute(key, value); - } - } + auto const& file = open_file(token.file_name(), token.technology(), settings); cont = read_containers_ .insert({cont_key, create_read_container(token.technology(), token.container_name())}) .first; - cont->second->set_file(file->second); + cont->second->set_file(file); for (auto const& [key, value] : get_container_table(settings, token.technology(), token.container_name())) { cont->second->set_attribute(key, value); diff --git a/form/storage/storage_reader.hpp b/form/storage/storage_reader.hpp index 3a8a1128a..e91b63690 100644 --- a/form/storage/storage_reader.hpp +++ b/form/storage/storage_reader.hpp @@ -9,9 +9,11 @@ #include #include #include +#include #include #include #include // for std::pair +#include namespace form::detail::experimental { @@ -33,8 +35,16 @@ namespace form::detail::experimental { void const** data, std::type_info const& type, form::experimental::config::tech_setting_config const& settings) override; + std::optional>> read_descriptor( + std::string const& file_name, + form::technology::id technology, + form::experimental::config::tech_setting_config const& settings) override; private: + std::shared_ptr const& open_file( + std::string const& file_name, + form::technology::id technology, + form::experimental::config::tech_setting_config const& settings); std::map const& index_map( token const& token, form::experimental::config::tech_setting_config const& settings); diff --git a/test/form/form_basics_test.cpp b/test/form/form_basics_test.cpp index 51f8cbbac..536f54290 100644 --- a/test/form/form_basics_test.cpp +++ b/test/form/form_basics_test.cpp @@ -495,13 +495,20 @@ TEST_CASE("storage_writer: the descriptor reaches the file it names", "[form]") TEST_CASE("storage_file: the generic backend keeps the descriptor it is given", "[form]") { storage_file file{"generic.dat", 'o'}; - CHECK(file.descriptor().empty()); + CHECK_FALSE(file.read_descriptor().has_value()); std::vector> const entries{ {std::string{descriptor_layout_version_key}, std::string{form_layout_version}}, {std::string{descriptor_technologies_key}, "generic"}}; file.write_descriptor(entries); - CHECK(file.descriptor() == entries); + CHECK(file.read_descriptor() == entries); +} + +TEST_CASE("storage_file: an empty descriptor is still a descriptor", "[form]") +{ + storage_file file{"generic_empty.dat", 'o'}; + file.write_descriptor({}); + CHECK(file.read_descriptor() == std::vector>{}); } TEST_CASE("persistence_reader basic operations", "[form]") diff --git a/test/form/form_storage_test.cpp b/test/form/form_storage_test.cpp index 7480982c9..5b96a1325 100644 --- a/test/form/form_storage_test.cpp +++ b/test/form/form_storage_test.cpp @@ -27,19 +27,23 @@ #include #include #include +#include #include #include #include #include +#include #include #include #include #include #include #include +#include #include #include +#include #include #include #include @@ -133,6 +137,22 @@ namespace { writer->finalize(); } + // Returns the value stored under key in a descriptor read back from a file, if both exist. + std::optional descriptor_value( + std::optional>> const& descriptor, + std::string_view key) + { + if (!descriptor.has_value()) { + return std::nullopt; + } + auto const found = std::ranges::find( + *descriptor, key, [](auto const& entry) { return std::string_view{entry.first}; }); + if (found == descriptor->end()) { + return std::nullopt; + } + return found->second; + } + token index_token_of(std::string const& file_name, std::string const& creator) { return token{file_name, build_full_label(row_space(creator), "index"), technology}; @@ -576,6 +596,153 @@ TEST_CASE("root_tfile: descriptor write failures are reported, not swallowed", " } } +TEST_CASE("root_tfile: read_descriptor returns what write_descriptor stored", "[form]") +{ + std::vector> const entries{ + {std::string{descriptor_layout_version_key}, std::string{form_layout_version}}, + {std::string{descriptor_technologies_key}, "root_ttree,root_rntuple"}}; + std::string const object_name{descriptor_object_name}; + auto const suffix = form::technology::to_string(technology); + + SECTION("no file attached") + { + root_tfile_imp file{"/nonexistent-directory-for-form-tests/x.root", 'i'}; + CHECK_THROWS_AS(file.read_descriptor(), std::runtime_error); + } + + SECTION("a file without a descriptor reads as none, not as an empty one") + { + std::string const file_name = "form_descriptor_absent_" + suffix + ".root"; + { + root_tfile_imp const file{file_name, 'o'}; + } + root_tfile_imp file{file_name, 'i'}; + CHECK_FALSE(file.read_descriptor().has_value()); + } + + SECTION("the entries read back in the order they were written") + { + std::string const file_name = "form_descriptor_read_" + suffix + ".root"; + { + root_tfile_imp file{file_name, 'o'}; + file.write_descriptor(entries); + } + root_tfile_imp file{file_name, 'i'}; + CHECK(file.read_descriptor() == entries); + } + + SECTION("an empty descriptor is still a descriptor") + { + std::string const file_name = "form_descriptor_empty_" + suffix + ".root"; + { + root_tfile_imp file{file_name, 'o'}; + file.write_descriptor({}); + } + root_tfile_imp file{file_name, 'i'}; + CHECK(file.read_descriptor() == std::vector>{}); + } + + SECTION("an object of the wrong type under the descriptor name is rejected") + { + std::string const file_name = "form_descriptor_wrong_type_" + suffix + ".root"; + { + std::unique_ptr const raw{TFile::Open(file_name.c_str(), "RECREATE")}; + REQUIRE(raw != nullptr); + TNamed const impostor{object_name.c_str(), "not a list"}; + REQUIRE(raw->WriteObject(&impostor, object_name.c_str()) > 0); + } + root_tfile_imp file{file_name, 'i'}; + CHECK_THROWS_AS(file.read_descriptor(), std::runtime_error); + } + + SECTION("a descriptor entry that is not a key/value pair is rejected") + { + std::string const file_name = "form_descriptor_bad_entry_" + suffix + ".root"; + { + std::unique_ptr const raw{TFile::Open(file_name.c_str(), "RECREATE")}; + REQUIRE(raw != nullptr); + TObjString entry{"form_layout_version=1.0"}; + TList list; + list.Add(&entry); + REQUIRE(raw->WriteObject(&list, object_name.c_str()) > 0); + } + root_tfile_imp file{file_name, 'i'}; + CHECK_THROWS_AS(file.read_descriptor(), std::runtime_error); + } +} + +TEST_CASE("storage_reader read_descriptor: a written file identifies its layout and technology", + "[form]") +{ + std::string const file_name = + "storage_reader_descriptor_" + form::technology::to_string(technology) + ".root"; + std::string const creator = "descriptor_creator"; + std::vector const cells{event_cell(4), event_cell(2)}; + write_cells(file_name, creator, cells); + + storage_reader reader; + auto const descriptor = reader.read_descriptor(file_name, technology, {}); + REQUIRE(descriptor.has_value()); + + CHECK(descriptor_value(descriptor, descriptor_layout_version_key) == + std::string{form_layout_version}); + + // A single-technology file names just that technology, with the token FORM writes to disk. + CHECK(descriptor_value(descriptor, descriptor_technologies_key) == technology_name(technology)); + + // The file opened for the descriptor is the one the reader goes on to read products from. + for (auto const& cell : cells) { + CHECK(read_cell(reader, file_name, creator, cell.id) == payload_of(cell)); + } +} + +#ifdef USE_RNTUPLE_STORAGE +TEST_CASE("storage_reader read_descriptor: any minor of a mixed file's major reads its descriptor", + "[form]") +{ + using form::technology::root_rntuple; + using form::technology::root_ttree; + + std::string const file_name = + "storage_reader_descriptor_mixed_" + form::technology::to_string(technology) + ".root"; + std::string const creator = "mixed_creator"; + { + auto writer = create_persistence_writer(); + REQUIRE(writer != nullptr); + writer->configure_tech_settings(form::experimental::config::tech_setting_config{}); + auto const ttree_place = make_placement(file_name, creator, "ttree_prod", root_ttree); + auto const rntuple_place = make_placement(file_name, creator, "rntuple_prod", root_rntuple); + writer->create_containers( + {{ttree_place, &typeid(std::vector)}, {rntuple_place, &typeid(std::vector)}}); + auto const cell = event_cell(1); + auto const payload = payload_of(cell); + writer->register_write( + identity(creator, "ttree_prod"), ttree_place, &payload, typeid(std::vector)); + writer->commit_place(ttree_place, cell); + writer->register_write( + identity(creator, "rntuple_prod"), rntuple_place, &payload, typeid(std::vector)); + writer->commit_place(rntuple_place, cell); + writer->finalize(); + } + + for (auto const tech : {root_ttree, root_rntuple}) { + CAPTURE(form::technology::to_string(tech)); + storage_reader reader; + auto const descriptor = reader.read_descriptor(file_name, tech, {}); + CHECK(descriptor_value(descriptor, descriptor_technologies_key) == + std::string{"root_ttree,root_rntuple"}); + } +} +#endif + +TEST_CASE("storage_reader read_descriptor: a file that cannot be opened is an error", "[form]") +{ + storage_reader reader; + CHECK_THROWS_AS( + reader.read_descriptor("/nonexistent-directory-for-form-tests/x.root", technology, {}), + std::runtime_error); +} + TEST_CASE("Persistence round-trip: every listed cell reads back its own payload", "[form]") { using namespace form::experimental::config;