ARROW-337: UnionListWriter.list() is doing more than it should, this …

…can cause data corruption

The general idea is to use the "inner" writer's position to update the offset. This involves making sure various writers do indeed update their positions.

UnionListWriter.startList() should explicitly set the inner writer position in case setPosition() was called to move the union list writer's position

Author: adeneche <adeneche@dremio.com>

Closes #183 from adeneche/ARROW-337 and squashes the following commits:

1ae7e00 [adeneche] updated TestComplexWriter to ensure position is set properly by the various writers
7d5aefc [adeneche] ARROW-337: UnionListWriter.list() is doing more than it should, this can cause data corruption

Author: adeneche

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

@@ -58,6 +58,7 @@ public void start() {
   @Override
   public void end() {
     getWriter(MinorType.MAP).end();
+    setPosition(idx() + 1);
   }
 
   @Override
@@ -68,6 +69,7 @@ public void startList() {
   @Override
   public void endList() {
     getWriter(MinorType.LIST).endList();
+    setPosition(idx() + 1);
   }
 
   <#list vv.types as type><#list type.minor as minor><#assign name = minor.class?cap_first />

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

@@ -185,6 +185,7 @@ public void start() {
 
   @Override
   public void end() {
+    setPosition(idx()+1);
   }
 
   <#list vv.types as type><#list type.minor as minor>

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

@@ -101,11 +101,7 @@ public void setPosition(int index) {
   public ${name}Writer <#if uncappedName == "int">integer<#else>${uncappedName}</#if>(String name) {
 //    assert inMap;
     mapName = name;
-    final int nextOffset = offsets.getAccessor().get(idx() + 1);
-    vector.getMutator().setNotNull(idx());
-    writer.setPosition(nextOffset);
-    ${name}Writer ${uncappedName}Writer = writer.<#if uncappedName == "int">integer<#else>${uncappedName}</#if>(name);
-    return ${uncappedName}Writer;
+    return writer.<#if uncappedName == "int">integer<#else>${uncappedName}</#if>(name);
   }
 
   </#if>
@@ -120,18 +116,11 @@ public MapWriter map() {
 
   @Override
   public ListWriter list() {
-    final int nextOffset = offsets.getAccessor().get(idx() + 1);
-    vector.getMutator().setNotNull(idx());
-    offsets.getMutator().setSafe(idx() + 1, nextOffset + 1);
-    writer.setPosition(nextOffset);
     return writer;
   }
 
   @Override
   public ListWriter list(String name) {
-    final int nextOffset = offsets.getAccessor().get(idx() + 1);
-    vector.getMutator().setNotNull(idx());
-    writer.setPosition(nextOffset);
     ListWriter listWriter = writer.list(name);
     return listWriter;
   }
@@ -145,30 +134,26 @@ public MapWriter map(String name) {
   @Override
   public void startList() {
     vector.getMutator().startNewValue(idx());
+    writer.setPosition(offsets.getAccessor().get(idx() + 1));
   }
 
   @Override
   public void endList() {
-
+    offsets.getMutator().set(idx() + 1, writer.idx());
+    setPosition(idx() + 1);
   }
 
   @Override
   public void start() {
 //    assert inMap;
-    final int nextOffset = offsets.getAccessor().get(idx() + 1);
-    vector.getMutator().setNotNull(idx());
-    offsets.getMutator().setSafe(idx() + 1, nextOffset);
-    writer.setPosition(nextOffset);
     writer.start();
   }
 
   @Override
   public void end() {
 //    if (inMap) {
-      writer.end();
-      inMap = false;
-      final int nextOffset = offsets.getAccessor().get(idx() + 1);
-      offsets.getMutator().setSafe(idx() + 1, nextOffset + 1);
+    writer.end();
+    inMap = false;
 //    }
   }
 
@@ -181,11 +166,8 @@ public void end() {
   @Override
   public void write${name}(<#list fields as field>${field.type} ${field.name}<#if field_has_next>, </#if></#list>) {
 //    assert !inMap;
-    final int nextOffset = offsets.getAccessor().get(idx() + 1);
-    vector.getMutator().setNotNull(idx());
-    writer.setPosition(nextOffset);
     writer.write${name}(<#list fields as field>${field.name}<#if field_has_next>, </#if></#list>);
-    offsets.getMutator().setSafe(idx() + 1, nextOffset + 1);
+    writer.setPosition(writer.idx()+1);
   }
 
   </#if>

java/vector/src/test/java/org/apache/arrow/vector/TestListVector.java

@@ -19,18 +19,14 @@
 
 import org.apache.arrow.memory.BufferAllocator;
 import org.apache.arrow.vector.complex.ListVector;
-import org.apache.arrow.vector.complex.impl.ComplexCopier;
-import org.apache.arrow.vector.complex.impl.UnionListReader;
 import org.apache.arrow.vector.complex.impl.UnionListWriter;
 import org.apache.arrow.vector.complex.reader.FieldReader;
-import org.apache.arrow.vector.types.pojo.Field;
 import org.junit.After;
 import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
 
 public class TestListVector {
-  private final static String EMPTY_SCHEMA_PATH = "";
 
   private BufferAllocator allocator;