ARROW-725: [Formats/Java] FixedSizeList message and java implementation #452

elahrvivaz · 2017-03-29T18:36:25Z

~~Currently only added minor type for 2-tuples~~

wesm · 2017-03-29T18:53:13Z

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

@@ -47,6 +47,7 @@ private static void writeValue(FieldReader reader, FieldWriter writer) {
      switch (mt) {

      case LIST:
+      case TUPLE_2:


Is there some way to make the particular size not a special case?

Not that I could figure out... I think this goes back to the problem with the java minor types - I believe @julienledem was planning to remove them eventually.

is this partially or fully addressed in #409? Can you take a look?

I think #409 would fix it - I'll wait until that is merged and rebase

wesm · 2017-04-03T22:06:40Z

It's safe to rebase now -- I also renamed FixedWidthBinary to FixedSizeBinary for better conformity with this

elahrvivaz · 2017-04-04T14:50:27Z

thanks, I've rebased

julienledem · 2017-04-05T02:10:56Z

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

+  @Override
+  public ListWriter list(int size) {
+    fail("FixedSizeList");
+    return null;


let's hold off adding the fixed width types to the writer APIs.
One of the goals of those APIs is to be able to ingest arbitrary data (for example JSON) and infer the schema on the fly (hence the PromotableWriter promoting vectors to union if the type is not homogenous). So it won't work well with fixed width list since nothing prevents having more than one size in this api.

If we don't add it to the apis, how would we write fixed size lists?

By the way, arbitrary data should still work fine and default to regular lists - you have to specifically set up the fixed size list if you want to use it. If you know you have a fixed size list, you can use that optimization, otherwise you can just use regular lists that will work the same with slightly more overhead.

you can write to the vector directly through its mutator. The Writer api is a facility to write in a row-oriented fashion to the vectors.

How does this interact with the PromotableWriter? what happens if we write both fixed lists and variable size lists to the same field?

julienledem · 2017-04-05T02:12:36Z

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

   * @return
   */
-  abstract protected FieldWriter getWriter(MinorType type);
+  abstract protected FieldWriter getWriter(ArrowType type);


2 things:

this kind of change should be in its own pull request.

For performance reason we may not want to do it.

I think the only performance penalty is comparing an ArrowType instance instead of an enum compare - seems pretty minor.
Would you like me to split out into a separate PR?
MinorType is still used extensively in the writer framework - I think we ultimately need to pull it out since it doesn't capture the full arrow type. I noticed that the writers don't really handle decimal types, for that reason.
The other alternative is to create minor types for various fixed size lists - that would work for me, as I really only need 2-tuples at the moment. But it isn't especially flexible and @wesm asked for a more generic implementation.

Agreed that we need a more flexible solution. A Minor type for every list size is very inconvenient.
Regarding perf: There are a few places were we convert between minorType and arrowType each way. We also use ArrowType.equals instead of == between MinorTypes in getWriter(ArrowType) which is called in startList() which is called for each list creation. Just saying that we need to actually measure the consequence of this. I'm not trying to make you do more work than necessary. Separating the refactoring from the new type and vector allows to get the later merged while we finalize the former.

elahrvivaz · 2017-04-10T14:01:59Z

@julienledem do you have any more comments, or what do I need to do to get this approved?

julienledem

Overall this looks good but we need to figure out the change to getWriter(ArrowType). Possibly we can have more than one method (getFixedWidthListWriter...) to avoid creating objects and calling equals. The UnionWriter can be limited to just arbitrary json writing type use case.

julienledem · 2017-04-11T00:00:33Z

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

+  @Override
+  public ListWriter list(int size) {
+    fail("FixedSizeList");
+    return null;


you can write to the vector directly through its mutator. The Writer api is a facility to write in a row-oriented fashion to the vectors.

How does this interact with the PromotableWriter? what happens if we write both fixed lists and variable size lists to the same field?

julienledem · 2017-04-11T00:13:18Z

java/vector/src/main/java/org/apache/arrow/vector/complex/impl/PromotableWriter.java

+    } else if (!Objects.equals(type, this.type)) {
      promoteToUnion();
-      ((UnionWriter)writer).getWriter(type);
+      ((UnionWriter) writer).getWriter(Types.getMinorTypeForArrowType(type));


There's a discrepancy here. We compare ArrowType but then we create the writer based on MinorType which the two types may have the same even if they are not equal (Decimal or FixedWidthList for example).
It looks like if we start writing fixed width lists and then we write a different width we will promote but write the different width lists to the old fixed width vector. Possibly we should fail in this case.

julienledem · 2017-04-11T00:25:46Z

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

   * @return
   */
-  abstract protected FieldWriter getWriter(MinorType type);
+  abstract protected FieldWriter getWriter(ArrowType type);


Agreed that we need a more flexible solution. A Minor type for every list size is very inconvenient.
Regarding perf: There are a few places were we convert between minorType and arrowType each way. We also use ArrowType.equals instead of == between MinorTypes in getWriter(ArrowType) which is called in startList() which is called for each list creation. Just saying that we need to actually measure the consequence of this. I'm not trying to make you do more work than necessary. Separating the refactoring from the new type and vector allows to get the later merged while we finalize the former.

julienledem · 2017-04-11T00:38:48Z

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


+  @Override
+  public ListWriter list(int size) {
+    // prime the writer with the fixed size list


we don't really need to got Through ArrowType nor UnionType to call getWriter(...).
We could have a getFixedWidthList(size) in writer.

julienledem · 2017-04-11T00:43:11Z

java/vector/src/main/java/org/apache/arrow/vector/complex/impl/PromotableWriter.java

+  protected FieldWriter getWriter(ArrowType type) {
    if (state == State.UNION) {
-      ((UnionWriter)writer).getWriter(type);
+      ((UnionWriter) writer).getWriter(Types.getMinorTypeForArrowType(type));


Would this work for fixed width lists?

UnionWriter doesn't work for Decimal it looks like (https://github.com/apache/arrow/blob/master/java/vector/src/main/codegen/templates/UnionWriter.java#L124)
It's fine not to support FixedWidthList in union either. (at least for now)

julienledem · 2017-04-11T00:46:12Z

java/vector/src/main/java/org/apache/arrow/vector/types/pojo/FieldType.java


+  public FieldWriter createNewFieldWriter(ValueVector vector) {
+    MinorType minorType = Types.getMinorTypeForArrowType(type);
+    return minorType.getNewFieldWriter(vector);


shouldn't this be done from the type in the vector?

elahrvivaz · 2017-04-11T15:01:05Z

@julienledem I will revert the writer changes and just add some unit tests that show how to use the mutators directly.

elahrvivaz · 2017-04-11T16:38:44Z

reverted - ready for another review

wesm · 2017-04-13T18:30:05Z

@julienledem @elahrvivaz I'll be happy to provide a C++ implementation whenever this goes in, then we can set up integration tests. Let me know

julienledem

LGTM.
@jacques-n ?

julienledem · 2017-04-14T20:03:43Z

Thank you @elahrvivaz !

jacques-n

It looks like there is one small spurious import reorder. Otherwise, lgtm.

jacques-n · 2017-04-14T20:25:03Z

java/vector/src/main/java/org/apache/arrow/vector/complex/ListVector.java

 import java.util.Collections;
 import java.util.List;

+import com.google.common.collect.ImmutableList;


Reordering on purpose?

that's just how my ide (intellij) sorts imports alphabetically. I'll revert.

elahrvivaz · 2017-04-14T20:40:02Z

hold off merging - I need to add reAlloc to FixeSizeListVector now that #534 is merged

wesm · 2017-04-14T20:41:41Z

Sounds good

…using vector mutators

elahrvivaz · 2017-04-14T20:44:00Z

updated and rebased - should be good now

wesm · 2017-04-15T02:56:06Z

+1. This is great, thanks @elahrvivaz!

~Currently only added minor type for 2-tuples~ Author: Emilio Lahr-Vivaz <elahrvivaz@ccri.com> Closes apache#452 from elahrvivaz/ARROW-725 and squashes the following commits: b139d3d [Emilio Lahr-Vivaz] adding reAlloc to FixedSizeListVector 229e24a [Emilio Lahr-Vivaz] re-ordering imports 594c0a2 [Emilio Lahr-Vivaz] simplifying writing of list vectors through mutator 7cb2324 [Emilio Lahr-Vivaz] reverting writer changes, adding examples of writing fixed size list using vector mutators 756dc8a [Emilio Lahr-Vivaz] ARROW-725: [Formats/Java] FixedSizeList message and java implementation

Author: Deepak Majeti <deepak.majeti@hpe.com> Closes apache#452 from majetideepak/PARQUET-1265 and squashes the following commits: 2972197 [Deepak Majeti] fix initialization error ab07a47 [Deepak Majeti] Simplify construction of static variables b23d3b3 [Deepak Majeti] PARQUET-1265: Segfault on static ApplicationVersion initialization Change-Id: I012fc78dd90686309bc1f9feac446c9324e931f5

~Currently only added minor type for 2-tuples~ Author: Emilio Lahr-Vivaz <elahrvivaz@ccri.com> Closes apache#452 from elahrvivaz/ARROW-725 and squashes the following commits: b139d3d [Emilio Lahr-Vivaz] adding reAlloc to FixedSizeListVector 229e24a [Emilio Lahr-Vivaz] re-ordering imports 594c0a2 [Emilio Lahr-Vivaz] simplifying writing of list vectors through mutator 7cb2324 [Emilio Lahr-Vivaz] reverting writer changes, adding examples of writing fixed size list using vector mutators 756dc8a [Emilio Lahr-Vivaz] ARROW-725: [Formats/Java] FixedSizeList message and java implementation

Update latest Java version used by Github workflows to Java 23.

wesm reviewed Mar 29, 2017

View reviewed changes

wesm mentioned this pull request Mar 30, 2017

ARROW-676: move from MinorType to FieldType in ValueVectors to carry all the relevant type bits #409

Closed

elahrvivaz force-pushed the ARROW-725 branch 2 times, most recently from f6aa6f1 to f42dac9 Compare April 4, 2017 14:46

elahrvivaz mentioned this pull request Apr 4, 2017

GEOMESA-1747, GEOMESA-1748 Arrow integration locationtech/geomesa#1457

Closed

julienledem reviewed Apr 5, 2017

View reviewed changes

julienledem reviewed Apr 11, 2017

View reviewed changes

julienledem approved these changes Apr 14, 2017

View reviewed changes

jacques-n reviewed Apr 14, 2017

View reviewed changes

elahrvivaz added 5 commits April 14, 2017 16:41

ARROW-725: [Formats/Java] FixedSizeList message and java implementation

756dc8a

reverting writer changes, adding examples of writing fixed size list …

7cb2324

…using vector mutators

simplifying writing of list vectors through mutator

594c0a2

re-ordering imports

229e24a

adding reAlloc to FixedSizeListVector

b139d3d

elahrvivaz force-pushed the ARROW-725 branch from 82eac8e to b139d3d Compare April 14, 2017 20:43

asfgit closed this in 0f9c88f Apr 15, 2017

elahrvivaz deleted the ARROW-725 branch April 21, 2017 15:38

pribor pushed a commit to GlobalWebIndex/arrow that referenced this pull request Oct 24, 2025

MINOR: Update latest java version to 23 (apache#452)

e82aba8

Update latest Java version used by Github workflows to Java 23.

ARROW-725: [Formats/Java] FixedSizeList message and java implementation #452

ARROW-725: [Formats/Java] FixedSizeList message and java implementation #452

Uh oh!

Conversation

elahrvivaz commented Mar 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wesm commented Apr 3, 2017

Uh oh!

elahrvivaz commented Apr 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elahrvivaz commented Apr 10, 2017

Uh oh!

julienledem left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elahrvivaz commented Apr 11, 2017

Uh oh!

elahrvivaz commented Apr 11, 2017

Uh oh!

wesm commented Apr 13, 2017

Uh oh!

julienledem left a comment

Choose a reason for hiding this comment

Uh oh!

julienledem commented Apr 14, 2017

Uh oh!

jacques-n left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elahrvivaz commented Apr 14, 2017

Uh oh!

wesm commented Apr 14, 2017

Uh oh!

elahrvivaz commented Apr 14, 2017

Uh oh!

wesm commented Apr 15, 2017

Uh oh!

elahrvivaz commented Mar 29, 2017 •

edited

Loading