arrow-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From jul...@apache.org
Subject arrow git commit: ARROW-337: UnionListWriter.list() is doing more than it should, this …
Date Wed, 26 Oct 2016 19:09:18 GMT
Repository: arrow
Updated Branches:
  refs/heads/master 2f8449337 -> 3d2e4df21


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


Project: http://git-wip-us.apache.org/repos/asf/arrow/repo
Commit: http://git-wip-us.apache.org/repos/asf/arrow/commit/3d2e4df2
Tree: http://git-wip-us.apache.org/repos/asf/arrow/tree/3d2e4df2
Diff: http://git-wip-us.apache.org/repos/asf/arrow/diff/3d2e4df2

Branch: refs/heads/master
Commit: 3d2e4df219d6b06a3d78821bbca6ba17188908c2
Parents: 2f84493
Author: adeneche <adeneche@dremio.com>
Authored: Wed Oct 26 12:09:26 2016 -0700
Committer: Julien Le Dem <julien@dremio.com>
Committed: Wed Oct 26 12:09:26 2016 -0700

----------------------------------------------------------------------
 .../AbstractPromotableFieldWriter.java          |   2 +
 .../src/main/codegen/templates/MapWriters.java  |   1 +
 .../main/codegen/templates/UnionListWriter.java |  32 +--
 .../org/apache/arrow/vector/TestListVector.java |   4 -
 .../complex/writer/TestComplexWriter.java       | 201 +++++++++++++------
 5 files changed, 154 insertions(+), 86 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/arrow/blob/3d2e4df2/java/vector/src/main/codegen/templates/AbstractPromotableFieldWriter.java
----------------------------------------------------------------------
diff --git a/java/vector/src/main/codegen/templates/AbstractPromotableFieldWriter.java b/java/vector/src/main/codegen/templates/AbstractPromotableFieldWriter.java
index d21dcd0..60dd0c7 100644
--- a/java/vector/src/main/codegen/templates/AbstractPromotableFieldWriter.java
+++ b/java/vector/src/main/codegen/templates/AbstractPromotableFieldWriter.java
@@ -58,6 +58,7 @@ abstract class AbstractPromotableFieldWriter extends AbstractFieldWriter
{
   @Override
   public void end() {
     getWriter(MinorType.MAP).end();
+    setPosition(idx() + 1);
   }
 
   @Override
@@ -68,6 +69,7 @@ abstract class AbstractPromotableFieldWriter extends AbstractFieldWriter
{
   @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
/>

http://git-wip-us.apache.org/repos/asf/arrow/blob/3d2e4df2/java/vector/src/main/codegen/templates/MapWriters.java
----------------------------------------------------------------------
diff --git a/java/vector/src/main/codegen/templates/MapWriters.java b/java/vector/src/main/codegen/templates/MapWriters.java
index 51327b4..f41b600 100644
--- a/java/vector/src/main/codegen/templates/MapWriters.java
+++ b/java/vector/src/main/codegen/templates/MapWriters.java
@@ -185,6 +185,7 @@ public class ${mode}MapWriter extends AbstractFieldWriter {
 
   @Override
   public void end() {
+    setPosition(idx()+1);
   }
 
   <#list vv.types as type><#list type.minor as minor>

http://git-wip-us.apache.org/repos/asf/arrow/blob/3d2e4df2/java/vector/src/main/codegen/templates/UnionListWriter.java
----------------------------------------------------------------------
diff --git a/java/vector/src/main/codegen/templates/UnionListWriter.java b/java/vector/src/main/codegen/templates/UnionListWriter.java
index 04531a7..bb39fe8 100644
--- a/java/vector/src/main/codegen/templates/UnionListWriter.java
+++ b/java/vector/src/main/codegen/templates/UnionListWriter.java
@@ -101,11 +101,7 @@ public class UnionListWriter extends AbstractFieldWriter {
   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 class UnionListWriter extends AbstractFieldWriter {
 
   @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 class UnionListWriter extends AbstractFieldWriter {
   @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 class UnionListWriter extends AbstractFieldWriter {
   @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>

http://git-wip-us.apache.org/repos/asf/arrow/blob/3d2e4df2/java/vector/src/test/java/org/apache/arrow/vector/TestListVector.java
----------------------------------------------------------------------
diff --git a/java/vector/src/test/java/org/apache/arrow/vector/TestListVector.java b/java/vector/src/test/java/org/apache/arrow/vector/TestListVector.java
index bb71033..1f0baae 100644
--- a/java/vector/src/test/java/org/apache/arrow/vector/TestListVector.java
+++ b/java/vector/src/test/java/org/apache/arrow/vector/TestListVector.java
@@ -19,18 +19,14 @@ package org.apache.arrow.vector;
 
 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;
 

http://git-wip-us.apache.org/repos/asf/arrow/blob/3d2e4df2/java/vector/src/test/java/org/apache/arrow/vector/complex/writer/TestComplexWriter.java
----------------------------------------------------------------------
diff --git a/java/vector/src/test/java/org/apache/arrow/vector/complex/writer/TestComplexWriter.java
b/java/vector/src/test/java/org/apache/arrow/vector/complex/writer/TestComplexWriter.java
index 9419f88..6e0e617 100644
--- a/java/vector/src/test/java/org/apache/arrow/vector/complex/writer/TestComplexWriter.java
+++ b/java/vector/src/test/java/org/apache/arrow/vector/complex/writer/TestComplexWriter.java
@@ -65,10 +65,10 @@ public class TestComplexWriter {
     IntWriter intWriter = rootWriter.integer("int");
     BigIntWriter bigIntWriter = rootWriter.bigInt("bigInt");
     for (int i = 0; i < COUNT; i++) {
-      intWriter.setPosition(i);
+      rootWriter.start();
       intWriter.writeInt(i);
-      bigIntWriter.setPosition(i);
       bigIntWriter.writeBigInt(i);
+      rootWriter.end();
     }
     writer.setValueCount(COUNT);
     MapReader rootReader = new SingleMapReaderImpl(parent).reader("root");
@@ -83,23 +83,52 @@ public class TestComplexWriter {
 
   @Test
   public void nullableMap() {
-    MapVector parent = new MapVector("parent", allocator, null);
-    ComplexWriter writer = new ComplexWriterImpl("root", parent);
-    MapWriter rootWriter = writer.rootAsMap();
-    for (int i = 0; i < COUNT; i++) {
-      rootWriter.setPosition(i);
-      rootWriter.start();
-      if (i % 2 == 0) {
-        MapWriter mapWriter = rootWriter.map("map");
-        mapWriter.setPosition(i);
-        mapWriter.start();
-        mapWriter.bigInt("nested").writeBigInt(i);
-        mapWriter.end();
+    try (MapVector mapVector = new MapVector("parent", allocator, null)) {
+      ComplexWriter writer = new ComplexWriterImpl("root", mapVector);
+      MapWriter rootWriter = writer.rootAsMap();
+      for (int i = 0; i < COUNT; i++) {
+        rootWriter.start();
+        if (i % 2 == 0) {
+          MapWriter mapWriter = rootWriter.map("map");
+          mapWriter.setPosition(i);
+          mapWriter.start();
+          mapWriter.bigInt("nested").writeBigInt(i);
+          mapWriter.end();
+        }
+        rootWriter.end();
       }
-      rootWriter.end();
+      writer.setValueCount(COUNT);
+      checkNullableMap(mapVector);
     }
-    writer.setValueCount(COUNT);
-    MapReader rootReader = new SingleMapReaderImpl(parent).reader("root");
+  }
+
+  /**
+   * This test is similar to {@link #nullableMap()} ()} but we get the inner map writer once
at the beginning
+   */
+  @Test
+  public void nullableMap2() {
+    try (MapVector mapVector = new MapVector("parent", allocator, null)) {
+      ComplexWriter writer = new ComplexWriterImpl("root", mapVector);
+      MapWriter rootWriter = writer.rootAsMap();
+      MapWriter mapWriter = rootWriter.map("map");
+
+      for (int i = 0; i < COUNT; i++) {
+        rootWriter.start();
+        if (i % 2 == 0) {
+          mapWriter.setPosition(i);
+          mapWriter.start();
+          mapWriter.bigInt("nested").writeBigInt(i);
+          mapWriter.end();
+        }
+        rootWriter.end();
+      }
+      writer.setValueCount(COUNT);
+      checkNullableMap(mapVector);
+    }
+  }
+
+  private void checkNullableMap(MapVector mapVector) {
+    MapReader rootReader = new SingleMapReaderImpl(mapVector).reader("root");
     for (int i = 0; i < COUNT; i++) {
       rootReader.setPosition(i);
       assertTrue("index is set: " + i, rootReader.isSet());
@@ -113,11 +142,10 @@ public class TestComplexWriter {
         assertNull("index is not set: " + i, map.readObject());
       }
     }
-    parent.close();
   }
 
   @Test
-  public void listOfLists() {
+  public void testList() {
     MapVector parent = new MapVector("parent", allocator, null);
     ComplexWriter writer = new ComplexWriterImpl("root", parent);
     MapWriter rootWriter = writer.rootAsMap();
@@ -129,7 +157,6 @@ public class TestComplexWriter {
     rootWriter.list("list").endList();
     rootWriter.end();
 
-    rootWriter.setPosition(1);
     rootWriter.start();
     rootWriter.bigInt("int").writeBigInt(1);
     rootWriter.end();
@@ -152,7 +179,6 @@ public class TestComplexWriter {
     listVector.allocateNew();
     UnionListWriter listWriter = new UnionListWriter(listVector);
     for (int i = 0; i < COUNT; i++) {
-      listWriter.setPosition(i);
       listWriter.startList();
       for (int j = 0; j < i % 7; j++) {
         listWriter.writeInt(j);
@@ -206,7 +232,6 @@ public class TestComplexWriter {
     UnionListWriter listWriter = new UnionListWriter(listVector);
     MapWriter mapWriter = listWriter.map();
     for (int i = 0; i < COUNT; i++) {
-      listWriter.setPosition(i);
       listWriter.startList();
       for (int j = 0; j < i % 7; j++) {
         mapWriter.start();
@@ -230,23 +255,53 @@ public class TestComplexWriter {
 
   @Test
   public void listListType() {
-    ListVector listVector = new ListVector("list", allocator, null);
-    listVector.allocateNew();
-    UnionListWriter listWriter = new UnionListWriter(listVector);
-    for (int i = 0; i < COUNT; i++) {
-      listWriter.setPosition(i);
-      listWriter.startList();
-      for (int j = 0; j < i % 7; j++) {
-        ListWriter innerListWriter = listWriter.list();
-        innerListWriter.startList();
-        for (int k = 0; k < i % 13; k++) {
-          innerListWriter.integer().writeInt(k);
+    try (ListVector listVector = new ListVector("list", allocator, null)) {
+      listVector.allocateNew();
+      UnionListWriter listWriter = new UnionListWriter(listVector);
+      for (int i = 0; i < COUNT; i++) {
+        listWriter.startList();
+        for (int j = 0; j < i % 7; j++) {
+          ListWriter innerListWriter = listWriter.list();
+          innerListWriter.startList();
+          for (int k = 0; k < i % 13; k++) {
+            innerListWriter.integer().writeInt(k);
+          }
+          innerListWriter.endList();
         }
-        innerListWriter.endList();
+        listWriter.endList();
       }
-      listWriter.endList();
+      listWriter.setValueCount(COUNT);
+      checkListOfLists(listVector);
     }
-    listWriter.setValueCount(COUNT);
+  }
+
+  /**
+   * This test is similar to {@link #listListType()} but we get the inner list writer once
at the beginning
+   */
+  @Test
+  public void listListType2() {
+    try (ListVector listVector = new ListVector("list", allocator, null)) {
+      listVector.allocateNew();
+      UnionListWriter listWriter = new UnionListWriter(listVector);
+      ListWriter innerListWriter = listWriter.list();
+
+      for (int i = 0; i < COUNT; i++) {
+        listWriter.startList();
+        for (int j = 0; j < i % 7; j++) {
+          innerListWriter.startList();
+          for (int k = 0; k < i % 13; k++) {
+            innerListWriter.integer().writeInt(k);
+          }
+          innerListWriter.endList();
+        }
+        listWriter.endList();
+      }
+      listWriter.setValueCount(COUNT);
+      checkListOfLists(listVector);
+    }
+  }
+
+  private void checkListOfLists(final ListVector listVector) {
     UnionListReader listReader = new UnionListReader(listVector);
     for (int i = 0; i < COUNT; i++) {
       listReader.setPosition(i);
@@ -259,32 +314,65 @@ public class TestComplexWriter {
         }
       }
     }
-    listVector.clear();
   }
 
   @Test
   public void unionListListType() {
-    ListVector listVector = new ListVector("list", allocator, null);
-    listVector.allocateNew();
-    UnionListWriter listWriter = new UnionListWriter(listVector);
-    for (int i = 0; i < COUNT; i++) {
-      listWriter.setPosition(i);
-      listWriter.startList();
-      for (int j = 0; j < i % 7; j++) {
-        ListWriter innerListWriter = listWriter.list();
-        innerListWriter.startList();
-        for (int k = 0; k < i % 13; k++) {
-          if (k % 2 == 0) {
-            innerListWriter.integer().writeInt(k);
-          } else {
-            innerListWriter.bigInt().writeBigInt(k);
+    try (ListVector listVector = new ListVector("list", allocator, null)) {
+      listVector.allocateNew();
+      UnionListWriter listWriter = new UnionListWriter(listVector);
+      for (int i = 0; i < COUNT; i++) {
+        listWriter.startList();
+        for (int j = 0; j < i % 7; j++) {
+          ListWriter innerListWriter = listWriter.list();
+          innerListWriter.startList();
+          for (int k = 0; k < i % 13; k++) {
+            if (k % 2 == 0) {
+              innerListWriter.integer().writeInt(k);
+            } else {
+              innerListWriter.bigInt().writeBigInt(k);
+            }
           }
+          innerListWriter.endList();
         }
-        innerListWriter.endList();
+        listWriter.endList();
       }
-      listWriter.endList();
+      listWriter.setValueCount(COUNT);
+      checkUnionList(listVector);
     }
-    listWriter.setValueCount(COUNT);
+  }
+
+  /**
+   * This test is similar to {@link #unionListListType()} but we get the inner list writer
once at the beginning
+   */
+  @Test
+  public void unionListListType2() {
+    try (ListVector listVector = new ListVector("list", allocator, null)) {
+      listVector.allocateNew();
+      UnionListWriter listWriter = new UnionListWriter(listVector);
+      ListWriter innerListWriter = listWriter.list();
+
+      for (int i = 0; i < COUNT; i++) {
+        listWriter.startList();
+        for (int j = 0; j < i % 7; j++) {
+          innerListWriter.startList();
+          for (int k = 0; k < i % 13; k++) {
+            if (k % 2 == 0) {
+              innerListWriter.integer().writeInt(k);
+            } else {
+              innerListWriter.bigInt().writeBigInt(k);
+            }
+          }
+          innerListWriter.endList();
+        }
+        listWriter.endList();
+      }
+      listWriter.setValueCount(COUNT);
+      checkUnionList(listVector);
+    }
+  }
+
+  private void checkUnionList(ListVector listVector) {
     UnionListReader listReader = new UnionListReader(listVector);
     for (int i = 0; i < COUNT; i++) {
       listReader.setPosition(i);
@@ -301,7 +389,6 @@ public class TestComplexWriter {
         }
       }
     }
-    listVector.clear();
   }
 
   @Test
@@ -384,8 +471,8 @@ public class TestComplexWriter {
     MapVector parent = new MapVector("parent", allocator, null);
     ComplexWriter writer = new ComplexWriterImpl("root", parent);
     MapWriter rootWriter = writer.rootAsMap();
-    BigIntWriter bigIntWriter = rootWriter.bigInt("a");
-    VarCharWriter varCharWriter = rootWriter.varChar("a");
+    rootWriter.bigInt("a");
+    rootWriter.varChar("a");
 
     Field field = parent.getField().getChildren().get(0).getChildren().get(0);
     Assert.assertEquals("a", field.getName());


Mime
View raw message