-
Notifications
You must be signed in to change notification settings - Fork 81
Rearrange bytes from split format to normal format for Unsigned Column types #357
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
Rearrange bytes from split format to normal format for Unsigned Column types #357
Conversation
modules/rntuple.mjs
Outdated
const byteSize = getTypeByteSize(coltype), | ||
splitView = new DataView(blob.buffer, blob.byteOffset, blob.byteLength), | ||
count = blob.byteLength / byteSize, | ||
fullBuffer = new ArrayBuffer(blob.byteLength), |
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.
Rather than fullBuffer
/fullBytes
I'd call them outBuffer
/outBytes
modules/rntuple.mjs
Outdated
for (let b = 0; b < byteSize; ++b) { | ||
const splitIndex = b * count + i, | ||
byte = splitView.getUint8(splitIndex), | ||
writeIndex = i * byteSize + (LITTLE_ENDIAN ? b : byteSize - 1 - b); |
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.
You can assume LITTLE_ENDIAN
is true (if that were not the case, several other things would break at the moment and there is no big endian platform I'm aware of where you would use a browser anyway)
modules/rntuple.mjs
Outdated
coltype === ENTupleColumnType.kSplitReal64 ? ENTupleColumnType.kReal64 : | ||
coltype === ENTupleColumnType.kSplitIndex64 ? ENTupleColumnType.kIndex64 : | ||
coltype === ENTupleColumnType.kSplitUInt32 ? ENTupleColumnType.kUInt32 : | ||
coltype; |
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.
You should assert that coltype
is one of those listed above; maybe use a switch
instead of a chain of ternary operators and have a throw
in the default
case
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.
Please check @silverweed the commit 2 I did all your suggested changes
modules/rntuple.mjs
Outdated
numValues = byteSize ? blob.byteLength / byteSize : undefined; | ||
reader = new RBufferReader(processedBlob), | ||
values = [], | ||
numValues = byteSize ? processedBlob.byteLength / byteSize : undefined; |
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.
I don't think it makes sense to continue running if byteSize
is not defined - you should throw rather than running the loop on processedBlob.byteLength
…g logic Suggested Changes done
In this commit I can successfuly deserialise

unsigned slip columns
In the below image you can see the test I ran for compressed
ntpl001_staff.root
file where I checked for flag what was unsigned integer split column which is successfully deserialised but for other fields (e.g Nation) which is not unsigned I get the the error as expected.