ARROW-3336: [JS] Fix IPC writer serializing sliced arrays by trxcllnt · Pull Request #2638 · apache/arrow

trxcllnt · 2018-09-26T23:20:32Z

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).

trxcllnt · 2018-09-27T00:03:24Z

looks like JS is passing, but the R build has failed

trxcllnt · 2018-09-27T00:34:13Z

@TheNeuralBit also commit 49c601c makes the Table constructor arg handling significantly less confusing

… sliced

…ble-width vectors, as all other buffers have already been adjusted as part of the original call to Vector#slice()

wesm · 2018-09-27T09:49:36Z

Just rebased this after merging #2616, #2635

wesm · 2018-09-27T10:42:25Z

js/src/ipc/writer/binary.ts

+            this.addBuffer(this.getZeroBasedValueOffsets(0, length, valueOffsets));
        }
        // Then insert the List's values child
        return this.visit((vector as any as ListVector<T>).getChildAt(0)!);


What went wrong here, exactly? Is the offset being handled elsewhere?

I think this offset was intended to handle the buffers for vectors that had been sliced, but that function is already slicing all of the vector's child buffers here, so this is redundant. Note that subarray is zero-copy, it's effectively managing the offset for us.

@wesm Brian is right, most of the internal buffers are zero-copy sliced up front in the ArrayData internals. This is mostly a performance optimization/complexity reduction, since it allows us to index into those TypedArrays starting from zero in most cases. Notable exceptions are when the indexing is dependent on other buffers (e.g. we do slice the valueOffsets buffer of a Utf8Vector, but not the data buffer), or when multiple values are packed into < 1 byte like the validity bitmap or BoolVector's data buffer. So we still need to track the logical offset internally, but only use it in those special cases.

@wesm if you want to see a simplified version of the internal buffer management/slicing that's going on here, I have one here in my unfinished js-data-refactor branch that I'm reeeally looking forward to getting time to finish soon.

Sounds good, looking forward to getting that in :)

Yeah, especially since it lets us take full advantage of TypeScript 3's more advanced type inference:

TheNeuralBit

Couple of minor comments, but this looks good. Thanks for adding tests to cover the original problem :)

TheNeuralBit · 2018-09-27T14:00:20Z

js/test/unit/writer-tests.ts

+        (expect([v1, `batch`, name]) as any).toEqualVector([v0, `source`]);
+        (expect([v2, `table`, name]) as any).toEqualVector([v0, `source`]);
+    }
+}


What do you think about pulling some of this logic out into a toEqualTable function? I think that would be generally useful, especially if we want to start doing some round-trip tests now that we have a writer.

Yeah totally, we should add more helpers to the jest-extensions file. We can also clean up the tests to take advantage of TS's new type X = import('some-module').X feature, to import just type definitions and not the full implementation. I think that would simplify the dynamic import logic in test/Arrow.ts and hopefully make it impossible to accidentally do this.

TheNeuralBit · 2018-09-27T14:12:25Z

js/src/ipc/writer/binary.ts

        // If we have valueOffsets (ListVector), push that buffer first
        if (valueOffsets) {
-            this.addBuffer(this.getZeroBasedValueOffsets(offset, length, valueOffsets));
+            this.addBuffer(this.getZeroBasedValueOffsets(0, length, valueOffsets));


It looks like getZeroBasedValueOffsets is always called with a hard-coded 0 now, which makes it a no-op. Should we just get rid of it?

@TheNeuralBit you're right that we subarray the valueOffsets buffer when we slice, but we still need to call getZeroBasedValueOffsets() in all these places because it also does work in the case that valueOffsets[0] != 0. That will be true if we performed any meaningful valueOffsets.subarray() slice in the data's sliceInternal() method, in which case we need to adjust all the values in the valueOffsets Array on the way out.

TheNeuralBit · 2018-09-27T14:16:59Z

js/src/ipc/writer/binary.ts

+            this.addBuffer(this.getZeroBasedValueOffsets(0, length, valueOffsets));
        }
        // Then insert the List's values child
        return this.visit((vector as any as ListVector<T>).getChildAt(0)!);


I think this offset was intended to handle the buffers for vectors that had been sliced, but that function is already slicing all of the vector's child buffers here, so this is redundant. Note that subarray is zero-copy, it's effectively managing the offset for us.

wesm · 2018-09-27T16:50:40Z

No more comments from me, let me know if you want me to merge this or anything else

trxcllnt · 2018-09-27T17:16:38Z

@wesm I'm happy to do the merge also, if for no other reason than to get my PR merge stats up :-)

wesm · 2018-09-27T17:21:34Z

OK, I'll leave it to you both

I just noticed this commit wasn't included in #2638 :-[. My bad, here's the rest of the fix for ARROW-3336 Author: ptaylor <paul.e.taylor@me.com> Closes #2742 from trxcllnt/js-fix-writing-sliced-arrays and squashes the following commits: 3ab9da0 <ptaylor> fix serializing sliced Utf8 vectors

I just noticed this commit wasn't included in apache/arrow#2638 :-[. My bad, here's the rest of the fix for ARROW-3336 Author: ptaylor <paul.e.taylor@me.com> Closes #2742 from trxcllnt/js-fix-writing-sliced-arrays and squashes the following commits: 3ab9da080 <ptaylor> fix serializing sliced Utf8 vectors

trxcllnt requested a review from TheNeuralBit September 26, 2018 23:20

trxcllnt added 6 commits September 27, 2018 05:48

fix table constructor args

380d655

move vector comparison extension into separate module

f9dd91b

add test validating writer serializes sliced arrays correctly

e093f7c

reset internal children Array in case the childData for each has been…

5ece080

… sliced

only use vector offset to serialize bitmaps and data buffers of varia…

fa4d8f5

…ble-width vectors, as all other buffers have already been adjusted as part of the original call to Vector#slice()

fix writer-tests.ts import

bcd58ca

wesm force-pushed the js-fix-writing-sliced-arrays branch from e473e73 to bcd58ca Compare September 27, 2018 09:49

wesm reviewed Sep 27, 2018

View reviewed changes

TheNeuralBit reviewed Sep 27, 2018

View reviewed changes

trxcllnt closed this in b796b57 Sep 27, 2018

trxcllnt mentioned this pull request Oct 10, 2018

ARROW-3336: [JS] Fix IPC writer serializing sliced Utf8Vectors #2742

Closed

Conversation

trxcllnt commented Sep 26, 2018

Uh oh!

trxcllnt commented Sep 27, 2018

Uh oh!

trxcllnt commented Sep 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wesm commented Sep 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheNeuralBit left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trxcllnt Sep 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wesm commented Sep 27, 2018

Uh oh!

trxcllnt commented Sep 27, 2018

Uh oh!

wesm commented Sep 27, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

trxcllnt commented Sep 27, 2018 •

edited

Loading

trxcllnt Sep 27, 2018 •

edited

Loading