ARROW-6352: [Java] Add implementation of DenseUnionVector #5473

liyafan82 · 2019-09-23T10:56:44Z

Today only Sparse unions are supported. We should have a dense union implementation vector that conforms to the IPC protocol (the current sparse union vector doesn't do this and there are other JIRAs covering making it compatible).

emkornfield

This change seems to propagate the fact that there can only be one Vector of a given minor type within the union. This isn't according to the spec, I would prefer to see something that might not be as efficient but implements the specification cleanly, instead of copying the same template pattern that SparseUnion uses.

emkornfield · 2019-09-24T04:12:30Z

java/vector/src/main/codegen/templates/DenseUnionVector.java

+      list.add(offsetBuffer);
+      list.addAll(java.util.Arrays.asList(internalStruct.getBuffers(clear)));
+    }
+    if (clear) {


can this call clear instead?

yes, I think so. Calling clear/close is essentially the same as calling release, which decrements the refcount by 1.

emkornfield · 2019-09-24T04:14:07Z

java/vector/src/main/codegen/templates/DenseUnionVector.java

+ * each time the vector is accessed.
+ * Source code generated using FreeMarker template ${.template_name}
+ */
+public class DenseUnionVector extends UnionVector {


I don't think this relationship should necessarily hold without a larger refactoring.

Sure. The implementations are separated now.

liyafan82 · 2019-09-24T04:32:33Z

This change seems to propagate the fact that there can only be one Vector of a given minor type within the union. This isn't according to the spec, I would prefer to see something that might not be as efficient but implements the specification cleanly, instead of copying the same template pattern that SparseUnion uses.

Thanks for your comments. You mean DenseUnionVector and SparseUnionVector should have separate minor type, and separate implementation, right?

emkornfield · 2019-09-24T04:36:45Z

Thanks for your comments. You mean DenseUnionVector and SparseUnionVector should have separate minor type, and separate implementation, right?

Separate implemention yes. Seperate minor type I'm not sure about I need to refresh myself on the implications of having two minor types corresponding to the same parameterized IPC specification type.

liyafan82 · 2019-09-25T03:16:59Z

Thanks for your comments. You mean DenseUnionVector and SparseUnionVector should have separate minor type, and separate implementation, right?

Separate implemention yes. Seperate minor type I'm not sure about I need to refresh myself on the implications of having two minor types corresponding to the same parameterized IPC specification type.

Thanks for your feedback. I have separated the implementations for UnionVector and DenseUnionVector. In addition, the implementations for reader/writer are also separate. However, implementations for list/struct reader/writer are shared. Please take a look.

emkornfield · 2019-10-04T21:38:24Z

java/vector/src/main/codegen/templates/DenseUnionReader.java

+@SuppressWarnings("unused")
+public class DenseUnionReader extends AbstractFieldReader {
+
+  private BaseReader[] readers = new BaseReader[43];


This looks like it is propagating the incompatibity with the IPC specification. Specifically a union should be able to have more then one vector for each minor type. is this the case?

Are you asking this question: UnionVector and DenseUnionVector are sharing the same minor type? If so, the answer is yes for now.
Do you think we should provide separate minor types for them?

No I'm asking if there can be two different vectors of the same type in the union (e.g. two different struct vectors).

I see. Thanks for the clarification.

You are right. This implementation does not support more than one vector for each minor type.
This is aligning with the current (Sparse) UnionVector.

Should we resolve this problem?

IMO densevector shouldn't propagate the same problems the existing UnionVector has with compatibility.

Thanks a lot for your feedback. I will resolve it later.

@emkornfield I have updated the code to allow arbitrary relative types for the desne union vector. Please take a look.

Brief change list:

For StructVector and ListVector, a custom type id is registered when they are added, if the same relative type has never occured.

the type id of the custom relative types start from MinorType.values().length, and the maximum value is 128.

some additional data structures are added to track the existing custom relative types, struct vector, and list vectors.

jacques-n

I would like to see this refactored so we do base classes of the vector and reader as non-templated code and then the implementation adds the templated code (or vice versa). This is to minimize the use of java code in freemarker templates. We have tried to eliminate that in a bunch of places. We haven't refactored in the readers/writers but we should avoid introducing new debt on this front.

emkornfield · 2020-01-10T09:11:23Z

java/vector/src/main/codegen/templates/DenseUnionVector.java

+      </#list>
+    </#list>
+
+  public void setType(int index, int typeId) {


typeIds should probably be "byte" everywhere.

Revised accordingly. Thanks.

emkornfield · 2020-01-10T09:13:00Z

java/vector/src/main/codegen/templates/DenseUnionVector.java

+  }
+
+  public boolean isNull(int index) {
+    return (typeBuffer.getByte(index * TYPE_WIDTH) == 0);


there should be a separate validity buffer or there needs to be a more discussion the mailing list. As the spec is currently written this doesn't conform.

This is a real problem, and UnionVector also had this problem.
We solved it by using the underlying vector's validity buffer (please see #5752 (comment)).

Here we solve it with a similar method. Please see if it is OK.

After more thoughts, I think the solution for UnionVector does not work for DenseUnionVector. So I have revised the code to add a validity buffer.

emkornfield · 2020-01-10T09:16:07Z

java/vector/src/main/codegen/templates/DenseUnionVector.java

+
+  private ValueVector getVector(int index) {
+    int type = typeBuffer.getByte(index * TYPE_WIDTH);
+    if (type < Types.MinorType.values().length) {


this still doesn't conform to the spec as written. There can pontentially be more then one vector of each primitive type (e.g. 2 IntVectors). Again we should try to close on this on the mailing list, but I thought last time it was discussed we agreed to keep it general.

Sure. I will fix this.

I am a little curious about in what scenario do we need to have 2 IntVectors? Two IntVectors with different meta data?

Another question is, do we need to fix this problem for the (sparse) UnionVector in a follow up issue?

I have revised to code to support more than one vector for each vector type. Please take a look.

I will try to review over the weekend. To answer the question on use-case Arrow's unions underlying vectors are named meaning they could have two integers with different semantics. Another example of a data format that allows this is protobufs with oneof fields. Whether this is a common case is up for debate (I can't think of a good use-case at the moment) and the last time it was discussed on the mailing list the most common case is "json" representations which would exclude the duplicate integer use-cases. This is how the SparseUnionVector is implemented in Java.

I see. Thanks for the clarification.Have a great weekend.

liyafan82 · 2020-01-13T10:33:11Z

I would like to see this refactored so we do base classes of the vector and reader as non-templated code and then the implementation adds the templated code (or vice versa). This is to minimize the use of java code in freemarker templates. We have tried to eliminate that in a bunch of places. We haven't refactored in the readers/writers but we should avoid introducing new debt on this front.

Thanks for the suggestion. I will refactor the code towards that direction.

…UnionVector

vectors

emkornfield · 2020-01-20T03:46:52Z

java/vector/src/main/codegen/templates/DenseUnionVector.java

+  /**
+   * The key is type Id, and the value is vector.
+   */
+  private ValueVector[] childVectors = new ValueVector[Byte.MAX_VALUE + 1];


why the +1 ?

The valid type id should be from 0 to Byte.MAX_VALUE, both inclusive, so the total number is Byte.MAX_VALUE - 0 + 1.

emkornfield · 2020-01-20T03:47:54Z

java/vector/src/main/codegen/templates/DenseUnionVector.java

+  private static final byte TYPE_WIDTH = 1;
+  public static final byte OFFSET_WIDTH = 4;
+
+  private static final FieldType INTERNAL_STRUCT_TYPE = new FieldType(false /*nullable*/,


nit: lets use /nullable=/false, instead of putting it after the values.

Revised. Thank you.

emkornfield · 2020-01-20T03:52:33Z

java/vector/src/main/codegen/templates/DenseUnionVector.java

+    if (ownBuffers.size() != 3) {
+      throw new IllegalArgumentException("Illegal buffer count, expected " + 2 + ", got: " + ownBuffers.size());
+    }
+


should this reference typeIds metadata data someplace?

Sure. Revised the exception message.

emkornfield · 2020-01-20T03:54:00Z

java/vector/src/main/java/org/apache/arrow/vector/compare/RangeEqualsVisitor.java

@@ -170,6 +171,11 @@ public Boolean visit(UnionVector left, Range range) {
    return compareUnionVectors(range);
  }

+  @Override
+  public Boolean visit(DenseUnionVector left, Range range) {
+    throw new UnsupportedOperationException();


Open a JIRA to fill this and add a comment here?

Sure. Will create one.

emkornfield · 2020-01-20T03:58:39Z

java/vector/src/main/codegen/templates/DenseUnionVector.java

+    FieldReader reader = new DenseUnionReader(DenseUnionVector.this);
+    int offset = offsetBuffer.getInt(index * OFFSET_WIDTH);
+    reader.setPosition(offset);
+    holder.reader = reader;


should there be a check to see if reader is already initialized?

You mean we can reuse the reader here? I am not sure, because the reader is stateful and there can be more than one client for this vector.
The implementation here aligns with the sparse union vector.

emkornfield · 2020-01-20T04:00:09Z

java/vector/src/main/codegen/templates/DenseUnionVector.java

+    if (this.fieldType == null) {
+      fieldType = FieldType.nullable(new ArrowType.Union(Dense, typeIds));
+    } else {
+      final UnionMode mode = ((ArrowType.Union) this.fieldType.getType()).getMode();


shouldn't this always be Dense?

Nice catch. Revised accordingly.

emkornfield

Took a quick pass through and I think a a high level this looks like a reasonable start (modulo comments). The real test will be adding this to integration tests. Did you want to do this in this review or open up a JIRA for follow-up.

I'm also not sure if this adresses @jacques-n comment about removing templates, but I'll let him comment on that.

liyafan82 · 2020-01-20T11:02:09Z

Took a quick pass through and I think a a high level this looks like a reasonable start (modulo comments). The real test will be adding this to integration tests. Did you want to do this in this review or open up a JIRA for follow-up.

I'm also not sure if this adresses @jacques-n comment about removing templates, but I'll let him comment on that.

@emkornfield First, I would like to express my sincere appreciation to you, as this PR is so large and reviewing it is so effort-consuming.

To make the code easier to review, I want to do integration test in a follow-up issue.

@jacques-n 's comment has not been resolved yet, although I believe the suggested changes/refactorings are reasonable and beneficial. If @jacques-n agrees, I think we can do it in a follow up issue (this PR is already large enough). In addition, I think we can also apply the refactoring to the sparse union vector.

emkornfield · 2020-01-24T06:00:52Z

OK, going to merge now.

fsaintjacques added the Component: Java label Sep 23, 2019

emkornfield requested changes Sep 24, 2019

View reviewed changes

emkornfield reviewed Oct 4, 2019

View reviewed changes

kszucs force-pushed the fly_0923_union branch from 2104657 to 4ddd2c1 Compare October 5, 2019 10:08

liyafan82 mentioned this pull request Nov 26, 2019

ARROW-7177: [Java] Provide a utility to improve the performance of vector loading/unloading #5842

Merged

jacques-n reviewed Jan 9, 2020

View reviewed changes

emkornfield reviewed Jan 10, 2020

View reviewed changes

liyafan82 force-pushed the fly_0923_union branch from cc452bc to 65167b3 Compare January 13, 2020 12:09

liyafan82 added 5 commits January 14, 2020 11:06

[ARROW-6352][Java] Add implementation of DenseUnionVector

b1cf2bb

[ARROW-6352][Java] Separate implementations for UnionVector and Dense…

afd9199

…UnionVector

[ARROW-6352][Java] Replace release method with close method

1d52a6a

[ARROW-6352][Java] Support arbitrary relative type

86e92dc

[ARROW-6352][Java] Support type id with byte

fdf9f3a

liyafan82 force-pushed the fly_0923_union branch from c5f030d to 01393ad Compare January 14, 2020 04:11

[ARROW-6352][Java] Support isNull with validity buffer of underlying

95c8afb

vectors

liyafan82 force-pushed the fly_0923_union branch from 01393ad to 95c8afb Compare January 14, 2020 04:40

[ARROW-6352][Java] Support multiple vectors for each vector type

d952880

liyafan82 force-pushed the fly_0923_union branch from 50a990f to d952880 Compare January 19, 2020 02:09

emkornfield reviewed Jan 20, 2020

View reviewed changes

[ARROW-6352][Java] Resolve comments

47b31e7

liyafan82 force-pushed the fly_0923_union branch from 60ff3cc to 47b31e7 Compare January 20, 2020 10:49

emkornfield closed this in 24a39dc Jan 24, 2020

asfimport mentioned this pull request Jan 24, 2020

[Java] Add implementation of DenseUnionVector. #22728

Closed

ARROW-6352: [Java] Add implementation of DenseUnionVector #5473

ARROW-6352: [Java] Add implementation of DenseUnionVector #5473

Conversation

liyafan82 commented Sep 23, 2019

emkornfield left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liyafan82 commented Sep 24, 2019 • edited Loading

emkornfield commented Sep 24, 2019

liyafan82 commented Sep 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jacques-n left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liyafan82 Jan 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liyafan82 commented Jan 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emkornfield left a comment

Choose a reason for hiding this comment

liyafan82 commented Jan 20, 2020

emkornfield commented Jan 24, 2020

liyafan82 commented Sep 24, 2019 •

edited

Loading

liyafan82 Jan 13, 2020 •

edited

Loading