[ntuple] Add initial in-memory index prototype #15116

enirolf · 2024-04-03T08:43:39Z

This PR adds (a first version of) the RNTupleIndex, which is an in-memory structure that maps RNTuple field values (or combinations thereof) to an entry index in the RNTuple for which the index was built. Currently, the index only resides in memory and thus has to be (re)build each time. RNTupleIndex will be used by the RNTupleProcessor to enable dataset joins and will be as transparent as possible to users. Currently, no public interface is foreseen.

At this point, no persistification is foreseen, but this might be added in the future. The implementation of the RNTupleIndex in this PR is hash-based. An implementation that is vector-based (but with the same interface) will also be considered.

The idea is to benchmark and evaluate both implementations (and potentially more). Based on the results we can decide which one to actually use (or alternatively make multiple implementations available if they show clear tradeoffs in different use cases).

phsft-bot · 2024-04-03T08:43:47Z

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default
How to customize builds

phsft-bot · 2024-04-03T09:34:10Z

Build failed on ROOT-ubuntu2204/nortcxxmod.
Running on root-ubuntu-2204-3.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

projectroot.roottest.python.distrdf.backends.roottest_python_distrdf_backends_test_all

phsft-bot · 2024-04-03T09:41:22Z

Build failed on mac12arm/cxx20.
Running on 194.12.161.128:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Errors:

[2024-04-03T09:41:19.032Z] FAILED: tree/ntuple/CMakeFiles/ROOTNTuple.dir/v7/src/RField.cxx.o
[2024-04-03T09:41:19.641Z] /Users/sftnight/build/workspace/root-pullrequests-build/root/tree/ntuple/v7/src/RField.cxx:1076:47: error: return type of out-of-line definition of 'ROOT::Experimental::RFieldBase::GetIndexRepresentation' differs from that in the declaration

Warnings:

[2024-04-03T09:41:19.641Z] /Users/sftnight/build/workspace/root-pullrequests-build/root/tree/ntuple/v7/inc/ROOT/RNTupleIndex.hxx:76:68: warning: unused parameter 'lowerBound' [-Wunused-parameter]
[2024-04-03T09:41:20.407Z] /Users/sftnight/build/workspace/root-pullrequests-build/root/tree/ntuple/v7/inc/ROOT/RNTupleIndex.hxx:76:68: warning: unused parameter 'lowerBound' [-Wunused-parameter]

phsft-bot · 2024-04-03T09:50:05Z

Build failed on windows10/default.
Running on null:C:\build\workspace\root-pullrequests-build
See console output.

Errors:

[2024-04-03T09:31:03.191Z] C:\build\workspace\root-pullrequests-build\root\tree\ntuple\v7\src\RField.cxx(1077,1): error C2556: 'uint64_t ROOT::Experimental::RFieldBase::GetIndexRepresentation(void *)': overloaded function differs only by return type from 'ROOT::Experimental::NTupleIndexValue_t ROOT::Experimental::RFieldBase::GetIndexRepresentation(void *)' [C:\build\workspace\root-pullrequests-build\build\tree\ntuple\ROOTNTuple.vcxproj]
[2024-04-03T09:31:03.192Z] C:\build\workspace\root-pullrequests-build\root\tree\ntuple\v7\src\RField.cxx(1076,47): error C2371: 'ROOT::Experimental::RFieldBase::GetIndexRepresentation': redefinition; different basic types [C:\build\workspace\root-pullrequests-build\build\tree\ntuple\ROOTNTuple.vcxproj]

github-actions · 2024-04-03T09:58:06Z

Test Results

13 files 13 suites 3d 1h 45m 34s ⏱️
3 030 tests 3 028 ✅ 0 💤 2 ❌
33 869 runs 33 867 ✅ 0 💤 2 ❌

For more details on these failures, see this check.

Results for commit f4ae977.

♻️ This comment has been updated with latest results.

phsft-bot · 2024-04-03T14:55:18Z

Build failed on ROOT-ubuntu2004/python3.
Running on root-ubuntu-2004-1.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

tree/ntuple/v7/inc/ROOT/RField.hxx

tree/ntuple/v7/inc/ROOT/RNTupleIndex.hxx

tree/ntuple/v7/src/RNTupleIndex.cxx

jblomer

Cool new functionality!

Some comments:

I think that is a fundamental new functionality that should have a section in the architecture.md
Instead of adding the GetHash() member function, perhaps we can use a "hash visitor". That would allow for using only available public interfaces. The RPrintValueVisitor can serve as a blueprint (the actual hash visitor class is probably better placed in RNTupleIndex.[hxx,cxx] or in a separate compilation unit).

jblomer · 2024-04-22T22:24:19Z

tree/ntuple/v7/src/RField.cxx

@@ -1076,6 +1076,12 @@ void ROOT::Experimental::RFieldBase::AcceptVisitor(Detail::RFieldVisitor &visito
   visitor.VisitField(*this);
 }

+ROOT::Experimental::NTupleIndexValue_t ROOT::Experimental::RFieldBase::GetIndexRepresentation(void * /*from*/)
+{
+   R__ASSERT(false && "indexing is not supported for this field type");


Throw exception instead?

jblomer · 2024-07-01T12:20:03Z

tree/ntuple/v7/test/CMakeLists.txt

@@ -35,6 +35,7 @@ ROOT_GENERATE_DICTIONARY(RXTupleDict ${CMAKE_CURRENT_SOURCE_DIR}/RXTuple.hxx
 ROOT_ADD_GTEST(ntuple_descriptor ntuple_descriptor.cxx LIBRARIES ROOTNTuple CustomStruct)
 ROOT_ADD_GTEST(ntuple_endian ntuple_endian.cxx LIBRARIES ROOTNTuple)
 ROOT_ADD_GTEST(ntuple_friends ntuple_friends.cxx LIBRARIES ROOTNTuple CustomStruct)
+ROOT_ADD_GTEST(ntuple_index ntuple_index.cxx LIBRARIES ROOTNTuple CustomStruct)


Suggested change

ROOT_ADD_GTEST(ntuple_index ntuple_index.cxx LIBRARIES ROOTNTuple CustomStruct)

ROOT_ADD_GTEST(ntuple_index ntuple_index.cxx LIBRARIES ROOTNTuple)

jblomer · 2024-07-01T12:37:33Z

tree/ntuple/v7/inc/ROOT/RField.hxx

@@ -542,6 +544,8 @@ protected:
   /// Called by `ConnectPageSource()` once connected; derived classes may override this as appropriate
   virtual void OnConnectPageSource() {}

+   virtual std::uint64_t GetHash(const void *from) const;


Since this is calculating a hash, I'd suggest

Suggested change

virtual std::uint64_t GetHash(const void *from) const;

virtual std::uint64_t Hash(const void *from) const;

jblomer · 2024-07-01T13:04:55Z

tree/ntuple/v7/src/RNTupleIndex.cxx

+      std::vector<void *> ptrs;
+      ptrs.reserve(fieldValues.size());
+      for (auto &fieldValue : fieldValues) {
+         fieldValue.Read(i);


This is a good use for bulk reading. Doesn't necessarily need to be deployed in this commit but a TODO could be added.

jblomer · 2024-07-01T13:07:53Z

tree/ntuple/v7/inc/ROOT/RNTupleIndex.hxx

+/////////////////////////////////////////////////////////////////////////////
+/// Container for the combined hash of the indexed fields. Uses the implementation from `boost::hash_combine` (see
+/// https://www.boost.org/doc/libs/1_55_0/doc/html/hash/reference.html#boost.hash_combine).
+struct RIndexValue {


If possible, make it a private sub class of RNTupleIndex because the name RIndexValue may be a little too generic to be directly part of the ROOT namespace.

jblomer · 2024-07-01T17:53:11Z

tree/ntuple/v7/inc/ROOT/RNTupleIndex.hxx

+
+   std::size_t GetNElems() const { return fIndex.size(); }
+
+   void Add(const std::vector<void *> &valuePtrs, NTupleSize_t entry);


Make private?

jblomer · 2024-07-01T17:55:20Z

tree/ntuple/v7/inc/ROOT/RNTupleIndex.hxx

+   ///
+   /// Note that in case multiple entries corresponding to the provided index value exist, the first occurrence is
+   /// returned. Use RNTupleIndex::GetEntryIndices to get all entries.
+   NTupleSize_t GetEntryIndex(const std::vector<void *> &valuePtrs) const;


Here, and further down: how about using "entry number" instead of "entry index" to not confuse it with the index itself. Also, I'd suggest for clarity GetFirstEntryNumber() and GetAllEntryNumbers()

Suggested change

NTupleSize_t GetEntryIndex(const std::vector<void *> &valuePtrs) const;

NTupleSize_t GetFirstEntryNumber(const std::vector<void *> &valuePtrs) const;

jblomer · 2024-07-01T19:04:34Z

tree/ntuple/v7/inc/ROOT/RNTupleIndex.hxx

+   {
+      if (sizeof...(Ts) != fFields.size())
+         throw RException(R__FAIL("number of value pointers must match number of indexed fields"));
+


We may also want to check that the types match, e.g. RField<T>::TypeName() == fFields[i]->GetTypeName()

Perhaps a TODO at this point. We probably need a typeid like check as we don't want a string comparison every time.

jblomer · 2024-07-01T19:06:57Z

tree/ntuple/v7/src/RNTupleIndex.cxx

+#include <ROOT/RNTupleIndex.hxx>
+
+ROOT::Experimental::Internal::RNTupleIndex::RNTupleIndex(std::vector<std::unique_ptr<RFieldBase>> &fields,
+                                                         RPageSource &pageSource)


We only need it for the number of entries.

Suggested change

RPageSource &pageSource)

const RPageSource &pageSource)

You can also consider passing just that number.

Yep, I agree

jblomer · 2024-07-01T19:13:42Z

tree/ntuple/v7/src/RNTupleIndex.cxx

+      auto fieldOrException = RFieldBase::Create(fieldDesc.GetFieldName(), fieldDesc.GetTypeName());
+      if (!fieldOrException) {
+         throw RException(R__FAIL("could not construct field \"" + std::string(fieldName) + "\""));
+      }
+      auto field = fieldOrException.Unwrap();
+      field->SetOnDiskId(fieldDesc.GetId());


Better: use RFieldDescriptor::CreateField()

vepadulano

I really like the direction of this new feature, it shows a great deal of thinking! I left just some small comments that came up while reading the code.

vepadulano · 2024-07-07T21:05:15Z

tree/ntuple/v7/inc/ROOT/RNTupleIndex.hxx

+   RNTupleIndex &operator=(const RNTupleIndex &other) = delete;
+   RNTupleIndex(RNTupleIndex &&other) = delete;
+   RNTupleIndex &operator=(RNTupleIndex &&other) = delete;
+


Add destructor for rule of five

vepadulano · 2024-07-07T21:08:19Z

tree/ntuple/v7/inc/ROOT/RNTupleIndex.hxx

+   /// \brief Get the entry number containing the given index value.
+   ///
+   /// \param[in] value The indexed value


Comment is outdated

vepadulano · 2024-07-07T21:11:06Z

tree/ntuple/v7/src/RNTupleIndex.cxx

+#include <ROOT/RNTupleIndex.hxx>
+
+ROOT::Experimental::Internal::RNTupleIndex::RNTupleIndex(std::vector<std::unique_ptr<RFieldBase>> &fields,
+                                                         RPageSource &pageSource)


Yep, I agree

vepadulano · 2024-07-07T21:16:33Z

tree/ntuple/v7/test/ntuple_index.cxx

+   auto secondaryPageSource = RPageSource::Create("secondary", fileGuardSecondary.GetPath());
+   auto index = ROOT::Experimental::Internal::CreateRNTupleIndex({"event"}, *secondaryPageSource);
+   auto secondaryNTuple = RNTupleReader::Open("secondary", fileGuardSecondary.GetPath());


This pattern of creating a page source, then the index, then using the RNTupleReader seems a bit odd. For now the API is not the main concern since anyway we'll integrate it in the RDF processing. But something to consider in general

This is only for testing. Initially, I added a CreateIndex method to the RNTupleReader, but since we want to use the index only internally (at least for the foreseeable future) I removed it.

silverweed · 2024-07-08T08:38:23Z

tree/ntuple/v7/src/RNTupleIndex.cxx

+   if (!fIndex.count(indexValue))
+      return {};
+
+   return fIndex.at(indexValue);


Given that here we're not constructing the returned vector, but rather copying an existing one, we might want to return a const vector<> * instead.
But this depends on 3 factors which I'm not aware of, so I'll let you decide what's best:

how big can this vector get? If it's just a few values, the copy is no problem at all.

how hot will this method be?

if later we will want to change the implementation so that we actually need to construct a vector rather than just referencing it, how big of a problem would that be?

~~What would be the reason to return a const pointer rather than a const ref in this case?~~

EDIT: I realize it's due to the empty vector case

how big can this vector get? If it's just a few values, the copy is no problem at all.

Very likely it will only contain one element. Maybe in some exotic cases a couple of them (depending on pileup), but that will definitely not be a common case.

how hot will this method be?

Since it's called by GetFirstEntryNumber it will be quite hot (i.e. once every event in the event loop). Possibly GetFirstEntryNumber can be refactored to not use this one.

if later we will want to change the implementation so that we actually need to construct a vector rather than just referencing it, how big of a problem would that be?

I think not a problem at all, based the points you raised I think it makes sense to change it to a reference.

hahnjo · 2024-07-22T13:40:03Z

While thinking about collisions and the index storage, one thought that crossed my mind is to template the index based on the index field type(s). I'm not sure if that's a good idea, one of the immediate question being "do we want RNTupleIndex<std::string> and RNTupleIndex<std::uint64_t> to be different types?" But still maybe something to consider, it might simplify the field value storage (if needed) and the entire hashing business...

enirolf · 2024-08-13T07:46:11Z

While thinking about collisions and the index storage, one thought that crossed my mind is to template the index based on the index field type(s). I'm not sure if that's a good idea, one of the immediate question being "do we want RNTupleIndex<std::string> and RNTupleIndex<std::uint64_t> to be different types?" But still maybe something to consider, it might simplify the field value storage (if needed) and the entire hashing business...

This is how I initially implemented it. Indeed it makes the index itself much more straightforward. However, when I started prototyping the actual join/unaligned friends it really didn't work without making that interface overly complicated so in the end I opted for a non-templated version. Perhaps there's some template trickery to still make it play nice with the foreseen interface (or allow for a slightly different but still simple enough interface), I will think about it for a bit.

hahnjo · 2024-08-14T11:24:56Z

Not sure if it applies here, but a common "template trickery" is to have a type-punned base class that provides enough functionality to cater for the hot paths during the event loop. We already use this in RNTuple, for example with RFieldBase that can be conveniently passed around if you only need its interface functions.

enirolf · 2024-09-10T15:08:23Z

After going back and forth between different resolutions, I've decided to for now only allow integral-type fields as index fields (thus dropping floating-point types and strings), and storing their values as std::uint64_t. For now, we don't have any concrete examples where a non-integral field is desired to be used for indexing, so to stay within the scope of "initial prototype", I think this approach should suffice.

While it will solve the potential index collisions, storing the separate values will incur some memory overhead, but preliminary benchmarks indicate it should be manageable (to be followed-up, though).

This adds (a first version of) the `RNTupleIndex`, which is an in-memory structure that maps RNTuple field values (or combinations thereof) to an entry index in the RNTuple for which the index was built. At this point, the index only resides in memory and thus has to be (re)build each time. `RNTupleIndex` will be used by the `RNTupleProcessor` to enable dataset joins and will be as transparent as possible to users. Currently, no public interface is foreseen.

jblomer

Nice and clean! Some comments for consideration before merging.

jblomer · 2024-09-20T20:09:52Z

tree/ntuple/v7/src/RNTupleIndex.cxx

+
+void ROOT::Experimental::Internal::RNTupleIndex::Build()
+{
+   std::vector<RFieldBase::RValue> fieldValues;


I think it would be nice to make this method idempotent, i.e. start with

if (fIsBuilt) return;

jblomer · 2024-09-20T20:12:16Z

tree/ntuple/v7/src/RNTupleIndex.cxx

+      if (!field->IsSimple() || field->GetTypeName() == "float" || field->GetTypeName() == "double") {
+         throw RException(R__FAIL("Cannot use field \"" + field->GetFieldName() + "\" with type \"" +
+                                  field->GetTypeName() + "\" for indexing. Only integral types are allowed."));
+      }


Perhaps better to explicitly enumerate the supported types, e.g. check field->GetTypeName() against a static std::unordered_set<std::string>. Or use a dynamic cast with the recent RIntegralField.

pcanal · 2024-09-20T21:33:57Z

I've decided to for now only allow integral-type fields as index fields (thus dropping floating-point types

Good. Having a floating point 'as is' as the index would be a disaster due to the instability of equality. (One could envision having a floating pointer index only the user provide a transformation from floating point to integer).

enirolf added the in:RNTuple label Apr 3, 2024

enirolf requested a review from vepadulano April 3, 2024 08:43

enirolf self-assigned this Apr 3, 2024

enirolf force-pushed the ntuple-index branch 2 times, most recently from 510337a to 4dc20b9 Compare April 10, 2024 10:07

enirolf force-pushed the ntuple-index branch from 4dc20b9 to 94f87da Compare April 19, 2024 09:06

enirolf force-pushed the ntuple-index branch 2 times, most recently from fd64176 to 825de2f Compare May 8, 2024 18:14

enirolf force-pushed the ntuple-index branch from 825de2f to 7053cf1 Compare May 21, 2024 12:10

enirolf force-pushed the ntuple-index branch from 7053cf1 to cd57560 Compare June 27, 2024 12:50

enirolf changed the title ~~[ntuple] Add indexing prototype~~ [ntuple] Add initial in-memory index prototype Jun 27, 2024

enirolf force-pushed the ntuple-index branch 3 times, most recently from 672671b to 68b602a Compare June 28, 2024 07:59

enirolf marked this pull request as ready for review June 28, 2024 08:00

enirolf requested review from bellenot and jblomer as code owners June 28, 2024 08:00

enirolf requested review from hahnjo, pcanal and silverweed June 28, 2024 08:00

silverweed requested changes Jun 28, 2024

View reviewed changes

jblomer requested changes Jul 1, 2024

View reviewed changes

enirolf force-pushed the ntuple-index branch 2 times, most recently from 94a517c to 67cbc14 Compare July 2, 2024 11:12

vepadulano reviewed Jul 7, 2024

View reviewed changes

silverweed reviewed Jul 8, 2024

View reviewed changes

enirolf force-pushed the ntuple-index branch 2 times, most recently from d15cfe3 to 2cae07a Compare July 9, 2024 09:32

enirolf requested review from silverweed, vepadulano and jblomer July 9, 2024 13:07

enirolf force-pushed the ntuple-index branch from 2cae07a to eeacbc6 Compare September 10, 2024 15:00

enirolf force-pushed the ntuple-index branch from eeacbc6 to 9c757b8 Compare September 11, 2024 15:15

enirolf added 2 commits September 11, 2024 17:25

[ntuple] Add option to defer index building

f4ae977

enirolf force-pushed the ntuple-index branch from 9c757b8 to f4ae977 Compare September 11, 2024 15:26

jblomer approved these changes Sep 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ntuple] Add initial in-memory index prototype #15116

[ntuple] Add initial in-memory index prototype #15116

enirolf commented Apr 3, 2024 •

edited

Loading

phsft-bot commented Apr 3, 2024

phsft-bot commented Apr 3, 2024

phsft-bot commented Apr 3, 2024

phsft-bot commented Apr 3, 2024

github-actions bot commented Apr 3, 2024 •

edited

Loading

phsft-bot commented Apr 3, 2024

jblomer left a comment

jblomer Apr 22, 2024

jblomer Jul 1, 2024

jblomer Jul 1, 2024

jblomer Jul 1, 2024

jblomer Jul 1, 2024

jblomer Jul 1, 2024

jblomer Jul 1, 2024

jblomer Jul 1, 2024

jblomer Jul 1, 2024

vepadulano Jul 7, 2024

jblomer Jul 1, 2024

vepadulano left a comment

vepadulano Jul 7, 2024

vepadulano Jul 7, 2024

vepadulano Jul 7, 2024

vepadulano Jul 7, 2024

enirolf Jul 9, 2024

silverweed Jul 8, 2024

enirolf Jul 9, 2024 •

edited

Loading

hahnjo commented Jul 22, 2024

enirolf commented Aug 13, 2024

hahnjo commented Aug 14, 2024

enirolf commented Sep 10, 2024

jblomer left a comment

jblomer Sep 20, 2024

jblomer Sep 20, 2024

pcanal commented Sep 20, 2024

	ROOT_ADD_GTEST(ntuple_index ntuple_index.cxx LIBRARIES ROOTNTuple CustomStruct)
	ROOT_ADD_GTEST(ntuple_index ntuple_index.cxx LIBRARIES ROOTNTuple)

	virtual std::uint64_t GetHash(const void *from) const;
	virtual std::uint64_t Hash(const void *from) const;


		std::size_t GetNElems() const { return fIndex.size(); }

		void Add(const std::vector<void *> &valuePtrs, NTupleSize_t entry);

	NTupleSize_t GetEntryIndex(const std::vector<void *> &valuePtrs) const;
	NTupleSize_t GetFirstEntryNumber(const std::vector<void *> &valuePtrs) const;

[ntuple] Add initial in-memory index prototype #15116

Are you sure you want to change the base?

[ntuple] Add initial in-memory index prototype #15116

Conversation

enirolf commented Apr 3, 2024 • edited Loading

phsft-bot commented Apr 3, 2024

phsft-bot commented Apr 3, 2024

Failing tests:

phsft-bot commented Apr 3, 2024

Errors:

Warnings:

phsft-bot commented Apr 3, 2024

Errors:

github-actions bot commented Apr 3, 2024 • edited Loading

Test Results

phsft-bot commented Apr 3, 2024

jblomer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vepadulano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enirolf Jul 9, 2024 • edited Loading

Choose a reason for hiding this comment

hahnjo commented Jul 22, 2024

enirolf commented Aug 13, 2024

hahnjo commented Aug 14, 2024

enirolf commented Sep 10, 2024

jblomer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcanal commented Sep 20, 2024

enirolf commented Apr 3, 2024 •

edited

Loading

github-actions bot commented Apr 3, 2024 •

edited

Loading

enirolf Jul 9, 2024 •

edited

Loading