fix: Ensure ConstantTypedExpr and variant types are equivalent

Author: pramodsatya

velox/core/Expressions.h

@@ -61,7 +61,13 @@ class ConstantTypedExpr : public ITypedExpr {
   // Variant::null() value is supported.
   ConstantTypedExpr(TypePtr type, Variant value)
       : ITypedExpr{ExprKind::kConstant, std::move(type)},
-        value_{std::move(value)} {}
+        value_{std::move(value)} {
+    VELOX_CHECK(
+        value_.isTypeCompatible(ITypedExpr::type()),
+        "Expression type {} does not match variant type {}",
+        ITypedExpr::type()->toString(),
+        value_.inferType()->toString());
+  }
 
   // Creates constant expression of scalar or complex type. The value comes from
   // index zero.

velox/core/tests/ConstantTypedExprTest.cpp

@@ -15,6 +15,7 @@
  */
 #include <gtest/gtest.h>
 
+#include "velox/common/base/tests/GTestUtils.h"
 #include "velox/common/memory/Memory.h"
 #include "velox/core/Expressions.h"
 #include "velox/functions/prestosql/types/HyperLogLogType.h"
@@ -74,11 +75,10 @@ class ConstantTypedExprTest : public ::testing::Test,
   }
 
   // Helper functions
-  template <typename T>
   std::shared_ptr<ConstantTypedExpr> createVariantExpr(
       const TypePtr& type,
-      const T& value) {
-    return std::make_shared<ConstantTypedExpr>(type, variant(value));
+      const Variant& value) {
+    return std::make_shared<ConstantTypedExpr>(type, value);
   }
 
   std::shared_ptr<ConstantTypedExpr> createNullVariantExpr(
@@ -579,4 +579,38 @@ TEST_F(ConstantTypedExprTest, toStringComplexTypes) {
       << "toString mismatch for OPAQUE variant vs vector";
 }
 
+TEST_F(ConstantTypedExprTest, variantTypeCheck) {
+  auto testVariantExpr = [&](const Variant& value,
+                             const TypePtr& type,
+                             const TypePtr& expectedType) {
+    VELOX_ASSERT_THROW(
+        createVariantExpr(type, value),
+        fmt::format(
+            "Expression type {} does not match variant type {}",
+            type->toString(),
+            expectedType->toString()));
+    if (type->isPrimitiveType()) {
+      VELOX_ASSERT_THROW(
+          createVariantExpr(type, Variant::null(expectedType->kind())),
+          fmt::format(
+              "Expression type {} does not match variant type {}",
+              type->toString(),
+              expectedType->toString()));
+    } else {
+      ASSERT_NO_THROW(
+          createVariantExpr(type, Variant::null(expectedType->kind())));
+    }
+  };
+
+  testVariantExpr("abc", INTEGER(), VARCHAR());
+  testVariantExpr(variant(123LL), INTEGER(), BIGINT());
+  testVariantExpr(2.0, BIGINT(), DOUBLE());
+  testVariantExpr(
+      variant::array({1, 2, 3}), ARRAY(VARCHAR()), ARRAY(INTEGER()));
+  testVariantExpr(
+      variant::map({{2.0, "xyz"}}),
+      MAP(INTEGER(), VARCHAR()),
+      MAP(DOUBLE(), VARCHAR()));
+}
+
 } // namespace facebook::velox::core::test

velox/expression/tests/ExprRewriteRegistryTest.cpp

@@ -31,8 +31,7 @@ TEST_F(ExprRewriteRegistryTest, basic) {
       };
   registry.registerRewrite(testRewrite);
 
-  auto input =
-      std::make_shared<core::ConstantTypedExpr>(BIGINT(), variant(123));
+  auto input = std::make_shared<core::ConstantTypedExpr>(BIGINT(), 123LL);
   const auto rewritten = registry.rewrite(input);
   ASSERT_TRUE(rewritten->isCallKind());
   ASSERT_TRUE(rewritten->type()->isBigint());

velox/expression/tests/ExprTest.cpp

@@ -2595,8 +2595,8 @@ TEST_P(ParameterizedExprTest, constantToString) {
 
 TEST_F(ExprTest, constantEqualsNullConsistency) {
   // Constant expr created using variant
-  auto nullVariantToExpr =
-      std::make_shared<core::ConstantTypedExpr>(VARCHAR(), Variant{});
+  auto nullVariantToExpr = std::make_shared<core::ConstantTypedExpr>(
+      VARCHAR(), variant::null(TypeKind::VARCHAR));
   auto nonNullVariantToExpr =
       std::make_shared<core::ConstantTypedExpr>(VARCHAR(), Variant{"test"});