[JS] Error reading dictionary-encoded integration test files

[asfimport]

The JS implementation crashes when reading the dictionary test case from the integration tests.

To replicate, first generate the test files with java and cpp impls:

$ cd ${ARROW_HOME}/integration/
$ python -c 'from integration_test import generate_dictionary_case; generate_dictionary_case().write("dictionary.json")'
$ ../cpp/debug/debug/json-integration-test --integration --json=dictionary.json --arrow=dictionary-cpp.arrow --mode=JSON_TO_ARROW
$ java -cp ../java/tools/target/arrow-tools-0.8.0-SNAPSHOT-jar-with-dependencies.jar org.apache.arrow.tools.Integration -c JSON_TO_ARROW -a dictionary-java.arrow -j dictionary.json

Attempt to read the files with the JS impl:

$ cd ${ARROW_HOME}/js/
$ ./bin/arrow2csv.js -s dict1_0 -f ../integration/dictionary-{java,cpp}.arrow

Both files result in an error for me on a8f51858:
TypeError: Cannot read property 'buffer' of undefined

Reporter: Brian Hulette / @TheNeuralBit
Assignee: Paul Taylor / @trxcllnt

Related issues:

• [JS] JavaScript integration tests (blocks)
• [JS] File reader attempts to load the same dictionary batch more than once (incorporates)
• [Format/C++/Java] Remove VectorLayout metadata from Flatbuffers metadata (is related to)

Original Issue Attachments:

• dictionary.json
• dictionary-cpp.arrow
• dictionary-java.arrow

PRs and other links:

• GitHub Pull Request #1294

_{Note: This issue was originally created as ARROW-1693. Please see the migration documentation for further details.}

[asfimport]

Brian Hulette / @TheNeuralBit:
Pretty sure this is related to the vector layout representing the index vs. the dictionary data

[asfimport]

Wes McKinney / @wesm:
This was fixed in the C++ implementation in ARROW-1363 0ced74e

[asfimport]

Brian Hulette / @TheNeuralBit:
ARROW-1698 is only part of the problem. After fixing that bug, we can successfully read dictionary-java.arrow, but not dictionary-cpp.arrow. It looks like there's actually a difference in the schema between these two files: dictionary-java.arrow has 3 items in dict1_0's buffer layout (validity, offset, data - reflects the value type), but dictionary-cpp.arrow has 2 (validity, data - reflects the index type).

@wesm Is this a bug in the java writer?

[asfimport]

Wes McKinney / @wesm:
That's interesting. I'm surprised that the integration tests work – it must be that Java is not using the buffer layout information. Frankly, having the JS reader be so sensitive to the buffer layout is a source of brittleness. We do not use this data in C++, it is there for informational purposes.

Because the buffer structure of record batches will contain only the indexes, I think using the index buffer layout is the "right" answer, though nowhere are we encoding the buffer layout for the dictionary values. So this is a deficiency in the Arrow format that we may contemplate fixing.

[asfimport]

Paul Taylor / @trxcllnt:
@TheNeuralBit thanks I can dig into this too. FYI there's a workaround for Dictionary vectors written before ARROW-1363 was fixed here and here.

If the Java writer encodes a data buffer that isn't the offsets, that totally invalidates this assumption in the comment under line 108:

...if we're parsing an Arrow file written by a version of the library published before ARROW-1363 was fixed, the IntVector's data buffer will be null, and the offset buffer will be the actual data. If data is null, it's safe to assume the offset buffer is the data, because IntVectors don't have offsets.

[asfimport]

Brian Hulette / @TheNeuralBit:
[~paul.e.taylor] feel free to dig into this, I'm wrapping up the ARROW-1698 PR and then I probably won't get to it any more over the weekend.

[asfimport]

Paul Taylor / @trxcllnt:
@wesm digging into this now, yeah it looks like the the DictionaryBatch UTF8Vector field layout doesn't include the offsets buffer. Sounds like I should get those integration tests up and running.

I wanna offer some push back on your comment about brittleness though. Maybe I'm alone on this, but seems like a cross-platform ipc format should strictly enforce its own spec – anything less and end up with a bunch of maybe-compatible implementations, right?

[asfimport]

Wes McKinney / @wesm:
I think the idea of including the buffer layouts was hypothetically to permit an implementation to, say, omit the validity bitmap buffer without consequences. In practice, both the Java and C++ implementations are presuming to be sent the same buffer layout that the emit – i.e. with the buffers in same order (so in the case of strings, you would have validity, offsets, then data). But validating our presumptions is useful. So what we probably need to do is implement buffer layout validation in both Java and C++ so that we can assert that a sender has prepared the buffers in the way that are supported. I was wrong to call the JS implementation "brittle" in this regard, really it's that the more rigorous checking exposed bugs in the other implementations

[asfimport]

Wes McKinney / @wesm:
See ARROW-1362

[asfimport]

Jacques Nadeau / @jacques-n:
I actually think having the vector layout was a mistake. I think we should remove it. It is a constant that is defined by the spec. We actually implemented an alternative representation internally where we skip inclusion of it because we don't want to send around information that is useless (and can be fairly substantial when talking about five record, several thousand field datasets).

[asfimport]

Wes McKinney / @wesm:
Issue resolved by pull request 1294
#1294