Skip to content

Commit fcef84a

Browse files
silverweedsilverweed
committed
[ntuple] Rework feature flag code a bit to handle actual flags
1 parent 292aa9a commit fcef84a

4 files changed

Lines changed: 140 additions & 14 deletions

File tree

‎tree/ntuple/inc/ROOT/RNTupleDescriptor.hxx‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -765,7 +765,18 @@ private:
765765
RNTupleDescriptor CloneSchema() const;
766766

767767
public:
768-
static constexpr unsigned int kFeatureFlagTest = 137; // Bit reserved for forward-compatibility testing
768+
/// All known feature flags.
769+
/// Note that the flag values represent the bit _index_, not the already-bitshifted integer.
770+
enum EFeatureFlags {
771+
// Insert new feature flags here, with contiguous values. If at any point a "hole" appears in the valid feature
772+
// flags values, the check in RNTupleSerialize must be updated.
773+
774+
// End of regular feature flags
775+
kFeatureFlag_COUNT,
776+
777+
/// Reserved for forward-compatibility testing
778+
kFeatureFlag_Test = 137
779+
};
769780

770781
class RColumnDescriptorIterable;
771782
class RFieldDescriptorIterable;
@@ -1736,6 +1747,8 @@ public:
17361747
void SetVersionForWriting();
17371748

17381749
void SetNTuple(const std::string_view name, const std::string_view description);
1750+
/// Sets the `flag`-th bit of the feature flag to 1.
1751+
/// Note that `flag` itself is not a bitmask, just the bit index of the flag to enable.
17391752
void SetFeature(unsigned int flag);
17401753

17411754
void SetOnDiskHeaderXxHash3(std::uint64_t xxhash3) { fDescriptor.fOnDiskHeaderXxHash3 = xxhash3; }

‎tree/ntuple/src/RNTupleDescriptor.cxx‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1105,7 +1105,7 @@ void ROOT::Internal::RNTupleDescriptorBuilder::SetNTuple(const std::string_view
11051105

11061106
void ROOT::Internal::RNTupleDescriptorBuilder::SetFeature(unsigned int flag)
11071107
{
1108-
if (flag % 64 == 0)
1108+
if (flag > 0 && flag % 64 == 0)
11091109
throw RException(R__FAIL("invalid feature flag: " + std::to_string(flag)));
11101110
fDescriptor.fFeatureFlags.insert(flag);
11111111
}

‎tree/ntuple/src/RNTupleSerialize.cxx‎

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
#include <ROOT/RNTupleSerialize.hxx>
1919
#include <ROOT/RNTupleTypes.hxx>
2020
#include <ROOT/RNTupleUtils.hxx>
21+
#include <ROOT/BitUtils.hxx>
2122

2223
#include <RVersion.h>
2324
#include <TBufferFile.h>
@@ -1621,7 +1622,6 @@ ROOT::Internal::RNTupleSerializer::SerializeHeader(void *buffer, const ROOT::RNT
16211622
void **where = (buffer == nullptr) ? &buffer : reinterpret_cast<void **>(&pos);
16221623

16231624
pos += SerializeEnvelopePreamble(kEnvelopeTypeHeader, *where);
1624-
// So far we don't make use of feature flags
16251625
if (auto res = SerializeFeatureFlags(desc.GetFeatureFlags(), *where)) {
16261626
pos += res.Unwrap();
16271627
} else {
@@ -1761,7 +1761,6 @@ ROOT::RResult<std::uint32_t> ROOT::Internal::RNTupleSerializer::SerializeFooter(
17611761

17621762
pos += SerializeEnvelopePreamble(kEnvelopeTypeFooter, *where);
17631763

1764-
// So far we don't make use of footer feature flags
17651764
// NOTE: we currently serialize all feature flags in the footer, even those that were already written in the
17661765
// header. This is fine, as they will be logically OR-ed together during deserialization.
17671766
if (auto res = SerializeFeatureFlags(desc.GetFeatureFlags(), *where)) {
@@ -1871,10 +1870,10 @@ static ROOT::RResult<void> CheckFeatureFlags(const std::vector<std::uint64_t> &f
18711870
for (std::size_t i = 0; i < featureFlags.size(); ++i) {
18721871
if (!featureFlags[i])
18731872
continue;
1874-
unsigned int bit = 0;
1875-
while (!(featureFlags[i] & (static_cast<uint64_t>(1) << bit)))
1876-
bit++;
1877-
return R__FAIL("unsupported format feature: " + std::to_string(i * 64 + bit));
1873+
// NOTE: this assumes all valid feature flags are consecutive, thus we can just check the highest one set.
1874+
unsigned int highestBitSet = 64 * i + (63 - ROOT::Internal::LeadingZeroes(featureFlags[i]));
1875+
if (highestBitSet >= ROOT::RNTupleDescriptor::kFeatureFlag_COUNT)
1876+
return R__FAIL("unsupported format feature: " + std::to_string(highestBitSet));
18781877
}
18791878
return ROOT::RResult<void>::Success();
18801879
}

‎tree/ntuple/test/ntuple_compat.cxx‎

Lines changed: 120 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -29,14 +29,85 @@ TEST(RNTupleCompat, Epoch)
2929
}
3030
}
3131

32-
TEST(RNTupleCompat, FeatureFlag)
32+
TEST(RNTupleCompat, FeatureFlagSupported)
3333
{
34-
FileRaii fileGuard("test_ntuple_compat_feature_flag.root");
34+
// Write all known feature flags in the header and verify we can read the RNTuple correctly.
35+
FileRaii fileGuard("test_ntuple_compat_feature_flag_supported.root");
3536

3637
RNTupleDescriptorBuilder descBuilder;
3738
descBuilder.SetVersionForWriting();
3839
descBuilder.SetNTuple("ntpl", "");
39-
descBuilder.SetFeature(RNTupleDescriptor::kFeatureFlagTest);
40+
for (unsigned int flag = 0; flag < RNTupleDescriptor::kFeatureFlag_COUNT; ++flag)
41+
descBuilder.SetFeature(flag);
42+
descBuilder.AddField(RFieldDescriptorBuilder::FromField(ROOT::RFieldZero()).FieldId(0).MakeDescriptor().Unwrap());
43+
ASSERT_TRUE(static_cast<bool>(descBuilder.EnsureValidDescriptor()));
44+
45+
RNTupleWriteOptions options;
46+
auto writer = RNTupleFileWriter::Recreate("ntpl", fileGuard.GetPath(), EContainerFormat::kTFile, options);
47+
RNTupleSerializer serializer;
48+
49+
auto ctx = serializer.SerializeHeader(nullptr, descBuilder.GetDescriptor()).Unwrap();
50+
auto buffer = std::make_unique<unsigned char[]>(ctx.GetHeaderSize());
51+
ctx = serializer.SerializeHeader(buffer.get(), descBuilder.GetDescriptor()).Unwrap();
52+
writer->WriteNTupleHeader(buffer.get(), ctx.GetHeaderSize(), ctx.GetHeaderSize());
53+
54+
auto szFooter = serializer.SerializeFooter(nullptr, descBuilder.GetDescriptor(), ctx).Unwrap();
55+
buffer = std::make_unique<unsigned char[]>(szFooter);
56+
serializer.SerializeFooter(buffer.get(), descBuilder.GetDescriptor(), ctx);
57+
writer->WriteNTupleFooter(buffer.get(), szFooter, szFooter);
58+
59+
writer->Commit();
60+
// Call destructor to flush data to disk
61+
writer = nullptr;
62+
63+
auto pageSource = RPageSource::Create("ntpl", fileGuard.GetPath());
64+
EXPECT_NO_THROW(pageSource->Attach());
65+
}
66+
67+
TEST(RNTupleCompat, FeatureFlagSupportedFooter)
68+
{
69+
// Write all known feature flags in the footer and verify we can read the RNTuple correctly.
70+
FileRaii fileGuard("test_ntuple_compat_feature_flag_supported_footer.root");
71+
72+
RNTupleDescriptorBuilder descBuilder;
73+
descBuilder.SetVersionForWriting();
74+
descBuilder.SetNTuple("ntpl", "");
75+
descBuilder.AddField(RFieldDescriptorBuilder::FromField(ROOT::RFieldZero()).FieldId(0).MakeDescriptor().Unwrap());
76+
ASSERT_TRUE(static_cast<bool>(descBuilder.EnsureValidDescriptor()));
77+
78+
RNTupleWriteOptions options;
79+
auto writer = RNTupleFileWriter::Recreate("ntpl", fileGuard.GetPath(), EContainerFormat::kTFile, options);
80+
RNTupleSerializer serializer;
81+
82+
auto ctx = serializer.SerializeHeader(nullptr, descBuilder.GetDescriptor()).Unwrap();
83+
auto buffer = std::make_unique<unsigned char[]>(ctx.GetHeaderSize());
84+
ctx = serializer.SerializeHeader(buffer.get(), descBuilder.GetDescriptor()).Unwrap();
85+
writer->WriteNTupleHeader(buffer.get(), ctx.GetHeaderSize(), ctx.GetHeaderSize());
86+
87+
for (unsigned int flag = 0; flag < RNTupleDescriptor::kFeatureFlag_COUNT; ++flag)
88+
descBuilder.SetFeature(flag);
89+
90+
auto szFooter = serializer.SerializeFooter(nullptr, descBuilder.GetDescriptor(), ctx).Unwrap();
91+
buffer = std::make_unique<unsigned char[]>(szFooter);
92+
serializer.SerializeFooter(buffer.get(), descBuilder.GetDescriptor(), ctx);
93+
writer->WriteNTupleFooter(buffer.get(), szFooter, szFooter);
94+
95+
writer->Commit();
96+
// Call destructor to flush data to disk
97+
writer = nullptr;
98+
99+
auto pageSource = RPageSource::Create("ntpl", fileGuard.GetPath());
100+
EXPECT_NO_THROW(pageSource->Attach());
101+
}
102+
103+
TEST(RNTupleCompat, FeatureFlagUnsupported)
104+
{
105+
FileRaii fileGuard("test_ntuple_compat_feature_flag_unsupported.root");
106+
107+
RNTupleDescriptorBuilder descBuilder;
108+
descBuilder.SetVersionForWriting();
109+
descBuilder.SetNTuple("ntpl", "");
110+
descBuilder.SetFeature(RNTupleDescriptor::kFeatureFlag_Test);
40111
descBuilder.AddField(RFieldDescriptorBuilder::FromField(ROOT::RFieldZero()).FieldId(0).MakeDescriptor().Unwrap());
41112
ASSERT_TRUE(static_cast<bool>(descBuilder.EnsureValidDescriptor()));
42113

@@ -67,9 +138,9 @@ TEST(RNTupleCompat, FeatureFlag)
67138
}
68139
}
69140

70-
TEST(RNTupleCompat, FeatureFlagInFooter)
141+
TEST(RNTupleCompat, FeatureFlagUnsupportedInFooter)
71142
{
72-
FileRaii fileGuard("test_ntuple_compat_feature_flag_footer.root");
143+
FileRaii fileGuard("test_ntuple_compat_feature_flag_unsupported_footer.root");
73144

74145
RNTupleDescriptorBuilder descBuilder;
75146
descBuilder.SetVersionForWriting();
@@ -87,7 +158,7 @@ TEST(RNTupleCompat, FeatureFlagInFooter)
87158
writer->WriteNTupleHeader(buffer.get(), ctx.GetHeaderSize(), ctx.GetHeaderSize());
88159

89160
// Write the feature flags in the footer
90-
descBuilder.SetFeature(RNTupleDescriptor::kFeatureFlagTest);
161+
descBuilder.SetFeature(RNTupleDescriptor::kFeatureFlag_Test);
91162

92163
auto szFooter = serializer.SerializeFooter(nullptr, descBuilder.GetDescriptor(), ctx).Unwrap();
93164
buffer = std::make_unique<unsigned char[]>(szFooter);
@@ -107,6 +178,49 @@ TEST(RNTupleCompat, FeatureFlagInFooter)
107178
}
108179
}
109180

181+
TEST(RNTupleCompat, FeatureFlagMixSupportedUnsupported)
182+
{
183+
FileRaii fileGuard("test_ntuple_compat_feature_flag_mix_supported.root");
184+
185+
RNTupleDescriptorBuilder descBuilder;
186+
descBuilder.SetVersionForWriting();
187+
descBuilder.SetNTuple("ntpl", "");
188+
for (unsigned int flag = 0; flag < RNTupleDescriptor::kFeatureFlag_COUNT; ++flag)
189+
descBuilder.SetFeature(flag);
190+
descBuilder.AddField(RFieldDescriptorBuilder::FromField(ROOT::RFieldZero()).FieldId(0).MakeDescriptor().Unwrap());
191+
ASSERT_TRUE(static_cast<bool>(descBuilder.EnsureValidDescriptor()));
192+
193+
RNTupleWriteOptions options;
194+
auto writer = RNTupleFileWriter::Recreate("ntpl", fileGuard.GetPath(), EContainerFormat::kTFile, options);
195+
RNTupleSerializer serializer;
196+
197+
auto ctx = serializer.SerializeHeader(nullptr, descBuilder.GetDescriptor()).Unwrap();
198+
auto buffer = std::make_unique<unsigned char[]>(ctx.GetHeaderSize());
199+
ctx = serializer.SerializeHeader(buffer.get(), descBuilder.GetDescriptor()).Unwrap();
200+
writer->WriteNTupleHeader(buffer.get(), ctx.GetHeaderSize(), ctx.GetHeaderSize());
201+
202+
// This is unsupported!
203+
descBuilder.SetFeature(RNTupleDescriptor::kFeatureFlag_COUNT);
204+
205+
auto szFooter = serializer.SerializeFooter(nullptr, descBuilder.GetDescriptor(), ctx).Unwrap();
206+
buffer = std::make_unique<unsigned char[]>(szFooter);
207+
serializer.SerializeFooter(buffer.get(), descBuilder.GetDescriptor(), ctx);
208+
writer->WriteNTupleFooter(buffer.get(), szFooter, szFooter);
209+
210+
writer->Commit();
211+
// Call destructor to flush data to disk
212+
writer = nullptr;
213+
214+
auto pageSource = RPageSource::Create("ntpl", fileGuard.GetPath());
215+
try {
216+
pageSource->Attach();
217+
FAIL() << "opening an RNTuple that uses an unsupported feature should fail";
218+
} catch (const ROOT::RException &err) {
219+
EXPECT_THAT(err.what(), testing::HasSubstr("unsupported format feature: " +
220+
std::to_string(RNTupleDescriptor::kFeatureFlag_COUNT)));
221+
}
222+
}
223+
110224
TEST(RNTupleCompat, FwdCompat_FutureNTupleAnchor)
111225
{
112226
using ROOT::RXTuple;

0 commit comments

Comments
 (0)