Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

RNTupleDS should check for column type mismatches #10749

Open
eguiraud opened this issue Jun 13, 2022 · 2 comments · May be fixed by #17221
Open

RNTupleDS should check for column type mismatches #10749

eguiraud opened this issue Jun 13, 2022 · 2 comments · May be fixed by #17221
Assignees
@jblomer
Labels
improvement in:RDataFrame in:RNTuple

Comments

@eguiraud
Copy link
Member

eguiraud commented Jun 13, 2022 •
edited
Loading

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

root/tree/dataframe/src/RNTupleDS.cxx

Lines 288 to 296 in 89776f4

RNTupleDS::GetColumnReaders(unsigned int slot, std::string_view name, const std::type_info & /*tid*/)
{
// at this point we can assume that `name` will be found in fColumnNames, RDF is in charge validation
// TODO(jblomer): check incoming type
const auto index = std::distance(fColumnNames.begin(), std::find(fColumnNames.begin(), fColumnNames.end(), name));
auto clone = fColumnReaderPrototypes[index]->Clone();
clone->Connect(*fSources[slot]);
return clone;
}

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
Copy link
Contributor

jblomer commented Jun 14, 2022

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
Copy link
Member

pcanal commented Jun 14, 2022

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).

This was referenced Dec 3, 2024
Merged
Closed
ferdymercury added a commit to ferdymercury/root that referenced this issue Dec 6, 2024
@ferdymercury
[DF] Check column types in GetColumnReadersImpl()
25a4757 
Fixes root-project#10749

Commit by jblomer, just rebased here
@ferdymercury ferdymercury linked a pull request Dec 6, 2024 that will close this issue
Open
2 tasks
jblomer pushed a commit to jblomer/root that referenced this issue Dec 18, 2024
@ferdymercury @jblomer
[DF] Check column types in GetColumnReadersImpl()
8f39579 
Fixes root-project#10749

Commit by jblomer, just rebased here
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jblomer jblomer

Labels
improvement in:RDataFrame in:RNTuple
Projects
None yet
Milestone
No milestone
3 participants
@jblomer @pcanal @eguiraud