[ntuple] Properly set bit width/value range when generating column elements from on disk info

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 <typename CppT = void>
-   static std::unique_ptr<RColumnElementBase> Generate(ROOT::ENTupleColumnType type);
+   template <typename CppT>
+   static std::unique_ptr<RColumnElementBase> Generate(ENTupleColumnType onDiskType);
+   /// Use the default C++ type for the given column type
+   static std::unique_ptr<RColumnElementBase> 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
@@ -102,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
@@ -137,17 +139,18 @@ struct RTestFutureColumn {
 std::unique_ptr<RColumnElementBase>
 GenerateColumnElement(std::type_index inMemoryType, ROOT::ENTupleColumnType onDiskType);
 
+std::unique_ptr<RColumnElementBase>
+GenerateColumnElement(std::type_index inMemoryType, const ROOT::RColumnDescriptor &onDisk);
+
 std::unique_ptr<RColumnElementBase> GenerateColumnElement(const RColumnElementBase::RIdentifier &elementId);
 
 template <typename CppT>
-std::unique_ptr<RColumnElementBase> RColumnElementBase::Generate(ROOT::ENTupleColumnType onDiskType)
+std::unique_ptr<RColumnElementBase> RColumnElementBase::Generate(ENTupleColumnType onDiskType)
 {
+   static_assert(!std::is_same_v<CppT, void>, "Generate<void> is invalid: use non-templated Generate instead.");
    return GenerateColumnElement(std::type_index(typeid(CppT)), onDiskType);
 }
 
-template <>
-std::unique_ptr<RColumnElementBase> RColumnElementBase::Generate<void>(ROOT::ENTupleColumnType onDiskType);
-
 struct RColumnFormat {
    ENTupleColumnType fType = ENTupleColumnType::kUnknown;
    // 0 means "use default". Only valid for fixed-bitwidth column types.

tree/ntuple/src/RColumnElement.cxx

@@ -105,50 +105,56 @@ const char *ROOT::Internal::RColumnElementBase::GetColumnTypeName(ENTupleColumnT
    }
 }
 
-template <>
 std::unique_ptr<ROOT::Internal::RColumnElementBase>
-ROOT::Internal::RColumnElementBase::Generate<void>(ENTupleColumnType onDiskType)
+ROOT::Internal::RColumnElementBase::Generate(const ROOT::RColumnDescriptor &onDisk)
 {
+   const auto onDiskType = onDisk.GetType();
+   std::unique_ptr<ROOT::Internal::RColumnElementBase> elem;
+
    //clang-format off
    switch (onDiskType) {
-   case ENTupleColumnType::kIndex64: return std::make_unique<RColumnElement<RColumnIndex, ENTupleColumnType::kIndex64>>();
-   case ENTupleColumnType::kIndex32: return std::make_unique<RColumnElement<RColumnIndex, ENTupleColumnType::kIndex32>>();
-   case ENTupleColumnType::kSwitch: return std::make_unique<RColumnElement<RColumnSwitch, ENTupleColumnType::kSwitch>>();
-   case ENTupleColumnType::kByte: return std::make_unique<RColumnElement<std::byte, ENTupleColumnType::kByte>>();
-   case ENTupleColumnType::kChar: return std::make_unique<RColumnElement<char, ENTupleColumnType::kChar>>();
-   case ENTupleColumnType::kBit: return std::make_unique<RColumnElement<bool, ENTupleColumnType::kBit>>();
-   case ENTupleColumnType::kReal64: return std::make_unique<RColumnElement<double, ENTupleColumnType::kReal64>>();
-   case ENTupleColumnType::kReal32: return std::make_unique<RColumnElement<float, ENTupleColumnType::kReal32>>();
+   case ENTupleColumnType::kIndex64: elem = std::make_unique<RColumnElement<RColumnIndex, ENTupleColumnType::kIndex64>>(); break;
+   case ENTupleColumnType::kIndex32: elem = std::make_unique<RColumnElement<RColumnIndex, ENTupleColumnType::kIndex32>>(); break;
+   case ENTupleColumnType::kSwitch: elem = std::make_unique<RColumnElement<RColumnSwitch, ENTupleColumnType::kSwitch>>(); break;
+   case ENTupleColumnType::kByte: elem = std::make_unique<RColumnElement<std::byte, ENTupleColumnType::kByte>>(); break;
+   case ENTupleColumnType::kChar: elem = std::make_unique<RColumnElement<char, ENTupleColumnType::kChar>>(); break;
+   case ENTupleColumnType::kBit: elem = std::make_unique<RColumnElement<bool, ENTupleColumnType::kBit>>(); break;
+   case ENTupleColumnType::kReal64: elem = std::make_unique<RColumnElement<double, ENTupleColumnType::kReal64>>(); break;
+   case ENTupleColumnType::kReal32: elem = std::make_unique<RColumnElement<float, ENTupleColumnType::kReal32>>(); break;
    // TODO: Change to std::float16_t in-memory type once available (from C++23).
-   case ENTupleColumnType::kReal16: return std::make_unique<RColumnElement<float, ENTupleColumnType::kReal16>>();
-   case ENTupleColumnType::kInt64: return std::make_unique<RColumnElement<std::int64_t, ENTupleColumnType::kInt64>>();
-   case ENTupleColumnType::kUInt64: return std::make_unique<RColumnElement<std::uint64_t, ENTupleColumnType::kUInt64>>();
-   case ENTupleColumnType::kInt32: return std::make_unique<RColumnElement<std::int32_t, ENTupleColumnType::kInt32>>();
-   case ENTupleColumnType::kUInt32: return std::make_unique<RColumnElement<std::uint32_t, ENTupleColumnType::kUInt32>>();
-   case ENTupleColumnType::kInt16: return std::make_unique<RColumnElement<std::int16_t, ENTupleColumnType::kInt16>>();
-   case ENTupleColumnType::kUInt16: return std::make_unique<RColumnElement<std::uint16_t, ENTupleColumnType::kUInt16>>();
-   case ENTupleColumnType::kInt8: return std::make_unique<RColumnElement<std::int8_t, ENTupleColumnType::kInt8>>();
-   case ENTupleColumnType::kUInt8: return std::make_unique<RColumnElement<std::uint8_t, ENTupleColumnType::kUInt8>>();
-   case ENTupleColumnType::kSplitIndex64: return std::make_unique<RColumnElement<RColumnIndex, ENTupleColumnType::kSplitIndex64>>();
-   case ENTupleColumnType::kSplitIndex32: return std::make_unique<RColumnElement<RColumnIndex, ENTupleColumnType::kSplitIndex32>>();
-   case ENTupleColumnType::kSplitReal64: return std::make_unique<RColumnElement<double, ENTupleColumnType::kSplitReal64>>();
-   case ENTupleColumnType::kSplitReal32: return std::make_unique<RColumnElement<float, ENTupleColumnType::kSplitReal32>>();
-   case ENTupleColumnType::kSplitInt64: return std::make_unique<RColumnElement<std::int64_t, ENTupleColumnType::kSplitInt64>>();
-   case ENTupleColumnType::kSplitUInt64: return std::make_unique<RColumnElement<std::uint64_t, ENTupleColumnType::kSplitUInt64>>();
-   case ENTupleColumnType::kSplitInt32: return std::make_unique<RColumnElement<std::int32_t, ENTupleColumnType::kSplitInt32>>();
-   case ENTupleColumnType::kSplitUInt32: return std::make_unique<RColumnElement<std::uint32_t, ENTupleColumnType::kSplitUInt32>>();
-   case ENTupleColumnType::kSplitInt16: return std::make_unique<RColumnElement<std::int16_t, ENTupleColumnType::kSplitInt16>>();
-   case ENTupleColumnType::kSplitUInt16: return std::make_unique<RColumnElement<std::uint16_t, ENTupleColumnType::kSplitUInt16>>();
-   case ENTupleColumnType::kReal32Trunc: return std::make_unique<RColumnElement<float, ENTupleColumnType::kReal32Trunc>>();
-   case ENTupleColumnType::kReal32Quant: return std::make_unique<RColumnElement<float, ENTupleColumnType::kReal32Quant>>();
+   case ENTupleColumnType::kReal16: elem = std::make_unique<RColumnElement<float, ENTupleColumnType::kReal16>>(); break;
+   case ENTupleColumnType::kInt64: elem = std::make_unique<RColumnElement<std::int64_t, ENTupleColumnType::kInt64>>(); break;
+   case ENTupleColumnType::kUInt64: elem = std::make_unique<RColumnElement<std::uint64_t, ENTupleColumnType::kUInt64>>(); break;
+   case ENTupleColumnType::kInt32: elem = std::make_unique<RColumnElement<std::int32_t, ENTupleColumnType::kInt32>>(); break;
+   case ENTupleColumnType::kUInt32: elem = std::make_unique<RColumnElement<std::uint32_t, ENTupleColumnType::kUInt32>>(); break;
+   case ENTupleColumnType::kInt16: elem = std::make_unique<RColumnElement<std::int16_t, ENTupleColumnType::kInt16>>(); break;
+   case ENTupleColumnType::kUInt16: elem = std::make_unique<RColumnElement<std::uint16_t, ENTupleColumnType::kUInt16>>(); break;
+   case ENTupleColumnType::kInt8: elem = std::make_unique<RColumnElement<std::int8_t, ENTupleColumnType::kInt8>>(); break;
+   case ENTupleColumnType::kUInt8: elem = std::make_unique<RColumnElement<std::uint8_t, ENTupleColumnType::kUInt8>>(); break;
+   case ENTupleColumnType::kSplitIndex64: elem = std::make_unique<RColumnElement<RColumnIndex, ENTupleColumnType::kSplitIndex64>>(); break;
+   case ENTupleColumnType::kSplitIndex32: elem = std::make_unique<RColumnElement<RColumnIndex, ENTupleColumnType::kSplitIndex32>>(); break;
+   case ENTupleColumnType::kSplitReal64: elem = std::make_unique<RColumnElement<double, ENTupleColumnType::kSplitReal64>>(); break;
+   case ENTupleColumnType::kSplitReal32: elem = std::make_unique<RColumnElement<float, ENTupleColumnType::kSplitReal32>>(); break;
+   case ENTupleColumnType::kSplitInt64: elem = std::make_unique<RColumnElement<std::int64_t, ENTupleColumnType::kSplitInt64>>(); break;
+   case ENTupleColumnType::kSplitUInt64: elem = std::make_unique<RColumnElement<std::uint64_t, ENTupleColumnType::kSplitUInt64>>(); break;
+   case ENTupleColumnType::kSplitInt32: elem = std::make_unique<RColumnElement<std::int32_t, ENTupleColumnType::kSplitInt32>>(); break;
+   case ENTupleColumnType::kSplitUInt32: elem = std::make_unique<RColumnElement<std::uint32_t, ENTupleColumnType::kSplitUInt32>>(); break;
+   case ENTupleColumnType::kSplitInt16: elem = std::make_unique<RColumnElement<std::int16_t, ENTupleColumnType::kSplitInt16>>(); break;
+   case ENTupleColumnType::kSplitUInt16: elem = std::make_unique<RColumnElement<std::uint16_t, ENTupleColumnType::kSplitUInt16>>(); break;
+   case ENTupleColumnType::kReal32Trunc: elem = std::make_unique<RColumnElement<float, ENTupleColumnType::kReal32Trunc>>(); break;
+   case ENTupleColumnType::kReal32Quant: elem = std::make_unique<RColumnElement<float, ENTupleColumnType::kReal32Quant>>(); break;
    default:
       if (onDiskType == kTestFutureColumnType)
          return std::make_unique<RColumnElement<Internal::RTestFutureColumn, kTestFutureColumnType>>();
       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<ROOT::Internal::RColumnElementBase>
@@ -193,6 +199,16 @@ ROOT::Internal::GenerateColumnElement(std::type_index inMemoryType, ENTupleColum
    return nullptr;
 }
 
+std::unique_ptr<ROOT::Internal::RColumnElementBase>
+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::RColumnElementBase>
 ROOT::Internal::GenerateColumnElement(const RColumnElementBase::RIdentifier &elementId)
 {

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<void>(c.GetType());
+                     const auto element = ROOT::Internal::RColumnElementBase::Generate(c);
                      pageRange.ExtendToFitColumnRange(columnRange, *element, ROOT::Internal::RPage::kPageZeroSize);
                   }
                } else if (!columnRange.IsSuppressed()) {

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();

tree/ntuple/src/RNTupleMerger.cxx

@@ -1184,7 +1184,7 @@ ROOT::RResult<void> 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
@@ -1246,8 +1246,8 @@ ROOT::RResult<void> 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());
+                                    ? ROOT::Internal::GenerateColumnElement(*column.fInMemoryType, columnDesc)
+                                    : RColumnElementBase::Generate(columnDesc);
 
       // Now get the pages for this column in this cluster
       const auto &pages = clusterDesc.GetPageRange(columnId);

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<RField<float>>("truncated");
+      truncated->SetTruncated(20);
+      model->AddField(std::move(truncated));
+      auto quantized = std::make_unique<RField<float>>("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<float>("truncated");
+      auto quantizedValue = writer->GetModel().GetDefaultEntry().GetPtr<float>("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<RPageSource *> sources{source.get()};
+   RNTupleWriteOptions writeOptions;
+   writeOptions.SetCompression(101);
+   auto destination = std::make_unique<RPageSinkFile>("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<float>(name);
+      auto mergedValue = merged->GetView<float>(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");

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<float>(ROOT::ENTupleColumnType::kReal16)->GetSize());
 
    const auto &desc = reader->GetDescriptor();
    EXPECT_EQ(ROOT::ENTupleColumnType::kReal16, (*desc.GetColumnIterable(desc.FindFieldId("f1")).begin()).GetType());

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<void>(colInfo.fColDesc->GetType());
+         auto colElement = ROOT::Internal::RColumnElementBase::Generate(*colInfo.fColDesc);
          colElement->SetBitsOnStorage(colInfo.fColDesc->GetBitsOnStorage());
 
          std::uint64_t pageIdx = 0;

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<std::uint64_t> compressedPageSizes{};