Skip to content

[ntuple] Check TKey class name is RNTuple#8285

Merged
jblomer merged 1 commit intoroot-project:masterfrom
mxxo:key-class-name
Jun 2, 2021
Merged

[ntuple] Check TKey class name is RNTuple#8285
jblomer merged 1 commit intoroot-project:masterfrom
mxxo:key-class-name

Conversation

@mxxo
Copy link
Copy Markdown
Contributor

@mxxo mxxo commented May 31, 2021

Fixes issue #8284 where TKeys with the same name as the requested RNTuple
would be attempted to be parsed as an RNTuple, leading to internal
parser assert failures later on.

e.g.

// actually holds a TTree named "Events"
auto reader = RNTupleReader::Open("Events", "test80X_NANO.root");

Internal error before:

Fatal: nread == nbytes violated at line 1011 of `~/root/tree/ntuple/v7/src/RMiniFile.cxx'
aborting

Exception thrown after:

C++ exception with description "no RNTuple named 'Events' in file 'test80X_NANO.root' (unchecked RResult access!)

@mxxo mxxo requested a review from pcanal as a code owner May 31, 2021 13:20
@mxxo mxxo requested a review from jblomer May 31, 2021 13:21
@phsft-bot
Copy link
Copy Markdown

phsft-bot commented May 31, 2021

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

@mxxo mxxo linked an issue May 31, 2021 that may be closed by this pull request
[ntuple] TKey with the same name as requested RNTuple causes internal RNTupleReader errors #8284
Closed
1 task
@mxxo mxxo marked this pull request as draft May 31, 2021 16:02
@phsft-bot
Copy link
Copy Markdown

phsft-bot commented May 31, 2021

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

Failing tests:

@mxxo
Copy link
Copy Markdown
Contributor Author

mxxo commented May 31, 2021

Converted to draft: after this change, using ntuple_info.C from https://github.com/jblomer/iotools at the interpreter was no longer able to open the Events NanoAOD RNTuple (it succeeded as a compiled gtest however).

C++ exception with description "no RNTuple named 'Events' in file 'test80X_NANO_rntuple.root' (unchecked RResult access!)

@mxxo
Copy link
Copy Markdown
Contributor Author

mxxo commented May 31, 2021 •
edited
Loading

Observing some strange behaviour when running ROOT from the command line... [edit] which was from not correctly setting the offset if the class name wasn't RNTuple here:

root/tree/ntuple/v7/src/RMiniFile.cxx

Lines 965 to 967 in 28ea81b

if (std::string_view(name.fData, name.fLName) != kNTupleClassName) {
offset = offsetNextKey;
continue;

Bug symptoms

If the className check is commented out we get the following (expected) output:

      offset += key.GetHeaderSize();
      ReadBuffer(&name, 1, offset);
      ReadBuffer(&name, name.GetSize(), offset);
      std::cout << "got class name size: " << std::to_string(name.GetSize()) << "\n";
      auto className = std::string(std::string_view(name.fData, name.fLName));
      std::cout << "got class name: " << className << "\n";
      //if (className != "ROOT::Experimental::RNTuple") {
      //   continue;
      //}

commented out:

root [1] ntuple_info("test80X_NANO_rntuple.root", "Events")
got class name size: 11
got class name: TObjString
got name: tag
got class name size: 28
got class name: ROOT::Experimental::RNTuple
got name: ParameterSets
got class name size: 28
got class name: ROOT::Experimental::RNTuple
got name: MetaData
got class name size: 28
got class name: ROOT::Experimental::RNTuple
got name: Events

uncommented:

root [1] ntuple_info("test80X_NANO_rntuple.root", "Events")
got class name size: 11
got class name: TObjString
got class name size: 33
got class name: class��:iyK�Qqc�d�
got class name size: 80
ParameterSetse: OT::Experimental::RNTuple
             object title��:iyK�Lqjd�
got class name size: 102
got class name: rSets
                     object title��:iyK�Lqjd�ROOT::Experimental::RNTuplMetaData
                                                                               object title��
got class name size: 77
got class name: qjd�ROOT::Experimental::RNTuplMetaData
                                                      object title��:iyK�J
got class name size: 85
got class name: uplMetaData
                           object title��:iyK�J�A�d�ROOT::Experimental::RNTuple�Eve
Error in <TRint::HandleTermInput()>: ROOT::Experimental::RException caught: no RNTuple named 'Events' in file 'test80X_NANO_rntuple.root' (unchecked RResult access!)
At:
  ROOT::Experimental::RResult<ROOT::Experimental::RNTuple> ROOT::Experimental::Internal::RMiniFileReader::GetNTupleProper(std::string_view) [/home/max/projects/rootdev/root/tree/ntuple/v7/src/RMiniFile.cxx:983]

@mxxo mxxo force-pushed the key-class-name branch from e05c834 to 28ea81b Compare June 1, 2021 15:38
@phsft-bot
Copy link
Copy Markdown

phsft-bot commented Jun 1, 2021

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

@mxxo mxxo marked this pull request as ready for review June 1, 2021 15:39
@jblomer jblomer self-assigned this Jun 2, 2021
jblomer
jblomer approved these changes Jun 2, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@jblomer jblomer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

jblomer
jblomer reviewed Jun 2, 2021
View reviewed changes
tree/ntuple/v7/test/ntuple_minifile.cxx Outdated
auto tree = std::make_unique<TTree>("Events", "");
file->Write();
file->Close();
tree.release();
Copy link
Copy Markdown
Contributor

@jblomer jblomer Jun 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that the CI issues a warning here

@mxxo
[ntuple] Check TKey class name is RNTuple
a6da7fe 
Fixes issue where TKeys with the same name as the requested RNTuple
would be attempted to be parsed as an RNTuple, leading to internal
parser assert failures later on.
@mxxo mxxo force-pushed the key-class-name branch from 28ea81b to a6da7fe Compare June 2, 2021 14:21
@phsft-bot
Copy link
Copy Markdown

phsft-bot commented Jun 2, 2021

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

@jblomer jblomer merged commit 80d4d91 into root-project:master Jun 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jblomer jblomer jblomer approved these changes

@pcanal pcanal Awaiting requested review from pcanal

Assignees

@jblomer jblomer

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[ntuple] TKey with the same name as requested RNTuple causes internal RNTupleReader errors

3 participants

@mxxo @phsft-bot @jblomer