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

[ntuple] Show all fields unless a model is specified #12026

Merged
merged 4 commits into from Jan 20, 2023
Merged

[ntuple] Show all fields unless a model is specified #12026

merged 4 commits into from Jan 20, 2023

Conversation

enirolf
Copy link
Contributor

@enirolf enirolf commented Jan 13, 2023 •
edited by jalopezg-git

This PR introduces a new format option for RNTupleReader::Show() that shows all fields if no model is specified, and only the fields present in the model if there is one. It also changes the default show format to this option.

Checklist:

  • tested changes locally
  • updated the docs (if necessary)

Fixes #12031.

@phsft-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@enirolf
Copy link
Contributor Author

enirolf commented Jan 13, 2023

I realize this commit also includes some changes to unrelated parts of the code made by clang-format. Let me know if I should revert those/put them in a separate commit.

@enirolf enirolf marked this pull request as ready for review January 13, 2023 14:57
@enirolf enirolf requested a review from jblomer as a code owner January 13, 2023 14:57
@bellenot
Copy link
Member

@phsft-bot build

@phsft-bot
Copy link
Collaborator

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac12/noimt, mac11/cxx14, windows10/cxx14
How to customize builds

@phsft-bot
Copy link
Collaborator

Build failed on mac12/noimt.
Running on macphsft17.dyndns.cern.ch:/Users/sftnight/build/jenkins/workspace/root-pullrequests-build
See console output.

Warnings:

  • [2023-01-13T16:00:22.594Z] /Users/sftnight/build/jenkins/workspace/root-pullrequests-build/root/core/base/src/TDirectory.cxx:1245:7: warning: 'sprintf' is deprecated: This function is provided for compatibility reasons only. Due to security concerns inherent in the design of sprintf(3), it is highly recommended that you use snprintf(3) instead. [-Wdeprecated-declarations]

Failing tests:

And 63 more

@enirolf enirolf mentioned this pull request Jan 16, 2023
Closed
1 task
@jalopezg-git jalopezg-git self-requested a review January 16, 2023 19:38
jalopezg-git
jalopezg-git reviewed Jan 16, 2023
View reviewed changes
Copy link
Collaborator

@jalopezg-git jalopezg-git left a comment

Choose a reason for hiding this comment

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

I think it should be enough if, instead of defining a new option for Show(), we adjust the current behaviour of kCurrentModelJSON to trigger model generation from the ntuple descriptor if there’s no current model.

Rationale: this is the current behaviour of LoadEntry(); why should Show() behave differently? What do you think, @jblomer?

@enirolf
Copy link
Contributor Author

enirolf commented Jan 17, 2023

I think it should be enough if, instead of defining a new option for Show(), we adjust the current behaviour of kCurrentModelJSON to trigger model generation from the ntuple descriptor if there’s no current model.

Jakob and I had a quick chat yesterday and the conclusion was that this would indeed make the most sense. I thought initially that this meant we could get rid of the format option altogether, but now I think it might be good to keep the kCurrentModelJSON option in a model is present but the user would still like to output all fields.

jblomer
jblomer requested changes Jan 18, 2023
View reviewed changes
Copy link
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.

Very nice!

I think in the ntuple_show unit test, we should suppress the clang-format changes for the expected strings (// clang-format off). Otherwise it will be difficult to adjust the strings in the future.

@enirolf enirolf changed the title [ntuple] Add option to show all fields unless a model is specified [ntuple] Show all fields unless a model is specified Jan 18, 2023
jalopezg-git
jalopezg-git approved these changes Jan 20, 2023
View reviewed changes
Copy link
Collaborator

@jalopezg-git jalopezg-git left a comment •
edited

Choose a reason for hiding this comment

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

LGTM! @jblomer? Please, consider also squashing on merge :-).

@jalopezg-git
Copy link
Collaborator

@phsft-bot build

@phsft-bot
Copy link
Collaborator

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac12/noimt, mac11/cxx14, windows10/cxx14
How to customize builds

@phsft-bot
Copy link
Collaborator

Build failed on mac12/noimt.
Running on macphsft17.dyndns.cern.ch:/Users/sftnight/build/jenkins/workspace/root-pullrequests-build
See console output.

Warnings:

  • [2023-01-20T13:01:30.895Z] /Users/sftnight/build/jenkins/workspace/root-pullrequests-build/root/core/base/src/TDirectory.cxx:1245:7: warning: 'sprintf' is deprecated: This function is provided for compatibility reasons only. Due to security concerns inherent in the design of sprintf(3), it is highly recommended that you use snprintf(3) instead. [-Wdeprecated-declarations]

Failing tests:

And 58 more

@jblomer jblomer merged commit ff36bde into root-project:master Jan 20, 2023
@phsft-bot
Copy link
Collaborator

Build failed on mac11/cxx14.
Running on macphsft20.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

@phsft-bot
Copy link
Collaborator

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac12/noimt, mac11/cxx14, windows10/cxx14
How to customize builds

@phsft-bot
Copy link
Collaborator

Build failed on ROOT-ubuntu18.04/nortcxxmod.
Running on sft-ubuntu-1804-2.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Errors:

  • [2023-01-20T21:06:10.126Z] stderr: error: Failed to merge in the changes.
  • [2023-01-20T21:06:16.798Z] CMake Error at /mnt/build/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1138 (message):

@phsft-bot
Copy link
Collaborator

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

Errors:

  • [2023-01-20T21:07:40.858Z] stderr: error: Failed to merge in the changes.
  • [2023-01-20T21:07:46.300Z] CMake Error at /home/sftnight/build/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1138 (message):

@phsft-bot
Copy link
Collaborator

Build failed on mac12/noimt.
Running on macphsft17.dyndns.cern.ch:/Users/sftnight/build/jenkins/workspace/root-pullrequests-build
See console output.

Errors:

  • [2023-01-20T21:07:50.610Z] CMake Error at /Users/sftnight/build/jenkins/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1138 (message):

@phsft-bot
Copy link
Collaborator

Build failed on mac11/cxx14.
See console output.

@enirolf enirolf deleted the ntuple-reader-show branch February 23, 2023 08:04
omazapa pushed a commit to omazapa/root that referenced this pull request Apr 13, 2023
@enirolf @omazapa
[ntuple] Show all fields unless a model is specified (root-project#12026
2824936 
)

* [ntuple] Add option to show all fields unless a model is specified

* [ntuple] Generate model unless already present for kCurrentModelJSON

* [ntuple] Don't clang-format expected output

* [ntuple] Remove unnecessary model generation trigger
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

@jalopezg-git jalopezg-git jalopezg-git approved these changes

Assignees

@jblomer jblomer

Labels
in:RNTuple
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[ntuple] RNTupleReader::Show() produces empty output if user does not impose a model
5 participants
@enirolf @phsft-bot @bellenot @jalopezg-git @jblomer