fix: Ensure ConstantTypedExpr and variant types are compatible #15217

pramodsatya · 2025-10-17T23:55:07Z

No description provided.

netlify · 2025-10-17T23:55:19Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`a017702`
🔍 Latest deploy log	https://app.netlify.com/projects/meta-velox/deploys/68f9288d55b0780008eab98d

mbasmanova · 2025-10-18T10:51:55Z

velox/core/Expressions.h

-        value_{std::move(value)} {}
+        value_{std::move(value)} {
+    VELOX_CHECK_EQ(
+        value_.kind(),


This check is not sufficient for complex types. Use isTypeCompatible instead. Please, add tests to ensure that ConstantTypedExpr(ARRAY(VARCHAR()), variant::array({1,2,3}) fails.

Thanks @mbasmanova, updated to use isTypeCompatible. Could you PTAL?

mbasmanova · 2025-10-18T10:53:08Z

velox/expression/tests/ExprTest.cpp

  // Constant expr created using variant
-  auto nullVariantToExpr =
-      std::make_shared<core::ConstantTypedExpr>(VARCHAR(), Variant{});
+  auto nullVariantToExpr = std::make_shared<core::ConstantTypedExpr>(


This change is incorrect. Once you use isTypeCompatible it won't be needed.

Variant{} is initialized with TypeKind::INVALID here, so the null variant needs to be initialized with matching type, otherwise the test fails with below error using isTypeCompatible:

C++ exception with description "Exception: VeloxRuntimeError Error Source: RUNTIME Error Code: INVALID_STATE Reason: Expression type does not match variant type Retriable: False Expression: value_.isTypeCompatible(ITypedExpr::type()) Function: ConstantTypedExpr

Is this fine?

@pramodsatya I see. This is interesting. We may need to look into the use of TypeKind::INVALID. It feels like this shouldn't exist in the first place. Especially since I assume Variant::isNull() returns true for Variant{}. Perhaps, we need another fix to replace TypeKind::INVALID with TypeKind::UNKNOWN in Variant{} constructor or just handle INVALID the same way as UNKNOWN everywhere else. Would you create an issue about this? The change in this PR is fine.

Thanks @mbasmanova, I agree the use of TypeKind::INVALID seems incorrect here and changing to TypeKind::UNKNOWN would be better: pramodsatya@b760e87. Should I open an issue for further discussion or shall I open a PR with this commit?

@pramodsatya Please, open a PR. Thanks.

Thanks @mbasmanova, opened #15250, please take a look when you get a chance

mbasmanova · 2025-10-20T10:56:12Z

velox/core/Expressions.h

+        value_{std::move(value)} {
+    VELOX_CHECK(
+        value_.isTypeCompatible(ITypedExpr::type()),
+        "Expression type does not match variant type");


Let's include both types in the error message to help troubleshooting?

mbasmanova · 2025-10-20T10:58:07Z

velox/core/tests/ConstantTypedExprTest.cpp


+TEST_F(ConstantTypedExprTest, variantTypeCheck) {
+  VELOX_ASSERT_THROW(
+      std::make_shared<core::ConstantTypedExpr>(INTEGER(), "abc"),


Let's introduce a helper lambda and a constant for error message to reduce copy-paste.

VELOX_ASSERT_THROW(makeConstant(INTEGER(), "abc"), ...), kTypeMismatch);

mbasmanova · 2025-10-20T10:58:33Z

velox/core/tests/ConstantTypedExprTest.cpp

+      std::make_shared<core::ConstantTypedExpr>(INTEGER(), "abc"),
+      "Expression type does not match variant type");
+  VELOX_ASSERT_THROW(
+      std::make_shared<core::ConstantTypedExpr>(INTEGER(), variant(123LL)),


Let's add tests that use null variants.

mbasmanova · 2025-10-22T11:15:39Z

velox/core/tests/ConstantTypedExprTest.cpp


+TEST_F(ConstantTypedExprTest, variantTypeCheck) {
+  auto testVariantExpr =
+      [&](Variant value, const TypePtr type, const TypePtr expectedType) {


pass arguments by const &

mbasmanova · 2025-10-22T11:17:11Z

velox/core/tests/ConstantTypedExprTest.cpp

+                "Expression type {} does not match variant type {}",
+                type->toString(),
+                expectedType->toString()));
+        if (type->isPrimitiveType()) {


Would you add an else branch to verify that createVariantExpr succeeds?

mbasmanova

Thanks.

meta-codesync · 2025-10-22T20:21:54Z

@mbasmanova has imported this pull request. If you are a Meta employee, you can view this in D85276445.

meta-codesync · 2025-10-23T01:53:50Z

@mbasmanova merged this pull request in cf403b8.

…ookincubator#15217) Summary: Pull Request resolved: facebookincubator#15217 Reviewed By: peterenescu Differential Revision: D85276445 Pulled By: mbasmanova fbshipit-source-id: ddf6a5f25541343d479bf0c00e6d48f3de79da0b

meta-cla bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 17, 2025

pramodsatya mentioned this pull request Oct 17, 2025

feat: Add ExprOptimizer for constant folding and rewrites #14717

Closed

pramodsatya requested review from aditi-pandit, czentgr and mbasmanova October 18, 2025 03:47

mbasmanova reviewed Oct 18, 2025

View reviewed changes

pramodsatya force-pushed the const_expr_type_chk branch from 3df1b49 to de923ea Compare October 19, 2025 01:30

mbasmanova reviewed Oct 20, 2025

View reviewed changes

pramodsatya force-pushed the const_expr_type_chk branch from de923ea to d908948 Compare October 22, 2025 05:47

pramodsatya requested a review from mbasmanova October 22, 2025 05:47

mbasmanova reviewed Oct 22, 2025

View reviewed changes

fix: Ensure ConstantTypedExpr and variant types are equivalent

a017702

pramodsatya force-pushed the const_expr_type_chk branch from d908948 to a017702 Compare October 22, 2025 18:55

mbasmanova changed the title ~~fix: Ensure ConstantTypedExpr and variant types are equivalent~~ fix: Ensure ConstantTypedExpr and variant types are compatible Oct 22, 2025

mbasmanova approved these changes Oct 22, 2025

View reviewed changes

mbasmanova added the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label Oct 22, 2025

meta-codesync bot closed this in cf403b8 Oct 23, 2025

facebook-github-bot added the Merged label Oct 23, 2025

fix: Ensure ConstantTypedExpr and variant types are compatible #15217

fix: Ensure ConstantTypedExpr and variant types are compatible #15217

Conversation

pramodsatya commented Oct 17, 2025 • edited by mbasmanova Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify bot commented Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for meta-velox canceled.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pramodsatya Oct 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbasmanova left a comment

Choose a reason for hiding this comment

Uh oh!

meta-codesync bot commented Oct 22, 2025

Uh oh!

meta-codesync bot commented Oct 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pramodsatya commented Oct 17, 2025 •

edited by mbasmanova

Loading

netlify bot commented Oct 17, 2025 •

edited

Loading

pramodsatya Oct 19, 2025 •

edited

Loading