Skip to content

Commit de923ea

Browse files
committed
fix: Ensure ConstantTypedExpr and variant types are equivalent
1 parent 68ce43b commit de923ea

File tree

4 files changed

+26
-5
lines changed

4 files changed

+26
-5
lines changed

velox/core/Expressions.h

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,11 @@ class ConstantTypedExpr : public ITypedExpr {
6161
// Variant::null() value is supported.
6262
ConstantTypedExpr(TypePtr type, Variant value)
6363
: ITypedExpr{ExprKind::kConstant, std::move(type)},
64-
value_{std::move(value)} {}
64+
value_{std::move(value)} {
65+
VELOX_CHECK(
66+
value_.isTypeCompatible(ITypedExpr::type()),
67+
"Expression type does not match variant type");
68+
}
6569

6670
// Creates constant expression of scalar or complex type. The value comes from
6771
// index zero.

velox/core/tests/ConstantTypedExprTest.cpp

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
*/
1616
#include <gtest/gtest.h>
1717

18+
#include "velox/common/base/tests/GTestUtils.h"
1819
#include "velox/common/memory/Memory.h"
1920
#include "velox/core/Expressions.h"
2021
#include "velox/functions/prestosql/types/HyperLogLogType.h"
@@ -579,4 +580,21 @@ TEST_F(ConstantTypedExprTest, toStringComplexTypes) {
579580
<< "toString mismatch for OPAQUE variant vs vector";
580581
}
581582

583+
TEST_F(ConstantTypedExprTest, variantTypeCheck) {
584+
VELOX_ASSERT_THROW(
585+
std::make_shared<core::ConstantTypedExpr>(INTEGER(), "abc"),
586+
"Expression type does not match variant type");
587+
VELOX_ASSERT_THROW(
588+
std::make_shared<core::ConstantTypedExpr>(INTEGER(), variant(123LL)),
589+
"Expression type does not match variant type");
590+
VELOX_ASSERT_THROW(
591+
std::make_shared<core::ConstantTypedExpr>(
592+
ARRAY(VARCHAR()), variant::array({1, 2, 3})),
593+
"Expression type does not match variant type");
594+
VELOX_ASSERT_THROW(
595+
std::make_shared<core::ConstantTypedExpr>(
596+
MAP(INTEGER(), VARCHAR()), variant::map({{"abc", "xyz"}})),
597+
"Expression type does not match variant type");
598+
}
599+
582600
} // namespace facebook::velox::core::test

velox/expression/tests/ExprRewriteRegistryTest.cpp

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -31,8 +31,7 @@ TEST_F(ExprRewriteRegistryTest, basic) {
3131
};
3232
registry.registerRewrite(testRewrite);
3333

34-
auto input =
35-
std::make_shared<core::ConstantTypedExpr>(BIGINT(), variant(123));
34+
auto input = std::make_shared<core::ConstantTypedExpr>(BIGINT(), 123LL);
3635
const auto rewritten = registry.rewrite(input);
3736
ASSERT_TRUE(rewritten->isCallKind());
3837
ASSERT_TRUE(rewritten->type()->isBigint());

velox/expression/tests/ExprTest.cpp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2595,8 +2595,8 @@ TEST_P(ParameterizedExprTest, constantToString) {
25952595

25962596
TEST_F(ExprTest, constantEqualsNullConsistency) {
25972597
// Constant expr created using variant
2598-
auto nullVariantToExpr =
2599-
std::make_shared<core::ConstantTypedExpr>(VARCHAR(), Variant{});
2598+
auto nullVariantToExpr = std::make_shared<core::ConstantTypedExpr>(
2599+
VARCHAR(), variant::null(TypeKind::VARCHAR));
26002600
auto nonNullVariantToExpr =
26012601
std::make_shared<core::ConstantTypedExpr>(VARCHAR(), Variant{"test"});
26022602

0 commit comments

Comments
 (0)