[ntuple] Merger: pass the destination in the ctor rather than in Merge()

[silverweed]

RNTupleMerger should share the lifetime of its destination file to better support incremental merging, e.g.

// open the destination file once
auto destination = CreatePageSink();
RNTupleMerger merger { *destination };
auto opts = RNTupleMergeOptions{};
opts.fMergingMode = ENTupleMergingMode::kUnion;
// keep merging stuff into it as it arrives (e.g. from the network)
while (auto result = AcquireSources()) {
  merger.Merge(result->sources, opts);
}

Checklist:

• tested changes locally
• updated the docs (if necessary)

[pcanal]

Wouldn't passing a std::shared_ptr be a better indication of the required lifetime?

[silverweed]

I don't think so, a) because it's clear that the merger should have a strictly smaller lifetime than the Sink it's operating on (whereas the shared_ptr would blur this concept) and, more importantly b) our API always returns PageSinks/Sources as unique_ptr.

[hahnjo]

Should the RNTupleMerger maybe own the sink via a unique_ptr?

[silverweed]

Hm, but how would the user get back ownership if they need it when they are done with it? If we add a RetrieveSink() method (or whatever) we leave the Merger in an invalid state..

[hahnjo]

I don't think the user should ever get it back; as we discussed the merger will soon own the model, so I'm not sure the user could even do something useful with the sink except merging in more files

[silverweed]

Makes sense

[pcanal]

Using the unique_ptr solves the problem.

because it's clear that the merger should have a strictly smaller lifetime than the Sink

Yes for a use case where the user still need access to the object (and thus unique_ptr is not an option), capturing the reference is 'silently' sharing the ownership/usage of the object and will lead to user error. We are trying to move away from implicitness. i.e. should have a strictly smaller lifetime might be true but can not be enforced nor checked by the compiler (nor simple static analysis tools).

[silverweed]

@pcanal so we have an agreement on the unique_ptr right? Can this thread be resolved?

[github-actions]

Test Results

    17 files      17 suites   3d 21h 12m 48s ⏱️
 2 687 tests  2 664 ✅ 23 💤 0 ❌
44 030 runs  44 030 ✅  0 💤 0 ❌

Results for commit 2c80009.

♻️ This comment has been updated with latest results.