Skip to content

Commit 381db28

Browse files
silverweedsilverweed
committed
[ntuple] Make the last fields of RPageRange private
1 parent b9ad3b0 commit 381db28

16 files changed

+140
-130
lines changed

Diff for: ‎tree/ntuple/v7/inc/ROOT/RNTupleDescriptor.hxx

+22-13
Original file line numberDiff line numberDiff line change
@@ -311,17 +311,6 @@ public:
311311
// clang-format on
312312
class RPageRange {
313313
friend class Internal::RClusterDescriptorBuilder;
314-
/// Extend this RPageRange to fit the given RColumnRange, i.e. prepend as many synthetic RPageInfos as needed to
315-
/// cover the range in `columnRange`. `RPageInfo`s are constructed to contain as many elements of type `element`
316-
/// given a page size limit of `pageSize` (in bytes); the locator for the referenced pages is `kTypePageZero`.
317-
/// This function is used to make up `RPageRange`s for clusters that contain deferred columns.
318-
/// \return The number of column elements covered by the synthesized RPageInfos
319-
std::size_t ExtendToFitColumnRange(const RColumnRange &columnRange, const Internal::RColumnElementBase &element,
320-
std::size_t pageSize);
321-
322-
/// Has the same length than fPageInfos and stores the sum of the number of elements of all the pages
323-
/// up to and including a given index. Used for binary search in Find().
324-
std::vector<ROOT::NTupleSize_t> fCumulativeNElements;
325314

326315
public:
327316
/// We do not need to store the element size / uncompressed page size because we know to which column
@@ -380,6 +369,23 @@ public:
380369
void SetPageNumber(ROOT::NTupleSize_t pageNumber) { fPageNumber = pageNumber; }
381370
};
382371

372+
private:
373+
/// Extend this RPageRange to fit the given RColumnRange, i.e. prepend as many synthetic RPageInfos as needed to
374+
/// cover the range in `columnRange`. `RPageInfo`s are constructed to contain as many elements of type `element`
375+
/// given a page size limit of `pageSize` (in bytes); the locator for the referenced pages is `kTypePageZero`.
376+
/// This function is used to make up `RPageRange`s for clusters that contain deferred columns.
377+
/// \return The number of column elements covered by the synthesized RPageInfos
378+
std::size_t ExtendToFitColumnRange(const RColumnRange &columnRange, const Internal::RColumnElementBase &element,
379+
std::size_t pageSize);
380+
381+
/// Has the same length than fPageInfos and stores the sum of the number of elements of all the pages
382+
/// up to and including a given index. Used for binary search in Find().
383+
std::vector<ROOT::NTupleSize_t> fCumulativeNElements;
384+
385+
ROOT::DescriptorId_t fPhysicalColumnId = ROOT::kInvalidDescriptorId;
386+
std::vector<RPageInfo> fPageInfos;
387+
388+
public:
383389
RPageRange() = default;
384390
RPageRange(const RPageRange &other) = delete;
385391
RPageRange &operator=(const RPageRange &other) = delete;
@@ -398,8 +404,11 @@ public:
398404
/// Find the page in the RPageRange that contains the given element. The element must exist.
399405
RPageInfoExtended Find(ROOT::NTupleSize_t idxInCluster) const;
400406

401-
ROOT::DescriptorId_t fPhysicalColumnId = ROOT::kInvalidDescriptorId;
402-
std::vector<RPageInfo> fPageInfos;
407+
ROOT::DescriptorId_t GetPhysicalColumnId() const { return fPhysicalColumnId; }
408+
void SetPhysicalColumnId(ROOT::DescriptorId_t id) { fPhysicalColumnId = id; }
409+
410+
const std::vector<RPageInfo> &GetPageInfos() const { return fPageInfos; }
411+
std::vector<RPageInfo> &GetPageInfos() { return fPageInfos; }
403412

404413
bool operator==(const RPageRange &other) const
405414
{

Diff for: ‎tree/ntuple/v7/src/RNTupleDescriptor.cxx

+1-1
Original file line numberDiff line numberDiff line change
@@ -273,7 +273,7 @@ std::uint64_t ROOT::Experimental::RClusterDescriptor::GetNBytesOnStorage() const
273273
{
274274
std::uint64_t nbytes = 0;
275275
for (const auto &pr : fPageRanges) {
276-
for (const auto &pi : pr.second.fPageInfos) {
276+
for (const auto &pi : pr.second.GetPageInfos()) {
277277
nbytes += pi.GetLocator().GetNBytesOnStorage();
278278
}
279279
}

Diff for: ‎tree/ntuple/v7/src/RNTupleDescriptorFmt.cxx

+1-1
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,7 @@ void ROOT::Experimental::RNTupleDescriptor::PrintInfo(std::ostream &output) cons
126126
}
127127
const auto &pageRange = cluster.second.GetPageRange(column.second.GetPhysicalId());
128128
auto idx = cluster2Idx[cluster.first];
129-
for (const auto &page : pageRange.fPageInfos) {
129+
for (const auto &page : pageRange.GetPageInfos()) {
130130
nBytesOnStorage += page.GetLocator().GetNBytesOnStorage();
131131
nBytesInMemory += page.GetNElements() * elementSize;
132132
clusters[idx].fNBytesOnStorage += page.GetLocator().GetNBytesOnStorage();

Diff for: ‎tree/ntuple/v7/src/RNTupleMerger.cxx

+3-3
Original file line numberDiff line numberDiff line change
@@ -708,7 +708,7 @@ void RNTupleMerger::MergeCommonColumns(RClusterPool &clusterPool, const RCluster
708708
const auto &pages = clusterDesc.GetPageRange(columnId);
709709

710710
RPageStorage::SealedPageSequence_t sealedPages;
711-
sealedPages.resize(pages.fPageInfos.size());
711+
sealedPages.resize(pages.GetPageInfos().size());
712712

713713
// Each column range potentially has a distinct compression settings
714714
const auto colRangeCompressionSettings = clusterDesc.GetColumnRange(columnId).GetCompressionSettings().value();
@@ -730,7 +730,7 @@ void RNTupleMerger::MergeCommonColumns(RClusterPool &clusterPool, const RCluster
730730
// If the column range already has the right compression we don't need to allocate any new buffer, so we don't
731731
// bother reserving memory for them.
732732
if (needsResealing)
733-
sealedPageData.fBuffers.resize(sealedPageData.fBuffers.size() + pages.fPageInfos.size());
733+
sealedPageData.fBuffers.resize(sealedPageData.fBuffers.size() + pages.GetPageInfos().size());
734734

735735
// If this column is deferred, we may need to fill "holes" until its real start. We fill any missing entry
736736
// with zeroes, like we do for extraDstColumns.
@@ -745,7 +745,7 @@ void RNTupleMerger::MergeCommonColumns(RClusterPool &clusterPool, const RCluster
745745

746746
// Loop over the pages
747747
std::uint64_t pageIdx = 0;
748-
for (const auto &pageInfo : pages.fPageInfos) {
748+
for (const auto &pageInfo : pages.GetPageInfos()) {
749749
assert(pageIdx < sealedPages.size());
750750
assert(sealedPageData.fBuffers.size() == 0 || pageIdx < sealedPageData.fBuffers.size());
751751
assert(pageInfo.GetLocator().GetType() != RNTupleLocator::kTypePageZero);

Diff for: ‎tree/ntuple/v7/src/RNTupleSerialize.cxx

+4-4
Original file line numberDiff line numberDiff line change
@@ -1716,9 +1716,9 @@ ROOT::Experimental::Internal::RNTupleSerializer::SerializePageList(void *buffer,
17161716
pos += SerializeInt64(kSuppressedColumnMarker, *where);
17171717
} else {
17181718
const auto &pageRange = clusterDesc.GetPageRange(memId);
1719-
pos += SerializeListFramePreamble(pageRange.fPageInfos.size(), *where);
1719+
pos += SerializeListFramePreamble(pageRange.GetPageInfos().size(), *where);
17201720

1721-
for (const auto &pi : pageRange.fPageInfos) {
1721+
for (const auto &pi : pageRange.GetPageInfos()) {
17221722
std::int32_t nElements =
17231723
pi.HasChecksum() ? -static_cast<std::int32_t>(pi.GetNElements()) : pi.GetNElements();
17241724
pos += SerializeUInt32(nElements, *where);
@@ -2058,7 +2058,7 @@ ROOT::Experimental::Internal::RNTupleSerializer::DeserializePageListRaw(const vo
20582058
}
20592059

20602060
RClusterDescriptor::RPageRange pageRange;
2061-
pageRange.fPhysicalColumnId = j;
2061+
pageRange.SetPhysicalColumnId(j);
20622062
for (std::uint32_t k = 0; k < nPages; ++k) {
20632063
if (fnInnerFrameSizeLeft() < static_cast<int>(sizeof(std::uint32_t)))
20642064
return R__FAIL("inner frame too short");
@@ -2075,7 +2075,7 @@ ROOT::Experimental::Internal::RNTupleSerializer::DeserializePageListRaw(const vo
20752075
} else {
20762076
return R__FORWARD_ERROR(res);
20772077
}
2078-
pageRange.fPageInfos.push_back({static_cast<std::uint32_t>(nElements), locator, hasChecksum});
2078+
pageRange.GetPageInfos().push_back({static_cast<std::uint32_t>(nElements), locator, hasChecksum});
20792079
}
20802080

20812081
if (fnInnerFrameSizeLeft() < static_cast<int>(sizeof(std::int64_t)))

Diff for: ‎tree/ntuple/v7/src/RPageStorage.cxx

+13-13
Original file line numberDiff line numberDiff line change
@@ -267,7 +267,7 @@ void ROOT::Experimental::Internal::RPageSource::UnzipClusterImpl(RCluster *clust
267267
const auto &pageRange = clusterDescriptor.GetPageRange(columnId);
268268
std::uint64_t pageNo = 0;
269269
std::uint64_t firstInPage = 0;
270-
for (const auto &pi : pageRange.fPageInfos) {
270+
for (const auto &pi : pageRange.GetPageInfos()) {
271271
auto onDiskPage = cluster->GetOnDiskPage(ROnDiskPage::Key{columnId, pageNo});
272272
RSealedPage sealedPage;
273273
sealedPage.SetNElements(pi.GetNElements());
@@ -323,7 +323,7 @@ void ROOT::Experimental::Internal::RPageSource::PrepareLoadCluster(
323323

324324
const auto &pageRange = clusterDesc.GetPageRange(physicalColumnId);
325325
ROOT::NTupleSize_t pageNo = 0;
326-
for (const auto &pageInfo : pageRange.fPageInfos) {
326+
for (const auto &pageInfo : pageRange.GetPageInfos()) {
327327
if (pageInfo.GetLocator().GetType() == RNTupleLocator::kTypePageZero) {
328328
pageZeroMap.Register(
329329
ROnDiskPage::Key{physicalColumnId, pageNo},
@@ -879,7 +879,7 @@ void ROOT::Experimental::Internal::RPagePersistentSink::UpdateSchema(const RNTup
879879
columnRange.SetCompressionSettings(GetWriteOptions().GetCompression());
880880
fOpenColumnRanges.emplace_back(columnRange);
881881
RClusterDescriptor::RPageRange pageRange;
882-
pageRange.fPhysicalColumnId = i;
882+
pageRange.SetPhysicalColumnId(i);
883883
fOpenPageRanges.emplace_back(std::move(pageRange));
884884
}
885885

@@ -948,7 +948,7 @@ ROOT::Experimental::Internal::RPagePersistentSink::InitFromDescriptor(const RNTu
948948
columnRange.SetCompressionSettings(GetWriteOptions().GetCompression());
949949
fOpenColumnRanges.emplace_back(columnRange);
950950
RClusterDescriptor::RPageRange pageRange;
951-
pageRange.fPhysicalColumnId = i;
951+
pageRange.SetPhysicalColumnId(i);
952952
fOpenPageRanges.emplace_back(std::move(pageRange));
953953
}
954954

@@ -1010,7 +1010,7 @@ void ROOT::Experimental::Internal::RPagePersistentSink::CommitPage(ColumnHandle_
10101010
pageInfo.SetNElements(page.GetNElements());
10111011
pageInfo.SetLocator(CommitPageImpl(columnHandle, page));
10121012
pageInfo.SetHasChecksum(GetWriteOptions().GetEnablePageChecksums());
1013-
fOpenPageRanges.at(columnHandle.fPhysicalId).fPageInfos.emplace_back(pageInfo);
1013+
fOpenPageRanges.at(columnHandle.fPhysicalId).GetPageInfos().emplace_back(pageInfo);
10141014
}
10151015

10161016
void ROOT::Experimental::Internal::RPagePersistentSink::CommitSealedPage(ROOT::DescriptorId_t physicalColumnId,
@@ -1022,7 +1022,7 @@ void ROOT::Experimental::Internal::RPagePersistentSink::CommitSealedPage(ROOT::D
10221022
pageInfo.SetNElements(sealedPage.GetNElements());
10231023
pageInfo.SetLocator(CommitSealedPageImpl(physicalColumnId, sealedPage));
10241024
pageInfo.SetHasChecksum(sealedPage.GetHasChecksum());
1025-
fOpenPageRanges.at(physicalColumnId).fPageInfos.emplace_back(pageInfo);
1025+
fOpenPageRanges.at(physicalColumnId).GetPageInfos().emplace_back(pageInfo);
10261026
}
10271027

10281028
std::vector<ROOT::RNTupleLocator> ROOT::Experimental::Internal::RPagePersistentSink::CommitSealedPageVImpl(
@@ -1106,7 +1106,7 @@ void ROOT::Experimental::Internal::RPagePersistentSink::CommitSealedPageV(
11061106
pageInfo.SetNElements(sealedPageIt->GetNElements());
11071107
pageInfo.SetLocator(locators[locatorIndexes[i++]]);
11081108
pageInfo.SetHasChecksum(sealedPageIt->GetHasChecksum());
1109-
fOpenPageRanges.at(range.fPhysicalColumnId).fPageInfos.emplace_back(pageInfo);
1109+
fOpenPageRanges.at(range.fPhysicalColumnId).GetPageInfos().emplace_back(pageInfo);
11101110
}
11111111
}
11121112
}
@@ -1122,15 +1122,15 @@ ROOT::Experimental::Internal::RPagePersistentSink::StageCluster(ROOT::NTupleSize
11221122
RStagedCluster::RColumnInfo columnInfo;
11231123
columnInfo.fCompressionSettings = fOpenColumnRanges[i].GetCompressionSettings().value();
11241124
if (fOpenColumnRanges[i].IsSuppressed()) {
1125-
assert(fOpenPageRanges[i].fPageInfos.empty());
1126-
columnInfo.fPageRange.fPhysicalColumnId = i;
1125+
assert(fOpenPageRanges[i].GetPageInfos().empty());
1126+
columnInfo.fPageRange.SetPhysicalColumnId(i);
11271127
columnInfo.fIsSuppressed = true;
11281128
// We reset suppressed columns to the state they would have if they were active (not suppressed).
11291129
fOpenColumnRanges[i].SetNElements(0);
11301130
fOpenColumnRanges[i].SetIsSuppressed(false);
11311131
} else {
11321132
std::swap(columnInfo.fPageRange, fOpenPageRanges[i]);
1133-
fOpenPageRanges[i].fPhysicalColumnId = i;
1133+
fOpenPageRanges[i].SetPhysicalColumnId(i);
11341134

11351135
columnInfo.fNElements = fOpenColumnRanges[i].GetNElements();
11361136
fOpenColumnRanges[i].SetNElements(0);
@@ -1149,9 +1149,9 @@ void ROOT::Experimental::Internal::RPagePersistentSink::CommitStagedClusters(std
11491149
.FirstEntryIndex(fPrevClusterNEntries)
11501150
.NEntries(cluster.fNEntries);
11511151
for (const auto &columnInfo : cluster.fColumnInfos) {
1152-
const auto colId = columnInfo.fPageRange.fPhysicalColumnId;
1152+
const auto colId = columnInfo.fPageRange.GetPhysicalColumnId();
11531153
if (columnInfo.fIsSuppressed) {
1154-
assert(columnInfo.fPageRange.fPageInfos.empty());
1154+
assert(columnInfo.fPageRange.GetPageInfos().empty());
11551155
clusterBuilder.MarkSuppressedColumnRange(colId);
11561156
} else {
11571157
clusterBuilder.CommitColumnRange(colId, fOpenColumnRanges[colId].GetFirstElementIndex(),
@@ -1164,7 +1164,7 @@ void ROOT::Experimental::Internal::RPagePersistentSink::CommitStagedClusters(std
11641164
for (const auto &columnInfo : cluster.fColumnInfos) {
11651165
if (!columnInfo.fIsSuppressed)
11661166
continue;
1167-
const auto colId = columnInfo.fPageRange.fPhysicalColumnId;
1167+
const auto colId = columnInfo.fPageRange.GetPhysicalColumnId();
11681168
// For suppressed columns, we need to reset the first element index to the first element of the next (upcoming)
11691169
// cluster. This information has been determined for the committed cluster descriptor through
11701170
// CommitSuppressedColumnRanges(), so we can use the information from the descriptor.

Diff for: ‎tree/ntuple/v7/test/ntuple_basics.cxx

+1-1
Original file line numberDiff line numberDiff line change
@@ -429,7 +429,7 @@ TEST(RNTuple, PageSize)
429429
auto ntuple = RNTupleReader::Open("ntuple", fileGuard.GetPath());
430430
const auto &col0_pages = ntuple->GetDescriptor().GetClusterDescriptor(0).GetPageRange(0);
431431
// 1000 column elements / 50 elements per page
432-
EXPECT_EQ(20, col0_pages.fPageInfos.size());
432+
EXPECT_EQ(20, col0_pages.GetPageInfos().size());
433433
}
434434

435435
TEST(RNTupleWriteOptions, SamePageMergingChecksum)

Diff for: ‎tree/ntuple/v7/test/ntuple_checksum.cxx

+1-1
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,7 @@ TEST(RNTupleChecksum, OmitPageChecksum)
103103
const auto pxColId = descGuard->FindPhysicalColumnId(descGuard->FindFieldId("px"), 0, 0);
104104
const auto clusterId = descGuard->FindClusterId(pxColId, 0);
105105
const auto &clusterDesc = descGuard->GetClusterDescriptor(clusterId);
106-
const auto pageInfo = clusterDesc.GetPageRange(pxColId).fPageInfos[0];
106+
const auto pageInfo = clusterDesc.GetPageRange(pxColId).GetPageInfos()[0];
107107
EXPECT_EQ(4u, pageInfo.GetLocator().GetNBytesOnStorage());
108108
EXPECT_FALSE(pageInfo.HasChecksum());
109109

Diff for: ‎tree/ntuple/v7/test/ntuple_limits.cxx

+7-6
Original file line numberDiff line numberDiff line change
@@ -155,7 +155,7 @@ TEST(RNTuple, Limits_ManyPages)
155155

156156
EXPECT_EQ(reader->GetNEntries(), NumEntries);
157157
EXPECT_EQ(descriptor.GetNClusters(), 1);
158-
EXPECT_EQ(descriptor.GetClusterDescriptor(0).GetPageRange(columnId).fPageInfos.size(), NumPages);
158+
EXPECT_EQ(descriptor.GetClusterDescriptor(0).GetPageRange(columnId).GetPageInfos().size(), NumPages);
159159

160160
auto id = model.GetDefaultEntry().GetPtr<int>("id");
161161
for (int i = 0; i < NumEntries; i++) {
@@ -198,7 +198,7 @@ TEST(RNTuple, Limits_ManyPagesOneEntry)
198198

199199
EXPECT_EQ(reader->GetNEntries(), 1);
200200
EXPECT_EQ(descriptor.GetNClusters(), 1);
201-
EXPECT_EQ(descriptor.GetClusterDescriptor(0).GetPageRange(columnId).fPageInfos.size(), NumPages);
201+
EXPECT_EQ(descriptor.GetClusterDescriptor(0).GetPageRange(columnId).GetPageInfos().size(), NumPages);
202202

203203
auto ids = model.GetDefaultEntry().GetPtr<std::vector<int>>("ids");
204204
reader->LoadEntry(0);
@@ -246,9 +246,10 @@ TEST(RNTuple, DISABLED_Limits_LargePage)
246246

247247
EXPECT_EQ(reader->GetNEntries(), NumElements);
248248
EXPECT_EQ(descriptor.GetNClusters(), 1);
249-
EXPECT_EQ(descriptor.GetClusterDescriptor(0).GetPageRange(columnId).fPageInfos.size(), 1);
250-
EXPECT_GT(descriptor.GetClusterDescriptor(0).GetPageRange(columnId).fPageInfos[0].GetLocator().GetNBytesOnStorage(),
251-
static_cast<std::uint64_t>(std::numeric_limits<std::uint32_t>::max()));
249+
EXPECT_EQ(descriptor.GetClusterDescriptor(0).GetPageRange(columnId).GetPageInfos().size(), 1);
250+
EXPECT_GT(
251+
descriptor.GetClusterDescriptor(0).GetPageRange(columnId).GetPageInfos()[0].GetLocator().GetNBytesOnStorage(),
252+
static_cast<std::uint64_t>(std::numeric_limits<std::uint32_t>::max()));
252253

253254
auto id = model.GetDefaultEntry().GetPtr<std::uint64_t>("id");
254255
for (int i = 0; i < NumElements; i++) {
@@ -291,7 +292,7 @@ TEST(RNTuple, DISABLED_Limits_LargePageOneEntry)
291292

292293
EXPECT_EQ(reader->GetNEntries(), 1);
293294
EXPECT_EQ(descriptor.GetNClusters(), 1);
294-
EXPECT_EQ(descriptor.GetClusterDescriptor(0).GetPageRange(columnId).fPageInfos.size(), 1);
295+
EXPECT_EQ(descriptor.GetClusterDescriptor(0).GetPageRange(columnId).GetPageInfos().size(), 1);
295296

296297
auto ids = model.GetDefaultEntry().GetPtr<std::vector<int>>("ids");
297298
reader->LoadEntry(0);

Diff for: ‎tree/ntuple/v7/test/ntuple_merger.cxx

+2-2
Original file line numberDiff line numberDiff line change
@@ -76,9 +76,9 @@ TEST(RPageStorage, ReadSealedPages)
7676
ASSERT_NE(clusterId, index.GetClusterId());
7777
const auto clusterDesc = source.GetSharedDescriptorGuard()->GetClusterDescriptor(clusterId).Clone();
7878
const auto &pageRange = clusterDesc.GetPageRange(columnId);
79-
EXPECT_GT(pageRange.fPageInfos.size(), 1U);
79+
EXPECT_GT(pageRange.GetPageInfos().size(), 1U);
8080
std::uint32_t firstElementInPage = 0;
81-
for (const auto &pi : pageRange.fPageInfos) {
81+
for (const auto &pi : pageRange.GetPageInfos()) {
8282
sealedPage.SetBuffer(nullptr);
8383
source.LoadSealedPage(columnId, RNTupleLocalIndex(clusterId, firstElementInPage), sealedPage);
8484
buffer = MakeUninitArray<unsigned char>(sealedPage.GetBufferSize());

Diff for: ‎tree/ntuple/v7/test/ntuple_parallel_writer.cxx

+1-1
Original file line numberDiff line numberDiff line change
@@ -331,7 +331,7 @@ TEST(RNTupleFillContext, FlushColumns)
331331

332332
auto fieldId = descriptor.FindFieldId("pt");
333333
auto columnId = descriptor.FindPhysicalColumnId(fieldId, 0, 0);
334-
auto &pageInfos = descriptor.GetClusterDescriptor(0).GetPageRange(columnId).fPageInfos;
334+
auto &pageInfos = descriptor.GetClusterDescriptor(0).GetPageRange(columnId).GetPageInfos();
335335
ASSERT_EQ(pageInfos.size(), 2);
336336
EXPECT_EQ(pageInfos[0].GetNElements(), 1);
337337
EXPECT_EQ(pageInfos[1].GetNElements(), 1);

0 commit comments

Comments
 (0)