ARROW-590: Add integration tests for Union types

[icexelloss]

Overview

This PR changes Java UnionVector to match Arrow spec. Passing integration test with C++ after the change.

TODO List:

• Pass Java integration test
• Pass CPP integration test
• Generator for union in integration_test.py
• Benchmark Java UnionVector performance change

Benchmark:

Write:

           before       after
count   29.000000   29.000000
mean   540.896552  626.034483
std     17.186674   22.504734
min    507.000000  587.000000
25%    532.000000  611.000000
50%    542.000000  625.000000
75%    548.000000  638.000000
max    596.000000  682.000000

Read:

           before       after
count   29.000000   29.000000
mean    92.000000   90.689655
std      6.112049    6.188142
min     85.000000   85.000000
25%     90.000000   87.000000
50%     91.000000   89.000000
75%     92.000000   92.000000
max    119.000000  119.000000

[icexelloss]

cc @wesm

The current state of this PR is that it passes all java unit test and integration test. The rest of the work is in the todo list on the top. Can you help get CPP integration test pass with the two example file? (union.json, union_with_no_type_id.json).

[icexelloss]

@wesm I also added you as a collaborator so you can modify this branch.

[wesm]

Cool, thanks for looking at this. I will look at the C++ failure tomorrow with any luck

[wesm]

Having a look at this tomorrow (Friday). Today got taken up by JupyterCon

[icexelloss]

I added generator for union in integration_test.py

[wesm]

Great; I wasn't able to get to this yesterday, but I will as soon as I can

[wesm]

@icexelloss I fixed the C++ integration test. The C++ -> Java fails, and it appears that what Java is parsing from JSON is malformed. The length of the UnionVector appears to be zero. I stepped into the failure in a Java debugger and the binary data from the C++-generated Arrow file appears well-formed (has length 10 in the union.json case) but the JSON-parsed vector has length 0.

[wesm]

This should be similar to the code in struct?

[icexelloss]

Struct doesn't do any buffer truncating:
https://github.com/apache/arrow/blob/master/java/vector/src/main/java/org/apache/arrow/vector/complex/NullableMapVector.java#L94

However, I am not sure about the purpose of buffer truncating here originally (and in other vectors too), maybe @siddharthteotia can help explain?

[wesm]

Is this called bits in other vectors? If so it might be worth renaming them to be more clear across the board.

We should also discuss whether a Union needs to be nullable. I think it probably does for consistency cc @jacques-n

[icexelloss]

bits is used in: https://github.com/apache/arrow/blob/master/java/vector/src/main/codegen/templates/NullableValueVectors.java#L57
I can rename it, but is probably cleaner in a separate PR?

The existing UnionVector supports nullable interface already, I keep it unchanged. There are other restriction in the existing UnionVector impl - the child vector must be nullable vectors.

[wesm]

Gotcha. I presume in existing Java use (e.g. in Dremio) that their unions are non-nullable, and nullability is addressed in the children. In general I think we need the ability to be nullable at the parent union level

[icexelloss]

Yeah, actually the existing UnionVector are already nullable on the parent level, but it uses type[i] == 0 to check null.

It seems sub-optimal to keep both validity maps in parent vector and children vectors, but the existing UnionVector java interface already returns nullable children vectors so changing it would break compatibility.

[wesm]

@jacques-n @siddharthteotia @StevenMPhillips we're going to need some eyes on these issues -- we should try to make the Union implementation consistent with the Arrow format spec without breaking existing downstream applications (or their binary format). I think that's possible but we will have to be careful

[icexelloss]

Thanks @wesm. I will investigate the json reader issue and why it didn't get caught by https://github.com/apache/arrow/blob/master/java/vector/src/test/java/org/apache/arrow/vector/file/json/TestJSONFile.java#L93

[icexelloss]

@wesm, I fixed Java JsonReader and the integration test is passing for union with type id.

Union without type id is failing because of a small schema issue, the arrow file produced by C++ seems to have typeIds = [0, 1] even though the source json file has typeIds = []. Can you take a look? Otherwise it looks good.

[wesm]

The empty list of type ids is a bit of an eyesore. Why does that break the Java side? In my opinion, it should always be there.

[icexelloss]

I don't like the empty list either. I only added it because it's optional. How about we always have type id set? It makes things easier. I can remove the empty type id test case.

[icexelloss]

All subtasks done.

@siddharthteotia @StevenMPhillips @jacques-n

This PR changes Java UnionVector to match Arrow spec. Passing integration test with C++ after the change. Please take a look, thank you.

[wesm]

Awesome, thanks @icexelloss! Since unions are used in Dremio we'll have to wait to get some feedback on the changes to make sure this doesn't impact the downstream use

[jacques-n]

@icexelloss, can you please provide insight into what you're trying to solve here?

[wesm]

I believe this is about the format specification provides for fixed type code mapping, e.g.

type 0 -> 5
type 1 -> 10
type 2 -> 15

I think the original motivation was to be able to utilized fixed type codes for certain data types

In C++ you can see the arrow::UnionType constructor takes a vector of type codes https://github.com/apache/arrow/blob/master/cpp/src/arrow/type.h#L531

[icexelloss]

Wes is correct. Before the change the Java class assumes the type id to be the int value of the enum MinorType, which breaks interoperability with C++.

[jacques-n]

I haven't spent the time against understanding this yet but to clarify:

do you believe:

• the java vector has an invalid representation?
  or
• the java vector assumes a certain possible subset of cases?

The fact that the java user layer uses a specific type mapping (rather than supporting every type mapping) isn't in itself a violation of the spec. Or am I missing something?

[icexelloss]

The second. I believe the java vector assumes a certain possible subset of the cases -

specifically the java vector assumes the typeIds are the ordinal value of the enum MinorType. If the java class is consuming an union vector that happens to using ordinal value of enum MinorType for typeIds (for instance, a union vector produced by java) then it would work. Otherwise it would not work.

[icexelloss]

@jacques-n This is the code I am referring to that assumes type id is the value of the enum MinorType rather than typeIds in the schema.

[jacques-n]

Per my comment above, this isn't a violation of the spec as I read it, it simply doesn't support all variations of the spec.

[wesm]

The idea is that an implementation might send codes over the wire like

[5, 10, 15, 5, 10, 15]

and then the type codes tell you that 5 is the 1st type in the union, 10 the 2nd, etc.

It was my recollection during the early Arrow days that there was a desire for certain codes to have a fixed interpretation. e.g. "4" might always be "JSON_STRING" in some Arrow implementation

[jacques-n]

Yes, my perspective on arrow types in the java layer has been that one should collapse to leaf nodes (in general, I've always been against the more complex arbritrary approach the spec allows). In the situation we usually see, it doesn't make sense to ever have duplicate types within a union and we wanted to have a simplified path for this scenario.

For example:

v1_record = {
date
record_id
measure1
}

v2_record = {
date
record_id
measure2
}

I would suggest using this representation:

record = {
date
record_id
measure1
measure2
}

This typically works well because initial union values usually come from separate stream (e.g. files) and processing is cleaner if we don't have indirect for common fields (a frequent case).

I'll take a look at the changes. I want to understand if you're introducing indirection or not.

[wesm]

Having the additional type codes metadata does add some complexity in using unions. If we're going to make a change to the format, we should do it for 0.8.0 since we are already breaking the metadata

[emkornfield]

@icexelloss would you mind if I tried to take this over to update the PR and to make a proposal that keeps backwards compatibility with existing Java Union implementation? Also could you point me to the benchmark you ran, I'm having trouble finding it.

[wesm]

@emkornfield I think it's OK if you take over the project of reconciling the Union implementations, since this is one of the last remaining items we've wanted to tackle for a 1.0 release. I'm closing this PR for now