fix: Expand operator with constants specified using ConstantVector by jinchengchenghh · Pull Request #15684 · facebookincubator/velox

jinchengchenghh · 2025-12-03T14:49:42Z

The constantExpr->value() only exists when the constant is of scalar type, consider the value vector for complex data type.

-- Expand[1][[1, null, null, n0_1, n0_2, n0_3, n0_4], [4, n0_0, {-1, {{-1}, {k => v}}}, null, null, null, null]] -> n1_0:INTEGER, n1_1:INTEGER, n1_2:ROW<c1:INTEGER,c2:ROW<a:ARRAY<INTEGER>,m:MAP<VARCHAR,VARCHAR>>>, n1_3:VARCHAR, n1_4:BIGINT, n1_5:INTEGER, n1_6:ROW<>
  -- ValueStream[0][] -> n0_0:INTEGER, n0_1:VARCHAR, n0_2:BIGINT, n0_3:INTEGER, n0_4:ROW<>

Returns null for complex constant before this fix.

VeloxColumnarToRowConverter 0: {4, 1, null, null, null, ...2 more}

Create the constant vector in Expand operator constructor to reuse it.

netlify · 2025-12-03T14:49:48Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`c9e3e87`
🔍 Latest deploy log	https://app.netlify.com/projects/meta-velox/deploys/694a1660b6c0390008714d66

jinchengchenghh · 2025-12-03T15:00:01Z

velox/exec/Expand.cpp

@@ -86,16 +86,7 @@ RowVectorPtr Expand::getOutput() {

  for (auto i = 0; i < numColumns; ++i) {
    if (rowProjection[i] == kConstantChannel) {
-      const auto& constantExpr = constantProjection[i];
-      if (constantExpr->value().isNull()) {


This check is not correct

JkSelf

LGTM. Thanks for your fix.

jinchengchenghh · 2025-12-05T03:40:48Z

Hi, @mbasmanova , could you help review this PR? Thanks

velox/exec/tests/ExpandTest.cpp

mbasmanova · 2025-12-05T12:58:13Z

velox/exec/tests/ExpandTest.cpp

+  auto constantExpr = std::make_shared<core::ConstantTypedExpr>(arrayVector);
+
+  auto plan =
+      PlanBuilder().values({data}).expand({{constantExpr}}, {"c1"}).planNode();


Can we do something like

.expand({ {"k1", "k2", "a", "b", "[1, 2, 3] as c"}, })

I receive the exception after updated to this

unknown file: Failure C++ exception with description "Exception: VeloxUserError Error Source: USER Error Code: INVALID_ARGUMENT Reason: Scalar function doesn't exist: list_value. Retriable: False Function: resolveScalarFunctionType File: /Users/chengchengjin/code/velox/velox/parse/TypeResolver.cpp Line: 97

TEST_F(ExpandTest, complexConstant) { auto data = makeRowVectorData(1); auto arrayVector = makeArrayVector<int32_t>({{1, 2, 3}}); // auto constantExpr = std::make_shared<core::ConstantTypedExpr>(arrayVector); auto plan = PlanBuilder() .values({data}) .expand({ {"k1", "k2", "a", "b", "[1, 2, 3] as c"}, }) .planNode(); auto children = data->children(); children.push_back(arrayVector); auto expected = makeRowVector(children); assertQuery(plan, expected); }

Can not use array_constructor also, the list_value is parsed as array_constructor in here, so even if we resolve the parse issue, we still cannot use the projection to test expand node.

velox/velox/parse/QueryPlanner.cpp

Line 87 in a886644

{"list_value", "array_constructor"},

Error Code: INVALID_ARGUMENT Reason: Unsupported projection expression in Expand plan node. Expected field reference or constant. Got: array_constructor(1,2,3) Retriable: False Expression: TypedExprs::isFieldAccess(columnProjection) || TypedExprs::isConstant(columnProjection) Function: ExpandNode File: /Users/chengchengjin/code/velox/velox/core/PlanNode.cpp

This can work well, I will update to this, thanks for your suggestion!

TEST_F(ExpandTest, complexConstant) { auto data = makeRowVectorData(1); auto children = data->children(); auto arrayVector = makeArrayVector<int32_t>({{1, 2, 3}}); auto complexConstants = makeRowVector({arrayVector}); children.push_back(arrayVector); auto expected = makeRowVector({"k1", "k2", "a", "b", "c0"}, children); // auto constantExpr = std::make_shared<core::ConstantTypedExpr>(arrayVector); auto plan = PlanBuilder() .values({expected}) .expand({ {"k1", "k2", "a", "b", "__complex_constant(c0) as c"} }, complexConstants) .planNode(); assertQuery(plan, expected); }

I think I missed 'ARRAY' keyword in the original example. Something like this should work:

PlanBuilder(pool()) ... .expand({ {"k1", "k2", "a", "b", "ARRAY[1, 2, 3] as c"}, })

Thanks! Updated and works well.

mbasmanova · 2025-12-05T12:59:52Z

velox/exec/Expand.cpp

-        outputColumns[i] = BaseVector::createConstant(
-            constantExpr->type(), constantExpr->value(), numInput, pool());
-      }
+      outputColumns[i] = constantProjection[i]->toConstantVector(pool());


This fix looks good. I wonder if we can avoid making new vector for each output batch and instead create these once and reuse.

mbasmanova · 2025-12-22T06:58:42Z

velox/exec/tests/ExpandTest.cpp

+  auto children = otherColumns->children();
+  auto arrayVector = makeArrayVector<int32_t>({{1, 2, 3}});
+  children.push_back(arrayVector);
+  std::vector<VectorPtr> complexConstants(children.size());


complexConstants not needed anymore

mbasmanova · 2025-12-22T06:59:32Z

velox/exec/tests/ExpandTest.cpp

+  auto plan = PlanBuilder(pool())
+                  .values({expected})
+                  .expand(
+                      {{"k1", "k2", "a", "b", "ARRAY[1, 2, 3] as c"}})
+                  .planNode();
+
+  assertQuery(plan, expected);


It is a bit strange that 'expected' is used as both the input and output of the plan; is this intentional?

mbasmanova · 2025-12-22T07:04:24Z

velox/exec/Expand.cpp


  const auto& rowProjection = fieldProjections_[rowIndex_];
-  const auto& constantProjection = constantProjections_[rowIndex_];
+  const auto& constantProjection = constantOutputs_[rowIndex_];


constantOutputs_ contains vectors of size 1, but here we need to produce a vector of size numInput

I just realized that we cannot reuse the vector and may need to call BaseVector::wrapInConstant(numInput, 0, constantOutputs_[rowIndex_]).

mbasmanova · 2025-12-22T07:05:28Z

velox/exec/tests/ExpandTest.cpp

-} // anonymous namespace
+
+TEST_F(ExpandTest, complexConstant) {
+  auto otherColumns = makeRowVectorData(1);


Let's use size > 1. Otherwise, we won't be able to catch a bug mentioned above.

mbasmanova · 2025-12-22T07:50:16Z

velox/exec/tests/ExpandTest.cpp

+
+  auto plan = PlanBuilder(pool())
+                  .values({data})
+                  .expand({{"k1", "k2", "a", "b", "ARRAY[1, 2, 3] as c"}})


Should we also test null value?

null::integer[]

mbasmanova

Thanks.

jinchengchenghh · 2025-12-22T08:36:55Z

After add more tests, I receive the exception which requires not allocate memory in Operator constructor, so I move the memory allocation to initialize CC @mbasmanova

E20251222 16:23:01.206250 24729043 Exceptions.h:53] Line: /Users/chengchengjin/code/velox/velox/exec/Task.cpp:1054, Function:createAndStartDrivers, Expression:  Unexpected memory pool allocations during task[test_cursor_1] driver initialization: 
Top 1 leaf memory pool usages:
    op.1.0.0.Expand usage 768B reserved 1.00MB peak 768B

query.TaskCursorQuery_0.0 usage 768B reserved 1.00MB peak 1.00MB
    task.test_cursor_1 usage 768B reserved 1.00MB peak 1.00MB
        node.1 usage 768B reserved 1.00MB peak 1.00MB
            op.1.0.0.Expand usage 768B reserved 1.00MB peak 768B

, Source: RUNTIME, ErrorCode: INVALID_STATE
unknown file: Failure
C++ exception with description "Exception: VeloxRuntimeError
Error Source: RUNTIME
Error Code: INVALID_STATE
Reason: Unexpected memory pool allocations during task[test_cursor_1] driver initialization:

meta-codesync · 2025-12-22T14:52:23Z

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

kKPulla · 2025-12-22T14:53:36Z

@jinchengchenghh I just imported the PR to merge and while I wait for all approvals and signals to pass, can you please take a look at the test failures from the CI signals above and fix if relevant?

jinchengchenghh · 2025-12-23T08:18:22Z

Could you help import it? I have resolved the failed CI. Thanks! @kKPulla

meta-codesync · 2025-12-23T21:12:57Z

@kKPulla merged this pull request in 45093a7.

…15684) Summary: The constantExpr->value() only exists when the constant is of scalar type, consider the value vector for complex data type. ``` -- Expand[1][[1, null, null, n0_1, n0_2, n0_3, n0_4], [4, n0_0, {-1, {{-1}, {k => v}}}, null, null, null, null]] -> n1_0:INTEGER, n1_1:INTEGER, n1_2:ROW<c1:INTEGER,c2:ROW<a:ARRAY<INTEGER>,m:MAP<VARCHAR,VARCHAR>>>, n1_3:VARCHAR, n1_4:BIGINT, n1_5:INTEGER, n1_6:ROW<> -- ValueStream[0][] -> n0_0:INTEGER, n0_1:VARCHAR, n0_2:BIGINT, n0_3:INTEGER, n0_4:ROW<> ``` Returns null for complex constant before this fix. ``` VeloxColumnarToRowConverter 0: {4, 1, null, null, null, ...2 more} ``` Create the constant vector in Expand operator constructor to reuse it. Pull Request resolved: facebookincubator/velox#15684 Reviewed By: kagamiori Differential Revision: D89665504 Pulled By: kKPulla fbshipit-source-id: d0167e35a1ee2a8a8e0230a27dfe9fd0e424f923

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 Dec 3, 2025

jinchengchenghh force-pushed the fixExpand branch from d3e3537 to c1bbbee Compare December 3, 2025 14:58

jinchengchenghh requested review from JkSelf and mbasmanova December 3, 2025 14:59

jinchengchenghh commented Dec 3, 2025

View reviewed changes

JkSelf approved these changes Dec 4, 2025

View reviewed changes

mbasmanova reviewed Dec 5, 2025

View reviewed changes

velox/exec/tests/ExpandTest.cpp Show resolved Hide resolved

mbasmanova reviewed Dec 5, 2025

View reviewed changes

jinchengchenghh force-pushed the fixExpand branch from d9a2a1a to e0d0b86 Compare December 22, 2025 06:01

fix: Fix Expand operator for constant vector with value vector

b8c5993

jinchengchenghh force-pushed the fixExpand branch from e0d0b86 to b8c5993 Compare December 22, 2025 06:25

mbasmanova reviewed Dec 22, 2025

View reviewed changes

fix multiple rows

9ed08bd

mbasmanova reviewed Dec 22, 2025

View reviewed changes

mbasmanova approved these changes Dec 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 Dec 22, 2025

mbasmanova changed the title ~~fix: Fix Expand operator for constant vector with value vector~~ fix: Expand operator with constants specified using ConstantVector Dec 22, 2025

add more tests and move memory allocation to function initialize

a7680a2

fix the constantOutputs_ initialization

c9e3e87

meta-codesync bot closed this in 45093a7 Dec 23, 2025

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

jinchengchenghh mentioned this pull request Dec 30, 2025

[GLUTEN-11145][VL] Iceberg test supports Spark3.5 and Spark4.0 apache/gluten#11146

Merged

Conversation

jinchengchenghh commented Dec 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify bot commented Dec 3, 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!

JkSelf left a comment

Choose a reason for hiding this comment

Uh oh!

jinchengchenghh commented Dec 5, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jinchengchenghh Dec 22, 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!

mbasmanova Dec 22, 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!

mbasmanova left a comment

Choose a reason for hiding this comment

Uh oh!

jinchengchenghh commented Dec 22, 2025

Uh oh!

meta-codesync bot commented Dec 22, 2025

Uh oh!

kKPulla commented Dec 22, 2025

Uh oh!

jinchengchenghh commented Dec 23, 2025

Uh oh!

meta-codesync bot commented Dec 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jinchengchenghh commented Dec 3, 2025 •

edited

Loading

netlify bot commented Dec 3, 2025 •

edited

Loading

jinchengchenghh Dec 22, 2025 •

edited

Loading

mbasmanova Dec 22, 2025 •

edited

Loading