[ntuple] forbid ClusterSize_t as an on-disk type

The RField<ClusterSize_t> field must only be used for reading.
root-project · Oct 10, 2024 · 1c358d4 · 1c358d4
1 parent a4ee33c
commit 1c358d4
Show file tree

Hide file tree

Showing 3 changed files with 35 additions and 9 deletions.
diff --git a/tree/ntuple/v7/inc/ROOT/RField.hxx b/tree/ntuple/v7/inc/ROOT/RField.hxx
@@ -348,6 +348,8 @@ protected:
    }
 
    const RColumnRepresentations &GetColumnRepresentations() const final;
+   using RSimpleField<ClusterSize_t>::GenerateColumns;
+   void GenerateColumns() final { throw RException(R__FAIL("RField<ClusterSize_t> must only be used for reading")); }
 
 public:
    static std::string TypeName() { return "ROOT::Experimental::ClusterSize_t"; }

diff --git a/tree/ntuple/v7/src/RField.cxx b/tree/ntuple/v7/src/RField.cxx
@@ -150,8 +150,7 @@ std::tuple<std::string, std::vector<size_t>> ParseArrayType(std::string_view typ
 std::string GetCanonicalTypeName(const std::string &typeName)
 {
    // The following types are asummed to be canonical names; thus, do not perform `typedef` resolution on those
-   if (typeName == "ROOT::Experimental::ClusterSize_t" || typeName.substr(0, 5) == "std::" ||
-       typeName.substr(0, 39) == "ROOT::Experimental::RNTupleCardinality<")
+   if (typeName.substr(0, 5) == "std::" || typeName.substr(0, 39) == "ROOT::Experimental::RNTupleCardinality<")
       return typeName;
 
    return TClassEdit::ResolveTypedef(typeName.c_str());
@@ -652,9 +651,7 @@ ROOT::Experimental::RFieldBase::Create(const std::string &fieldName, const std::
 
    std::unique_ptr<ROOT::Experimental::RFieldBase> result;
 
-   if (canonicalType == "ROOT::Experimental::ClusterSize_t") {
-      result = std::make_unique<RField<ClusterSize_t>>(fieldName);
-   } else if (canonicalType == "bool") {
+   if (canonicalType == "bool") {
       result = std::make_unique<RField<bool>>(fieldName);
    } else if (canonicalType == "char") {
       result = std::make_unique<RField<char>>(fieldName);

diff --git a/tree/ntuple/v7/test/ntuple_packing.cxx b/tree/ntuple/v7/test/ntuple_packing.cxx
@@ -263,6 +263,30 @@ AddReal32QuantField(RNTupleModel &model, const std::string &fieldName, std::size
    model.AddField(std::move(fld));
 }
 
+namespace {
+
+// Test writing index32/64 columns
+class RFieldTestIndexColumn final : public ROOT::Experimental::RSimpleField<ClusterSize_t> {
+protected:
+   std::unique_ptr<RFieldBase> CloneImpl(std::string_view newName) const final
+   {
+      return std::make_unique<RFieldTestIndexColumn>(newName);
+   }
+   const RColumnRepresentations &GetColumnRepresentations() const final
+   {
+      static RColumnRepresentations representations({{EColumnType::kSplitIndex64}, {EColumnType::kSplitIndex32}}, {});
+      return representations;
+   }
+
+public:
+   explicit RFieldTestIndexColumn(std::string_view name) : RSimpleField(name, "ROOT::Experimental::ClusterSize_t") {}
+   RFieldTestIndexColumn(RFieldTestIndexColumn &&other) = default;
+   RFieldTestIndexColumn &operator=(RFieldTestIndexColumn &&other) = default;
+   ~RFieldTestIndexColumn() override = default;
+};
+
+} // anonymous namespace
+
 TEST(Packing, OnDiskEncoding)
 {
    FileRaii fileGuard("test_ntuple_packing_ondiskencoding.root");
@@ -280,10 +304,14 @@ TEST(Packing, OnDiskEncoding)
    AddField<float, EColumnType::kSplitReal32>(*model, "float");
    AddField<float, EColumnType::kReal16>(*model, "float16");
    AddField<double, EColumnType::kSplitReal64>(*model, "double");
-   AddField<ClusterSize_t, EColumnType::kSplitIndex32>(*model, "index32");
-   AddField<ClusterSize_t, EColumnType::kSplitIndex64>(*model, "index64");
    AddReal32TruncField(*model, "float32Trunc", 11);
    AddReal32QuantField(*model, "float32Quant", 7, 0.0, 1.0);
+   auto fldIdx32 = std::make_unique<RFieldTestIndexColumn>("index32");
+   fldIdx32->SetColumnRepresentatives({{EColumnType::kSplitIndex32}});
+   model->AddField(std::move(fldIdx32));
+   auto fldIdx64 = std::make_unique<RFieldTestIndexColumn>("index64");
+   fldIdx64->SetColumnRepresentatives({{EColumnType::kSplitIndex64}});
+   model->AddField(std::move(fldIdx64));
    auto fldStr = std::make_unique<RField<std::string>>("str");
    model->AddField(std::move(fldStr));
    {
@@ -396,8 +424,7 @@ TEST(Packing, OnDiskEncoding)
    unsigned char expF32Quant[] = {0xd8, 0x37};
    EXPECT_EQ(memcmp(sealedPage.GetBuffer(), expF32Quant, sizeof(expF32Quant)), 0);
 
-   auto reader = RNTupleReader::Open("ntuple", fileGuard.GetPath());
-   EXPECT_EQ(EColumnType::kIndex64, reader->GetModel().GetField("str").GetColumnRepresentatives()[0][0]);
+   auto reader = RNTupleReader::Open(RNTupleModel::Create(), "ntuple", fileGuard.GetPath());
    EXPECT_EQ(2u, reader->GetNEntries());
    auto viewStr = reader->GetView<std::string>("str");
    EXPECT_EQ(std::string("abc"), viewStr(0));