root-project / root

The official repository for ROOT: analyzing, storing and visualizing big data, scientifically
https://root.cern
Other
2.72k stars 1.3k forks source link

RNTupleDS should check for column type mismatches #10749

Open eguiraud opened 2 years ago

eguiraud commented 2 years ago

This issue is just a reminder that we need to resolve this TODO:

https://github.com/root-project/root/blob/89776f40f6f9a0f1ea769ed86f8280696b85e01f/tree/dataframe/src/RNTupleDS.cxx#L288-L296

so that e.g. the following wrong usage errors out:

#include <ROOT/RNTuple.hxx>
#include <ROOT/RNTupleDS.hxx>
#include <ROOT/RDataFrame.hxx>

int main() {
  {
    auto m = ROOT::Experimental::RNTupleModel::Create();
    auto x = m->MakeField<int>("x", 42);
    auto r = ROOT::Experimental::RNTupleWriter::Recreate(std::move(m), "n",
                                                         "f.root");
    r->Fill();
  }

  auto df = ROOT::Experimental::MakeNTupleDataFrame("n", "f.root");
  df.Filter([] (std::string &x) { return !x.empty(); }, {"x"}).Count().GetValue();
}

(on my machine the test above executes without crashes, but that's just luck)

jblomer commented 2 years ago

We have a draft PR for fixing it it #6548 which is blocked by #6607 @pcanal Do you think that issue can be addressed in TClassEdit?

pcanal commented 2 years ago

Yes, of course. There is code to explicitly detect and remove the allocator. Most likely TClassEdit::IsDefAlloc needs to be updated to take into account that the word class might be prefixed. Albeit it is odd/unexpected that Clang would put the class keyword there (it is supposed be configured not to).