ARROW-3256,3304: [JS] fix file footer inconsistency, yield all messages from the stream reader #2616
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…tables from the same source stream
trxcllnt
changed the title
trxcllnt
changed the title
wesm
reviewed
Sep 26, 2018
| @@ -53,7 +52,7 @@ export class RecordBatchMetadata extends Message { | |||
| public buffers: BufferMetadata[]; | |||
| constructor(version: MetadataVersion, length: Long | number, nodes: FieldMetadata[], buffers: BufferMetadata[], bodyLength?: Long | number) { | |||
| if (bodyLength === void(0)) { | |||
| bodyLength = buffers.reduce((s, b) => align(s + b.length + (b.offset - s), 8), 0); | |||
| bodyLength = buffers.reduce((bodyLength, buffer) => bodyLength + buffer.length, 0); | |||
There was a problem hiding this comment.
Are your buffer lengths here guaranteed to a multiple of 8? Otherwise should round up
There was a problem hiding this comment.
Yep, this size is rounded up in the writer here
|
Cool, @TheNeuralBit does this look good? |
TheNeuralBit
changed the title
|
👍 LGTM |
trxcllnt
added a commit
that referenced
this pull request
Sep 27, 2018
Fixes https://issues.apache.org/jira/browse/ARROW-3336. Realized too late I branched from the #2616, so that's why there's some extra commits at the front. Check out the relevant tests here: https://github.com/trxcllnt/arrow/blob/860f61046fca0081dbe0ce986a97408ed5934e22/js/test/unit/writer-tests.ts#L26. This test covers the primitive, nested, and dictionary vectors, but it'd be worth adding more (for example, Unions). Author: ptaylor <paul.e.taylor@me.com> Closes #2638 from trxcllnt/js-fix-writing-sliced-arrays and squashes the following commits: bcd58ca <ptaylor> fix writer-tests.ts import fa4d8f5 <ptaylor> only use vector offset to serialize bitmaps and data buffers of variable-width vectors, as all other buffers have already been adjusted as part of the original call to Vector#slice() 5ece080 <ptaylor> reset internal children Array in case the childData for each has been sliced e093f7c <ptaylor> add test validating writer serializes sliced arrays correctly f9dd91b <ptaylor> move vector comparison extension into separate module 380d655 <ptaylor> fix table constructor args
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Yield all messages from the stream reader to support reading multiple tables from the same source stream. This is something a stream-reader refactor should support out of the box, but this PR adds this ability to master until that refactor is done. I added an integration test that shows what such a reader would look like today.