[JAVA] Restructure ValueVector hierarchy to minimize compile-time generated code

[asfimport]

The templates used in the java package are very high mainteance and the if conditions are hard to track. As started in the discussion here: #1012, I'd like to propose that we modify the structure of the internal value vectors and code generation dynamics.

Create new abstract base vectors:

BaseFixedVector
BaseVariableVector
BaseNullableVector

For each of these, implement all the basic functionality of a vector without using templating.

Evaluate whether to use code generation to generate specific specializations of this functionality for each type where needed for performance purposes (probably constrained to mutator and accessor set/get methods). Giant and complex if conditions in the templates are actually worse from my perspective than a small amount of hand written duplicated code since templates are much harder to work with.

Reporter: Jacques Nadeau / @jacques-n
Assignee: Siddharth Teotia / @siddharthteotia

Subtasks:

• [JAVA] Document requirements and non/requirements for ValueVector updates
• [JAVA] Design updated ValueVector Object Hierarchy
• [JAVA] Create Prototype Code Hierarchy (Implementation Phase 1)
• [JAVA] ValueVector hierarchy (Implementation Phase 2)
• [JAVA] Create Benchmarking Suite for prototypes
• [JAVA] Implement final ValueVector updates
• [JAVA] Create Benchmarking Suite for final ValueVector updates
• [Java] Remove non-nullable vectors in new vector class hierarchy
• [Java] Remove public static helper method in vector classes for JSONReader/Writer
• [Java] Remove accessor/mutator
• [Java] Integration test breaks without zeroing out validity vectors
• [Java] Rename MapVector to StructVector
• [Java] Resolve new vector classes structure for timestamp, date and maybe interval
• [Java] Remove legacy vector classes
• [Java] Combine MapVector and NonNullableMapVector Classes
• [Java] Add BitVector APIs from old vector class
• [Java] Change vector getMinorType to use MinorType instead of Types.MinorType
• [Java] Restore previous MapVector class names

Related issues:

• [Integration] Add integration tests for Union types (is related to)
• [Java] Add support for FixedSizeBinary type (is related to)

PRs and other links:

• GitHub Pull Request apache/arrow#1164

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

[asfimport]

Wes McKinney / @wesm:
+1, my outside perspective looking at patches that touch the templates is that it seems a recipe for brittleness that makes refactoring difficult. It would make the codebase easier for newcomers, too

[asfimport]

Li Jin / @icexelloss:
+1, the complexity of those templates is bothering me as well.

[asfimport]

Bryan Cutler / @BryanCutler:
+1 on fixing this up also, however there are already quite a lot of base classes and interfaces. @jacques-n, following our other discussion from ARROW-1361, would it be possible to make FixedVector and VariableVector generic classes that take a ArrowType as type parameter and then specialize for types as needed? NullableValueVectors could remain a template, possibly simplified, or use a base class as mentioned above. I'm not entirely sure how feasible this is though..

[asfimport]

Jacques Nadeau / @jacques-n:
It seems like there a milestone here that we should figure out.

1. Come up with a set of goals (we can use this jira to describe potential places for improvement)
2. Propose an object hierarchy (I think there may be a jira that Julien wrote that covers this.)
3. Prototype a couple vectors to evaluate against goals (I suggest having maybe two people do this in parallel to see what different ideas people come up).
4. Implement the updated vectors.

I see @siddharthteotia assigned this to himself. I'd like to also make sure we have others involved in these subtasks and would like to break them out here. Other opinions on subtasks we should have? @BryanCutler and/or @icexelloss either of you interested in working on pieces of this? Definitely think it is overdue.

[asfimport]

Jacques Nadeau / @jacques-n:
Some initial thoughts on requirements:

• Existing consumers should have minimal changes to continue to leverage the FieldVectors.
• No performance loss on hot paths.
• Minimize or remove use of code templates except where very simple/easy to maintain.
• Rationalize object hiearchy
• Continue to make engines that use code generation on top of vectors have predictable interfaces.
• Better Javadocs on interfaces/base classes.

[asfimport]

Wes McKinney / @wesm:
I think @alphalfalfa may be interested in helping, since he has dug through the templates a couple of times for adding more logical types to the hierarchy. It'll be worthwhile to harden the object hierarchy before we accumulate a lot more downstream users and risk unpleasant disruption. Certainly this should take place before a 1.0.0 release. Feedback from other active users like @elahrvivaz will be helpful

[asfimport]

Gonzalo Ortiz / @gortiz:
+1

I have found that the current hierarchy and heavily use of delegation makes implementations like IntVector very slow compared to ByteBuffer. I have some JMH benchmarks where I iterate on a buffer, reading the content as ints to accumulate them. When using ByteBuffer it takes 17us to execute an iteration. With IntVector.Accessor it gets 100 us!

[asfimport]

Emilio Lahr-Vivaz / @elahrvivaz:
+1 agree with everything said so far. I'd prefer not to have any code generation at all, unless it provides some performance gains.

[asfimport]

Jingyuan Wang / @alphalfalfa:
I would certainly like to help. We should probably consider to add a benchmark suite to measure both the performance of assembling and retrieval of value vectors.

[asfimport]

Jacques Nadeau / @jacques-n:
I've created an initial list of subtasks. Who wants to work on what?

thx

[asfimport]

Li Jin / @icexelloss:
[~jacques@dremio.com] Thank much for putting this together. I will be out next week so I will avoid being on any critical path. But I'd like to check status and see what I can help when I get back.

[asfimport]

Wes McKinney / @wesm:
This might be a good opportunity to remove non-nullable container types. Nullability in C++ is a property of the metadata only, and computationally non-null arrays and nullable arrays with 0 null count are semantically equal. It would make the code simpler to maintain

[asfimport]

Gonzalo Ortiz / @gortiz:
On the one hand, what @wesm is right. They could be the same structure. On the other, they are not exactly semantically equal. Non nullable structures doesn't return nulls, which can be used by some static checkers or languages like Kotlin.

There are also two performance issues, one on each model. They are just a theoretical reasoning (that may be wrong!), we would need to verify that some JMH test (and it can change from one JVM to another):

• A structure that doesn't need to check the nullable array would have shorter and simpler methods (without branches) and therefore it is more probable that the JVM can optimize that. So this is a point on having two implementations: one with nulls and one without them.
• If there are two structures (a nullable and a non nullable) and the nullable is just a decoration/delegation on the non-nullable, nullable structures may behave quite worst than we expect for the same reason. As soon as the methods get complex (calling the delegate class) the JVM may or may not optimize them.

TL;RD: From the library developer perspective, I would like to have only one implementation. From a user perspective, I would like to have both implementations, ideally without delegation (which means repeated code on nullable-structure)

[asfimport]

Wes McKinney / @wesm:
@gortiz this is probably a discussion for a whole separate JIRA. A couple points:

Non nullable structures doesn't return nulls, which can be used by some static checkers or languages like Kotlin.

This is true, hypothetically, but I question whether this benefit can only be reaped by doubling the size of the type hierarchy. In the instances where you can choose an optimized code path for non-null data, you could elect to invoke that same code path when the null count is zero (which you will want to do regardless of the nullability). This is sort of a "composition over inheritance" argument – it's not to say that we can have some helper interfaces that specialize based on metadata, but as a property of the type hierarchy I am not sure if it's worth the increased code size.

So this is a point on having two implementations: one with nulls and one without them.

Since you can branch at the vector level, the performance implications are less obvious (benchmarks could help settle it). Somewhere you must dynamic dispatch to the correct kernel function for some analytical operation, and the dispatch could be based on null count.

[if] the nullable is just a decoration/delegation on the non-nullable, nullable structures may behave quite worst than we expect for the same reason

The performance issues you cite would appear mostly in non-vectorized code. If you're doing something performance sensitive, you're more likely to be working directly with the buffers, and selecting kernel functions based on the dynamic metadata (which can include nullability / null count). So if you implement Sum, you will want to have a sum with nulls and a sum without. Somewhere you have to decide which one to call. You will want to elect to call the SumWithoutNulls when you have a nullable vector but with zero null count anyway, so this actually makes your kernel dispatch more complex.

[asfimport]

Gonzalo Ortiz / @gortiz:
From a Java developer perspective, I can only implement a SumWithIntVector and prey to the JIT lord, hoping that the JVM discovers it can use vectorization... and JVM 8 it is not very good at it. But you are right. This sounds a little bit offtopic.