ARROW-363: [Java/C++] integration testing harness, initial integration tests

[wesm]

This also includes format reconciliation as discussed in ARROW-384.

[wesm]

@julienledem alright, I found a couple initial issues in the file implementation.

1. In C++, we're writing the record batch metadata after the buffers have been written to the file. In Java the metadata is written first https://github.com/apache/arrow/blob/master/java/vector/src/main/java/org/apache/arrow/vector/file/ArrowWriter.java#L104. This is related to the next point:

2. We've been writing the serialized IPC metadata size at the after the flatbuffer bytes: https://github.com/apache/arrow/blob/master/cpp/src/arrow/ipc/metadata-internal.cc#L320 . One benefit of this in an IPC context is that if you have a shared memory offset to the end of the record batch, and the metadata size is the last 4 bytes of the blob, then you can read the metadata, then the record batch.

so here is what things look like right now on the C++ side:

<buffers> <metadata> <metadata size: int32>

and on the java side:

<metadata> <buffers>

In Java, the metadata size is encoded in the Block component in the footer: https://github.com/apache/arrow/blob/master/format/File.fbs#L36

There's a couple ways forward

• Option A: Java adapts what is currently being done in C++
• Option B: C++ drops the metadata length, conforms to the current Java layout, then presumes that in a shared memory IPC setting that there is some Block metadata obtained (containing file/shared memory offset, metadata size, and body size) from some other place

The reason we have things as they are right now was so that you could reconstruct a record batch from shared memory having only the offset to the end of the payload. Since we don't have a rigid model for handling IPC metadata (i.e. the Block struct used in the file footer) I don't feel very strongly about this; I would be fine doing what Java is doing

[wesm]

cc @emkornfield

[wesm]

@julienledem another thing is the record batch metadata itself

• In the Java implementation, the buffer offsets are relative to the start of the record batch buffer payload:

  arrow/java/vector/src/main/java/org/apache/arrow/vector/schema/ArrowRecordBatch.java

  Line 64 in 6996c17

  arrowBuffers.add(new ArrowBuffer(0, offset, size));

• In the C++ implementation, the buffer offsets are absolute offsets into the output file / shared memory (

  arrow/cpp/src/arrow/ipc/adapter.cc

  Line 170 in ed6ec3b

  buffer_meta_.push_back(flatbuf::Buffer(0, position, size + padding));

  )

I believe this is one of the reasons why the metadata is being written after the buffers (since we need to know the absolute positions of everything prior to writing out the flatbuffer)

The problem with absolute offsets is that the record batches are not relocatable (the metadata would have to be rewritten). This seems like a flaw -- I'm going to change the C++ to use relative offsets, and make the record batch layout conform to the Java implementation as it is right now so we can proceed with the integration tests

[julienledem]

@wesm sounds good.
I have created a jira to discuss further if needed.
I don't see why we couldn't add the metadata size in there. Thank you
https://issues.apache.org/jira/browse/ARROW-384

[wesm]

@julienledem I refactored to account for our discussion in ARROW-384. I left a bunch of notes in the format/ documents in this PR, could you take a look when you can? One item is padding / alignment generally. I have 64-byte alignment in there now, but I think 8 byte alignment is sufficient for a minimal format (for SIMD, alignment is marginally important, but things have to be padded at least).

[wesm]

@julienledem we'll need to make the corresponding changes in the Java format (adding the metadata length prefix to record batches in the file) -- how do you want to proceed?

[julienledem]

This LGTM.
Do we want to increment MetadataVersion since the change to the file format is incompatible?
like V0_2SNAPSHOT?

[wesm]

Sure, sounds good

[wesm]

Finally I have got a passing minimal integration test:

$ python integration_test.py
-- Java producing, C++ consuming
Testing with /home/wesm/code/arrow/integration/data/simple.json
-- C++ producing, Java consuming
Testing with /home/wesm/code/arrow/integration/data/simple.json

I suggest merging this and proceeding to write more integration tests in a new patch.

[wesm]

@julienledem this include some Java changes, if you wouldn't mind taking a look. thanks!

[wesm]

Just incremented the metadata version. I'm just using V1 and V2 for now to avoid having to do code changes at release time.

[wesm]

I added a broken string data test case. It fails in C++ with:

Breakpoint 1, arrow::io::BufferReader::Seek (this=0x7fffffff67d0, position=112)
    at /home/wesm/code/arrow/cpp/src/arrow/io/memory.cc:310
310	    return Status::IOError("position out of bounds");
(gdb) l
305	  return Status::OK();
306	}
307	
308	Status BufferReader::Seek(int64_t position) {
309	  if (position < 0 || position >= size_) {
310	    return Status::IOError("position out of bounds");
311	  }
312	
313	  position_ = position;
314	  return Status::OK();
(gdb) where
#0  arrow::io::BufferReader::Seek (this=0x7fffffff67d0, position=112) at /home/wesm/code/arrow/cpp/src/arrow/io/memory.cc:310
#1  0x00007ffff7bc8413 in non-virtual thunk to arrow::io::BufferReader::Seek(long) ()
    at /home/wesm/code/arrow/cpp/src/arrow/io/memory.cc:308
#2  0x00007ffff7bc6956 in arrow::io::ReadableFileInterface::ReadAt (this=0x7fffffff67d0, position=112, nbytes=0, 
    out=0x7fffffff64c0) at /home/wesm/code/arrow/cpp/src/arrow/io/interfaces.cc:43
#3  0x00007ffff791e6a8 in arrow::ipc::RecordBatchReader::GetBuffer (this=0x7fffffff66a8, buffer_index=6, out=0x7fffffff64c0)
    at /home/wesm/code/arrow/cpp/src/arrow/ipc/adapter.cc:405
#4  0x00007ffff791d251 in arrow::ipc::RecordBatchReader::NextArray (this=0x7fffffff66a8, field=0x707d20, 
    max_recursion_depth=64, out=0x708070) at /home/wesm/code/arrow/cpp/src/arrow/ipc/adapter.cc:359
#5  0x00007ffff7919ede in arrow::ipc::RecordBatchReader::Read (this=0x7fffffff66a8, out=0x7fffffff6b98)
    at /home/wesm/code/arrow/cpp/src/arrow/ipc/adapter.cc:310
#6  0x00007ffff7918382 in arrow::ipc::ReadRecordBatch (metadata=..., schema=..., max_recursion_depth=64, file=0x7fffffff67d0, 
    out=0x7fffffff6b98) at /home/wesm/code/arrow/cpp/src/arrow/ipc/adapter.cc:448
#7  0x00007ffff7918315 in arrow::ipc::ReadRecordBatch (metadata=..., schema=..., file=0x7fffffff67d0, out=0x7fffffff6b98)
    at /home/wesm/code/arrow/cpp/src/arrow/ipc/adapter.cc:441
#8  0x00007ffff792a195 in arrow::ipc::FileReader::GetRecordBatch (this=0x6f65f0, i=0, batch=0x7fffffff6b98)
    at /home/wesm/code/arrow/cpp/src/arrow/ipc/file.cc:213
#9  0x0000000000436c30 in arrow::ValidateArrowVsJson (arrow_path=..., json_path=...)
    at /home/wesm/code/arrow/cpp/src/arrow/ipc/json-integration-test.cc:169
#10 0x0000000000434d67 in arrow::RunCommand (json_path=..., arrow_path=..., command=...)
    at /home/wesm/code/arrow/cpp/src/arrow/ipc/json-integration-test.cc:208
#11 0x0000000000438ceb in main (argc=1, argv=0x7fffffff7378)
    at /home/wesm/code/arrow/cpp/src/arrow/ipc/json-integration-test.cc:376
(gdb) q

Let's address this and other data type tests in a follow up JIRA

[wesm]

+1, we can continue to iterate on fine details while writing more tests