From 24fd255d934d5fe6eeda34e0cbd9ac38c151b23f Mon Sep 17 00:00:00 2001 From: wwuoneway Date: Tue, 20 Oct 2026 13:06:08 -0500 Subject: [PATCH] FORM: fixed issues when reading single-layer is such as [event:N], related to issue #992 - Resolve every id, single- or multi-layer, through the index map. - Key index maps by (file, container) instead of the container alone. - Throw on ids missing from the indes instead of falling back to row 0. - Drop fuzzy id matching; only the canonical id written by FORM matches. - Add tests: per-cell read-back checks for out-of-order and offset cells, the job cell, per-file indices, plus job-level write/read tests under -j 8. --- form/storage/storage_reader.cpp | 313 +++------------- form/storage/storage_reader.hpp | 5 +- test/form/CMakeLists.txt | 42 +++ test/form/cell_number_vector.cpp | 70 ++++ test/form/form_basics_test.cpp | 7 +- test/form/form_cell_numbers.libsonnet | 35 ++ .../form_cell_numbers_offset_read.jsonnet | 2 + .../form_cell_numbers_offset_write.jsonnet | 2 + .../form_cell_numbers_unordered_read.jsonnet | 2 + .../form_cell_numbers_unordered_write.jsonnet | 2 + test/form/form_storage_test.cpp | 335 ++++++++++++------ 11 files changed, 441 insertions(+), 374 deletions(-) create mode 100644 test/form/cell_number_vector.cpp create mode 100644 test/form/form_cell_numbers.libsonnet create mode 100644 test/form/form_cell_numbers_offset_read.jsonnet create mode 100644 test/form/form_cell_numbers_offset_write.jsonnet create mode 100644 test/form/form_cell_numbers_unordered_read.jsonnet create mode 100644 test/form/form_cell_numbers_unordered_write.jsonnet diff --git a/form/storage/storage_reader.cpp b/form/storage/storage_reader.cpp index 43f02bfdb..5b223ecb0 100644 --- a/form/storage/storage_reader.cpp +++ b/form/storage/storage_reader.cpp @@ -8,11 +8,9 @@ #include "storage/factories.hpp" #include "storage/istorage.hpp" -#include -#include +#include #include #include -#include #include #include #include @@ -52,139 +50,6 @@ namespace { } return per_container->second; } - - bool is_structured_index_id(std::string const& id) - { - return id.size() >= 2 && id.front() == '[' && id.back() == ']'; - } - - std::string trim_copy(std::string const& value) - { - auto begin = value.find_first_not_of(' '); - if (begin == std::string::npos) { - return {}; - } - auto end = value.find_last_not_of(' '); - return value.substr(begin, end - begin + 1); - } - - std::optional parse_index_number(std::string const& value) - { - if (value.empty()) { - return std::nullopt; - } - - for (char const ch : value) { - if (!std::isdigit(static_cast(ch))) { - return std::nullopt; - } - } - - try { - return std::stoll(value, nullptr, 10); - } catch (...) { - return std::nullopt; - } - } - - std::optional> normalize_structured_index(std::string const& id) - { - if (!is_structured_index_id(id) || id == "[]") { - return std::nullopt; - } - - std::string const body = id.substr(1, id.size() - 2); - std::string token; - std::map normalized; - - auto commit_token = [&normalized](std::string const& raw_token) -> bool { - auto const token_trimmed = trim_copy(raw_token); - if (token_trimmed.empty()) { - return true; - } - - auto const sep = token_trimmed.find_first_of(":="); - if (sep == std::string::npos) { - return false; - } - - auto key = trim_copy(token_trimmed.substr(0, sep)); - auto value = trim_copy(token_trimmed.substr(sep + 1)); - if (key.empty() || value.empty()) { - return false; - } - - for (char& ch : key) { - ch = static_cast(std::tolower(static_cast(ch))); - } - - auto parsed = parse_index_number(value); - if (!parsed) { - return false; - } - - normalized[key] = *parsed; - return true; - }; - - for (char const ch : body) { - if (ch == ',' || ch == ';') { - if (!commit_token(token)) { - return std::nullopt; - } - token.clear(); - } else { - token.push_back(ch); - } - } - - if (!commit_token(token)) { - return std::nullopt; - } - - if (normalized.empty()) { - return std::nullopt; - } - return normalized; - } - - bool all_components_zero(std::map const& components) - { - if (components.empty()) { - return false; - } - return std::ranges::all_of(components, - [](auto const& component) { return component.second == 0; }); - } - - std::optional sequential_row_from_index_id(std::string const& id) - { - if (id == "[]") { - return 0; - } - - if (id.size() < 2 || id.front() != '[' || id.back() != ']') { - return std::nullopt; - } - - auto const body = id.substr(1, id.size() - 2); - auto const comma = body.find(','); - if (comma != std::string::npos) { - return std::nullopt; - } - - auto const colon = body.find(':'); - if (colon == std::string::npos) { - return std::nullopt; - } - - try { - auto const number = std::stoi(body.substr(colon + 1)); - return number; - } catch (...) { - return std::nullopt; - } - } } // Factory function implementation namespace form::detail::experimental { @@ -198,85 +63,13 @@ int storage_reader::get_index(token const& token, std::string const& id, form::experimental::config::tech_setting_config const& settings) { - if (auto row = sequential_row_from_index_id(id)) { - return *row; - } - - if (index_maps_[token.container_name()].empty()) { - 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); - } - } - cont = - read_containers_ - .insert({cont_key, create_read_container(token.technology(), token.container_name())}) - .first; - for (auto const& [key, value] : - get_container_table(settings, token.technology(), token.container_name())) { - cont->second->set_attribute(key, value); - } - cont->second->set_file(file->second); - } - auto const& type = typeid(std::string); - int entry = 0; - void const* raw_data = nullptr; - while (cont->second->read(entry, &raw_data, type)) { - std::unique_ptr const data(static_cast(raw_data)); - index_maps_[token.container_name()].insert(std::make_pair(*data, entry)); - entry++; - } - - if (index_maps_[token.container_name()].empty()) { - if (!is_structured_index_id(id)) { - return 0; - } - throw std::runtime_error("Unable to read index data from container: " + - token.container_name()); - } - } - - auto const found = index_maps_[token.container_name()].find(id); - if (found != index_maps_[token.container_name()].end()) { - return found->second; + auto const& index = index_map(token, settings); + auto const found = index.find(id); + if (found == index.end()) { + throw std::runtime_error("Index id not found: " + id + " in container: " + + token.container_name() + " of file: " + token.file_name()); } - - auto const normalized_query = normalize_structured_index(id); - if (normalized_query) { - for (auto const& [existing_id, entry] : index_maps_[token.container_name()]) { - auto const normalized_existing = normalize_structured_index(existing_id); - if (normalized_existing && *normalized_existing == *normalized_query) { - return entry; - } - } - - if (all_components_zero(*normalized_query)) { - auto const empty_key = index_maps_[token.container_name()].find(""); - if (empty_key != index_maps_[token.container_name()].end()) { - return 0; - } - - // Compatibility fallback for backends that do not persist the first - // structured id string in a normalized round-trippable form. - return 0; - } - } - - if (!is_structured_index_id(id)) { - return 0; - } - - throw std::runtime_error("Index id not found: " + id + - " in container: " + token.container_name()); + return found->second; } void storage_reader::prime(token const& token, @@ -311,61 +104,61 @@ void storage_reader::prime(token const& token, std::vector storage_reader::list_indices( token const& token, form::experimental::config::tech_setting_config const& settings) { - if (index_maps_[token.container_name()].empty()) { - 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); - } - } - cont = - read_containers_ - .insert({cont_key, create_read_container(token.technology(), token.container_name())}) + auto const& index = index_map(token, settings); + std::vector result(index.size()); + for (auto const& [id, row] : index) { + result[static_cast(row)] = id; + } + return result; +} + +std::map const& storage_reader::index_map( + token const& token, form::experimental::config::tech_setting_config const& settings) +{ + auto const cont_key = std::make_pair(token.file_name(), token.container_name()); + if (auto const built = index_maps_.find(cont_key); built != index_maps_.end()) { + return built->second; + } + + 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_container_table(settings, token.technology(), token.container_name())) { - cont->second->set_attribute(key, value); + get_file_table(settings, token.technology(), token.file_name())) { + file->second->set_attribute(key, value); } - cont->second->set_file(file->second); } - - auto const& type = typeid(std::string); - int entry = 0; - void const* raw_data = nullptr; - while (cont->second->read(entry, &raw_data, type)) { - std::unique_ptr const data(static_cast(raw_data)); - index_maps_[token.container_name()].insert(std::make_pair(*data, entry)); - entry++; - } - - if (index_maps_[token.container_name()].empty()) { - throw std::runtime_error("Unable to enumerate indices from container: " + - token.container_name()); + cont = read_containers_ + .insert({cont_key, create_read_container(token.technology(), token.container_name())}) + .first; + for (auto const& [key, value] : + get_container_table(settings, token.technology(), token.container_name())) { + cont->second->set_attribute(key, value); } + cont->second->set_file(file->second); } - std::vector> ordered; - ordered.reserve(index_maps_[token.container_name()].size()); - for (auto const& [index_string, entry] : index_maps_[token.container_name()]) { - ordered.emplace_back(entry, index_string); + std::map index; + auto const& type = typeid(std::string); + int row = 0; + void const* raw_data = nullptr; + while (cont->second->read(row, &raw_data, type)) { + std::unique_ptr const id(static_cast(raw_data)); + if (!index.emplace(*id, row).second) { + throw std::runtime_error("Index id " + *id + " appears more than once in container: " + + token.container_name() + " of file: " + token.file_name()); + } + ++row; } - std::ranges::sort(ordered, - [](auto const& lhs, auto const& rhs) { return lhs.first < rhs.first; }); - - std::vector result; - result.reserve(ordered.size()); - for (auto const& [_, index_string] : ordered) { - result.push_back(index_string); + if (index.empty()) { + throw std::runtime_error("Unable to read index data from container: " + token.container_name() + + " of file: " + token.file_name()); } - return result; + return index_maps_.emplace(cont_key, std::move(index)).first->second; } void storage_reader::read_container(token const& token, diff --git a/form/storage/storage_reader.hpp b/form/storage/storage_reader.hpp index 386b228d6..dea2f054f 100644 --- a/form/storage/storage_reader.hpp +++ b/form/storage/storage_reader.hpp @@ -33,12 +33,15 @@ namespace form::detail::experimental { form::experimental::config::tech_setting_config const& settings) override; private: + std::map const& index_map( + token const& token, form::experimental::config::tech_setting_config const& settings); + std::map> files_; std::unordered_map, std::shared_ptr, pair_hash> read_containers_; - std::map> index_maps_; + std::map, std::map> index_maps_; }; } // namespace form::detail::experimental diff --git a/test/form/CMakeLists.txt b/test/form/CMakeLists.txt index ac84c5625..5ee72857e 100644 --- a/test/form/CMakeLists.txt +++ b/test/form/CMakeLists.txt @@ -444,6 +444,48 @@ cet_test( "PHLEX_PLUGIN_PATH=${PROJECT_BINARY_DIR}/${phlex_LIBRARY_DIR}:${CMAKE_BINARY_DIR}/form" ) +add_library(cell_number_vector MODULE cell_number_vector.cpp) +target_link_libraries(cell_number_vector PRIVATE phlex::module) + +foreach(case unordered offset) + cet_test( + job:form_cell_numbers_${case}_write + HANDBUILT + TEST_EXEC + phlex::phlex + TEST_ARGS + -c + ${CMAKE_CURRENT_SOURCE_DIR}/form_cell_numbers_${case}_write.jsonnet + --stage test + -j 8 + TEST_WORKDIR + form_cell_numbers_${case} + TEST_PROPERTIES + ENVIRONMENT + "PHLEX_PLUGIN_PATH=${PROJECT_BINARY_DIR}/${phlex_LIBRARY_DIR}:${CMAKE_BINARY_DIR}/form" + ) + + cet_test( + job:form_cell_numbers_${case}_read + HANDBUILT + TEST_EXEC + phlex::phlex + TEST_ARGS + -c + ${CMAKE_CURRENT_SOURCE_DIR}/form_cell_numbers_${case}_read.jsonnet + --stage test + -j 8 + DIRTY_WORKDIR + TEST_WORKDIR + form_cell_numbers_${case} + REQUIRED_FIXTURES + job:form_cell_numbers_${case}_write + TEST_PROPERTIES + ENVIRONMENT + "PHLEX_PLUGIN_PATH=${PROJECT_BINARY_DIR}/${phlex_LIBRARY_DIR}:${CMAKE_BINARY_DIR}/form" + ) +endforeach() + if(FORM_USE_ROOT_STORAGE AND FORM_USE_RNTUPLE_STORAGE) cet_test( "form_rntuple_error_handling_extra" diff --git a/test/form/cell_number_vector.cpp b/test/form/cell_number_vector.cpp new file mode 100644 index 000000000..61d6f7d3b --- /dev/null +++ b/test/form/cell_number_vector.cpp @@ -0,0 +1,70 @@ +// Writes each data cell's own number so that read-back can verify which cell a row belongs to. +// +// The "stamp" transform sleeps for a random time so that stores reach the output out of cell +// order; the "check_cell_numbers" observer fails the job if a served value belongs to another cell. + +#include "phlex/core/product_selector.hpp" +#include "phlex/model/data_cell_index.hpp" +#include "phlex/model/handle.hpp" +#include "phlex/model/identifier.hpp" +#include "phlex/module.hpp" +#include "phlex/source.hpp" + +#include +#include +#include +#include +#include +#include + +using namespace phlex; + +namespace { + std::vector number_of(data_cell_index const& id) + { + return std::vector{static_cast(id.number())}; + } +} + +PHLEX_REGISTER_PROVIDERS(graph, config) +{ + auto const creator = config.get("creator"); + graph.provide("cell_numbers", number_of, concurrency::unlimited) + .output_product( + creator, experimental::identifier("numbers"), experimental::identifier("event")); +} + +PHLEX_REGISTER_ALGORITHMS(m, config) +{ + auto const input_creator = config.get("input_creator"); + if (config.get("check", false)) { + m.observe( + "check_cell_numbers", + [](handle> const values) { + if (*values != number_of(values.data_cell_index())) { + auto const served = + values->empty() ? std::string{"nothing"} : std::to_string(values->front()); + throw std::runtime_error("cell " + values.data_cell_index().to_string() + + " was served the value " + served); + } + }, + concurrency::unlimited) + .input_family(product_selector{.creator = experimental::identifier{input_creator}, + .layer = "event", + .suffix = "cell_numbers"}); + return; + } + + m.transform( + "stamp", + [](std::vector const& values) { + thread_local std::mt19937 gen{std::random_device{}()}; + std::this_thread::sleep_for( + std::chrono::microseconds(std::uniform_int_distribution(0, 2000)(gen))); + return values; + }, + concurrency::unlimited) + .input_family(product_selector{ + .creator = experimental::identifier{input_creator}, .layer = "event", .suffix = "numbers"}) + .output_product_suffixes("cell_numbers"); +} diff --git a/test/form/form_basics_test.cpp b/test/form/form_basics_test.cpp index b34521317..9790c0438 100644 --- a/test/form/form_basics_test.cpp +++ b/test/form/form_basics_test.cpp @@ -445,8 +445,7 @@ TEST_CASE("storage_reader basic operations", "[form]") void const* read_data = nullptr; storage->read_container(product_token, &read_data, typeid(int), settings); - int const index = storage->get_index(product_token, "some_id", settings); - CHECK(index == 0); + CHECK_THROWS_AS(storage->get_index(product_token, "[event:1]", settings), std::runtime_error); } TEST_CASE("storage_writer basic operations", "[form]") @@ -484,8 +483,8 @@ TEST_CASE("persistence_reader basic operations", "[form]") SECTION("Full Lifecycle") { void const* data = nullptr; - // This will call get_token -> get_index (returns 0 for Storage_Container) -> read_container - CHECK_NOTHROW(p->read("my_creator", test_stage, "prod", "event_1", &data, typeid(int))); + CHECK_THROWS_AS(p->read("my_creator", test_stage, "prod", "[event:1]", &data, typeid(int)), + std::runtime_error); } } diff --git a/test/form/form_cell_numbers.libsonnet b/test/form/form_cell_numbers.libsonnet new file mode 100644 index 000000000..a5dbde23b --- /dev/null +++ b/test/form/form_cell_numbers.libsonnet @@ -0,0 +1,35 @@ +// Round trip of each event's own number through a single-layer (job -> event) file. +local layers(starting_number) = { + event: { parent: 'job', total: 200, starting_number: starting_number }, +}; + +{ + write(output_file, starting_number):: { + driver: { cpp: 'generate_layers', layers: layers(starting_number) }, + sources: { + numbers: { cpp: 'cell_number_vector', creator: 'numbers' }, + }, + modules: { + stamper: { cpp: 'cell_number_vector', input_creator: 'numbers' }, + output: { cpp: 'form_module', output_file: output_file, products: ['cell_numbers'] }, + }, + }, + + read(input_file, starting_number):: { + driver: { cpp: 'generate_layers', layers: layers(starting_number) }, + sources: { + form_in: { + cpp: 'form_source', + stage: 'test', + input_file: input_file, + plugin: 'stamper', + algorithm: 'stamp', + creator: 'stamper', + products: ['cell_numbers'], + }, + }, + modules: { + check: { cpp: 'cell_number_vector', input_creator: 'stamper', check: true }, + }, + }, +} diff --git a/test/form/form_cell_numbers_offset_read.jsonnet b/test/form/form_cell_numbers_offset_read.jsonnet new file mode 100644 index 000000000..d1cca1ea6 --- /dev/null +++ b/test/form/form_cell_numbers_offset_read.jsonnet @@ -0,0 +1,2 @@ +local cells = import 'form_cell_numbers.libsonnet'; +cells.read('form_cell_numbers_offset.root', 1000) diff --git a/test/form/form_cell_numbers_offset_write.jsonnet b/test/form/form_cell_numbers_offset_write.jsonnet new file mode 100644 index 000000000..b5f7bc789 --- /dev/null +++ b/test/form/form_cell_numbers_offset_write.jsonnet @@ -0,0 +1,2 @@ +local cells = import 'form_cell_numbers.libsonnet'; +cells.write('form_cell_numbers_offset.root', 1000) diff --git a/test/form/form_cell_numbers_unordered_read.jsonnet b/test/form/form_cell_numbers_unordered_read.jsonnet new file mode 100644 index 000000000..e3648683a --- /dev/null +++ b/test/form/form_cell_numbers_unordered_read.jsonnet @@ -0,0 +1,2 @@ +local cells = import 'form_cell_numbers.libsonnet'; +cells.read('form_cell_numbers_unordered.root', 0) diff --git a/test/form/form_cell_numbers_unordered_write.jsonnet b/test/form/form_cell_numbers_unordered_write.jsonnet new file mode 100644 index 000000000..13496cf1d --- /dev/null +++ b/test/form/form_cell_numbers_unordered_write.jsonnet @@ -0,0 +1,2 @@ +local cells = import 'form_cell_numbers.libsonnet'; +cells.write('form_cell_numbers_unordered.root', 0) diff --git a/test/form/form_storage_test.cpp b/test/form/form_storage_test.cpp index 668a86fc0..c678db80b 100644 --- a/test/form/form_storage_test.cpp +++ b/test/form/form_storage_test.cpp @@ -25,6 +25,7 @@ #include #include #include +#include #include #include @@ -81,6 +82,80 @@ namespace { .hierarchy = {{"event", "segment"}}, .layer_values = {event, segment}}; } + + cell_index event_cell(std::uint64_t event) + { + return cell_index{.id = "[event:" + std::to_string(event) + "]", + .hierarchy = {{"event"}}, + .layer_values = {event}}; + } + + // Each cell's payload is its own layer values, so a read-back shows which cell a row belongs to. + std::vector payload_of(cell_index const& cell) + { + std::vector payload; + payload.reserve(cell.layer_values.size()); + for (auto const value : cell.layer_values) { + payload.push_back(static_cast(value)); + } + return payload; + } + + std::vector ids_of(std::vector const& cells) + { + std::vector ids; + ids.reserve(cells.size()); + for (auto const& cell : cells) { + ids.push_back(cell.id); + } + return ids; + } + + // Writes one "prod" row per cell, in the given order. + void write_cells(std::string const& file_name, + std::string const& creator, + std::vector const& cells) + { + auto writer = create_persistence_writer(); + REQUIRE(writer != nullptr); + writer->configure_tech_settings(form::experimental::config::tech_setting_config{}); + auto const prod_place = make_placement(file_name, creator, "prod", technology); + writer->create_containers({{prod_place, &typeid(std::vector)}}); + for (auto const& cell : cells) { + auto const payload = payload_of(cell); + writer->register_write( + identity(creator, "prod"), prod_place, &payload, typeid(std::vector)); + writer->commit_place(prod_place, cell); + } + writer->finalize(); + } + + 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}; + } + + // Locates a cell through the index and reads the "prod" payload stored in its row. + std::vector read_cell(storage_reader& reader, + std::string const& file_name, + std::string const& creator, + std::string const& id) + { + form::experimental::config::tech_setting_config const settings{}; + auto const row = reader.get_index(index_token_of(file_name, creator), id, settings); + void const* raw = nullptr; + reader.read_container(token{file_name, + build_full_label(row_space(creator), "prod"), + technology, + static_cast(row)}, + &raw, + typeid(std::vector), + settings); + std::unique_ptr const> const payload{ + static_cast const*>(raw)}; + REQUIRE(payload != nullptr); + return *payload; + } } int main(int const argc, char** const argv) @@ -448,56 +523,144 @@ TEST_CASE("Root TBranch fill: throws when TBranch::Fill() reports a write error" raw_tfile->SetWritable(true); } -TEST_CASE("Persistence round-trip: structured index normalization and listing", "[form]") +TEST_CASE("Persistence round-trip: every listed cell reads back its own payload", "[form]") { using namespace form::experimental::config; std::string const file_name = "persistence_roundtrip_" + form::technology::to_string(technology) + ".root"; - std::string const creator = "norm_creator"; + std::string const creator = "roundtrip_creator"; + std::vector const cells{make_cell(3, 1), make_cell(1, 2), make_cell(2, 0)}; + write_cells(file_name, creator, cells); item_config cfg; cfg.add_item("prod", file_name, technology); + auto reader = create_persistence_reader(); + REQUIRE(reader != nullptr); + reader->configure(cfg); + reader->configure_tech_settings(tech_setting_config{}); + reader->prime(creator, test_stage, "prod", typeid(std::vector)); - std::vector first = {10, 20, 30}; - std::vector second = {40, 50, 60}; + CHECK(reader->list_indices(creator, test_stage, "prod") == ids_of(cells)); + for (auto const& cell : cells) { + void const* raw = nullptr; + reader->read(creator, test_stage, "prod", cell.id, &raw, typeid(std::vector)); + std::unique_ptr const> const read_back{ + static_cast const*>(raw)}; + REQUIRE(read_back != nullptr); + CHECK(*read_back == payload_of(cell)); + } +} - cell_index const first_id = make_cell(1, 2); - cell_index const second_id = make_cell(3, 4); +TEST_CASE("storage_reader get_index: cells written out of order resolve to their own rows", + "[form]") +{ + auto const tech = form::technology::to_string(technology); + std::string const creator = "unordered_creator"; + + // Each cell must resolve to the row it was written to and read back its own payload. + auto const check_cells = [&creator](std::string const& file_name, + std::vector const& cells) { + write_cells(file_name, creator, cells); + storage_reader reader; + form::experimental::config::tech_setting_config const settings{}; + CHECK(reader.list_indices(index_token_of(file_name, creator), settings) == ids_of(cells)); + for (std::size_t row = 0; row != cells.size(); ++row) { + CHECK(reader.get_index(index_token_of(file_name, creator), cells[row].id, settings) == + static_cast(row)); + CHECK(read_cell(reader, file_name, creator, cells[row].id) == payload_of(cells[row])); + } + }; + SECTION("single layer") { - auto writer = create_persistence_writer(); - REQUIRE(writer != nullptr); - writer->configure_tech_settings(tech_setting_config{}); - auto const prod_place = make_placement(file_name, creator, "prod", technology); - // Persistence adds the place's index container itself; the caller names only the product. - writer->create_containers({{prod_place, &typeid(std::vector)}}); + // [event:N] is not row N: neither the write order nor the numbering (from 1000) follows rows. + check_cells("storage_reader_unordered_single_" + tech + ".root", + {event_cell(5), event_cell(2), event_cell(7), event_cell(0), event_cell(1000)}); + } + SECTION("multiple layers") + { + check_cells("storage_reader_unordered_multi_" + tech + ".root", + {make_cell(3, 1), make_cell(1, 2), make_cell(2, 0)}); + } +} - writer->register_write(identity(creator, "prod"), prod_place, &first, typeid(std::vector)); - writer->commit_place(prod_place, first_id); +TEST_CASE("storage_reader get_index: the job cell resolves through the index", "[form]") +{ + std::string const file_name = + "storage_reader_job_cell_" + form::technology::to_string(technology) + ".root"; + std::string const creator = "job_cell_creator"; + write_cells(file_name, creator, {cell_index{.id = "[]"}}); - writer->register_write( - identity(creator, "prod"), prod_place, &second, typeid(std::vector)); - writer->commit_place(prod_place, second_id); - } + storage_reader reader; + CHECK(reader.get_index(index_token_of(file_name, creator), "[]", {}) == 0); + CHECK(read_cell(reader, file_name, creator, "[]").empty()); +} - auto reader = create_persistence_reader(); - REQUIRE(reader != nullptr); - reader->configure(cfg); - reader->configure_tech_settings(tech_setting_config{}); +TEST_CASE("storage_reader get_index: index maps are kept per file", "[form]") +{ + auto const tech = form::technology::to_string(technology); + std::string const first_file = "storage_reader_per_file_a_" + tech + ".root"; + std::string const second_file = "storage_reader_per_file_b_" + tech + ".root"; + // Same creator and stage, so both files hold an index container of the same name. + std::string const creator = "per_file_creator"; + std::vector const first_cells{event_cell(0), event_cell(1), event_cell(2)}; + std::vector const second_cells{event_cell(2), event_cell(0), event_cell(3)}; + write_cells(first_file, creator, first_cells); + write_cells(second_file, creator, second_cells); - reader->prime(creator, test_stage, "prod", typeid(std::vector)); + storage_reader reader; + form::experimental::config::tech_setting_config const settings{}; + // Alternate between the files with one reader so that a shared index map would be reused. + for (std::size_t row = 0; row != first_cells.size(); ++row) { + for (auto const& [file_name, cells] : + {std::pair{first_file, first_cells}, std::pair{second_file, second_cells}}) { + CHECK(reader.get_index(index_token_of(file_name, creator), cells[row].id, settings) == + static_cast(row)); + CHECK(read_cell(reader, file_name, creator, cells[row].id) == payload_of(cells[row])); + } + } + CHECK(reader.list_indices(index_token_of(first_file, creator), settings) == ids_of(first_cells)); + CHECK(reader.list_indices(index_token_of(second_file, creator), settings) == + ids_of(second_cells)); + CHECK_THROWS_AS(reader.get_index(index_token_of(first_file, creator), "[event:3]", settings), + std::runtime_error); +} + +TEST_CASE("persistence_writer: a cell written twice by one stream throws", "[form]") +{ + std::string const file_name = + "persistence_writer_duplicate_" + form::technology::to_string(technology) + ".root"; + CHECK_THROWS_AS( + write_cells(file_name, "duplicate_creator", {event_cell(1), event_cell(2), event_cell(1)}), + std::runtime_error); +} - auto indices = reader->list_indices(creator, test_stage, "prod"); - REQUIRE_FALSE(indices.empty()); +TEST_CASE("storage_reader get_index: a cell recorded twice in the index throws", "[form]") +{ + std::string const file_name = + "storage_reader_duplicate_" + form::technology::to_string(technology) + ".root"; + std::string const creator = "duplicate_creator"; + { + form::experimental::config::tech_setting_config const settings{}; + auto storage = create_storage_writer(); + REQUIRE(storage != nullptr); + placement const index_place{ + file_name, build_full_label(row_space(creator), "index"), technology}; + storage->create_containers({{index_place, &typeid(std::string)}}, settings); + for (std::string const id : {"[event:1]", "[event:2]", "[event:1]"}) { + storage->fill_container(index_place, &id, typeid(std::string)); + storage->commit_containers(index_place); + } + } - void const* raw = nullptr; - // Backends may canonicalize persisted index strings differently. - // Use an index emitted by list_indices() to verify readback. - reader->read(creator, test_stage, "prod", indices.front(), &raw, typeid(std::vector)); - auto const* read_first = static_cast const*>(raw); - REQUIRE(read_first != nullptr); - CHECK((*read_first == first || *read_first == second)); + // Which row holds [event:1] is ambiguous, so the index is rejected rather than guessed. + storage_reader reader; + form::experimental::config::tech_setting_config const settings{}; + CHECK_THROWS_AS(reader.get_index(index_token_of(file_name, creator), "[event:2]", settings), + std::runtime_error); + CHECK_THROWS_AS(reader.list_indices(index_token_of(file_name, creator), settings), + std::runtime_error); } TEST_CASE("register_write returns a token locating the written product", "[form]") @@ -583,82 +746,33 @@ TEST_CASE("register_write throws when the backend does not address rows", "[form std::runtime_error); } -TEST_CASE("Persistence round-trip: all-zero structured id fallback", "[form]") -{ - using namespace form::experimental::config; - - std::string const file_name = - "persistence_zero_index_" + form::technology::to_string(technology) + ".root"; - std::string const creator = "zero_creator"; - - item_config cfg; - cfg.add_item("prod", file_name, technology); - - std::vector payload = {7, 8, 9}; - { - auto writer = create_persistence_writer(); - REQUIRE(writer != nullptr); - writer->configure_tech_settings(tech_setting_config{}); - auto const prod_place = make_placement(file_name, creator, "prod", technology); - // Persistence adds the place's index container itself; the caller names only the product. - writer->create_containers({{prod_place, &typeid(std::vector)}}); - writer->register_write( - identity(creator, "prod"), prod_place, &payload, typeid(std::vector)); - writer->commit_place(prod_place, cell_index{}); - } - - auto reader = create_persistence_reader(); - REQUIRE(reader != nullptr); - reader->configure(cfg); - reader->configure_tech_settings(tech_setting_config{}); - - void const* raw = nullptr; - reader->read(creator, test_stage, "prod", "[event:0, segment:0]", &raw, typeid(std::vector)); - - auto const* read_payload = static_cast const*>(raw); - REQUIRE(read_payload != nullptr); - CHECK(*read_payload == payload); -} - -TEST_CASE("storage_reader get_index: malformed ids and compatibility fallbacks", "[form]") +TEST_CASE("storage_reader get_index: ids not recorded in the index throw", "[form]") { - using namespace form::experimental::config; - std::string const file_name = "storage_reader_index_branches_" + form::technology::to_string(technology) + ".root"; std::string const creator = "storage_reader_creator"; - std::string const index_container = build_full_label(row_space(creator), "index"); - - std::vector payload = {1, 2, 3}; - { - auto writer = create_persistence_writer(); - REQUIRE(writer != nullptr); - writer->configure_tech_settings(tech_setting_config{}); - auto const prod_place = make_placement(file_name, creator, "prod", technology); - // Persistence adds the place's index container itself; the caller names only the product. - writer->create_containers({{prod_place, &typeid(std::vector)}}); - writer->register_write( - identity(creator, "prod"), prod_place, &payload, typeid(std::vector)); - writer->commit_place(prod_place, make_cell(1, 2)); - } + write_cells(file_name, creator, {make_cell(1, 2)}); storage_reader reader; - token const index_token{file_name, index_container, technology}; - tech_setting_config const settings{}; - - CHECK(reader.get_index(index_token, "[]", settings) == 0); - CHECK(reader.get_index(index_token, "plain-text-id", settings) == 0); - - // Malformed IDs must be rejected by the storage-layer index parser - CHECK_THROWS_AS(reader.get_index(index_token, "[EVENT,SEG=1]", settings), std::runtime_error); - CHECK_THROWS_AS( - reader.get_index(index_token, "[EVENT=99999999999999999999999999999999]", settings), - std::runtime_error); - CHECK_THROWS_AS(reader.get_index(index_token, "[=1]", settings), std::runtime_error); - CHECK_THROWS_AS(reader.get_index(index_token, "[EVENT]", settings), std::runtime_error); - CHECK_THROWS_AS(reader.get_index(index_token, "[ ]", settings), std::runtime_error); + auto const index_token = index_token_of(file_name, creator); + form::experimental::config::tech_setting_config const settings{}; + + CHECK(reader.get_index(index_token, "[event:1, segment:2]", settings) == 0); + + // Only the canonical id written for a cell locates it; nothing falls back to row 0. + for (auto const* id : {"[]", + "plain-text-id", + "[event:1]", + "[event:0, segment:0]", + "[event:1,segment:2]", + "[segment:2, event:1]", + "[EVENT:1, SEGMENT:2]", + "[EVENT=1;SEGMENT=2]", + "[event:01, segment:2]"}) { + INFO("id: " << id); + CHECK_THROWS_AS(reader.get_index(index_token, id, settings), std::runtime_error); + } } - TEST_CASE("storage_reader get_index: empty container and tech-table branches", "[form]") { using namespace form::experimental::config; @@ -695,19 +809,22 @@ TEST_CASE("storage_reader get_index: empty container and tech-table branches", " tech_setting_config attr_settings; attr_settings.file_settings[technology][file_name] = {{"compression", "1"}}; - CHECK( - reader.get_index(token{file_name, build_full_label(row_space(creator), "index"), technology}, - "missing-id", - attr_settings) == 0); + CHECK(reader.get_index( + index_token_of(file_name, creator), "[event:5, segment:6]", attr_settings) == 0); + CHECK_THROWS_AS(reader.get_index(index_token_of(file_name, creator), "missing-id", attr_settings), + std::runtime_error); + // The index of a (file, container) is read once; later settings do not rebuild it. tech_setting_config container_attr_settings; container_attr_settings .container_settings[technology][build_full_label(row_space(creator), "index")] = { {"split", "0"}}; - CHECK_NOTHROW( - reader.get_index(token{file_name, build_full_label(row_space(creator), "index"), technology}, - "missing-id", - container_attr_settings)); + CHECK(reader.get_index(index_token_of(file_name, creator), + "[event:5, segment:6]", + container_attr_settings) == 0); + CHECK_THROWS_AS( + reader.get_index(index_token_of(file_name, creator), "missing-id", container_attr_settings), + std::runtime_error); } TEST_CASE("storage_reader prime/list_indices/read_container: attribute and error branches",