[DF] Check column types in GetColumnReadersImpl() #17221
Conversation
Fixes root-project#10749 Commit by jblomer, just rebased here
Test Results 10 files 10 suites 1d 17h 8m 18s ⏱️ For more details on these failures, see this check. Results for commit 2d2066e. ♻️ This comment has been updated with latest results. |
|
I stopped the builds since the code deserves a try, but after correcting a typo... |
it was detected in CI with new checking code of normalized types
|
Somehow, |
|
We are missing something here... The code seems to throw an exception with the right type, the log event shows that, but EXPECT_THROW does not catch the exception... |
| EXPECT_EQ(1U, df.Take<UInt_t>("nevent").GetValue()[0]); | ||
| // jets is currently exposed as std::vector<float> and thus not usable as ROOT::RVec<float> | ||
| EXPECT_ANY_THROW(df.Sum<ROOT::RVec<float>>("jets")); | ||
| // EXPECT_THROW(df.Sum<ROOT::RVec<float>>("jets"), std::runtime_error); // This does not work directly, maybe due to jitting ? |
There was a problem hiding this comment.
We should understand this before merging.
There was a problem hiding this comment.
Yep, the test is still failing even after that change, so the attempt was not useful
| const auto typeName = cardinalityField->GetTypeName(); | ||
| fColumnTypes.emplace_back(typeName); | ||
| std::string normalized; | ||
| TClassEdit::GetNormalizedName(normalized, typeName); | ||
| fNormalizedColumnTypes.emplace_back(normalized); |
There was a problem hiding this comment.
This and further down: I wonder if instead of adding the normalized type name everywhere where we append to fColumnTypes, perhaps we should loop once over all the column types when they are fully build, i.e. at the end of the constructor.
Please take it from here ;) thanks! |
This Pull request:
Changes or fixes:
This is just an untested / rebased version of @jblomer 's PR #6548, just rebased here, now that the blocking issue has been solved in master. Feel free to take over or close.
Fixes #10749
Checklist: