[ntuple] Allow changing compression of sources in RNTupleMerger #15992
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
silverweed
changed the title
Test Results 13 files 13 suites 2d 20h 13m 2s ⏱️ Results for commit cc6252e. ♻️ This comment has been updated with latest results. |
silverweed
force-pushed
the
ntuple_merge_change_compression
branch
2 times, most recently
from
1d467aa
to
46b4236
Compare
silverweed
force-pushed
the
ntuple_merge_change_compression
branch
from
5429241
to
3a1ca08
Compare
enirolf
requested changes
Jul 9, 2024
| /// If `fCompressionSettings == -1` (the default), the merger will not change the compression | ||
| /// of any of its sources (fast merging). Otherwise, all sources will be converted to the specified | ||
| /// compression algorithm and level. | ||
| int fCompressionSettings = -1; |
There was a problem hiding this comment.
Perhaps use kUnknownCompressionSettings here for consistency.
| /// The fSize and fNElements member of the sealedPage parameters are always set. If sealedPage.fBuffer is nullptr, | ||
| /// no data will be copied but the returned size information can be used by the caller to allocate a large enough | ||
| /// buffer and call LoadSealedPage again. | ||
| /// Read the packed and compressed bytes of a page into the memory buffer provided by `sealedPage`. The sealed page |
There was a problem hiding this comment.
Very very nitpicky but if we're adding backticks here, they should probably also be added to the other comments (at least in this file).
tree/ntuple/v7/src/RNTupleMerger.cxx
Outdated
| sealedPageGroups.reserve(sealedPageGroups.size() + pages.fPageInfos.size()); | ||
|
|
||
| std::uint64_t pageNo = 0; |
There was a problem hiding this comment.
I'd suggest using pageNumber or pageIdx. I don't think we use the ..No naming anywhere else.
tree/ntuple/v7/src/RNTupleMerger.cxx
Outdated
|
|
||
| const auto colRangeCompressionSettings = clusterDesc.GetColumnRange(columnId).fCompressionSettings; | ||
| const bool needsCompressionChange = | ||
| options.fCompressionSettings != -1 && colRangeCompressionSettings != options.fCompressionSettings; |
There was a problem hiding this comment.
Suggested change
| options.fCompressionSettings != -1 && colRangeCompressionSettings != options.fCompressionSettings; | |
| options.fCompressionSettings != kUnknownCompressionSettings && colRangeCompressionSettings != options.fCompressionSettings; |
enirolf
approved these changes
Jul 10, 2024
jblomer
reviewed
Jul 10, 2024
| @@ -16,6 +16,7 @@ | |||
| #ifndef ROOT7_RNTupleMerger | |||
| #define ROOT7_RNTupleMerger | |||
|
|
|||
| #include "Compression.h" | |||
There was a problem hiding this comment.
Move below the include block of <ROOT/RHeader.hxx>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This Pull request:
gives RNTupleMerger the capability of changing the source RNTuples' compression while doing the merging. This can also be used to change the compression of a single RNTuple.
Depends on #15954
Changes or fixes:
Checklist: