Skip to content

[ntuple] Fields tree as dot #19319

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.

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

Merged
merged 1 commit into from
Jul 15, 2025

Conversation

magnustymoteus
Copy link
Contributor

This Pull request:

Changes or fixes:

This PR introduces the method RNTupleInspector::GetFieldsTreeAsDot which gives a string that represents the tree structure of RNTuple fields in .dot language (that can then be displayed in graphviz for example)

Checklist:

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

@magnustymoteus magnustymoteus requested a review from jblomer as a code owner July 9, 2025 13:12
@silverweed silverweed requested review from silverweed and enirolf July 9, 2025 13:22
@silverweed silverweed self-assigned this Jul 9, 2025
Copy link
Contributor

@silverweed silverweed 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, thanks! I left a couple of comments.
Also, do you have maybe a screenshot of an example rendered output?

@magnustymoteus
Copy link
Contributor Author

This is an example of graphviz's rendered output (where Placeholder is a user-defined class containing an integer and a float)
graphviz output

This is obviously very rudimentary right now, but I can work on expanding this today.

Copy link

github-actions bot commented Jul 9, 2025

Test Results

    21 files      21 suites   3d 16h 36m 52s ⏱️
 3 203 tests  3 198 ✅ 0 💤  5 ❌
65 594 runs  65 584 ✅ 0 💤 10 ❌

For more details on these failures, see this check.

Results for commit 5294d78.

♻️ This comment has been updated with latest results.

@magnustymoteus
Copy link
Contributor Author

This is the new rendered output with the same example:

graphviz2

Additionally, I removed the (tabular) styling to keep it simple.

Copy link
Contributor

@silverweed silverweed left a comment

Choose a reason for hiding this comment

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

Looks good! Just a couple more things before we merge

@silverweed silverweed changed the title Fields tree as dot [ntuple] Fields tree as dot Jul 11, 2025
@silverweed silverweed requested review from hahnjo and removed request for pcanal, bellenot and dpiparo July 11, 2025 09:11
Copy link
Contributor

@silverweed silverweed left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Copy link
Member

@vepadulano vepadulano 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 work! Can I suggest to clean the commit history before merging? That would include squashing formatting/small fixes commits together in one (or a few) main commits and using our convention for the commit message e.g.

[ntuple] VERB_IMPERATIVE_FORM description_of_commit_objective

@magnustymoteus magnustymoteus force-pushed the fields-tree-as-dot branch 2 times, most recently from d8f4631 to 66eb4d0 Compare July 11, 2025 16:45
Copy link
Contributor

@enirolf enirolf 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 work! I have some small nitpicks to address before merging, but after that it's good to go from my side :).

@magnustymoteus
Copy link
Contributor Author

The requested changes should be implemented, I'll squash the commits now.

Copy link
Contributor

@enirolf enirolf left a comment

Choose a reason for hiding this comment

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

Thank you for the changes, LGTM!

@silverweed silverweed merged commit 555c195 into root-project:master Jul 15, 2025
23 of 26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants