fix(variant): Variant string conversion logic on creation (#15855)

Summary:
Pull Request resolved: #15855

Fixing two issues with variant creation:
- Variant::create() was relying on implicit conversion from StringView into
  std::string. It also did not support construction with std::string_view.
  Adding overloads to make them explicit.
- Variant::typeWithCustomComparison() was also relying on implicit conversion.
  Making the conversion explicit.

Reviewed By: mbasmanova

Differential Revision: D89753870

fbshipit-source-id: 932417f82b115bfcb6314f46d00315ec75f30322

Author: pedroerp

velox/type/Variant.h

@@ -138,8 +138,7 @@ struct VariantTypeTraits<
     KIND,
     usesCustomComparison,
     std::enable_if_t<
-        TypeTraits<KIND>::isPrimitiveType && KIND != TypeKind::VARCHAR &&
-            KIND != TypeKind::VARBINARY,
+        TypeTraits<KIND>::isPrimitiveType && !is_string_kind(KIND),
         void>> {
   using native_type = typename TypeTraits<KIND>::NativeType;
   using stored_type =
@@ -151,9 +150,7 @@ template <TypeKind KIND, bool usesCustomComparison>
 struct VariantTypeTraits<
     KIND,
     usesCustomComparison,
-    std::enable_if_t<
-        KIND == TypeKind::VARCHAR || KIND == TypeKind::VARBINARY,
-        void>> {
+    std::enable_if_t<is_string_kind(KIND), void>> {
   using native_type = std::string_view;
   using stored_type =
       TypeStorage<scalar_stored_type<KIND>, KIND, usesCustomComparison>;
@@ -338,7 +335,25 @@ class Variant {
     return Variant{
         KIND,
         new typename detail::VariantTypeTraits<KIND, false>::stored_type{
-            std::move(v)}};
+            std::move(v)},
+    };
+  }
+
+  // Explicit specializations for other non-deep copied string types.
+  template <TypeKind KIND>
+  static std::enable_if_t<is_string_kind(KIND), Variant> create(StringView v) {
+    return create<KIND>(std::string(v));
+  }
+
+  template <TypeKind KIND>
+  static std::enable_if_t<is_string_kind(KIND), Variant> create(
+      std::string_view v) {
+    return create<KIND>(std::string(v));
+  }
+
+  template <TypeKind KIND>
+  static std::enable_if_t<is_string_kind(KIND), Variant> create(const char* v) {
+    return create<KIND>(std::string(v));
   }
 
   /// Creates a non-null Variant by deducing the TypeKind from the C++ template
@@ -489,10 +504,11 @@ class Variant {
   static Variant typeWithCustomComparison(
       typename TypeTraits<KIND>::NativeType input,
       const TypePtr& type) {
+    using variant_traits = detail::VariantTypeTraits<KIND, true>;
     return {
         KIND,
-        new typename detail::VariantTypeTraits<KIND, true>::stored_type{
-            input,
+        new typename variant_traits::stored_type{
+            typename variant_traits::value_type{std::move(input)},
             std::dynamic_pointer_cast<
                 const CanProvideCustomComparisonType<KIND>>(type)},
         true};

velox/type/tests/VariantTest.cpp

@@ -113,6 +113,15 @@ TEST(VariantCreateTest, varcharType) {
   auto unicodeVariant = Variant::create<TypeKind::VARCHAR>(unicodeStr);
   EXPECT_EQ(unicodeVariant.value<TypeKind::VARCHAR>(), unicodeStr);
 
+  // Other string types.
+  auto stdStringViewVariant =
+      Variant::create<TypeKind::VARCHAR>(std::string_view(longStr));
+  EXPECT_EQ(stdStringViewVariant.value<TypeKind::VARCHAR>(), longStr);
+
+  auto stringViewVariant =
+      Variant::create<TypeKind::VARCHAR>(StringView(longStr));
+  EXPECT_EQ(stringViewVariant.value<TypeKind::VARCHAR>(), longStr);
+
   // Test move semantics
   std::string moveStr = "Move me!";
   auto moveVariant = Variant::create<TypeKind::VARCHAR>(std::move(moveStr));
@@ -136,6 +145,15 @@ TEST(VariantCreateTest, varbinaryType) {
   auto largeVariant = Variant::create<TypeKind::VARBINARY>(largeBinary);
   EXPECT_EQ(largeVariant.value<TypeKind::VARBINARY>(), largeBinary);
 
+  // Other string types.
+  auto stdStringViewVariant =
+      Variant::create<TypeKind::VARBINARY>(std::string_view(simpleBinary));
+  EXPECT_EQ(stdStringViewVariant.value<TypeKind::VARBINARY>(), simpleBinary);
+
+  auto stringViewVariant =
+      Variant::create<TypeKind::VARBINARY>(StringView(simpleBinary));
+  EXPECT_EQ(stringViewVariant.value<TypeKind::VARBINARY>(), simpleBinary);
+
   // Test move semantics.
   std::string moveBinary = "Move binary!";
   auto moveVariant =