Merge pull request apache#247 from Parquet/fix/detect_extra_field_whe…

…n_index_is_not_start_from_zero fix bug: when field index is greater than zero
parthchandra · Dec 16, 2013 · cd00dc8 · cd00dc8
2 parents f5eb89d + e94b392
commit cd00dc8
Show file tree

Hide file tree

Showing 5 changed files with 49 additions and 7 deletions.
diff --git a/parquet-thrift/src/main/java/parquet/thrift/BufferedProtocolReadToWrite.java b/parquet-thrift/src/main/java/parquet/thrift/BufferedProtocolReadToWrite.java
@@ -373,7 +373,8 @@ public String toDebugString() {
     boolean hasFieldsIgnored = false;
     while ((field = in.readFieldBegin()).type != TType.STOP) {
       final TField currentField = field;
-      if (field.id > type.getChildren().size()) {
+      ThriftField expectedField;
+      if ((expectedField = type.getChildById(field.id)) == null) {
         notifyIgnoredFieldsOfRecord(field);
         hasFieldsIgnored |= true;
         //read the value and ignore it, NullProtocol will do nothing
@@ -391,7 +392,6 @@ public String toDebugString() {
           return "f=" + currentField.id + "<t=" + typeName(currentField.type) + ">: ";
         }
       });
-      ThriftField expectedField = type.getChildById(field.id);
       hasFieldsIgnored |= readOneValue(in, field.type, buffer, expectedField.getType());
       in.readFieldEnd();
       buffer.add(FIELD_END);

diff --git a/parquet-thrift/src/main/java/parquet/thrift/struct/CompatibilityChecker.java b/parquet-thrift/src/main/java/parquet/thrift/struct/CompatibilityChecker.java
@@ -144,10 +144,8 @@ public void visit(ThriftType.StructType newStruct) {
       if (fieldId > oldMaxId) {
         oldMaxId = fieldId;
       }
-      ThriftField newField = null;
-      try {
-        newField = newStruct.getChildById(fieldId);
-      } catch (ArrayIndexOutOfBoundsException e) {
+      ThriftField newField = newStruct.getChildById(fieldId);
+      if (newField == null) {
         fail("can not find index in new Struct: " + fieldId +" in " + newStruct);
         return;
       }

diff --git a/parquet-thrift/src/main/java/parquet/thrift/struct/ThriftType.java b/parquet-thrift/src/main/java/parquet/thrift/struct/ThriftType.java
@@ -176,7 +176,11 @@ public List<ThriftField> getChildren() {
 
     @JsonIgnore
     public ThriftField getChildById(short id) {
-      return childById[id];
+      if (id >= childById.length) {
+        return null;
+      } else {
+        return childById[id];
+      }
     }
 
     @Override

diff --git a/parquet-thrift/src/test/java/parquet/thrift/TestProtocolReadToWrite.java b/parquet-thrift/src/test/java/parquet/thrift/TestProtocolReadToWrite.java
@@ -32,6 +32,9 @@
 import java.util.Set;
 
 import org.apache.thrift.protocol.TField;
+import parquet.thrift.test.Phone;
+import parquet.thrift.test.StructWithExtraField;
+import parquet.thrift.test.StructWithIndexStartsFrom4;
 import parquet.thrift.test.compat.*;
 import thrift.test.OneOfEach;
 
@@ -220,6 +223,35 @@ public void handleFieldIgnored(TField field) {
     assertEquals(null, b.getAddedStruct());
   }
 
+  @Test
+  public void TestExtraFieldWhenFieldIndexIsNotStartFromZero() throws Exception {
+    BufferedProtocolReadToWrite structForRead = new BufferedProtocolReadToWrite(new ThriftSchemaConverter().toStructType(StructWithIndexStartsFrom4.class));
+
+    CountingErrorHandler countingHandler = new CountingErrorHandler() {
+      @Override
+      public void handleFieldIgnored(TField field) {
+        assertEquals(3,field.id);
+        fieldIgnoredCount++;
+      }
+    };
+    structForRead.unregisterAllErrorHandlers();
+    structForRead.registerErrorHandler(countingHandler);
+
+    //Data has an extra field of type struct
+    final ByteArrayOutputStream in = new ByteArrayOutputStream();
+    StructWithExtraField dataWithNewExtraField = new StructWithExtraField(new Phone("111","222"),new Phone("333","444"));
+    dataWithNewExtraField.write(protocol(in));
+
+    //read using the schema that doesn't have the extra field
+    final ByteArrayOutputStream out = new ByteArrayOutputStream();
+    structForRead.readOne(protocol(new ByteArrayInputStream(in.toByteArray())), protocol(out));
+
+    assertEquals(0, countingHandler.corruptedCount);
+    assertEquals(1, countingHandler.recordCountOfMissingFields);
+    assertEquals(1, countingHandler.fieldIgnoredCount);
+    assertEquals(0, countingHandler.schemaMismatchCount);
+  }
+
   private TCompactProtocol protocol(OutputStream to) {
     return new TCompactProtocol(new TIOStreamTransport(to));
   }

diff --git a/parquet-thrift/src/test/thrift/test.thrift b/parquet-thrift/src/test/thrift/test.thrift
@@ -63,3 +63,11 @@ struct TestPersonWithRequiredPhone {
   5: required Phone phone
 }
 
+struct StructWithIndexStartsFrom4 {
+  6: required Phone phone
+}
+
+struct StructWithExtraField {
+  3: required Phone extraPhone,
+  6: required Phone phone
+}