header checksum and Schema Extension frame added separately #341
Conversation
|
|
modules/rntuple.mjs
Outdated
| @@ -148,7 +149,7 @@ deserializeFooter(footer_blob) { | |||
| // Feature flag(32 bits) | |||
| this._readFeatureFlags(reader); | |||
| // Header checksum (64-bit xxhash3) | |||
| this.headerChecksum = reader.readU64(); | |||
| this.headerChecksumFromFooter = reader.readU64(); | |||
There was a problem hiding this comment.
The check on headerChecksumFromFooter === headerEnvelopeChecksum should happen here rather than in the test, as if it fails the RNTuple is corrupted and the reader should stop and report error.
There was a problem hiding this comment.
(also, this can be a local variable)
modules/rntuple.mjs
Outdated
| @@ -180,6 +178,14 @@ _readEnvelopeMetadata(reader) { | |||
| return { envelopeType, envelopeLength }; | |||
| } | |||
|
|
|||
| _readSchemaExtension(reader) { | |||
| this._readFieldDescriptors(reader); | |||
There was a problem hiding this comment.
Be careful: since in _readFieldDescriptors and _readColumnDescriptors you are doing this.fieldDescriptors = fieldDescriptors and similar, every time you call this function you will overwrite the previous ones.
You should either append to the existing array or return that array and handle it on the caller side.
Same goes for _readAliasColumns and _readExtraTypeInformation.
The Footer's schema description should be merged with the Header's to form the complete schema description (most of the time the Footer's schema extension will be empty btw).
modules/rntuple.mjs
Outdated
| @@ -180,6 +178,14 @@ _readEnvelopeMetadata(reader) { | |||
| return { envelopeType, envelopeLength }; | |||
| } | |||
|
|
|||
| _readSchemaExtension(reader) { | |||
There was a problem hiding this comment.
| _readSchemaExtension(reader) { | |
| _readSchemaDescription(reader) { |
|
@silverweed please again check the PR #341 I have done the suggested changes |
|
|
For that I have started implementation in a separate PR #342 ,kindly please check that pr and please provide help with its entry point |
|
@silverweed please merge this pr |
No description provided.