-
Notifications
You must be signed in to change notification settings - Fork 18
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
[RNTuple] Writing Phase 1 #343
Conversation
* Replace xrootdgo_jll by XRootD.jl
Release for XRootD.jl
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #343 +/- ##
==========================================
+ Coverage 84.24% 85.01% +0.76%
==========================================
Files 19 20 +1
Lines 2565 2950 +385
==========================================
+ Hits 2161 2508 +347
- Misses 404 442 +38 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Looking forward to the future of writing ;)
end | ||
end | ||
|
||
function rnt_write(io::IO, x::UnROOT.FileHeader32) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these legacy
flags something which should be propagated from the function call as keyword arguments?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in this case we know FileHeader32
will always be written out "legacy" (i.e. big endian) so we don't take keyword argument here.
Because it doesn't make sense to rnt_write()
a FileHeader32
with legacy=false
.
Although maybe this should be called bigendian=true|false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is difficult for me to review the technical changes. What surprises me is that the name "myntuple" for the created RNTuple is implicit.
I have tried to reproduce the C++ reading test but I failed because the ROOT.RDataFrame
is not yet provided by ROOT.jl I think.
Yeah that's something I still have to figure out. Basically changing that moves too many stuff at the moment so I haven't figured out what bytes to update etc. |
add Envelope, FieldRecord, ColumnRecord fix temp_io and RNTupleHeader without envelope id_length (8 bytes) and checksum (8 bytes)
finish out
phase 1 bump to 0.10.31 (#341) Release for XRootD.jl fix compat
add anchor checksum
e2be889
to
335e9f1
Compare
still works as of:
|
This implements Phase 1 as outline in #336
$ root --version ROOT Version: 6.33.01 Built for linuxx8664gcc on Jun 28 2024, 23:10:42 From heads/master@v6-31-01-2482-g50ec910e98