Form write output metadata - #647
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds product-name propagation and a ChangesROOT metadata tree generation and product-name propagation
Sequence Diagram(s)sequenceDiagram
rect rgba(100, 149, 237, 0.5)
Note over FormOutputModule: Construction
FormOutputModule->>form_writer_interface: declareProductName(product, product) ×N
end
rect rgba(144, 238, 144, 0.5)
Note over FormOutputModule: Per-event write
FormOutputModule->>form_writer_interface: write(creator, segment_id, product_with_name)
form_writer_interface->>form_writer_interface: lookup declared_name in m_label_to_product_name
form_writer_interface->>PersistenceWriter: registerWrite(creator, label, data, type, declared_name)
PersistenceWriter->>PersistenceWriter: cache creator as m_current_creator
PersistenceWriter->>StorageWriter: fillContainer(placement, data, type, product_name)
StorageWriter->>StorageWriter: update producer/product registry or index metadata
end
rect rgba(255, 165, 0, 0.5)
Note over FormOutputModule: Teardown / finalize
FormOutputModule->>form_writer_interface: finalize()
form_writer_interface->>PersistenceWriter: finalize()
PersistenceWriter->>StorageWriter: finalize(tech_settings)
StorageWriter->>RootFile: write FileCatalog (FileUUID, FileFormatVersion)
StorageWriter->>RootFile: write ProductRegistry (ProductID per producer/product)
StorageWriter->>RootFile: write IndexRegistry (layer branches, LayerSchema header, entries)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
31 fixed, 0 new since branch point (ac671f4) ✅ 31 CodeQL alerts resolved since the previous PR commit
✅ 31 CodeQL alerts resolved since the branch point
Review the full CodeQL report for details. |
Clang-Tidy Check Results2 new issue(s) introduced by this patch (6000 total). New issues on modified lines:
All issues by check:
See inline comments for details. Comment |
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/form/writer.cpp (1)
25-26: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueNaming convention: use
lower_casefor constants, notUPPER_CASE.Per the coding guidelines,
UPPER_CASEis reserved for macros. Static constants should uselower_casenaming (e.g.,number_event,number_segment).Suggested fix
-static int const NUMBER_EVENT = 4; -static int const NUMBER_SEGMENT = 15; +static int const number_event = 4; +static int const number_segment = 15;(And update references throughout the file accordingly.)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/form/writer.cpp` around lines 25 - 26, The static constants NUMBER_EVENT and NUMBER_SEGMENT are using UPPER_CASE naming convention, which is reserved for macros per the coding guidelines. Rename both constants to use lower_case naming: number_event and number_segment. Then search through the entire file and update all references to these constants to use the new lower_case names.Source: Coding guidelines
form/storage/istorage.hpp (1)
10-13:⚠️ Potential issue | 🟠 MajorAdd
<cstdint>and renamegetEntryCount()to comply with naming conventions.Line 71 introduces
std::uint64_twithout including<cstdint>, which breaks header self-sufficiency and creates a transitive dependency risk. Additionally,getEntryCount()violates the lower_case naming requirement for C++ identifiers—it should beget_entry_count(). This naming issue spans multiple files:storage_write_container.hpp,root_tbranch_write_container.hpp, androot_ttree_write_container.hppall declare this same method.Include fix for istorage.hpp
`#include` <map> `#include` <memory> +#include <cstdint> `#include` <string> `#include` <vector>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@form/storage/istorage.hpp` around lines 10 - 13, Add the missing `<cstdint>` header include to form/storage/istorage.hpp before the existing includes to ensure std::uint64_t is properly defined and resolve the header self-sufficiency issue. Additionally, rename the method getEntryCount() to get_entry_count() to comply with C++ lower_case naming conventions—this method needs to be renamed in all four files: form/storage/istorage.hpp (the interface declaration), form/storage/storage_write_container.hpp (implementation), form/storage/root_tbranch_write_container.hpp (implementation), and form/storage/root_ttree_write_container.hpp (implementation). Update all call sites that invoke getEntryCount() to use the new get_entry_count() name instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@form/form_module.cpp`:
- Around line 55-61: The ~FormOutputModule() destructor calls
m_form_interface->finalize() without exception handling, which can cause process
termination if an exception is thrown during stack unwinding. Wrap the
m_form_interface->finalize() call in a try-catch block to safely handle any
exceptions that may be thrown, or alternatively move the finalize() call to an
explicit lifecycle method (like a cleanup or shutdown method) where exceptions
can propagate safely rather than being called from the destructor.
In `@form/form/form_writer.cpp`:
- Line 87: Add an idempotency guard to the form_writer_interface::finalize()
method to prevent duplicate metadata writes. Introduce a boolean member variable
to track whether finalize has already been called. In the finalize() method,
check this flag before calling m_pers_writer->finalize(), and set the flag to
true after the first successful call. This ensures that even if finalize() is
called multiple times (by the destructor and by explicit user calls), the
underlying persister's finalize() method is only executed once, preventing
metadata corruption.
In `@form/form/form_writer.hpp`:
- Line 35: Rename the method `declareProductName` to `declare_product_name` to
adhere to the repository's C++ naming conventions which require all identifiers
to use lower_case naming. Update the method declaration in the header file and
ensure the same rename is applied to any implementations and call sites
throughout the codebase.
In `@form/root_storage/root_tbranch_write_container.cpp`:
- Around line 134-140: The getEntryCount() method in
ROOT_TBranch_Write_ContainerImp is returning the tree's entry count via
m_tree->GetEntries() when it should return this specific branch's entry count
instead. This causes incorrect payload_row metadata when fillContainer() uses
this count for IndexRegistry, especially when branches in the shared tree have
different entry counts. Replace the return statement to call GetEntries() on the
branch member variable (likely m_branch or similar) instead of m_tree to ensure
each container reports its own branch's entry count.
In `@form/root_storage/root_tfile.cpp`:
- Around line 14-28: The ROOT_TFileImp constructor is missing validation after
each TFile::Open() call. After each assignment to m_file (for the "RECREATE",
"UPDATE", and "READ" modes), add a check to verify that the returned pointer is
not nullptr and that the TFile is not in a zombie state by calling IsZombie().
If either validation fails, throw a std::runtime_error with a descriptive
message indicating the failure reason and the attempted file mode and path. This
ensures that invalid file states are caught immediately at construction time
rather than causing undefined behavior during later file access operations.
In `@form/storage/istorage.hpp`:
- Line 71: The method name getEntryCount violates the project's lower_case
naming convention for C++ identifiers. Rename the virtual method getEntryCount
to get_entry_count in the interface definition, and then update all
implementations and call sites of this method throughout the codebase to use the
new name instead.
In `@form/storage/storage_writer.cpp`:
- Around line 419-424: The code at lines 419-424 uses an unsafe static_cast to
downcast the TObject pointer retrieved from tfile->Get("FileCatalog") to a TTree
pointer without verifying the actual type. This is dangerous because if the
object exists but is not a TTree (due to file corruption, version mismatches, or
manual edits), the static_cast will produce a non-null pointer to garbage data,
causing undefined behavior. Replace the static_cast<TTree*> with
dynamic_cast<TTree*> to ensure type-safe downcasting, which will return nullptr
if the object is not actually a TTree and allow the null check to properly
detect invalid objects.
- Line 382: Remove the misleading `(void)product_name;` cast statement from the
code. This cast suggests that the product_name parameter is unused when it is
actually utilized within the function (specifically on line 321 in the
non-index-container branch), which will confuse future readers about the
parameter's purpose and usage. Simply delete this line to avoid sending false
signals about the parameter being unused.
---
Outside diff comments:
In `@form/storage/istorage.hpp`:
- Around line 10-13: Add the missing `<cstdint>` header include to
form/storage/istorage.hpp before the existing includes to ensure std::uint64_t
is properly defined and resolve the header self-sufficiency issue. Additionally,
rename the method getEntryCount() to get_entry_count() to comply with C++
lower_case naming conventions—this method needs to be renamed in all four files:
form/storage/istorage.hpp (the interface declaration),
form/storage/storage_write_container.hpp (implementation),
form/storage/root_tbranch_write_container.hpp (implementation), and
form/storage/root_ttree_write_container.hpp (implementation). Update all call
sites that invoke getEntryCount() to use the new get_entry_count() name instead.
In `@test/form/writer.cpp`:
- Around line 25-26: The static constants NUMBER_EVENT and NUMBER_SEGMENT are
using UPPER_CASE naming convention, which is reserved for macros per the coding
guidelines. Rename both constants to use lower_case naming: number_event and
number_segment. Then search through the entire file and update all references to
these constants to use the new lower_case names.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6cc1ca9f-d977-4db4-b88c-88f4ba8f8e1a
📒 Files selected for processing (18)
form/form/form_writer.cppform/form/form_writer.hppform/form_module.cppform/persistence/ipersistence_writer.hppform/persistence/persistence_writer.cppform/persistence/persistence_writer.hppform/root_storage/root_tbranch_write_container.cppform/root_storage/root_tbranch_write_container.hppform/root_storage/root_tfile.cppform/root_storage/root_ttree_write_container.cppform/root_storage/root_ttree_write_container.hppform/storage/istorage.hppform/storage/storage_write_container.cppform/storage/storage_write_container.hppform/storage/storage_writer.cppform/storage/storage_writer.hpptest/form/form_test_with_output.jsonnettest/form/writer.cpp
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Analyze cpp with CodeQL
- GitHub Check: build (gcc, none)
- GitHub Check: clang-tidy-check
- GitHub Check: coverage
🧰 Additional context used
📓 Path-based instructions (3)
**/*.jsonnet
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use
jsonnetfmtfor consistent Jsonnet formatting; CI enforces thisJsonnet workflow configuration files must be formatted with
jsonnetfmtand CI enforces this formatting
Files:
test/form/form_test_with_output.jsonnet
**/*.{cpp,cc,cxx,h,hpp}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{cpp,cc,cxx,h,hpp}: Use clang-format tool for all C++ code formatting (VS Code auto-formats on save); configuration defined in.clang-formatwith 100-character line limit and 2-space indentation
Follow clang-tidy recommendations defined in.clang-tidy
Files:
form/storage/istorage.hppform/root_storage/root_tbranch_write_container.hppform/root_storage/root_ttree_write_container.hppform/form/form_writer.hppform/persistence/ipersistence_writer.hppform/root_storage/root_tbranch_write_container.cppform/storage/storage_write_container.cppform/root_storage/root_ttree_write_container.cppform/storage/storage_write_container.hppform/root_storage/root_tfile.cppform/persistence/persistence_writer.hppform/persistence/persistence_writer.cppform/form_module.cpptest/form/writer.cppform/form/form_writer.cppform/storage/storage_writer.hppform/storage/storage_writer.cpp
**/*.{hpp,cpp}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{hpp,cpp}: Use.hppfor C++ header files and.cppfor implementation files; test files should be named*_test.cpp
All C++ identifiers (namespaces, classes, structs, enums, functions, variables, parameters, members, constants, type aliases) must uselower_casenaming
Template parameters in C++ must useCamelCasenaming
C++ macros must useUPPER_CASEnaming
Private, protected, and constant C++ members must have a trailing underscore (_)
Useeast-conststyle in C++:int const xnotconst int x
Preferenum classover plainenumin C++
Avoid boolean parameters in C++ function interfaces; prefer enumerations instead
Usestd::shared_ptrfor shared ownership,std::unique_ptrfor exclusive ownership, and raw pointers only for non-owning references in C++
C++ clang-format enforces 100-character line limit and 2-space indentation withQualifierAlignment: RightandPointerAlignment: Left
Use namespacesphlex::for core code andphlex::experimental::for experimental features in C++
Use functors with agent noun naming (e.g.,ModelEvaluator evaluate_model(...)) in C++
Files:
form/storage/istorage.hppform/root_storage/root_tbranch_write_container.hppform/root_storage/root_ttree_write_container.hppform/form/form_writer.hppform/persistence/ipersistence_writer.hppform/root_storage/root_tbranch_write_container.cppform/storage/storage_write_container.cppform/root_storage/root_ttree_write_container.cppform/storage/storage_write_container.hppform/root_storage/root_tfile.cppform/persistence/persistence_writer.hppform/persistence/persistence_writer.cppform/form_module.cpptest/form/writer.cppform/form/form_writer.cppform/storage/storage_writer.hppform/storage/storage_writer.cpp
🪛 Cppcheck (2.21.0)
form/root_storage/root_tbranch_write_container.cpp
[style] 134-134: The function 'getEntryCount' is never used.
(unusedFunction)
form/root_storage/root_ttree_write_container.cpp
[style] 60-60: The function 'getEntryCount' is never used.
(unusedFunction)
form/form/form_writer.cpp
[style] 81-81: The function 'declareProductName' is never used.
(unusedFunction)
form/storage/storage_writer.cpp
[style] 38-38: The function 'getItems' is never used.
(unusedFunction)
[style] 59-59: The function 'createReadContainer' is never used.
(unusedFunction)
🔇 Additional comments (30)
form/storage/storage_writer.hpp (3)
9-16: LGTM!
29-34: LGTM!
43-51: LGTM!form/storage/storage_writer.cpp (8)
8-27: LGTM!
31-62: LGTM!
64-74: LGTM!
76-103: LGTM!
105-153: LGTM!
155-240: LGTM!
266-268: LGTM!Also applies to: 291-293
396-401: LGTM!Also applies to: 426-452, 454-483, 485-582
form/persistence/persistence_writer.hpp (2)
34-40: LGTM!
49-49: LGTM!form/persistence/persistence_writer.cpp (3)
52-61: LGTM!
64-71: LGTM!
86-87: LGTM!test/form/writer.cpp (9)
10-12: LGTM!Also applies to: 21-22
161-164: LGTM!
166-212: LGTM!
214-269: LGTM!
271-330: LGTM!
332-387: LGTM!
478-494: LGTM!
497-532: LGTM!
452-458: Remove check: PayloadRow == entry assumption is correct.The test's expectation that
PayloadRowequals the IndexRegistry entry index is actually sound. Each product write increments the container's entry count—the code capturesgetEntryCount()before filling and stores that as the payload row. Since each product creates one index entry, the sequence is naturally 0, 1, 2, ... which matches the entry numbers. The code comment at line 333 even confirms this explicitly: "Keep payload row 0-based so it maps directly to entry/row ids."test/form/form_test_with_output.jsonnet (1)
1-33: LGTM!form/storage/storage_write_container.cpp (1)
31-31: LGTM!form/form/form_writer.hpp (1)
31-31: LGTM!Also applies to: 40-40
form/form/form_writer.cpp (1)
40-43: LGTM!Also applies to: 72-75
form/root_storage/root_ttree_write_container.cpp (1)
60-66: LGTM!
|
@phlexbot format |
|
No automatic format fixes were necessary. |
Clang-Tidy Check Results6 new issue(s) introduced by this patch (6019 total). New issues on modified lines:
All issues by check:
See inline comments for details. Comment |
Clang-Tidy Check Results6 new issue(s) introduced by this patch (6019 total). New issues on modified lines:
All issues by check:
See inline comments for details. Comment |
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #647 +/- ##
==========================================
+ Coverage 83.27% 83.86% +0.59%
==========================================
Files 162 166 +4
Lines 5912 6469 +557
Branches 670 771 +101
==========================================
+ Hits 4923 5425 +502
- Misses 796 807 +11
- Partials 193 237 +44
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
Clang-Tidy Check Results1 new issue(s) introduced by this patch (6012 total). New issues on modified lines:
All issues by check:
See inline comments for details. Comment |
Clang-Tidy Check ResultsFound 6011 issue(s); none are newly introduced by this patch. All issues by check:
See inline comments for details. Comment |
Clang-Tidy Check ResultsFound 6028 issue(s); none are newly introduced by this patch. All issues by check:
See inline comments for details. Comment |
Clang-Tidy Check ResultsFound 6029 issue(s); none are newly introduced by this patch. All issues by check:
See inline comments for details. Comment |
aolivier23
left a comment
There was a problem hiding this comment.
The big change I need to approve this is to make form_storage_test technology-independent again. I'm biased because making it work with other technologies was my work a few weeks ago. But I think RNTuple containers are important enough to the long-term project that we shouldn't undo that work without a compelling reason. I think we can do that with some small changes to the existing code in this PR. But a TTree-container-specific separate test program could also make sense.
04ff752 to
a4fa8d2
Compare
Clang-Tidy Check ResultsFound 6031 issue(s); none are newly introduced by this patch. All issues by check:
See inline comments for details. Comment |
…a in output FORM file
…red ProductID; add ContainerName and PayloadRow for physical location; support 0-based PayloadRow
…al Boost UUID dependency
a4fa8d2 to
8183569
Compare
Clang-Tidy Check ResultsFound 6029 issue(s); none are newly introduced by this patch. All issues by check:
See inline comments for details. Comment |
Clang-Tidy Check ResultsFound 6033 issue(s); none are newly introduced by this patch. All issues by check:
See inline comments for details. Comment |
gemmeren
left a comment
There was a problem hiding this comment.
I know this PR has been merged already, still adding a few experts for review.
My conclusion is that this introduces a new and unwanted dependency of the technology neutral Storage on ROOT and TTree that fundamentally violates FORM design.
@wwuoneway and I are not in agreement on whether this should be part of a public 0.3 release.
|
This PR introduced a use of I don't know why CI didn't catch this, though CodeRabbit did leave a comment about this (but hidden in the "outside diff range" section). |
Purpose
To enable fast, fine-grained reading, this PR writes the basic metadata and indexing information needed for file-level, product-level, and locator-lookup into the FORM output file. This provides the reading framework with the ability to locate and lazy-load data products without needing to scan the full payload up front.
What's Changed
FileCatalogy,ProductRegistry, andIndexRegistryTTrees in the output file containing the initial set of metadata for data products.ProductRegistryincludes necessary branches (ProductName,ProcessName,Producer,ProductID) to establish a unique logical identity for each product.IndexRegistryincludes layer identifiers(data cell indices),ProductID,ContainerName, andPayloadRow, which together provide the physical location of each product in the file.Next Steps
Code Changes
Core Writer Interface
form_writer_interface: AddeddeclareProductName(routing_label, product_name)to map routing labels to product names, andfinalize()to finalize metadata outputPersistence Layer
IPersistenceWriter::registerWrite: Extended with optionalproduct_nameparameter (defaults to empty string)PersistenceWriter: UpdatedregisterWrite()implementation to accept and cacheproduct_name; addedfinalize()method to delegate to storage writer finalizationStorage Layer
IStorageWriter::fillContainer: Extended with optionalproduct_nameparameterStorageWriter::fillContainer: Expanded to acceptproduct_nameand route it appropriately to index and product registriesStorageWriter::finalize: Major new implementation (+492/-3 lines) that:FileCatalogTTree with file-level metadata (FileUUID,FileFormatVersion)ProductRegistryTTree with per-product identification (ProductName,ProcessName,Producer,ProductID)IndexRegistryTTree with physical location mappings (layer identifiers,ProductID,ContainerName,PayloadRow)ROOT Storage Infrastructure
ROOT_TBranch_Write_ContainerImpandROOT_TTree_Write_ContainerImp: AddedgetEntryCount()method returning entry count or 0 if tree not attachedStorage_Write_Container: AddedgetEntryCount()override (returns 0)ROOT_TFileImp: Updated file open-mode handling with new mode'u'forUPDATE(conditional on existing file), preserving'c'/'o'asRECREATE, changing'r'/'i'toREAD, and throwing on unsupported modesContainer Interface
IStorage_Write_Container: AddedgetEntryCount()virtual method for entry count reportingModule Integration
FormOutputModule
declareProductName(product, product)m_form_interface->finalize()with logging to ensure metadata finalization during teardownTests
Metadata Validation
test/form/writer.cpp(+378 lines) that validates all three metadata registries post-write:FileCatalogexists with properFileUUIDandFileFormatVersionProductRegistryhas entries with properly formattedProductID(asProductName|Producer|ProcessName)IndexRegistrywith layer schema extraction fromTObjStringuser-info, branch ordering verification, and payload row monotonicity checksIndexRegistryandProductRegistryentriesEVENT,SEG)Test Configuration
test/form/form_test_with_output.jsonnetdefining a form test withgenerate_layersdriver,ij_sourceprovider, generic and form modules saving products['sum', 'i', 'j'], and output file mappingsSummary
The implementation establishes the foundational metadata and indexing infrastructure for fast, fine-grained reading capabilities, enabling future lazy-loading of data products without full file payload scanning. The three-registry approach cleanly separates file-level metadata, logical product identity, and physical location information, with comprehensive validation confirming end-to-end write correctness.