Expand push_back for arrayWriter support to generics and strings. (fa…

…cebookincubator#8334)

Summary:
Pull Request resolved: facebookincubator#8334

This simplify functions  in two ways:
-  one common pattern in functions is
```
     if (inputArray[i].has_value()) {
        auto& newItem = out.add_item();
        newItem.copy_from(inputArray[i].value());
      } else {
        out.add_null();
      }
````
now can be done in one line
      out.push_back(inputArray[i]);.

- since primitive and generic with this have same interface many duplicate of functions body wont be needed.

Reviewed By: amitkdutta

Differential Revision: D52672124

fbshipit-source-id: 1397461b59a82d7aec99d762e6f1c955191df55a

velox/expression/ComplexWriterTypes.h

@@ -137,6 +137,42 @@ class ArrayWriter {
     }
   }
 
+  void push_back(const OptionalAccessor<V>& item) {
+    if (!item.has_value()) {
+      add_null();
+    } else {
+      if constexpr (provide_std_interface<V>) {
+        push_back(item.value());
+      } else {
+        add_item().copy_from(item.value());
+      }
+    }
+  }
+
+  void push_back(const std::optional<GenericView>& inputView) {
+    if (inputView.has_value()) {
+      push_back(*inputView);
+    } else {
+      add_null();
+    }
+  }
+
+  void push_back(const GenericView& item) {
+    add_item().copy_from(item);
+  }
+
+  void push_back(const StringView& item) {
+    add_item().copy_from(item);
+  }
+
+  void push_back(const std::optional<StringView>& item) {
+    if (item.has_value()) {
+      push_back(*item);
+    } else {
+      add_null();
+    }
+  }
+
   // Add a new null item to the array.
   void add_null() {
     resize(length_ + 1);

velox/expression/tests/ArrayWriterTest.cpp

@@ -316,12 +316,16 @@ TEST_F(ArrayWriterTest, testVarChar) {
         stringWriter,
         "test a long string, a bit longer than that, longer, and longer");
   }
+
+  arrayWriter.push_back("new_feature"_sv);
+
   vectorWriter.commit();
   auto expected = std::vector<std::vector<std::optional<StringView>>>{
       {"hi"_sv,
        std::nullopt,
        "welcome"_sv,
-       "test a long string, a bit longer than that, longer, and longer"_sv}};
+       "test a long string, a bit longer than that, longer, and longer"_sv,
+       "new_feature"_sv}};
   assertEqualVectors(result, makeNullableArrayVector(expected));
 }
 
@@ -853,7 +857,6 @@ TEST_F(ArrayWriterTest, nestedArrayWriteThenCommitNull) {
     auto& nestedArray = current.add_item();
     nestedArray.push_back(1);
     nestedArray.push_back(2);
-
     writer.commitNull();
   }
 

velox/expression/tests/GenericWriterTest.cpp

@@ -116,12 +116,7 @@ struct ArrayTrimFunction {
       const int64_t& n) {
     int64_t end = inputArray.size() - n;
     for (int i = 0; i < end; ++i) {
-      if (inputArray[i].has_value()) {
-        auto& newItem = out.add_item();
-        newItem.copy_from(inputArray[i].value());
-      } else {
-        out.add_null();
-      }
+      out.push_back(inputArray[i]);
     }
   }
 };

velox/functions/prestosql/ArrayFunctions.h

@@ -276,10 +276,11 @@ struct CombinationsFunction {
         innerArray.add_null();
         continue;
       }
+
       if constexpr (std::is_same_v<T, Varchar>) {
         innerArray.add_item().setNoCopy(array[idx].value());
       } else {
-        innerArray.add_item() = array[idx].value();
+        innerArray.push_back(array[idx].value());
       }
     }
   }
@@ -573,33 +574,21 @@ struct ArrayConcatFunction {
     }
   }
 
-  template <typename B, typename U>
-  void assignElement(B& out, const U& input) {
-    if constexpr (SimpleTypeTrait<T>::isPrimitiveType) {
-      // Primitives we just assign. copy_from not defined for primitives.
-      out = input;
-    } else {
-      out.copy_from(input);
-    }
-  }
-
   void call(
       out_type<Array<T>>& out,
       const arg_type<Array<T>>& array,
       const arg_type<T>& element) {
     out.reserve(array.size() + 1);
     out.add_items(array);
-    auto& newItem = out.add_item();
-    assignElement(newItem, element);
+    out.push_back(element);
   }
 
   void call(
       out_type<Array<T>>& out,
       const arg_type<T>& element,
       const arg_type<Array<T>>& array) {
     out.reserve(array.size() + 1);
-    auto& newItem = out.add_item();
-    assignElement(newItem, element);
+    out.push_back(element);
     out.add_items(array);
   }
 };
@@ -628,12 +617,7 @@ struct ArrayTrimFunction {
 
     int64_t end = inputArray.size() - size;
     for (int i = 0; i < end; ++i) {
-      if (inputArray[i].has_value()) {
-        auto& newItem = out.add_item();
-        newItem = inputArray[i].value();
-      } else {
-        out.add_null();
-      }
+      out.push_back(inputArray[i]);
     }
   }
 
@@ -646,12 +630,7 @@ struct ArrayTrimFunction {
 
     int64_t end = inputArray.size() - size;
     for (int i = 0; i < end; ++i) {
-      if (inputArray[i].has_value()) {
-        auto& newItem = out.add_item();
-        newItem.copy_from(inputArray[i].value());
-      } else {
-        out.add_null();
-      }
+      out.push_back(inputArray[i]);
     }
   }
 };
@@ -690,8 +669,7 @@ struct ArrayRemoveNullFunction {
   FOLLY_ALWAYS_INLINE void call(Out& out, const In& inputArray) {
     for (int i = 0; i < inputArray.size(); ++i) {
       if (inputArray[i].has_value()) {
-        auto& newItem = out.add_item();
-        newItem = inputArray[i].value();
+        out.push_back(inputArray[i].value());
       }
     }
   }
@@ -702,8 +680,7 @@ struct ArrayRemoveNullFunction {
       const arg_type<Array<Generic<T1>>>& inputArray) {
     for (int i = 0; i < inputArray.size(); ++i) {
       if (inputArray[i].has_value()) {
-        auto& newItem = out.add_item();
-        newItem.copy_from(inputArray[i].value());
+        out.push_back(inputArray[i].value());
       }
     }
   }
@@ -766,7 +743,6 @@ template <typename T>
 struct ArrayUnionFunction {
   VELOX_DEFINE_FUNCTION_TYPES(T)
 
-  // Fast path for primitives.
   template <typename Out, typename In>
   void call(Out& out, const In& inputArray1, const In& inputArray2) {
     folly::F14FastSet<typename In::element_t> elementSet;
@@ -775,31 +751,7 @@ struct ArrayUnionFunction {
       for (const auto& item : inputArray) {
         if (item.has_value()) {
           if (elementSet.insert(item.value()).second) {
-            auto& newItem = out.add_item();
-            newItem = item.value();
-          }
-        } else if (!nullAdded) {
-          nullAdded = true;
-          out.add_null();
-        }
-      }
-    };
-    addItems(inputArray1);
-    addItems(inputArray2);
-  }
-
-  void call(
-      out_type<Array<Generic<T1>>>& out,
-      const arg_type<Array<Generic<T1>>>& inputArray1,
-      const arg_type<Array<Generic<T1>>>& inputArray2) {
-    folly::F14FastSet<exec::GenericView> elementSet;
-    bool nullAdded = false;
-    auto addItems = [&](auto& inputArray) {
-      for (const auto& item : inputArray) {
-        if (item.has_value()) {
-          if (elementSet.insert(item.value()).second) {
-            auto& newItem = out.add_item();
-            newItem.copy_from(item.value());
+            out.push_back(item.value());
           }
         } else if (!nullAdded) {
           nullAdded = true;
@@ -827,8 +779,7 @@ struct ArrayRemoveFunction {
     for (const auto& item : inputArray) {
       if (item.has_value()) {
         if (element != item.value()) {
-          auto& newItem = out.add_item();
-          newItem = item.value();
+          out.push_back(item.value());
         }
       } else {
         out.add_null();
@@ -845,26 +796,22 @@ struct ArrayRemoveFunction {
         CompareFlags::NullHandlingMode::kNullAsIndeterminate);
     std::vector<std::optional<exec::GenericView>> toCopyItems;
     for (const auto& item : array) {
-      if (item.has_value()) {
-        auto result = element.compare(item.value(), kFlags);
-        VELOX_USER_CHECK(
-            result.has_value(),
-            "array_remove does not support arrays with elements that are null or contain null")
-        if (result.value()) {
-          toCopyItems.push_back(item.value());
-        }
-      } else {
+      if (!item.has_value()) {
         toCopyItems.push_back(std::nullopt);
+        continue;
+      }
+
+      auto result = element.compare(item.value(), kFlags);
+      VELOX_USER_CHECK(
+          result.has_value(),
+          "array_remove does not support arrays with elements that are null or contain null")
+      if (result.value()) {
+        toCopyItems.push_back(item.value());
       }
     }
 
     for (const auto& item : toCopyItems) {
-      if (item.has_value()) {
-        auto& newItem = out.add_item();
-        newItem.copy_from(item.value());
-      } else {
-        out.add_null();
-      }
+      out.push_back(item);
     }
   }
 };
@@ -884,8 +831,7 @@ struct ArrayRemoveFunctionString {
       if (item.has_value()) {
         auto result = element.compare(item.value());
         if (result) {
-          auto& newItem = out.add_item();
-          newItem.setNoCopy(item.value());
+          out.push_back(item.value());
         }
       } else {
         out.add_null();

velox/functions/prestosql/MultimapFromEntries.h

@@ -45,12 +45,9 @@ struct MultimapFromEntriesFunction {
     for (const auto& [key, values] : keyValuesMap_) {
       auto [keyWriter, valueWriter] = out.add_item();
       keyWriter.copy_from(key);
+
       for (const auto& value : values) {
-        if (value.has_value()) {
-          valueWriter.add_item().copy_from(value.value());
-        } else {
-          valueWriter.add_null();
-        }
+        valueWriter.push_back(value);
       }
     }
   }