HIVE-29488: NPE in Kryo deserializer when query plan contains ExprNodeGenericFuncDesc with Guava collections

ql/src/java/org/apache/hadoop/hive/ql/plan/ExprNodeGenericFuncDesc.java

@@ -75,6 +75,12 @@ public class ExprNodeGenericFuncDesc extends ExprNodeDesc implements
   public ExprNodeGenericFuncDesc() {;
   }
 
+  /**
+   * Constructor.
+   *
+   * @param children the children; a copy is made, so later changes to the passed list
+   *                 do not affect the children of this instance
+   */
   /* If the function has an explicit name like func(args) then call a
    * constructor that explicitly provides the function name in the
    * funcText argument.
@@ -86,6 +92,12 @@ public ExprNodeGenericFuncDesc(TypeInfo typeInfo, GenericUDF genericUDF,
          genericUDF, funcText, children);
   }
 
+  /**
+   * Constructor.
+   *
+   * @param children the children; a copy is made, so later changes to the passed list
+   *                 do not affect the children of this instance
+   */
   public ExprNodeGenericFuncDesc(ObjectInspector oi, GenericUDF genericUDF,
       String funcText,
       List<ExprNodeDesc> children) {
@@ -94,16 +106,28 @@ public ExprNodeGenericFuncDesc(ObjectInspector oi, GenericUDF genericUDF,
         ObjectInspectorUtils.getWritableObjectInspector(oi);
     assert (genericUDF != null);
     this.genericUDF = genericUDF;
-    this.children = children;
+    this.children = children == null ? new ArrayList<>() : new ArrayList<>(children);
     this.funcText = funcText;
   }
 
+  /**
+   * Constructor.
+   *
+   * @param children the children; a copy is made, so later changes to the passed list
+   *                 do not affect the children of this instance
+   */
   // Backward-compatibility interfaces for functions without a user-visible name.
   public ExprNodeGenericFuncDesc(TypeInfo typeInfo, GenericUDF genericUDF,
       List<ExprNodeDesc> children) {
     this(typeInfo, genericUDF, null, children);
   }
 
+  /**
+   * Constructor.
+   *
+   * @param children the children; a copy is made, so later changes to the passed list
+   *                 do not affect the children of this instance
+   */
   public ExprNodeGenericFuncDesc(ObjectInspector oi, GenericUDF genericUDF,
       List<ExprNodeDesc> children) {
     this(oi, genericUDF, null, children);
@@ -125,8 +149,14 @@ public void setGenericUDF(GenericUDF genericUDF) {
     this.genericUDF = genericUDF;
   }
 
+  /**
+   * Sets the children.
+   *
+   * @param children the new children; a copy is made, so later changes to the passed list
+   *                 do not affect the children of this instance
+   */
   public void setChildren(List<ExprNodeDesc> children) {
-    this.children = children;
+    this.children = children == null ? new ArrayList<>() : new ArrayList<>(children);
   }
 
   @Override

ql/src/test/org/apache/hadoop/hive/ql/exec/TestSerializationUtilities.java

@@ -23,17 +23,21 @@
 import java.io.InputStream;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collection;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 import java.util.Properties;
 
+import com.google.common.collect.ArrayListMultimap;
 import org.apache.hadoop.conf.Configuration;
 import org.apache.hadoop.fs.Path;
 import org.apache.hadoop.hive.conf.HiveConf;
 import org.apache.hadoop.hive.ql.io.orc.OrcInputFormat;
 import org.apache.hadoop.hive.ql.io.orc.OrcOutputFormat;
 import org.apache.hadoop.hive.ql.plan.ExprNodeColumnDesc;
+import org.apache.hadoop.hive.ql.plan.ExprNodeConstantDesc;
 import org.apache.hadoop.hive.ql.plan.ExprNodeDesc;
 import org.apache.hadoop.hive.ql.plan.ExprNodeDescUtils;
 import org.apache.hadoop.hive.ql.plan.ExprNodeGenericFuncDesc;
@@ -153,6 +157,25 @@ public void testUnsupportedDeserialization() throws Exception {
     Assert.assertTrue(ExprNodeDescUtils.isSame(validExpr, desc));
   }
 
+  @Test
+  public void testSerializeChildrenFromGuavaCollection() throws Exception {
+    ExprNodeDesc column = new ExprNodeColumnDesc(TypeInfoFactory.stringTypeInfo, "foo", null, false);
+    ExprNodeDesc constant = new ExprNodeConstantDesc(TypeInfoFactory.stringTypeInfo, "bar");
+    ArrayListMultimap<Object, ExprNodeDesc> l = ArrayListMultimap.create();
+    l.put("coltype", column);
+    l.put("coltype", constant);
+
+    FunctionInfo inFunctionInfo = FunctionRegistry.getFunctionInfo("in");
+    Optional<Collection<ExprNodeDesc>> v = l.asMap().values().stream().findFirst();
+    Assert.assertTrue(v.isPresent());
+    List<ExprNodeDesc> children = (List<ExprNodeDesc>) v.get();
+    ExprNodeGenericFuncDesc node = ExprNodeGenericFuncDesc.newInstance(inFunctionInfo.getGenericUDF(), "in", children);
+
+    byte[] buf = SerializationUtilities.serializeObjectWithTypeInformation(node);
+    ExprNodeDesc desc = SerializationUtilities.deserializeObjectWithTypeInformation(buf, true);
+    Assert.assertTrue(ExprNodeDescUtils.isSame(node, desc));
+  }
+
   private MapWork doSerDeser(Configuration configuration) throws Exception, IOException {
     MapWork mapWork = mockMapWorkWithSomePartitionDescProperties();
     ByteArrayOutputStream baos = new ByteArrayOutputStream();