From d5cc049a14ae536faba82e660164d81d84fa5912 Mon Sep 17 00:00:00 2001 From: silverweed Date: Mon, 19 Oct 2026 11:02:26 +0200 Subject: [PATCH 1/4] [ntuple] Change RColElemBase::Generate arg to RColumnDescriptor The column type alone is insufficient to create a proper ColumnElement when the element type is one that carries additional info, such as Real32Trunc/Quant. When creating such types one must also set the BitsOnStorage and ValueRange appropriately. To prevent mistakes due to callers forgetting to do so, we force them to pass the complete column descriptor when generating a column element using the overload. --- tree/ntuple/inc/ROOT/RColumnElementBase.hxx | 14 +++++++------- tree/ntuple/src/RColumnElement.cxx | 19 +++++++++++++++---- tree/ntuple/src/RNTupleDescriptor.cxx | 2 +- tree/ntuple/src/RNTupleDescriptorFmt.cxx | 2 +- tree/ntuple/src/RNTupleMerger.cxx | 4 ++-- tree/ntuple/test/ntuple_types.cxx | 2 +- tree/ntupleutil/src/RNTupleExporter.cxx | 2 +- tree/ntupleutil/src/RNTupleInspector.cxx | 2 +- 8 files changed, 29 insertions(+), 18 deletions(-) diff --git a/tree/ntuple/inc/ROOT/RColumnElementBase.hxx b/tree/ntuple/inc/ROOT/RColumnElementBase.hxx index a0e9e1232e6fd..333fa9785b07a 100644 --- a/tree/ntuple/inc/ROOT/RColumnElementBase.hxx +++ b/tree/ntuple/inc/ROOT/RColumnElementBase.hxx @@ -83,9 +83,11 @@ public: RColumnElementBase &operator=(RColumnElementBase &&other) = default; virtual ~RColumnElementBase() = default; - /// If CppT == void, use the default C++ type for the given column type - template - static std::unique_ptr Generate(ROOT::ENTupleColumnType type); + template + static std::unique_ptr Generate(ENTupleColumnType onDiskType); + /// Use the default C++ type for the given column type + static std::unique_ptr Generate(const ROOT::RColumnDescriptor &type); + static const char *GetColumnTypeName(ROOT::ENTupleColumnType type); /// Most types have a fixed on-disk bit width. Some low-precision column types /// have a range of possible bit widths. Return the minimum and maximum allowed @@ -140,14 +142,12 @@ GenerateColumnElement(std::type_index inMemoryType, ROOT::ENTupleColumnType onDi std::unique_ptr GenerateColumnElement(const RColumnElementBase::RIdentifier &elementId); template -std::unique_ptr RColumnElementBase::Generate(ROOT::ENTupleColumnType onDiskType) +std::unique_ptr RColumnElementBase::Generate(ENTupleColumnType onDiskType) { + static_assert(!std::is_same_v, "Generate is invalid: use non-templated Generate instead."); return GenerateColumnElement(std::type_index(typeid(CppT)), onDiskType); } -template <> -std::unique_ptr RColumnElementBase::Generate(ROOT::ENTupleColumnType onDiskType); - struct RColumnFormat { ENTupleColumnType fType = ENTupleColumnType::kUnknown; // 0 means "use default". Only valid for fixed-bitwidth column types. diff --git a/tree/ntuple/src/RColumnElement.cxx b/tree/ntuple/src/RColumnElement.cxx index 835ce1efdb2be..d855b59e3276a 100644 --- a/tree/ntuple/src/RColumnElement.cxx +++ b/tree/ntuple/src/RColumnElement.cxx @@ -105,10 +105,11 @@ const char *ROOT::Internal::RColumnElementBase::GetColumnTypeName(ENTupleColumnT } } -template <> std::unique_ptr -ROOT::Internal::RColumnElementBase::Generate(ENTupleColumnType onDiskType) +ROOT::Internal::RColumnElementBase::Generate(const ROOT::RColumnDescriptor &onDisk) { + const auto onDiskType = onDisk.GetType(); + //clang-format off switch (onDiskType) { case ENTupleColumnType::kIndex64: return std::make_unique>(); @@ -139,8 +140,18 @@ ROOT::Internal::RColumnElementBase::Generate(ENTupleColumnType onDiskType) case ENTupleColumnType::kSplitUInt32: return std::make_unique>(); case ENTupleColumnType::kSplitInt16: return std::make_unique>(); case ENTupleColumnType::kSplitUInt16: return std::make_unique>(); - case ENTupleColumnType::kReal32Trunc: return std::make_unique>(); - case ENTupleColumnType::kReal32Quant: return std::make_unique>(); + case ENTupleColumnType::kReal32Trunc: { + auto elem = std::make_unique>(); + elem->SetBitsOnStorage(onDisk.GetBitsOnStorage()); + return elem; + } + case ENTupleColumnType::kReal32Quant: { + auto elem = std::make_unique>(); + elem->SetBitsOnStorage(onDisk.GetBitsOnStorage()); + if (auto range = onDisk.GetValueRange()) + elem->SetValueRange(range->fMin, range->fMax); + return elem; + } default: if (onDiskType == kTestFutureColumnType) return std::make_unique>(); diff --git a/tree/ntuple/src/RNTupleDescriptor.cxx b/tree/ntuple/src/RNTupleDescriptor.cxx index 90b1136b0ed34..f83a7e7d2defa 100644 --- a/tree/ntuple/src/RNTupleDescriptor.cxx +++ b/tree/ntuple/src/RNTupleDescriptor.cxx @@ -994,7 +994,7 @@ ROOT::Internal::RClusterDescriptorBuilder::AddExtendedColumnRanges(const RNTuple if (!columnRange.IsSuppressed()) { auto &pageRange = fCluster.fPageRanges[physicalId]; pageRange.fPhysicalColumnId = physicalId; - const auto element = ROOT::Internal::RColumnElementBase::Generate(c.GetType()); + const auto element = ROOT::Internal::RColumnElementBase::Generate(c); pageRange.ExtendToFitColumnRange(columnRange, *element, ROOT::Internal::RPage::kPageZeroSize); } } else if (!columnRange.IsSuppressed()) { diff --git a/tree/ntuple/src/RNTupleDescriptorFmt.cxx b/tree/ntuple/src/RNTupleDescriptorFmt.cxx index 8063a7e39622b..565f84595b49b 100644 --- a/tree/ntuple/src/RNTupleDescriptorFmt.cxx +++ b/tree/ntuple/src/RNTupleDescriptorFmt.cxx @@ -101,7 +101,7 @@ void ROOT::RNTupleDescriptor::PrintInfo(std::ostream &output) const // We generate the default memory representation for the given column type in order // to report the size _in memory_ of column elements - auto elementSize = ROOT::Internal::RColumnElementBase::Generate(column.second.GetType())->GetSize(); + auto elementSize = ROOT::Internal::RColumnElementBase::Generate(column.second)->GetSize(); ColumnInfo info; info.fPhysicalColumnId = column.second.GetPhysicalId(); diff --git a/tree/ntuple/src/RNTupleMerger.cxx b/tree/ntuple/src/RNTupleMerger.cxx index e666513c8dbf6..d042a1281ab7a 100644 --- a/tree/ntuple/src/RNTupleMerger.cxx +++ b/tree/ntuple/src/RNTupleMerger.cxx @@ -1184,7 +1184,7 @@ ROOT::RResult RNTupleMergeStrategyDefault::GenerateZeroPagesForColumns(siz structure == ROOT::ENTupleStructure::kPlain); const auto &columnDesc = fDstDescriptor.GetColumnDescriptor(column.fOutputId); - const auto colElement = RColumnElementBase::Generate(columnDesc.GetType()); + const auto colElement = RColumnElementBase::Generate(columnDesc); const auto nElements = nEntriesToGenerate * nRepetitions; const auto nBytesOnStorage = colElement->GetPackedSize(nElements); // TODO(gparolini): make this configurable @@ -1247,7 +1247,7 @@ ROOT::RResult RNTupleMergeStrategyDefault::MergeCommonColumns(ROOT::Intern const auto &columnDesc = fSrcDescriptor->GetColumnDescriptor(columnId); const auto srcColElement = column.fInMemoryType ? ROOT::Internal::GenerateColumnElement(*column.fInMemoryType, columnDesc.GetType()) - : RColumnElementBase::Generate(columnDesc.GetType()); + : RColumnElementBase::Generate(columnDesc); // Now get the pages for this column in this cluster const auto &pages = clusterDesc.GetPageRange(columnId); diff --git a/tree/ntuple/test/ntuple_types.cxx b/tree/ntuple/test/ntuple_types.cxx index 4df36424219cb..ec537feea7ba4 100644 --- a/tree/ntuple/test/ntuple_types.cxx +++ b/tree/ntuple/test/ntuple_types.cxx @@ -1728,7 +1728,7 @@ TEST(RNTuple, HalfPrecisionFloat) auto reader = RNTupleReader::Open("ntuple", fileGuard.GetPath()); - EXPECT_EQ(4, ROOT::Internal::RColumnElementBase::Generate(ROOT::ENTupleColumnType::kReal16)->GetSize()); + EXPECT_EQ(4, ROOT::Internal::RColumnElementBase::Generate(ROOT::ENTupleColumnType::kReal16)->GetSize()); const auto &desc = reader->GetDescriptor(); EXPECT_EQ(ROOT::ENTupleColumnType::kReal16, (*desc.GetColumnIterable(desc.FindFieldId("f1")).begin()).GetType()); diff --git a/tree/ntupleutil/src/RNTupleExporter.cxx b/tree/ntupleutil/src/RNTupleExporter.cxx index fdeab68633f90..dd82e812be73f 100644 --- a/tree/ntupleutil/src/RNTupleExporter.cxx +++ b/tree/ntupleutil/src/RNTupleExporter.cxx @@ -152,7 +152,7 @@ RNTupleExporter::ExportPages(ROOT::Internal::RPageSource &source, const RPagesOp auto columnId = colInfo.fColDesc->GetPhysicalId(); const auto &pages = clusterDesc.GetPageRange(columnId); const auto &colRange = clusterDesc.GetColumnRange(columnId); - auto colElement = ROOT::Internal::RColumnElementBase::Generate(colInfo.fColDesc->GetType()); + auto colElement = ROOT::Internal::RColumnElementBase::Generate(*colInfo.fColDesc); colElement->SetBitsOnStorage(colInfo.fColDesc->GetBitsOnStorage()); std::uint64_t pageIdx = 0; diff --git a/tree/ntupleutil/src/RNTupleInspector.cxx b/tree/ntupleutil/src/RNTupleInspector.cxx index 1b1ac6a66e71e..30f768e319b64 100644 --- a/tree/ntupleutil/src/RNTupleInspector.cxx +++ b/tree/ntupleutil/src/RNTupleInspector.cxx @@ -65,7 +65,7 @@ void ROOT::Experimental::RNTupleInspector::CollectColumnInfo() // We generate the default memory representation for the given column type in order // to report the size _in memory_ of column elements. - std::uint32_t elemSize = RColumnElementBase::Generate(colDesc.GetType())->GetSize(); + std::uint32_t elemSize = RColumnElementBase::Generate(colDesc)->GetSize(); std::uint64_t nElems = 0; std::vector compressedPageSizes{}; From 8745c46dac92d3861a8ea4cff65134f73f48b639 Mon Sep 17 00:00:00 2001 From: silverweed Date: Mon, 19 Oct 2026 11:31:22 +0200 Subject: [PATCH 2/4] [ntuple] clang-tidy: remove needless string conversion --- tree/ntuple/inc/ROOT/RColumnElementBase.hxx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tree/ntuple/inc/ROOT/RColumnElementBase.hxx b/tree/ntuple/inc/ROOT/RColumnElementBase.hxx index 333fa9785b07a..513f41aa06649 100644 --- a/tree/ntuple/inc/ROOT/RColumnElementBase.hxx +++ b/tree/ntuple/inc/ROOT/RColumnElementBase.hxx @@ -104,12 +104,12 @@ public: virtual void SetBitsOnStorage(std::size_t bitsOnStorage) { if (bitsOnStorage != fBitsOnStorage) - throw RException(R__FAIL(std::string("internal error: cannot change bit width of this column type"))); + throw RException(R__FAIL("internal error: cannot change bit width of this column type")); } virtual void SetValueRange(double, double) { - throw RException(R__FAIL(std::string("internal error: cannot change value range of this column type"))); + throw RException(R__FAIL("internal error: cannot change value range of this column type")); } /// If the on-storage layout and the in-memory layout differ, packing creates an on-disk page from an in-memory page From 8d4e5480784a926e5643c216edfe31cfe20217dd Mon Sep 17 00:00:00 2001 From: silverweed Date: Mon, 19 Oct 2026 11:47:41 +0200 Subject: [PATCH 3/4] [ntuple] Add overload to GenerateColumnElement that passes descriptor --- tree/ntuple/inc/ROOT/RColumnElementBase.hxx | 3 + tree/ntuple/src/RColumnElement.cxx | 87 +++++++++++---------- tree/ntuple/src/RNTupleMerger.cxx | 2 +- 3 files changed, 50 insertions(+), 42 deletions(-) diff --git a/tree/ntuple/inc/ROOT/RColumnElementBase.hxx b/tree/ntuple/inc/ROOT/RColumnElementBase.hxx index 513f41aa06649..c2453f396d858 100644 --- a/tree/ntuple/inc/ROOT/RColumnElementBase.hxx +++ b/tree/ntuple/inc/ROOT/RColumnElementBase.hxx @@ -139,6 +139,9 @@ struct RTestFutureColumn { std::unique_ptr GenerateColumnElement(std::type_index inMemoryType, ROOT::ENTupleColumnType onDiskType); +std::unique_ptr +GenerateColumnElement(std::type_index inMemoryType, const ROOT::RColumnDescriptor &onDisk); + std::unique_ptr GenerateColumnElement(const RColumnElementBase::RIdentifier &elementId); template diff --git a/tree/ntuple/src/RColumnElement.cxx b/tree/ntuple/src/RColumnElement.cxx index d855b59e3276a..c49bbb67ec30b 100644 --- a/tree/ntuple/src/RColumnElement.cxx +++ b/tree/ntuple/src/RColumnElement.cxx @@ -109,57 +109,52 @@ std::unique_ptr ROOT::Internal::RColumnElementBase::Generate(const ROOT::RColumnDescriptor &onDisk) { const auto onDiskType = onDisk.GetType(); + std::unique_ptr elem; //clang-format off switch (onDiskType) { - case ENTupleColumnType::kIndex64: return std::make_unique>(); - case ENTupleColumnType::kIndex32: return std::make_unique>(); - case ENTupleColumnType::kSwitch: return std::make_unique>(); - case ENTupleColumnType::kByte: return std::make_unique>(); - case ENTupleColumnType::kChar: return std::make_unique>(); - case ENTupleColumnType::kBit: return std::make_unique>(); - case ENTupleColumnType::kReal64: return std::make_unique>(); - case ENTupleColumnType::kReal32: return std::make_unique>(); + case ENTupleColumnType::kIndex64: elem = std::make_unique>(); break; + case ENTupleColumnType::kIndex32: elem = std::make_unique>(); break; + case ENTupleColumnType::kSwitch: elem = std::make_unique>(); break; + case ENTupleColumnType::kByte: elem = std::make_unique>(); break; + case ENTupleColumnType::kChar: elem = std::make_unique>(); break; + case ENTupleColumnType::kBit: elem = std::make_unique>(); break; + case ENTupleColumnType::kReal64: elem = std::make_unique>(); break; + case ENTupleColumnType::kReal32: elem = std::make_unique>(); break; // TODO: Change to std::float16_t in-memory type once available (from C++23). - case ENTupleColumnType::kReal16: return std::make_unique>(); - case ENTupleColumnType::kInt64: return std::make_unique>(); - case ENTupleColumnType::kUInt64: return std::make_unique>(); - case ENTupleColumnType::kInt32: return std::make_unique>(); - case ENTupleColumnType::kUInt32: return std::make_unique>(); - case ENTupleColumnType::kInt16: return std::make_unique>(); - case ENTupleColumnType::kUInt16: return std::make_unique>(); - case ENTupleColumnType::kInt8: return std::make_unique>(); - case ENTupleColumnType::kUInt8: return std::make_unique>(); - case ENTupleColumnType::kSplitIndex64: return std::make_unique>(); - case ENTupleColumnType::kSplitIndex32: return std::make_unique>(); - case ENTupleColumnType::kSplitReal64: return std::make_unique>(); - case ENTupleColumnType::kSplitReal32: return std::make_unique>(); - case ENTupleColumnType::kSplitInt64: return std::make_unique>(); - case ENTupleColumnType::kSplitUInt64: return std::make_unique>(); - case ENTupleColumnType::kSplitInt32: return std::make_unique>(); - case ENTupleColumnType::kSplitUInt32: return std::make_unique>(); - case ENTupleColumnType::kSplitInt16: return std::make_unique>(); - case ENTupleColumnType::kSplitUInt16: return std::make_unique>(); - case ENTupleColumnType::kReal32Trunc: { - auto elem = std::make_unique>(); - elem->SetBitsOnStorage(onDisk.GetBitsOnStorage()); - return elem; - } - case ENTupleColumnType::kReal32Quant: { - auto elem = std::make_unique>(); - elem->SetBitsOnStorage(onDisk.GetBitsOnStorage()); - if (auto range = onDisk.GetValueRange()) - elem->SetValueRange(range->fMin, range->fMax); - return elem; - } + case ENTupleColumnType::kReal16: elem = std::make_unique>(); break; + case ENTupleColumnType::kInt64: elem = std::make_unique>(); break; + case ENTupleColumnType::kUInt64: elem = std::make_unique>(); break; + case ENTupleColumnType::kInt32: elem = std::make_unique>(); break; + case ENTupleColumnType::kUInt32: elem = std::make_unique>(); break; + case ENTupleColumnType::kInt16: elem = std::make_unique>(); break; + case ENTupleColumnType::kUInt16: elem = std::make_unique>(); break; + case ENTupleColumnType::kInt8: elem = std::make_unique>(); break; + case ENTupleColumnType::kUInt8: elem = std::make_unique>(); break; + case ENTupleColumnType::kSplitIndex64: elem = std::make_unique>(); break; + case ENTupleColumnType::kSplitIndex32: elem = std::make_unique>(); break; + case ENTupleColumnType::kSplitReal64: elem = std::make_unique>(); break; + case ENTupleColumnType::kSplitReal32: elem = std::make_unique>(); break; + case ENTupleColumnType::kSplitInt64: elem = std::make_unique>(); break; + case ENTupleColumnType::kSplitUInt64: elem = std::make_unique>(); break; + case ENTupleColumnType::kSplitInt32: elem = std::make_unique>(); break; + case ENTupleColumnType::kSplitUInt32: elem = std::make_unique>(); break; + case ENTupleColumnType::kSplitInt16: elem = std::make_unique>(); break; + case ENTupleColumnType::kSplitUInt16: elem = std::make_unique>(); break; + case ENTupleColumnType::kReal32Trunc: elem = std::make_unique>(); break; + case ENTupleColumnType::kReal32Quant: elem = std::make_unique>(); break; default: if (onDiskType == kTestFutureColumnType) return std::make_unique>(); R__ASSERT(false); } //clang-format on - // never here - return nullptr; + + elem->SetBitsOnStorage(onDisk.GetBitsOnStorage()); + if (auto range = onDisk.GetValueRange()) + elem->SetValueRange(range->fMin, range->fMax); + + return elem; } std::unique_ptr @@ -204,6 +199,16 @@ ROOT::Internal::GenerateColumnElement(std::type_index inMemoryType, ENTupleColum return nullptr; } +std::unique_ptr +ROOT::Internal::GenerateColumnElement(std::type_index inMemoryType, const ROOT::RColumnDescriptor &onDisk) +{ + auto elem = GenerateColumnElement(inMemoryType, onDisk.GetType()); + elem->SetBitsOnStorage(onDisk.GetBitsOnStorage()); + if (auto range = onDisk.GetValueRange()) + elem->SetValueRange(range->fMin, range->fMax); + return elem; +} + std::unique_ptr ROOT::Internal::GenerateColumnElement(const RColumnElementBase::RIdentifier &elementId) { diff --git a/tree/ntuple/src/RNTupleMerger.cxx b/tree/ntuple/src/RNTupleMerger.cxx index d042a1281ab7a..689c611a045e0 100644 --- a/tree/ntuple/src/RNTupleMerger.cxx +++ b/tree/ntuple/src/RNTupleMerger.cxx @@ -1246,7 +1246,7 @@ ROOT::RResult RNTupleMergeStrategyDefault::MergeCommonColumns(ROOT::Intern const auto &columnDesc = fSrcDescriptor->GetColumnDescriptor(columnId); const auto srcColElement = column.fInMemoryType - ? ROOT::Internal::GenerateColumnElement(*column.fInMemoryType, columnDesc.GetType()) + ? ROOT::Internal::GenerateColumnElement(*column.fInMemoryType, columnDesc) : RColumnElementBase::Generate(columnDesc); // Now get the pages for this column in this cluster From cf6992046195631ad6e6f85d83259a760f7d29bf Mon Sep 17 00:00:00 2001 From: Elvand Lie Nababan Date: Sun, 18 Oct 2026 17:57:25 +0700 Subject: [PATCH 4/4] [ntuple] Use stored bit width when recompressing merged pages Apply the source column's stored bit width before computing packed page sizes during recompression. This allows truncated and quantized float pages to be merged with a different output compression. Compare both fields against their stored source values in a regression. Assisted-by: codex:gpt-6.1-sol Assisted-by: claude-code:opus --- tree/ntuple/test/ntuple_merger.cxx | 48 ++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/tree/ntuple/test/ntuple_merger.cxx b/tree/ntuple/test/ntuple_merger.cxx index 27287d804d165..05207197d02b1 100644 --- a/tree/ntuple/test/ntuple_merger.cxx +++ b/tree/ntuple/test/ntuple_merger.cxx @@ -938,6 +938,54 @@ TEST(RNTupleMerger, ChangeCompression) EXPECT_TRUE(VerifyPageCompression(fileGuardOutUncomp.GetPath(), 0)); } +TEST(RNTupleMerger, ChangeCompressionLowPrecision) +{ + FileRaii input("test_ntuple_merge_changecomp_lowprecision_in.root"); + FileRaii output("test_ntuple_merge_changecomp_lowprecision_out.root"); + { + auto model = RNTupleModel::Create(); + auto truncated = std::make_unique>("truncated"); + truncated->SetTruncated(20); + model->AddField(std::move(truncated)); + auto quantized = std::make_unique>("quantized"); + quantized->SetQuantized(20, {0., 20.}); + model->AddField(std::move(quantized)); + RNTupleWriteOptions writeOptions; + writeOptions.SetCompression(505); + auto writer = RNTupleWriter::Recreate(std::move(model), "ntuple", input.GetPath(), writeOptions); + auto truncatedValue = writer->GetModel().GetDefaultEntry().GetPtr("truncated"); + auto quantizedValue = writer->GetModel().GetDefaultEntry().GetPtr("quantized"); + for (unsigned int i = 0; i < 1000; ++i) { + *truncatedValue = *quantizedValue = (i % 100) / 7.f; + writer->Fill(); + } + } + + auto source = RPageSource::Create("ntuple", input.GetPath()); + std::vector sources{source.get()}; + RNTupleWriteOptions writeOptions; + writeOptions.SetCompression(101); + auto destination = std::make_unique("ntuple", output.GetPath(), writeOptions); + RNTupleMerger merger{std::move(destination)}; + RNTupleMergeOptions mergeOptions; + mergeOptions.fCompressionSettings = 101; + auto result = merger.Merge(sources, mergeOptions); + ASSERT_TRUE(bool(result)) << result.GetError()->GetReport(); + + auto original = RNTupleReader::Open("ntuple", input.GetPath()); + auto merged = RNTupleReader::Open("ntuple", output.GetPath()); + ASSERT_EQ(original->GetNEntries(), merged->GetNEntries()); + for (const auto *name : {"truncated", "quantized"}) { + SCOPED_TRACE(name); + auto originalValue = original->GetView(name); + auto mergedValue = merged->GetView(name); + for (auto i : original->GetEntryRange()) { + SCOPED_TRACE(i); + EXPECT_EQ(originalValue(i), mergedValue(i)); + } + } +} + TEST(RNTupleMerger, ChangeCompressionMixed) { FileRaii fileGuard("test_ntuple_merge_changecomp_mixed_in.root");