parquet-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From alexleven...@apache.org
Subject [2/2] parquet-mr git commit: PARQUET-287: Keep a least 1 column from union members when projecting thrift unions
Date Wed, 20 May 2015 02:36:08 GMT
PARQUET-287: Keep a least 1 column from union members when projecting thrift unions

Currently, the projection API allows you to select only some "kinds" of a union, or to drop a required union entirely. This becomes an issue when assembling these records, as they will be appear to be unions of an unknown type (how do you coerce an empty struct into a union?). The way this case is handled for primitives is by supplying a default value (like 0, or null). However, with a union, you have to choose what "kind" of the union it will act as, and in the interest of not being misleading, this PR reads one column to figure out what the correct "kind" is.

In the future, the better solution is to filter these records out -- a projection is really a request for a filter in this case. But for now, this should get us correctness without involving the filter API.

I think this PR needs some more tests before merging, but I wanted to get it out and get some feedback now.

I also refactored how ThriftSchemaVisitor works to not be stateful, by explicitly passing state through the recursion -- this makes it much easier to reason about.

*edit* This PR also includes a fix for PARQUET-275 because I encountered it during testing.

*edit 2* This PR also includes a fix for PARQUET-283

Author: Alex Levenson <alexlevenson@twitter.com>

Closes #189 from isnotinvain/alexlevenson/project-union and squashes the following commits:

c710702 [Alex Levenson] Avoid instantiating (unused) empty group type
c43a44c [Alex Levenson] Merge branch 'master' into alexlevenson/project-union
d62ee8c [Alex Levenson] Merge branch 'master' into alexlevenson/project-union
df51f41 [Alex Levenson] Fix tests
4d3f825 [Alex Levenson] Address review comments
6dd95f5 [Alex Levenson] Update tests to reflect changes
d7cee7e [Alex Levenson] Add tests for nested maps
9c34b20 [Alex Levenson] Keep a sentinel column in map values
53e5580 [Alex Levenson] Remove debug println
c525a65 [Alex Levenson] update docs to reflect set projection rules
aefb637 [Alex Levenson] Do not allow partial projection of keys or set elements
8b4e791 [Alex Levenson] Add tests for maps of unions
35de282 [Alex Levenson] Add test for list<union>
098630f [Alex Levenson] Merge branch 'master' into alexlevenson/project-union
77cc9e9 [Alex Levenson] Add license header
63b80fd [Alex Levenson] more clean up
6341747 [Alex Levenson] Clean up ConvertedField
dcd3ea9 [Alex Levenson] Merge branch 'master' into alexlevenson/project-union
9ce4781 [Alex Levenson] Some cleanup and comments
6964837 [Alex Levenson] Keep one sentinel column in projected unions that cannot be dropped entirely
37a9bef [Alex Levenson] Clean up visitor pattern for thrift types


Project: http://git-wip-us.apache.org/repos/asf/parquet-mr/repo
Commit: http://git-wip-us.apache.org/repos/asf/parquet-mr/commit/ded56ffd
Tree: http://git-wip-us.apache.org/repos/asf/parquet-mr/tree/ded56ffd
Diff: http://git-wip-us.apache.org/repos/asf/parquet-mr/diff/ded56ffd

Branch: refs/heads/master
Commit: ded56ffd598e41e32817f6c1b091595fe7122e8b
Parents: 181affd
Author: Alex Levenson <alexlevenson@twitter.com>
Authored: Tue May 19 19:36:04 2015 -0700
Committer: Alex Levenson <alexlevenson@twitter.com>
Committed: Tue May 19 19:36:04 2015 -0700

----------------------------------------------------------------------
 .../apache/parquet/thrift/ConvertedField.java   | 167 +++++++
 .../thrift/KeepOnlyFirstPrimitiveFilter.java    |  44 ++
 .../thrift/ThriftSchemaConvertVisitor.java      | 401 ++++++++++------
 .../parquet/thrift/ThriftSchemaConverter.java   |  11 +-
 .../projection/FieldProjectionFilter.java       |   3 +-
 .../parquet/thrift/projection/FieldsPath.java   |  18 +-
 .../projection/amend/DefaultEventsVisitor.java  |  43 +-
 .../amend/DefaultProtocolEventsGenerator.java   |   2 +-
 .../thrift/struct/CompatibilityChecker.java     |  65 ++-
 .../parquet/thrift/struct/ThriftType.java       | 121 ++---
 ...stParquetToThriftReadWriteAndProjection.java | 140 ++++++
 .../thrift/TestThriftSchemaConverter.java       |  93 +++-
 .../TestThriftSchemaConverterProjectUnion.java  | 480 +++++++++++++++++++
 .../thrift/projection/TestFieldsPath.java       |  80 ++--
 parquet-thrift/src/test/thrift/compat.thrift    |  94 ++++
 parquet_cascading.md                            |   6 +-
 16 files changed, 1420 insertions(+), 348 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/parquet-mr/blob/ded56ffd/parquet-thrift/src/main/java/org/apache/parquet/thrift/ConvertedField.java
----------------------------------------------------------------------
diff --git a/parquet-thrift/src/main/java/org/apache/parquet/thrift/ConvertedField.java b/parquet-thrift/src/main/java/org/apache/parquet/thrift/ConvertedField.java
new file mode 100644
index 0000000..9674054
--- /dev/null
+++ b/parquet-thrift/src/main/java/org/apache/parquet/thrift/ConvertedField.java
@@ -0,0 +1,167 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.parquet.thrift;
+
+import org.apache.parquet.ShouldNeverHappenException;
+import org.apache.parquet.schema.Type;
+import org.apache.parquet.thrift.projection.FieldsPath;
+
+import static org.apache.parquet.Preconditions.checkNotNull;
+
+/**
+ * This is the return value for the recursion done in {@link ThriftSchemaConvertVisitor}
+ * It represents a field that has been converted from a {@link org.apache.parquet.thrift.struct.ThriftType}
+ * to a {@link org.apache.parquet.schema.MessageType}, as well as whether this field is being
+ * projected away, kept, or kept only because we cannot safely drop all of its fields.
+ *
+ * This interface is essentially a union of {keep, drop, sentinelUnion}
+ */
+public interface ConvertedField {
+
+  /**
+   * The path from the root of the schema to this field.
+   */
+  FieldsPath path();
+
+  boolean isKeep();
+  Keep asKeep();
+
+  boolean isDrop();
+  Drop asDrop();
+
+  boolean isSentinelUnion();
+  SentinelUnion asSentinelUnion();
+
+  static abstract class ConvertedFieldBase implements ConvertedField {
+    private final FieldsPath path;
+
+    protected ConvertedFieldBase(FieldsPath path) {
+      this.path = checkNotNull(path, "path");
+    }
+
+    @Override
+    public boolean isKeep() {
+      return false;
+    }
+
+    @Override
+    public Keep asKeep() {
+      throw new ShouldNeverHappenException("asKeep called on " + this);
+    }
+
+    @Override
+    public boolean isDrop() {
+      return false;
+    }
+
+    @Override
+    public Drop asDrop() {
+      throw new ShouldNeverHappenException("asDrop called on " + this);
+    }
+
+    @Override
+    public boolean isSentinelUnion() {
+      return false;
+    }
+
+    @Override
+    public SentinelUnion asSentinelUnion() {
+      throw new ShouldNeverHappenException("asSentinelUnion called on " + this);
+    }
+
+    @Override
+    public FieldsPath path() {
+      return path;
+    }
+  }
+
+  /**
+   * Signals that the user explicitly requested either this field or one of its children.
+   */
+  public static final class Keep extends ConvertedFieldBase {
+    private final Type type;
+
+    public Keep(FieldsPath path, Type type) {
+      super(path);
+      this.type = checkNotNull(type, "type");
+    }
+
+    @Override
+    public boolean isKeep() {
+      return true;
+    }
+
+    @Override
+    public Keep asKeep() {
+      return this;
+    }
+
+    public Type getType() {
+      return type;
+    }
+  }
+
+  /**
+   * Signals that the user did not explicitly request this field nor its children, but
+   * we carry the converted type info anyway in case it is not possible to drop this union
+   * entirely.
+   */
+  public static final class SentinelUnion extends ConvertedFieldBase {
+    private final Type type;
+
+    public SentinelUnion(FieldsPath path, Type type) {
+      super(path);
+      this.type = checkNotNull(type, "type");
+    }
+
+    @Override
+    public boolean isSentinelUnion() {
+      return true;
+    }
+
+    @Override
+    public SentinelUnion asSentinelUnion() {
+      return this;
+    }
+
+    public Type getType() {
+      return type;
+    }
+  }
+
+  /**
+   * Signals the user did not request this field nor its children.
+   */
+  public static final class Drop extends ConvertedFieldBase {
+    public Drop(FieldsPath path) {
+      super(path);
+    }
+
+    @Override
+    public boolean isDrop() {
+      return true;
+    }
+
+    @Override
+    public Drop asDrop() {
+      return this;
+    }
+  }
+}
+

http://git-wip-us.apache.org/repos/asf/parquet-mr/blob/ded56ffd/parquet-thrift/src/main/java/org/apache/parquet/thrift/KeepOnlyFirstPrimitiveFilter.java
----------------------------------------------------------------------
diff --git a/parquet-thrift/src/main/java/org/apache/parquet/thrift/KeepOnlyFirstPrimitiveFilter.java b/parquet-thrift/src/main/java/org/apache/parquet/thrift/KeepOnlyFirstPrimitiveFilter.java
new file mode 100644
index 0000000..bc3df93
--- /dev/null
+++ b/parquet-thrift/src/main/java/org/apache/parquet/thrift/KeepOnlyFirstPrimitiveFilter.java
@@ -0,0 +1,44 @@
+/* 
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ * 
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ * 
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.parquet.thrift;
+
+import org.apache.parquet.thrift.projection.FieldProjectionFilter;
+import org.apache.parquet.thrift.projection.FieldsPath;
+import org.apache.parquet.thrift.projection.ThriftProjectionException;
+
+/**
+ * A {@link FieldProjectionFilter} that keeps only the first primitive field
+ * that it encounters.
+ */
+class KeepOnlyFirstPrimitiveFilter implements FieldProjectionFilter {
+  private boolean found = false;
+
+  @Override
+  public boolean keep(FieldsPath path) {
+    if (found) {
+      return false;
+    }
+
+    found = true;
+    return true;
+  }
+
+  @Override
+  public void assertNoUnmatchedPatterns() throws ThriftProjectionException { }
+}

http://git-wip-us.apache.org/repos/asf/parquet-mr/blob/ded56ffd/parquet-thrift/src/main/java/org/apache/parquet/thrift/ThriftSchemaConvertVisitor.java
----------------------------------------------------------------------
diff --git a/parquet-thrift/src/main/java/org/apache/parquet/thrift/ThriftSchemaConvertVisitor.java b/parquet-thrift/src/main/java/org/apache/parquet/thrift/ThriftSchemaConvertVisitor.java
index 3081d87..25347f8 100644
--- a/parquet-thrift/src/main/java/org/apache/parquet/thrift/ThriftSchemaConvertVisitor.java
+++ b/parquet-thrift/src/main/java/org/apache/parquet/thrift/ThriftSchemaConvertVisitor.java
@@ -18,36 +18,54 @@
  */
 package org.apache.parquet.thrift;
 
-import static org.apache.parquet.schema.ConversionPatterns.listType;
-import static org.apache.parquet.schema.ConversionPatterns.mapType;
-import static org.apache.parquet.schema.OriginalType.ENUM;
-import static org.apache.parquet.schema.OriginalType.UTF8;
-import static org.apache.parquet.schema.PrimitiveType.PrimitiveTypeName.BINARY;
-import static org.apache.parquet.schema.PrimitiveType.PrimitiveTypeName.BOOLEAN;
-import static org.apache.parquet.schema.PrimitiveType.PrimitiveTypeName.DOUBLE;
-import static org.apache.parquet.schema.PrimitiveType.PrimitiveTypeName.INT32;
-import static org.apache.parquet.schema.PrimitiveType.PrimitiveTypeName.INT64;
-import static org.apache.parquet.schema.Type.Repetition.OPTIONAL;
-import static org.apache.parquet.schema.Type.Repetition.REPEATED;
-import static org.apache.parquet.schema.Type.Repetition.REQUIRED;
-import static org.apache.parquet.schema.Types.primitive;
-
 import java.util.ArrayList;
 import java.util.List;
 
-import org.apache.parquet.Preconditions;
+import org.apache.parquet.ShouldNeverHappenException;
 import org.apache.parquet.schema.GroupType;
 import org.apache.parquet.schema.MessageType;
 import org.apache.parquet.schema.OriginalType;
 import org.apache.parquet.schema.PrimitiveType;
 import org.apache.parquet.schema.PrimitiveType.PrimitiveTypeName;
 import org.apache.parquet.schema.Type;
+import org.apache.parquet.schema.Type.Repetition;
 import org.apache.parquet.schema.Types.PrimitiveBuilder;
+import org.apache.parquet.thrift.ConvertedField.Drop;
+import org.apache.parquet.thrift.ConvertedField.Keep;
+import org.apache.parquet.thrift.ConvertedField.SentinelUnion;
 import org.apache.parquet.thrift.projection.FieldProjectionFilter;
 import org.apache.parquet.thrift.projection.FieldsPath;
 import org.apache.parquet.thrift.projection.ThriftProjectionException;
 import org.apache.parquet.thrift.struct.ThriftField;
 import org.apache.parquet.thrift.struct.ThriftType;
+import org.apache.parquet.thrift.struct.ThriftType.BoolType;
+import org.apache.parquet.thrift.struct.ThriftType.ByteType;
+import org.apache.parquet.thrift.struct.ThriftType.DoubleType;
+import org.apache.parquet.thrift.struct.ThriftType.EnumType;
+import org.apache.parquet.thrift.struct.ThriftType.I16Type;
+import org.apache.parquet.thrift.struct.ThriftType.I32Type;
+import org.apache.parquet.thrift.struct.ThriftType.I64Type;
+import org.apache.parquet.thrift.struct.ThriftType.ListType;
+import org.apache.parquet.thrift.struct.ThriftType.MapType;
+import org.apache.parquet.thrift.struct.ThriftType.SetType;
+import org.apache.parquet.thrift.struct.ThriftType.StringType;
+import org.apache.parquet.thrift.struct.ThriftType.StructType;
+import org.apache.parquet.thrift.struct.ThriftType.StructType.StructOrUnionType;
+
+import static org.apache.parquet.Preconditions.checkNotNull;
+import static org.apache.parquet.schema.ConversionPatterns.listType;
+import static org.apache.parquet.schema.ConversionPatterns.mapType;
+import static org.apache.parquet.schema.OriginalType.ENUM;
+import static org.apache.parquet.schema.OriginalType.UTF8;
+import static org.apache.parquet.schema.PrimitiveType.PrimitiveTypeName.BINARY;
+import static org.apache.parquet.schema.PrimitiveType.PrimitiveTypeName.BOOLEAN;
+import static org.apache.parquet.schema.PrimitiveType.PrimitiveTypeName.DOUBLE;
+import static org.apache.parquet.schema.PrimitiveType.PrimitiveTypeName.INT32;
+import static org.apache.parquet.schema.PrimitiveType.PrimitiveTypeName.INT64;
+import static org.apache.parquet.schema.Type.Repetition.OPTIONAL;
+import static org.apache.parquet.schema.Type.Repetition.REPEATED;
+import static org.apache.parquet.schema.Type.Repetition.REQUIRED;
+import static org.apache.parquet.schema.Types.primitive;
 
 /**
  * Visitor Class for converting a thrift definition to parquet message type.
@@ -55,203 +73,266 @@ import org.apache.parquet.thrift.struct.ThriftType;
  *
  * @author Tianshuo Deng
  */
-public class ThriftSchemaConvertVisitor implements ThriftType.TypeVisitor {
+public class ThriftSchemaConvertVisitor implements ThriftType.TypeVisitor<ConvertedField, ThriftSchemaConvertVisitor.State> {
   private final FieldProjectionFilter fieldProjectionFilter;
-  private final FieldsPath currentFieldPath = new FieldsPath();
+  private final boolean doProjection;
+
+  private ThriftSchemaConvertVisitor(FieldProjectionFilter fieldProjectionFilter, boolean doProjection) {
+    this.fieldProjectionFilter = checkNotNull(fieldProjectionFilter, "fieldProjectionFilter");
+    this.doProjection = doProjection;
+  }
 
-  private Type currentType;
-  private Type.Repetition currentRepetition = Type.Repetition.REPEATED; // MessageType is repeated GroupType
-  private String currentName = "ParquetSchema";
+  public static MessageType convert(StructType struct, FieldProjectionFilter filter) {
+    State state = new State(new FieldsPath(), REPEATED, "ParquetSchema");
 
-  public ThriftSchemaConvertVisitor(FieldProjectionFilter fieldProjectionFilter) {
-    this.fieldProjectionFilter = Preconditions.checkNotNull(fieldProjectionFilter, "fieldProjectionFilter");
+    ConvertedField converted = struct.accept(new ThriftSchemaConvertVisitor(filter, true), state);
+
+    if (!converted.isKeep()) {
+      throw new ThriftProjectionException("No columns have been selected");
+    }
+
+    return new MessageType(state.name, converted.asKeep().getType().asGroupType().getFields());
   }
 
   @Override
-  public void visit(ThriftType.MapType mapType) {
-    final ThriftField mapKeyField = mapType.getKey();
-    final ThriftField mapValueField = mapType.getValue();
-
-    //save env for map
-    String mapName = currentName;
-    Type.Repetition mapRepetition = currentRepetition;
-
-    //=========handle key
-    currentFieldPath.push(mapKeyField);
-    currentName = "key";
-    currentRepetition = REQUIRED;
-    mapKeyField.getType().accept(this);
-    Type keyType = currentType;//currentType is the already converted type
-    currentFieldPath.pop();
-
-    //=========handle value
-    currentFieldPath.push(mapValueField);
-    currentName = "value";
-    currentRepetition = OPTIONAL;
-    mapValueField.getType().accept(this);
-    Type valueType = currentType;
-    currentFieldPath.pop();
-
-    if (keyType == null && valueType == null) {
-      currentType = null;
-      return;
+  public ConvertedField visit(MapType mapType, State state) {
+    ThriftField keyField = mapType.getKey();
+    ThriftField valueField = mapType.getValue();
+
+    State keyState = new State(state.path.push(keyField), REQUIRED, "key");
+
+    // TODO: This is a bug! this should be REQUIRED but changing this will
+    // break the the schema compatibility check against old data
+    // Thrift does not support null / missing map values.
+    State valueState = new State(state.path.push(valueField), OPTIONAL, "value");
+
+    ConvertedField convertedKey = keyField.getType().accept(this, keyState);
+    ConvertedField convertedValue = valueField.getType().accept(this, valueState);
+
+    if (!convertedKey.isKeep()) {
+
+      if (convertedValue.isKeep()) {
+        throw new ThriftProjectionException(
+            "Cannot select only the values of a map, you must keep the keys as well: " + state.path);
+      }
+
+      // neither key nor value was requested
+      return new Drop(state.path);
     }
 
-    if (keyType == null && valueType != null)
-      throw new ThriftProjectionException("key of map is not specified in projection: " + currentFieldPath);
+    // we are keeping the key, but we do not allow partial projections on keys
+    // as that doesn't make sense when assembling back into a map.
+    // NOTE: doProjections prevents us from infinite recursion here.
+    if (doProjection) {
+      ConvertedField fullConvKey = keyField
+          .getType()
+          .accept(new ThriftSchemaConvertVisitor(FieldProjectionFilter.ALL_COLUMNS, false), keyState);
+
+      if (!fullConvKey.asKeep().getType().equals(convertedKey.asKeep().getType())) {
+        throw new ThriftProjectionException("Cannot select only a subset of the fields in a map key, " +
+            "for path " + state.path);
+      }
 
-    //restore Env
-    currentName = mapName;
-    currentRepetition = mapRepetition;
-    currentType = mapType(currentRepetition, currentName,
-            keyType,
-            valueType);
-  }
+    }
 
-  @Override
-  public void visit(ThriftType.SetType setType) {
-    final ThriftField setElemField = setType.getValues();
-    String setName = currentName;
-    Type.Repetition setRepetition = currentRepetition;
-    currentName = currentName + "_tuple";
-    currentRepetition = REPEATED;
-    setElemField.getType().accept(this);
-    //after conversion, currentType is the nested type
-    if (currentType != null) {
-      currentType = listType(setRepetition, setName, currentType);
+    // now, are we keeping the value?
+
+    if (convertedValue.isKeep()) {
+      // keep both key and value
+
+      Type mapField = mapType(
+          state.repetition,
+          state.name,
+          convertedKey.asKeep().getType(),
+          convertedValue.asKeep().getType());
+
+      return new Keep(state.path, mapField);
     }
+
+    // keep only the key, not the value
+
+    ConvertedField sentinelValue =
+        valueField.getType().accept(new ThriftSchemaConvertVisitor(new KeepOnlyFirstPrimitiveFilter(), true), valueState);
+
+    Type mapField = mapType(
+        state.repetition,
+        state.name,
+        convertedKey.asKeep().getType(),
+        sentinelValue.asKeep().getType()); // signals to mapType method to project the value
+
+    return new Keep(state.path, mapField);
   }
 
-  @Override
-  public void visit(ThriftType.ListType listType) {
-    final ThriftField setElemField = listType.getValues();
-    String listName = currentName;
-    Type.Repetition listRepetition = currentRepetition;
-    currentName = currentName + "_tuple";
-    currentRepetition = REPEATED;
-    setElemField.getType().accept(this);
-    //after conversion, currentType is the nested type
-    if (currentType != null) {
-      currentType = listType(listRepetition, listName, currentType);
+  private ConvertedField visitListLike(ThriftField listLike, State state, boolean isSet) {
+    State childState = new State(state.path, REPEATED, state.name + "_tuple");
+
+    ConvertedField converted = listLike.getType().accept(this, childState);
+
+    if (converted.isKeep()) {
+      // doProjection prevents an infinite recursion here
+      if (isSet && doProjection) {
+        ConvertedField fullConv = listLike
+            .getType()
+            .accept(new ThriftSchemaConvertVisitor(FieldProjectionFilter.ALL_COLUMNS, false), childState);
+        if (!converted.asKeep().getType().equals(fullConv.asKeep().getType())) {
+          throw new ThriftProjectionException("Cannot select only a subset of the fields in a set, " +
+              "for path " + state.path);
+        }
+      }
+
+      return new Keep(state.path, listType(state.repetition, state.name, converted.asKeep().getType()));
     }
+
+    return new Drop(state.path);
   }
 
-  public MessageType getConvertedMessageType() {
-    // the root should be a GroupType
-    if (currentType == null)
-      return new MessageType(currentName, new ArrayList<Type>());
+  @Override
+  public ConvertedField visit(SetType setType, State state) {
+    return visitListLike(setType.getValues(), state, true);
+  }
 
-    GroupType rootType = currentType.asGroupType();
-    return new MessageType(currentName, rootType.getFields());
+  @Override
+  public ConvertedField visit(ListType listType, State state) {
+    return visitListLike(listType.getValues(), state, false);
   }
 
   @Override
-  public void visit(ThriftType.StructType structType) {
-    List<ThriftField> fields = structType.getChildren();
+  public ConvertedField visit(StructType structType, State state) {
 
-    String oldName = currentName;
-    Type.Repetition oldRepetition = currentRepetition;
+    // special care is taken when converting unions,
+    // because we are actually both converting + projecting in
+    // one pass, and unions need special handling when projecting.
+    final boolean isUnion = isUnion(structType.getStructOrUnionType());
 
-    List<Type> types = getFieldsTypes(fields);
+    boolean hasSentinelUnionColumns = false;
+    boolean hasNonSentinelUnionColumns = false;
 
-    currentName = oldName;
-    currentRepetition = oldRepetition;
-    if (types.size() > 0) {
-      currentType = new GroupType(currentRepetition, currentName, types);
-    } else {
-      currentType = null;
-    }
-  }
+    List<Type> convertedChildren = new ArrayList<Type>();
+
+    for (ThriftField child : structType.getChildren()) {
+
+      State childState = new State(state.path.push(child), getRepetition(child), child.getName());
+
+      ConvertedField converted = child.getType().accept(this, childState);
 
-  private List<Type> getFieldsTypes(List<ThriftField> fields) {
-    List<Type> types = new ArrayList<Type>();
-    for (ThriftField field : fields) {
-      currentRepetition = getRepetition(field);
-      currentName = field.getName();
-      currentFieldPath.push(field);
-      field.getType().accept(this);
-      if (currentType != null) {
-        // currentType is converted with the currentName(fieldName)
-        types.add(currentType.withId(field.getFieldId()));
+      if (isUnion && !converted.isKeep()) {
+        // user is not keeping this "kind" of union, but we still need
+        // to keep at least one of the primitives of this union around.
+        // in order to know what "kind" of union each record is.
+        // TODO: in the future, we should just filter these records out instead
+
+        // re-do the recursion, with a new projection filter that keeps only
+        // the first primitive it encounters
+        ConvertedField firstPrimitive = child.getType().accept(
+            new ThriftSchemaConvertVisitor(new KeepOnlyFirstPrimitiveFilter(), true), childState);
+
+        convertedChildren.add(firstPrimitive.asKeep().getType().withId(child.getFieldId()));
+        hasSentinelUnionColumns = true;
       }
-      currentFieldPath.pop();
+
+      if (converted.isSentinelUnion()) {
+        // child field is a sentinel union that we should drop if possible
+        if (childState.repetition == REQUIRED) {
+          // but this field is required, so we may still need it
+          convertedChildren.add(converted.asSentinelUnion().getType().withId(child.getFieldId()));
+          hasSentinelUnionColumns = true;
+        }
+      } else if (converted.isKeep()) {
+        // user has selected this column, so we keep it.
+        convertedChildren.add(converted.asKeep().getType().withId(child.getFieldId()));
+        hasNonSentinelUnionColumns = true;
+      }
+
+    }
+
+    if (!hasNonSentinelUnionColumns && hasSentinelUnionColumns) {
+      // this is a union, and user has not requested any of the children
+      // of this union. We should drop this union, if possible, but
+      // we may not be able to, so tag this as a sentinel.
+      return new SentinelUnion(state.path, new GroupType(state.repetition, state.name, convertedChildren));
     }
-    return types;
-  }
 
-  private boolean isCurrentlyMatchedFilter(){
-     if(!fieldProjectionFilter.keep(currentFieldPath)){
-       currentType = null;
-       return false;
-     }
-    return true;
+    if (hasNonSentinelUnionColumns) {
+      // user requested some of the fields of this struct, so we keep the struct
+      return new Keep(state.path, new GroupType(state.repetition, state.name, convertedChildren));
+    } else {
+      // user requested none of the fields of this struct, so we drop it
+      return new Drop(state.path);
+    }
   }
 
-  private void primitiveType(PrimitiveTypeName type) {
-    primitiveType(type, null);
+  private ConvertedField visitPrimitiveType(PrimitiveTypeName type, State state) {
+    return visitPrimitiveType(type, null, state);
   }
 
-  private void primitiveType(PrimitiveTypeName type, OriginalType orig) {
-    if (isCurrentlyMatchedFilter()) {
-      PrimitiveBuilder<PrimitiveType> b = primitive(type, currentRepetition);
-      if (orig != null) {
-        b = b.as(orig);
-      }
-      currentType = b.named(currentName);
+  private ConvertedField visitPrimitiveType(PrimitiveTypeName type, OriginalType orig, State state) {
+    PrimitiveBuilder<PrimitiveType> b = primitive(type, state.repetition);
+
+    if (orig != null) {
+      b = b.as(orig);
+    }
+
+    if (fieldProjectionFilter.keep(state.path)) {
+      return new Keep(state.path, b.named(state.name));
+    } else {
+      return new Drop(state.path);
     }
   }
 
   @Override
-  public void visit(ThriftType.EnumType enumType) {
-    primitiveType(BINARY, ENUM);
+  public ConvertedField visit(EnumType enumType, State state) {
+    return visitPrimitiveType(BINARY, ENUM, state);
   }
 
   @Override
-  public void visit(ThriftType.BoolType boolType) {
-    primitiveType(BOOLEAN);
+  public ConvertedField visit(BoolType boolType, State state) {
+    return visitPrimitiveType(BOOLEAN, state);
   }
 
   @Override
-  public void visit(ThriftType.ByteType byteType) {
-    primitiveType(INT32);
+  public ConvertedField visit(ByteType byteType, State state) {
+    return visitPrimitiveType(INT32, state);
   }
 
   @Override
-  public void visit(ThriftType.DoubleType doubleType) {
-    primitiveType(DOUBLE);
+  public ConvertedField visit(DoubleType doubleType, State state) {
+    return visitPrimitiveType(DOUBLE, state);
   }
 
   @Override
-  public void visit(ThriftType.I16Type i16Type) {
-    primitiveType(INT32);
+  public ConvertedField visit(I16Type i16Type, State state) {
+    return visitPrimitiveType(INT32, state);
   }
 
   @Override
-  public void visit(ThriftType.I32Type i32Type) {
-    primitiveType(INT32);
+  public ConvertedField visit(I32Type i32Type, State state) {
+    return visitPrimitiveType(INT32, state);
   }
 
   @Override
-  public void visit(ThriftType.I64Type i64Type) {
-    primitiveType(INT64);
+  public ConvertedField visit(I64Type i64Type, State state) {
+    return visitPrimitiveType(INT64, state);
   }
 
   @Override
-  public void visit(ThriftType.StringType stringType) {
-    primitiveType(BINARY, UTF8);
+  public ConvertedField visit(StringType stringType, State state) {
+    return visitPrimitiveType(BINARY, UTF8, state);
   }
 
-  /**
-   * by default we can make everything optional
-   *
-   * @param thriftField
-   * @return
-   */
-  private Type.Repetition getRepetition(ThriftField thriftField) {
-    if (thriftField == null) {
-      return OPTIONAL;
+  private static boolean isUnion(StructOrUnionType s) {
+    switch (s) {
+      case STRUCT:
+        return false;
+      case UNION:
+        return true;
+      case UNKNOWN:
+        throw new ShouldNeverHappenException("Encountered UNKNOWN StructOrUnionType");
+      default:
+        throw new ShouldNeverHappenException("Unrecognized type: " + s);
     }
+  }
 
+  private Type.Repetition getRepetition(ThriftField thriftField) {
     switch (thriftField.getRequirement()) {
       case REQUIRED:
         return REQUIRED;
@@ -263,4 +344,20 @@ public class ThriftSchemaConvertVisitor implements ThriftType.TypeVisitor {
         throw new IllegalArgumentException("unknown requirement type: " + thriftField.getRequirement());
     }
   }
+
+  /**
+   * The state passed through the recursion as we traverse a thrift schema.
+   */
+  public static final class State {
+    public final FieldsPath path; // current field path
+    public final Type.Repetition repetition; // current repetition (no relation to parent's repetition)
+    public final String name; // name of the field located at path
+
+    public State(FieldsPath path, Repetition repetition, String name) {
+      this.path = path;
+      this.repetition = repetition;
+      this.name = name;
+    }
+  }
+
 }

http://git-wip-us.apache.org/repos/asf/parquet-mr/blob/ded56ffd/parquet-thrift/src/main/java/org/apache/parquet/thrift/ThriftSchemaConverter.java
----------------------------------------------------------------------
diff --git a/parquet-thrift/src/main/java/org/apache/parquet/thrift/ThriftSchemaConverter.java b/parquet-thrift/src/main/java/org/apache/parquet/thrift/ThriftSchemaConverter.java
index 112913e..4ce1e91 100644
--- a/parquet-thrift/src/main/java/org/apache/parquet/thrift/ThriftSchemaConverter.java
+++ b/parquet-thrift/src/main/java/org/apache/parquet/thrift/ThriftSchemaConverter.java
@@ -56,12 +56,10 @@ public class ThriftSchemaConverter {
     return convert(toStructType(thriftClass));
   }
 
-  public MessageType convert(StructType thriftClass) {
-    ThriftSchemaConvertVisitor visitor = new ThriftSchemaConvertVisitor(fieldProjectionFilter);
-    thriftClass.accept(visitor);
+  public MessageType convert(StructType struct) {
+    MessageType messageType = ThriftSchemaConvertVisitor.convert(struct, fieldProjectionFilter);
     fieldProjectionFilter.assertNoUnmatchedPatterns();
-    MessageType convertedMessageType = visitor.getConvertedMessageType();
-    return convertedMessageType;
+    return messageType;
   }
 
   public static <T extends TBase<?,?>> StructOrUnionType structOrUnionType(Class<T> klass) {
@@ -76,8 +74,7 @@ public class ThriftSchemaConverter {
   private static StructType toStructType(TStructDescriptor struct) {
     List<Field> fields = struct.getFields();
     List<ThriftField> children = new ArrayList<ThriftField>(fields.size());
-    for (int i = 0; i < fields.size(); i++) {
-      Field field = fields.get(i);
+    for (Field field : fields) {
       Requirement req =
           field.getFieldMetaData() == null ?
               Requirement.OPTIONAL :

http://git-wip-us.apache.org/repos/asf/parquet-mr/blob/ded56ffd/parquet-thrift/src/main/java/org/apache/parquet/thrift/projection/FieldProjectionFilter.java
----------------------------------------------------------------------
diff --git a/parquet-thrift/src/main/java/org/apache/parquet/thrift/projection/FieldProjectionFilter.java b/parquet-thrift/src/main/java/org/apache/parquet/thrift/projection/FieldProjectionFilter.java
index 5498c8e..53b8b59 100644
--- a/parquet-thrift/src/main/java/org/apache/parquet/thrift/projection/FieldProjectionFilter.java
+++ b/parquet-thrift/src/main/java/org/apache/parquet/thrift/projection/FieldProjectionFilter.java
@@ -24,12 +24,13 @@ package org.apache.parquet.thrift.projection;
  * be included when reading thrift data. It is used to implement projection push down.
  *
  * See {@link StrictFieldProjectionFilter} and
- * {@link parquet.thrift.projection.deprecated.DeprecatedFieldProjectionFilter}
+ * {@link org.apache.parquet.thrift.projection.deprecated.DeprecatedFieldProjectionFilter}
  */
 public interface FieldProjectionFilter {
 
   /**
    * Decide whether to keep the field (column) represented by path.
+   * This path always represents a primitive (leaf node) path.
    *
    * @param path the path to the field (column)
    * @return true to keep, false to discard (project out)

http://git-wip-us.apache.org/repos/asf/parquet-mr/blob/ded56ffd/parquet-thrift/src/main/java/org/apache/parquet/thrift/projection/FieldsPath.java
----------------------------------------------------------------------
diff --git a/parquet-thrift/src/main/java/org/apache/parquet/thrift/projection/FieldsPath.java b/parquet-thrift/src/main/java/org/apache/parquet/thrift/projection/FieldsPath.java
index aa5ebaf..239384d 100644
--- a/parquet-thrift/src/main/java/org/apache/parquet/thrift/projection/FieldsPath.java
+++ b/parquet-thrift/src/main/java/org/apache/parquet/thrift/projection/FieldsPath.java
@@ -24,23 +24,25 @@ import org.apache.parquet.thrift.struct.ThriftField;
 import org.apache.parquet.thrift.struct.ThriftType;
 
 /**
- * Represents a column path as a sequence of fields.
+ * Represents an immutable column path as a sequence of fields.
  *
  * @author Tianshuo Deng
  */
 public class FieldsPath {
-  private final ArrayList<ThriftField> fields = new ArrayList<ThriftField>();
+  private final ArrayList<ThriftField> fields;
 
-  public void push(ThriftField f) {
-    this.fields.add(f);
+  public FieldsPath() {
+    this(new ArrayList<ThriftField>());
   }
 
-  public ThriftField pop() {
-    return this.fields.remove(fields.size() - 1);
+  private FieldsPath(ArrayList<ThriftField> fields) {
+    this.fields = fields;
   }
 
-  public ArrayList<ThriftField> getFields() {
-    return fields;
+  public FieldsPath push(ThriftField f) {
+    ArrayList<ThriftField> copy = new ArrayList<ThriftField>(fields);
+    copy.add(f);
+    return new FieldsPath(copy);
   }
 
   public String toDelimitedString(String delim) {

http://git-wip-us.apache.org/repos/asf/parquet-mr/blob/ded56ffd/parquet-thrift/src/main/java/org/apache/parquet/thrift/projection/amend/DefaultEventsVisitor.java
----------------------------------------------------------------------
diff --git a/parquet-thrift/src/main/java/org/apache/parquet/thrift/projection/amend/DefaultEventsVisitor.java b/parquet-thrift/src/main/java/org/apache/parquet/thrift/projection/amend/DefaultEventsVisitor.java
index aeaf434..ec63d85 100644
--- a/parquet-thrift/src/main/java/org/apache/parquet/thrift/projection/amend/DefaultEventsVisitor.java
+++ b/parquet-thrift/src/main/java/org/apache/parquet/thrift/projection/amend/DefaultEventsVisitor.java
@@ -34,10 +34,10 @@ import java.util.List;
 /**
  * Create a dummy events for all required fields according to thrift definition
  */
-class DefaultEventsVisitor implements ThriftType.TypeVisitor {
+class DefaultEventsVisitor implements ThriftType.TypeVisitor<Void, Void> {
   List<ParquetProtocol> dummyEvents= new ArrayList<ParquetProtocol>();
   @Override
-  public void visit(ThriftType.MapType mapType) {
+  public Void visit(ThriftType.MapType mapType, Void v) {
      dummyEvents.add(new ParquetProtocol("readMapBegin()") {
        @Override
        public TMap readMapBegin() throws TException {
@@ -50,11 +50,11 @@ class DefaultEventsVisitor implements ThriftType.TypeVisitor {
       public void readMapEnd() throws TException {
       }
     });
-
+    return null;
   }
 
   @Override
-  public void visit(final ThriftType.SetType setType) {
+  public Void visit(final ThriftType.SetType setType, Void v) {
     dummyEvents.add(new ParquetProtocol("readSetBegin()") {
       @Override
       public TSet readSetBegin() throws TException {
@@ -67,11 +67,13 @@ class DefaultEventsVisitor implements ThriftType.TypeVisitor {
       public void readSetEnd() throws TException {
       }
     });
+
+    return null;
   }
 
 
   @Override
-  public void visit(final ThriftType.ListType listType) {
+  public Void visit(final ThriftType.ListType listType, Void v) {
     dummyEvents.add(new ParquetProtocol("readListBegin()") {
       @Override
       public TList readListBegin() throws TException {
@@ -84,96 +86,107 @@ class DefaultEventsVisitor implements ThriftType.TypeVisitor {
       public void readListEnd() throws TException {
       }
     });
+
+    return null;
   }
 
   @Override
-  public void visit(ThriftType.StructType structType) {
+  public Void visit(ThriftType.StructType structType, Void v) {
     dummyEvents.add(new StructBeginProtocol("struct"));
     List<ThriftField> children = structType.getChildren();
     for (ThriftField child : children) {
       dummyEvents.add(new ReadFieldBeginProtocol(child));
-      child.getType().accept(this); //currently will create all the attributes in struct, it's safer
+      child.getType().accept(this, null); //currently will create all the attributes in struct, it's safer
       dummyEvents.add(DefaultProtocolEventsGenerator.READ_FIELD_END);
     }
     dummyEvents.add(DefaultProtocolEventsGenerator.READ_FIELD_STOP);
     dummyEvents.add(DefaultProtocolEventsGenerator.READ_STRUCT_END);
 
+    return null;
   }
 
   @Override
-  public void visit(ThriftType.EnumType enumType) {
+  public Void visit(ThriftType.EnumType enumType, Void v) {
     dummyEvents.add(new ParquetProtocol("readI32() enum") {
       @Override
       public int readI32() throws TException {
         return 0;
       }
     });
+    return null;
   }
 
   @Override
-  public void visit(ThriftType.BoolType boolType) {
+  public Void visit(ThriftType.BoolType boolType, Void v) {
     dummyEvents.add(new ParquetProtocol("readBool()") {
       @Override
       public boolean readBool() throws TException {
         return false;
       }
     });
+    return null;
   }
 
 
   @Override
-  public void visit(ThriftType.ByteType byteType) {
+  public Void visit(ThriftType.ByteType byteType, Void v) {
     dummyEvents.add(new ParquetProtocol("readByte() int") {
       @Override
       public byte readByte() throws TException {
         return (byte) 0;
       }
     });
+    return null;
   }
 
   @Override
-  public void visit(ThriftType.DoubleType doubleType) {
+  public Void visit(ThriftType.DoubleType doubleType, Void v) {
     dummyEvents.add(new ParquetProtocol("readDouble()") {
       @Override
       public double readDouble() throws TException {
         return 0.0;
       }
     });
+    return null;
   }
 
   @Override
-  public void visit(ThriftType.I16Type i16Type) {
+  public Void visit(ThriftType.I16Type i16Type, Void v) {
     dummyEvents.add(new ParquetProtocol("readI16()") {
       @Override
       public short readI16() throws TException {
         return (short) 0;
       }
     });
+    return null;
   }
 
   @Override
-  public void visit(ThriftType.I32Type i32Type) {
+  public Void visit(ThriftType.I32Type i32Type, Void v) {
     dummyEvents.add(new ParquetProtocol("readI32()") {
       @Override
       public int readI32() throws TException {
         return 0;
       }
     });
+    return null;
   }
 
   @Override
-  public void visit(ThriftType.I64Type i64Type) {
+  public Void visit(ThriftType.I64Type i64Type, Void v) {
     dummyEvents.add(new ParquetProtocol("readI64()") {
       @Override
       public long readI64() throws TException {
         return 0;
       }
     });
+    return null;
   }
 
   @Override
-  public void visit(ThriftType.StringType stringType) {
+  public Void visit(ThriftType.StringType stringType, Void v) {
     dummyEvents.add(new StringProtocol(""));
+    return null;
   }
 
   public List<ParquetProtocol> getEvents() {

http://git-wip-us.apache.org/repos/asf/parquet-mr/blob/ded56ffd/parquet-thrift/src/main/java/org/apache/parquet/thrift/projection/amend/DefaultProtocolEventsGenerator.java
----------------------------------------------------------------------
diff --git a/parquet-thrift/src/main/java/org/apache/parquet/thrift/projection/amend/DefaultProtocolEventsGenerator.java b/parquet-thrift/src/main/java/org/apache/parquet/thrift/projection/amend/DefaultProtocolEventsGenerator.java
index 99d0171..6d22e7f 100644
--- a/parquet-thrift/src/main/java/org/apache/parquet/thrift/projection/amend/DefaultProtocolEventsGenerator.java
+++ b/parquet-thrift/src/main/java/org/apache/parquet/thrift/projection/amend/DefaultProtocolEventsGenerator.java
@@ -59,7 +59,7 @@ class DefaultProtocolEventsGenerator {
     createdEvents.add(fieldBegin);
 
     DefaultEventsVisitor dummyCreatorvisitor = new DefaultEventsVisitor();
-    missingField.getType().accept(dummyCreatorvisitor);
+    missingField.getType().accept(dummyCreatorvisitor, null);
     createdEvents.addAll(dummyCreatorvisitor.getEvents());
     createdEvents.add(READ_FIELD_END);
     return createdEvents;

http://git-wip-us.apache.org/repos/asf/parquet-mr/blob/ded56ffd/parquet-thrift/src/main/java/org/apache/parquet/thrift/struct/CompatibilityChecker.java
----------------------------------------------------------------------
diff --git a/parquet-thrift/src/main/java/org/apache/parquet/thrift/struct/CompatibilityChecker.java b/parquet-thrift/src/main/java/org/apache/parquet/thrift/struct/CompatibilityChecker.java
index 04a106a..95a6d27 100644
--- a/parquet-thrift/src/main/java/org/apache/parquet/thrift/struct/CompatibilityChecker.java
+++ b/parquet-thrift/src/main/java/org/apache/parquet/thrift/struct/CompatibilityChecker.java
@@ -22,6 +22,15 @@ package org.apache.parquet.thrift.struct;
 import java.util.ArrayList;
 import java.util.List;
 
+import org.apache.parquet.thrift.struct.ThriftType.BoolType;
+import org.apache.parquet.thrift.struct.ThriftType.ByteType;
+import org.apache.parquet.thrift.struct.ThriftType.DoubleType;
+import org.apache.parquet.thrift.struct.ThriftType.EnumType;
+import org.apache.parquet.thrift.struct.ThriftType.I16Type;
+import org.apache.parquet.thrift.struct.ThriftType.I32Type;
+import org.apache.parquet.thrift.struct.ThriftType.I64Type;
+import org.apache.parquet.thrift.struct.ThriftType.StringType;
+
 /**
  * A checker for thrift struct, returns compatibility report based on following rules:
  * 1. Should not add new REQUIRED field in new thrift struct. Adding optional field is OK
@@ -35,7 +44,7 @@ public class CompatibilityChecker {
 
   public CompatibilityReport checkCompatibility(ThriftType.StructType oldStruct, ThriftType.StructType newStruct) {
     CompatibleCheckerVisitor visitor = new CompatibleCheckerVisitor(oldStruct);
-    newStruct.accept(visitor);
+    newStruct.accept(visitor, null);
     return visitor.getReport();
   }
 
@@ -59,7 +68,7 @@ class CompatibilityReport {
   }
 }
 
-class CompatibleCheckerVisitor implements ThriftType.TypeVisitor {
+class CompatibleCheckerVisitor implements ThriftType.TypeVisitor<Void, Void> {
   ThriftType oldType;
   CompatibilityReport report = new CompatibilityReport();
 
@@ -72,7 +81,7 @@ class CompatibleCheckerVisitor implements ThriftType.TypeVisitor {
   }
 
   @Override
-  public void visit(ThriftType.MapType mapType) {
+  public Void visit(ThriftType.MapType mapType, Void v) {
     ThriftType.MapType currentOldType = ((ThriftType.MapType) oldType);
     ThriftField oldKeyField = currentOldType.getKey();
     ThriftField newKeyField = mapType.getKey();
@@ -84,24 +93,27 @@ class CompatibleCheckerVisitor implements ThriftType.TypeVisitor {
     checkField(oldValueField, newValueField);
 
     oldType = currentOldType;
+    return null;
   }
 
   @Override
-  public void visit(ThriftType.SetType setType) {
+  public Void visit(ThriftType.SetType setType, Void v) {
     ThriftType.SetType currentOldType = ((ThriftType.SetType) oldType);
     ThriftField oldField = currentOldType.getValues();
     ThriftField newField = setType.getValues();
     checkField(oldField, newField);
     oldType = currentOldType;
+    return null;
   }
 
   @Override
-  public void visit(ThriftType.ListType listType) {
+  public Void visit(ThriftType.ListType listType, Void v) {
     ThriftType.ListType currentOldType = ((ThriftType.ListType) oldType);
     ThriftField oldField = currentOldType.getValues();
     ThriftField newField = listType.getValues();
     checkField(oldField, newField);
     oldType = currentOldType;
+    return null;
   }
 
   public void fail(String message) {
@@ -126,7 +138,7 @@ class CompatibleCheckerVisitor implements ThriftType.TypeVisitor {
     }
 
     oldType = oldField.getType();
-    newField.getType().accept(this);
+    newField.getType().accept(this, null);
   }
 
   private boolean firstIsMoreRestirctive(ThriftField.Requirement firstReq, ThriftField.Requirement secReq) {
@@ -139,7 +151,7 @@ class CompatibleCheckerVisitor implements ThriftType.TypeVisitor {
   }
 
   @Override
-  public void visit(ThriftType.StructType newStruct) {
+  public Void visit(ThriftType.StructType newStruct, Void v) {
     ThriftType.StructType currentOldType = ((ThriftType.StructType) oldType);
     short oldMaxId = 0;
     for (ThriftField oldField : currentOldType.getChildren()) {
@@ -150,7 +162,7 @@ class CompatibleCheckerVisitor implements ThriftType.TypeVisitor {
       ThriftField newField = newStruct.getChildById(fieldId);
       if (newField == null) {
         fail("can not find index in new Struct: " + fieldId +" in " + newStruct);
-        return;
+        return null;
       }
       checkField(oldField, newField);
     }
@@ -164,57 +176,58 @@ class CompatibleCheckerVisitor implements ThriftType.TypeVisitor {
       short newFieldId = newField.getFieldId();
       if (newFieldId > oldMaxId) {
         fail("new required field " + newField.getName() + " is added");
-        return;
+        return null;
       }
       if (newFieldId < oldMaxId && currentOldType.getChildById(newFieldId) == null) {
         fail("new required field " + newField.getName() + " is added");
-        return;
+        return null;
       }
 
     }
 
     //restore
     oldType = currentOldType;
+    return null;
   }
 
   @Override
-  public void visit(ThriftType.EnumType enumType) {
-    return;
+  public Void visit(EnumType enumType, Void v) {
+    return null;
   }
 
   @Override
-  public void visit(ThriftType.BoolType boolType) {
-    return;
+  public Void visit(BoolType boolType, Void v) {
+    return null;
   }
 
   @Override
-  public void visit(ThriftType.ByteType byteType) {
-    return;
+  public Void visit(ByteType byteType, Void v) {
+    return null;
   }
 
   @Override
-  public void visit(ThriftType.DoubleType doubleType) {
-    return;
+  public Void visit(DoubleType doubleType, Void v) {
+    return null;
   }
 
   @Override
-  public void visit(ThriftType.I16Type i16Type) {
-    return;
+  public Void visit(I16Type i16Type, Void v) {
+    return null;
   }
 
   @Override
-  public void visit(ThriftType.I32Type i32Type) {
-    return;
+  public Void visit(I32Type i32Type, Void v) {
+    return null;
   }
 
   @Override
-  public void visit(ThriftType.I64Type i64Type) {
-    return;
+  public Void visit(I64Type i64Type, Void v) {
+    return null;
   }
 
   @Override
-  public void visit(ThriftType.StringType stringType) {
-    return;
+  public Void visit(StringType stringType, Void v) {
+    return null;
   }
 }
 

http://git-wip-us.apache.org/repos/asf/parquet-mr/blob/ded56ffd/parquet-thrift/src/main/java/org/apache/parquet/thrift/struct/ThriftType.java
----------------------------------------------------------------------
diff --git a/parquet-thrift/src/main/java/org/apache/parquet/thrift/struct/ThriftType.java b/parquet-thrift/src/main/java/org/apache/parquet/thrift/struct/ThriftType.java
index 2f0f597..92d12b4 100644
--- a/parquet-thrift/src/main/java/org/apache/parquet/thrift/struct/ThriftType.java
+++ b/parquet-thrift/src/main/java/org/apache/parquet/thrift/struct/ThriftType.java
@@ -98,75 +98,31 @@ public abstract class ThriftType {
     return toJSON();
   }
 
-  public interface TypeVisitor {
+  public interface TypeVisitor<R, S> {
 
-    void visit(MapType mapType);
+    R visit(MapType mapType, S state);
 
-    void visit(SetType setType);
+    R visit(SetType setType, S state);
 
-    void visit(ListType listType);
+    R visit(ListType listType, S state);
 
-    void visit(StructType structType);
+    R visit(StructType structType, S state);
 
-    void visit(EnumType enumType);
+    R visit(EnumType enumType, S state);
 
-    void visit(BoolType boolType);
+    R visit(BoolType boolType, S state);
 
-    void visit(ByteType byteType);
+    R visit(ByteType byteType, S state);
 
-    void visit(DoubleType doubleType);
+    R visit(DoubleType doubleType, S state);
 
-    void visit(I16Type i16Type);
+    R visit(I16Type i16Type, S state);
 
-    void visit(I32Type i32Type);
+    R visit(I32Type i32Type, S state);
 
-    void visit(I64Type i64Type);
+    R visit(I64Type i64Type, S state);
 
-    void visit(StringType stringType);
-
-  }
-
-  public static abstract class ComplexTypeVisitor implements TypeVisitor {
-
-    @Override
-    final public void visit(EnumType enumType) {
-      throw new IllegalArgumentException("Expected complex type");
-    }
-
-    @Override
-    final public void visit(BoolType boolType) {
-      throw new IllegalArgumentException("Expected complex type");
-    }
-
-    @Override
-    final public void visit(ByteType byteType) {
-      throw new IllegalArgumentException("Expected complex type");
-    }
-
-    @Override
-    final public void visit(DoubleType doubleType) {
-      throw new IllegalArgumentException("Expected complex type");
-    }
-
-    @Override
-    final public void visit(I16Type i16Type) {
-      throw new IllegalArgumentException("Expected complex type");
-    }
-
-    @Override
-    final public void visit(I32Type i32Type) {
-      throw new IllegalArgumentException("Expected complex type");
-    }
-
-    @Override
-    final public void visit(I64Type i64Type) {
-      throw new IllegalArgumentException("Expected complex type");
-    }
-
-    @Override
-    final public void visit(StringType stringType) {
-      throw new IllegalArgumentException("Expected complex type");
-    }
+    R visit(StringType stringType, S state);
 
   }
 
@@ -233,8 +189,8 @@ public abstract class ThriftType {
     }
 
     @Override
-    public void accept(TypeVisitor visitor) {
-      visitor.visit(this);
+    public <R, S> R accept(TypeVisitor<R, S> visitor, S state) {
+      return visitor.visit(this, state);
     }
 
     @Override
@@ -276,8 +232,8 @@ public abstract class ThriftType {
     }
 
     @Override
-    public void accept(TypeVisitor visitor) {
-      visitor.visit(this);
+    public <R, S> R accept(TypeVisitor<R, S> visitor, S state) {
+      return visitor.visit(this, state);
     }
 
     @Override
@@ -317,8 +273,8 @@ public abstract class ThriftType {
     }
 
     @Override
-    public void accept(TypeVisitor visitor) {
-      visitor.visit(this);
+    public <R, S> R accept(TypeVisitor<R, S> visitor, S state) {
+      return visitor.visit(this, state);
     }
 
     @Override
@@ -356,8 +312,8 @@ public abstract class ThriftType {
     }
 
     @Override
-    public void accept(TypeVisitor visitor) {
-      visitor.visit(this);
+    public <R, S> R accept(TypeVisitor<R, S> visitor, S state) {
+      return visitor.visit(this, state);
     }
 
     @Override
@@ -452,8 +408,8 @@ public abstract class ThriftType {
     }
 
     @Override
-    public void accept(TypeVisitor visitor) {
-      visitor.visit(this);
+    public <R, S> R accept(TypeVisitor<R, S> visitor, S state) {
+      return visitor.visit(this, state);
     }
 
     @Override
@@ -484,8 +440,8 @@ public abstract class ThriftType {
       super(BOOL);
     }
     @Override
-    public void accept(TypeVisitor visitor) {
-      visitor.visit(this);
+    public <R, S> R accept(TypeVisitor<R, S> visitor, S state) {
+      return visitor.visit(this, state);
     }
   }
 
@@ -496,8 +452,8 @@ public abstract class ThriftType {
       super(BYTE);
     }
     @Override
-    public void accept(TypeVisitor visitor) {
-      visitor.visit(this);
+    public <R, S> R accept(TypeVisitor<R, S> visitor, S state) {
+      return visitor.visit(this, state);
     }
   }
 
@@ -508,8 +464,8 @@ public abstract class ThriftType {
       super(DOUBLE);
     }
     @Override
-    public void accept(TypeVisitor visitor) {
-      visitor.visit(this);
+    public <R, S> R accept(TypeVisitor<R, S> visitor, S state) {
+      return visitor.visit(this, state);
     }
   }
 
@@ -520,8 +476,8 @@ public abstract class ThriftType {
       super(I16);
     }
     @Override
-    public void accept(TypeVisitor visitor) {
-      visitor.visit(this);
+    public <R, S> R accept(TypeVisitor<R, S> visitor, S state) {
+      return visitor.visit(this, state);
     }
   }
 
@@ -532,8 +488,8 @@ public abstract class ThriftType {
       super(I32);
     }
     @Override
-    public void accept(TypeVisitor visitor) {
-      visitor.visit(this);
+    public <R, S> R accept(TypeVisitor<R, S> visitor, S state) {
+      return visitor.visit(this, state);
     }
   }
 
@@ -544,9 +500,10 @@ public abstract class ThriftType {
       super(I64);
     }
     @Override
-    public void accept(TypeVisitor visitor) {
-      visitor.visit(this);
+    public <R, S> R accept(TypeVisitor<R, S> visitor, S state) {
+      return visitor.visit(this, state);
     }
+
   }
 
   public static class StringType extends ThriftType {
@@ -556,8 +513,8 @@ public abstract class ThriftType {
       super(STRING);
     }
     @Override
-    public void accept(TypeVisitor visitor) {
-      visitor.visit(this);
+    public <R, S> R accept(TypeVisitor<R, S> visitor, S state) {
+      return visitor.visit(this, state);
     }
   }
 
@@ -568,7 +525,7 @@ public abstract class ThriftType {
     this.type = type;
   }
 
-  public abstract void accept(TypeVisitor visitor);
+  public abstract <R, S> R accept(TypeVisitor<R, S> visitor, S state);
 
   @JsonIgnore
   public ThriftTypeID getType() {

http://git-wip-us.apache.org/repos/asf/parquet-mr/blob/ded56ffd/parquet-thrift/src/test/java/org/apache/parquet/hadoop/thrift/TestParquetToThriftReadWriteAndProjection.java
----------------------------------------------------------------------
diff --git a/parquet-thrift/src/test/java/org/apache/parquet/hadoop/thrift/TestParquetToThriftReadWriteAndProjection.java b/parquet-thrift/src/test/java/org/apache/parquet/hadoop/thrift/TestParquetToThriftReadWriteAndProjection.java
index bf9b2a3..aa0b81d 100644
--- a/parquet-thrift/src/test/java/org/apache/parquet/hadoop/thrift/TestParquetToThriftReadWriteAndProjection.java
+++ b/parquet-thrift/src/test/java/org/apache/parquet/hadoop/thrift/TestParquetToThriftReadWriteAndProjection.java
@@ -27,6 +27,11 @@ import org.apache.hadoop.fs.FileSystem;
 import org.apache.hadoop.fs.Path;
 import org.apache.hadoop.io.BytesWritable;
 import org.apache.hadoop.mapreduce.*;
+import org.apache.parquet.thrift.test.compat.MapWithPrimMapValue;
+import org.apache.parquet.thrift.test.compat.MapWithStructMapValue;
+import org.apache.parquet.thrift.test.compat.MapWithStructValue;
+import org.apache.parquet.thrift.test.compat.StructV3;
+import org.apache.parquet.thrift.test.compat.StructV4WithExtracStructField;
 import org.apache.thrift.TBase;
 import org.apache.thrift.protocol.TCompactProtocol;
 import org.apache.thrift.protocol.TProtocol;
@@ -157,6 +162,141 @@ public class TestParquetToThriftReadWriteAndProjection {
   }
 
   @Test
+  public void testDropMapValuePrimitive() throws Exception {
+    String filter = "mavalue/key";
+
+    Map<String, String> mapValue = new HashMap<String, String>();
+    mapValue.put("a", "1");
+    mapValue.put("b", "2");
+    RequiredMapFixture toWrite = new RequiredMapFixture(mapValue);
+    toWrite.setName("testName");
+
+    // for now we expect no value projection to happen
+    // because a sentinel value is selected from the value
+    Map<String, String> readValue = new HashMap<String, String>();
+    readValue.put("a", "1");
+    readValue.put("b", "2");
+
+    RequiredMapFixture toRead = new RequiredMapFixture(readValue);
+
+    shouldDoProjectionWithThriftColumnFilter(filter, toWrite, toRead, RequiredMapFixture.class);
+  }
+
+  private StructV4WithExtracStructField makeStructV4WithExtracStructField(String id) {
+    StructV4WithExtracStructField sv4 = new StructV4WithExtracStructField();
+    StructV3 sv3 = new StructV3();
+    sv3.setAge("age " + id);
+    sv3.setGender("gender" + id);
+    sv3.setName("inner name " + id);
+    sv4.setAge("outer age " + id);
+    sv4.setAddedStruct(sv3);
+    sv4.setGender("outer gender " + id);
+    sv4.setName("outer name " + id);
+    return sv4;
+  }
+
+
+  @Test
+  public void testDropMapValueStruct() throws Exception {
+    String filter = "reqMap/key";
+
+    Map<String, StructV4WithExtracStructField> mapValue = new HashMap<String, StructV4WithExtracStructField>();
+
+    StructV4WithExtracStructField v1 = makeStructV4WithExtracStructField("1");
+    StructV4WithExtracStructField v2 = makeStructV4WithExtracStructField("2");
+
+    mapValue.put("key 1", v1);
+    mapValue.put("key 2", v2);
+    MapWithStructValue toWrite = new MapWithStructValue(mapValue);
+
+    // for now we expect a sentinel column to be kept
+    HashMap<String, StructV4WithExtracStructField> readValue = new HashMap<String, StructV4WithExtracStructField>();
+    readValue.put("key 1", new StructV4WithExtracStructField("outer name 1"));
+    readValue.put("key 2", new StructV4WithExtracStructField("outer name 2"));
+
+    MapWithStructValue toRead = new MapWithStructValue(readValue);
+
+    shouldDoProjectionWithThriftColumnFilter(filter, toWrite, toRead, MapWithStructValue.class);
+  }
+
+  @Test
+  public void testDropMapValueNestedPrim() throws Exception {
+    String filter = "reqMap/key";
+
+    Map<String, Map<String, String>> mapValue =
+        new HashMap<String, Map<String, String>>();
+
+    Map<String, String> innerValue1 = new HashMap<String, String>();
+    innerValue1.put("inner key (1, 1)", "inner (1, 1)");
+    innerValue1.put("inner key (1, 2)", "inner (1, 2)");
+
+    Map<String, String> innerValue2 = new HashMap<String, String>();
+    innerValue2.put("inner key (2, 1)", "inner (2, 1)");
+    innerValue2.put("inner key (2, 2)", "inner (2, 2)");
+
+    mapValue.put("outer key 1", innerValue1);
+    mapValue.put("outer key 2", innerValue2);
+
+    MapWithPrimMapValue toWrite = new MapWithPrimMapValue(mapValue);
+
+    Map<String, Map<String, String>> expected = new HashMap<String, Map<String, String>>();
+
+    Map<String, String> expectedInnerValue1 = new HashMap<String, String>();
+    expectedInnerValue1.put("inner key (1, 1)", "inner (1, 1)");
+    expectedInnerValue1.put("inner key (1, 2)", "inner (1, 2)");
+
+    Map<String, String> expectedInnerValue2 = new HashMap<String, String>();
+    expectedInnerValue2.put("inner key (2, 1)", "inner (2, 1)");
+    expectedInnerValue2.put("inner key (2, 2)", "inner (2, 2)");
+
+    expected.put("outer key 1", expectedInnerValue1);
+    expected.put("outer key 2", expectedInnerValue2);
+
+    MapWithPrimMapValue toRead = new MapWithPrimMapValue(expected);
+
+    shouldDoProjectionWithThriftColumnFilter(filter, toWrite, toRead, MapWithPrimMapValue.class);
+  }
+
+
+  @Test
+  public void testDropMapValueNestedStruct() throws Exception {
+    String filter = "reqMap/key";
+
+    Map<String, Map<String, StructV4WithExtracStructField>> mapValue =
+        new HashMap<String, Map<String, StructV4WithExtracStructField>>();
+
+    Map<String, StructV4WithExtracStructField> innerValue1 = new HashMap<String, StructV4WithExtracStructField>();
+    innerValue1.put("inner key (1, 1)", makeStructV4WithExtracStructField("inner (1, 1)"));
+    innerValue1.put("inner key (1, 2)", makeStructV4WithExtracStructField("inner (1, 2)"));
+
+    Map<String, StructV4WithExtracStructField> innerValue2 = new HashMap<String, StructV4WithExtracStructField>();
+    innerValue2.put("inner key (2, 1)", makeStructV4WithExtracStructField("inner (2, 1)"));
+    innerValue2.put("inner key (2, 2)", makeStructV4WithExtracStructField("inner (2, 2)"));
+
+    mapValue.put("outer key 1", innerValue1);
+    mapValue.put("outer key 2", innerValue2);
+
+    MapWithStructMapValue toWrite = new MapWithStructMapValue(mapValue);
+
+    Map<String, Map<String, StructV4WithExtracStructField>> expected = new HashMap<String, Map<String, StructV4WithExtracStructField>>();
+
+    Map<String, StructV4WithExtracStructField> expectedInnerValue1 = new HashMap<String, StructV4WithExtracStructField>();
+    expectedInnerValue1.put("inner key (1, 1)", new StructV4WithExtracStructField("outer name inner (1, 1)"));
+    expectedInnerValue1.put("inner key (1, 2)", new StructV4WithExtracStructField("outer name inner (1, 2)"));
+
+    Map<String, StructV4WithExtracStructField> expectedInnerValue2 = new HashMap<String, StructV4WithExtracStructField>();
+    expectedInnerValue2.put("inner key (2, 1)", new StructV4WithExtracStructField("outer name inner (2, 1)"));
+    expectedInnerValue2.put("inner key (2, 2)", new StructV4WithExtracStructField("outer name inner (2, 2)"));
+
+    expected.put("outer key 1", expectedInnerValue1);
+    expected.put("outer key 2", expectedInnerValue2);
+
+    MapWithStructMapValue toRead = new MapWithStructMapValue(expected);
+
+    shouldDoProjectionWithThriftColumnFilter(filter, toWrite, toRead, MapWithStructMapValue.class);
+  }
+
+  @Test
   public void testPullInRequiredLists() throws Exception {
     String filter = "info";
 

http://git-wip-us.apache.org/repos/asf/parquet-mr/blob/ded56ffd/parquet-thrift/src/test/java/org/apache/parquet/thrift/TestThriftSchemaConverter.java
----------------------------------------------------------------------
diff --git a/parquet-thrift/src/test/java/org/apache/parquet/thrift/TestThriftSchemaConverter.java b/parquet-thrift/src/test/java/org/apache/parquet/thrift/TestThriftSchemaConverter.java
index 97e1a12..cd5fc47 100644
--- a/parquet-thrift/src/test/java/org/apache/parquet/thrift/TestThriftSchemaConverter.java
+++ b/parquet-thrift/src/test/java/org/apache/parquet/thrift/TestThriftSchemaConverter.java
@@ -18,25 +18,26 @@
  */
 package org.apache.parquet.thrift;
 
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.fail;
-import static org.apache.parquet.schema.MessageTypeParser.parseMessageType;
-
-import org.apache.thrift.TBase;
-import org.junit.Test;
-
 import org.apache.parquet.schema.MessageType;
 import org.apache.parquet.schema.MessageTypeParser;
 import org.apache.parquet.thrift.projection.StrictFieldProjectionFilter;
-import org.apache.parquet.thrift.projection.deprecated.DeprecatedFieldProjectionFilter;
 import org.apache.parquet.thrift.projection.ThriftProjectionException;
+import org.apache.parquet.thrift.projection.deprecated.DeprecatedFieldProjectionFilter;
 import org.apache.parquet.thrift.struct.ThriftType;
 import org.apache.parquet.thrift.struct.ThriftType.StructType;
+import org.apache.parquet.thrift.test.compat.MapStructV2;
+import org.apache.parquet.thrift.test.compat.SetStructV2;
+import org.apache.thrift.TBase;
+import org.junit.Test;
 
 import com.twitter.data.proto.tutorial.thrift.AddressBook;
 import com.twitter.data.proto.tutorial.thrift.Person;
 import com.twitter.elephantbird.thrift.test.TestStructInMap;
 
+import static org.apache.parquet.schema.MessageTypeParser.parseMessageType;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.fail;
+
 public class TestThriftSchemaConverter {
 
   @Test
@@ -188,6 +189,11 @@ public class TestThriftSchemaConverter {
             "  optional group names (MAP) = 2 {\n" +
             "    repeated group map (MAP_KEY_VALUE) {\n" +
             "      required binary key (UTF8);\n" +
+            "      optional group value {\n" +
+            "        optional group name = 1 {\n" +
+            "          optional binary first_name (UTF8) = 1;\n" +
+            "        }\n" +
+            "      }" +
             "    }\n" +
             "  }\n" +
             "}",TestStructInMap.class);
@@ -203,25 +209,86 @@ public class TestThriftSchemaConverter {
     }
   }
 
+  private void shouldThrowWhenNoColumnsAreSelected(String filters, Class<? extends TBase<?, ?>> thriftClass) {
+    try {
+      getDeprecatedFilteredSchema(filters, thriftClass);
+      fail("should throw projection exception when no columns are selected");
+    } catch (ThriftProjectionException e) {
+      assertEquals("No columns have been selected", e.getMessage());
+    }
+  }
+
+  @Test
+  public void testThrowWhenNoColumnsAreSelected() {
+    shouldThrowWhenNoColumnsAreSelected("non_existing", TestStructInMap.class);
+  }
+
   @Test
   public void testThrowWhenProjectionFilterMatchesNothing() {
-    shouldThrowWhenProjectionFilterMatchesNothing("non_existing", "non_existing", TestStructInMap.class);
     shouldThrowWhenProjectionFilterMatchesNothing("name;non_existing", "non_existing", TestStructInMap.class);
     shouldThrowWhenProjectionFilterMatchesNothing("**;non_existing", "non_existing", TestStructInMap.class);
     shouldThrowWhenProjectionFilterMatchesNothing("**;names/non_existing", "names/non_existing", TestStructInMap.class);
     shouldThrowWhenProjectionFilterMatchesNothing("**;names/non_existing;non_existing", "names/non_existing\nnon_existing", TestStructInMap.class);
   }
 
+  @Test
   public void testProjectOnlyValueInMap() {
     try {
       getDeprecatedFilteredSchema("name;names/value/**", TestStructInMap.class);
       fail("this should throw");
     } catch (ThriftProjectionException e) {
-      assertEquals("", e.getMessage());
+      assertEquals("Cannot select only the values of a map, you must keep the keys as well: names", e.getMessage());
+    }
+
+    try {
+      getStrictFilteredSchema("name;names.value", TestStructInMap.class);
+      fail("this should throw");
+    } catch (ThriftProjectionException e) {
+      assertEquals("Cannot select only the values of a map, you must keep the keys as well: names", e.getMessage());
+    }
+
+  }
+
+  private void doTestPartialKeyProjection(String deprecated, String strict) {
+    try {
+      getDeprecatedFilteredSchema(deprecated, MapStructV2.class);
+      fail("this should throw");
+    } catch (ThriftProjectionException e) {
+      assertEquals("Cannot select only a subset of the fields in a map key, for path map1", e.getMessage());
+    }
+
+    try {
+      getStrictFilteredSchema(strict, MapStructV2.class);
+      fail("this should throw");
+    } catch (ThriftProjectionException e) {
+      assertEquals("Cannot select only a subset of the fields in a map key, for path map1", e.getMessage());
+    }
+  }
+
+  @Test
+  public void testPartialKeyProjection() {
+    doTestPartialKeyProjection("map1/key/age", "map1.key.age");
+    doTestPartialKeyProjection("map1/key/age;map1/value/**", "map1.{key.age,value}");
+  }
+
+  @Test
+  public void testSetPartialProjection() {
+    try {
+      getDeprecatedFilteredSchema("set1/age", SetStructV2.class);
+      fail("this should throw");
+    } catch (ThriftProjectionException e) {
+      assertEquals("Cannot select only a subset of the fields in a set, for path set1", e.getMessage());
+    }
+
+    try {
+      getStrictFilteredSchema("set1.age", SetStructV2.class);
+      fail("this should throw");
+    } catch (ThriftProjectionException e) {
+      assertEquals("Cannot select only a subset of the fields in a set, for path set1", e.getMessage());
     }
   }
 
-  private void shouldGetProjectedSchema(String deprecatedFilterDesc, String strictFilterDesc, String expectedSchemaStr, Class<? extends TBase<?,?>> thriftClass) {
+  public static void shouldGetProjectedSchema(String deprecatedFilterDesc, String strictFilterDesc, String expectedSchemaStr, Class<? extends TBase<?,?>> thriftClass) {
     MessageType depRequestedSchema = getDeprecatedFilteredSchema(deprecatedFilterDesc, thriftClass);
     MessageType strictRequestedSchema = getStrictFilteredSchema(strictFilterDesc, thriftClass);
     MessageType expectedSchema = parseMessageType(expectedSchemaStr);
@@ -229,12 +296,12 @@ public class TestThriftSchemaConverter {
     assertEquals(expectedSchema, strictRequestedSchema);
   }
 
-  private MessageType getDeprecatedFilteredSchema(String filterDesc, Class<? extends TBase<?,?>> thriftClass) {
+  private static MessageType getDeprecatedFilteredSchema(String filterDesc, Class<? extends TBase<?,?>> thriftClass) {
     DeprecatedFieldProjectionFilter fieldProjectionFilter = new DeprecatedFieldProjectionFilter(filterDesc);
     return new ThriftSchemaConverter(fieldProjectionFilter).convert(thriftClass);
   }
 
-  private MessageType getStrictFilteredSchema(String semicolonDelimitedString, Class<? extends TBase<?,?>> thriftClass) {
+  private static MessageType getStrictFilteredSchema(String semicolonDelimitedString, Class<? extends TBase<?,?>> thriftClass) {
     StrictFieldProjectionFilter fieldProjectionFilter = StrictFieldProjectionFilter.fromSemicolonDelimitedString(semicolonDelimitedString);
     return new ThriftSchemaConverter(fieldProjectionFilter).convert(thriftClass);
   }


Mime
View raw message