Unnest array of rows based on unnestArrayOfRows #7477

aditi-pandit · 2023-11-08T23:50:44Z

Velox unnests array of row type into a single column of the
element row type. Presto also supports unnesting such arrays
into multiple columns one for each child type of element row
type. Adds unnestArrayOfRows to add such unnesting support
in Unnest PlanNode.

Needed for prestodb/presto#20643

netlify · 2023-11-08T23:50:58Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`72d07d4`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/6581ed91418d6a000895bd26

velox/exec/Unnest.cpp

aditi-pandit · 2023-11-17T00:00:01Z

@mbasmanova : Updated the code. PTAL.

velox/core/PlanNode.h

mbasmanova

@aditi-pandit Aditi, thank you for working on this. Overall looks good % a comment about parameter name, another comment about output of UnnestNode::toString() and some comments about tests.

velox/core/PlanNode.cpp

velox/exec/Unnest.cpp

velox/exec/tests/utils/PlanBuilder.h

velox/exec/tests/utils/PlanBuilder.cpp

velox/exec/tests/UnnestTest.cpp

aditi-pandit · 2023-12-15T22:43:00Z

@mbasmanova : Have addressed all the comments. PTAL. #8077 is separated for refactoring the long method in Unnest logic pointed in review.

Velox unnests array of row type into a single column of the element row type. Presto also supports unnesting such arrays into multiple columns one for each child type of element row type. Adds unnestArrayOfRows to add such unnesting support in Unnest PlanNode.

mbasmanova · 2023-12-20T13:14:36Z

@aditi-pandit CI is red. Would you try rebasing?

mbasmanova

@aditi-pandit Aditi, this PR is shaping up nicely. A few remaining comments.

mbasmanova · 2023-12-20T13:15:46Z

velox/core/PlanNode.h

  /// @param ordinalityName Optional name for the ordinality columns. If not
  /// present, ordinality column is not produced.
+  /// @param unnestArrayOfRows Expands array of rows to multiple columns, one


nit: If true, expands .... Otherwise, ...

mbasmanova · 2023-12-20T13:16:44Z

velox/core/PlanNode.h

+            unnestVariables,
+            unnestNames,
+            ordinalityName,
+            true,


Curious if default should be false. Doesn't current logic match unnestArrayOfRows = false?

mbasmanova · 2023-12-20T13:17:48Z

velox/docs/develop/operators.rst

@@ -665,9 +667,11 @@ number starting with 1.
   * - unnestVariables
     - Input columns of type array or map to expand.
   * - unnestNames
-     - Names to use for expanded columns. One name per array column. Two names per map column.
+     - Names to use for expanded columns. One name per array column. Two names per map column. If array of rows (and not legacyUnnest), then one column per child type of the row.


legacyUnnest

Should we update this?

mbasmanova · 2023-12-20T13:19:21Z

velox/exec/tests/PlanNodeToStringTest.cpp

+
+  ASSERT_EQ("-- Unnest\n", plan->toString());
+  ASSERT_EQ(
+      "-- Unnest[a0, unnest arrays of rows] -> c1:INTEGER, a0_e:SMALLINT\n",


Should this example use array(row) and not array(smallint)?

mbasmanova · 2023-12-20T13:20:15Z

velox/exec/tests/QueryAssertionsTest.cpp

@@ -493,4 +494,23 @@ TEST_F(QueryAssertionsTest, intervalDayTime) {
  assertQuery(plan, "SELECT * FROM tmp");
 }

+TEST_F(QueryAssertionsTest, equalPlans) {


What is this test for?

mbasmanova · 2023-12-20T13:21:46Z

velox/exec/tests/UnnestTest.cpp

+
+  // Tests with an array with dictionary encoded elements.
+  VectorFuzzer::Options opts;
+  opts.containerVariableLength = false;


Why false? Usually, these kinds of tests have a loop with 100 or so iterations and allow Fuzzer to generate whatever encoding it chooses using fuzzInputRow.

mbasmanova · 2023-12-20T13:22:05Z

velox/exec/tests/UnnestTest.cpp

+
+  // Test with array of rows (one of which is a constant). Each array is
+  // encoded.
+  vector_size_t size = array1.size();


Let's use fuzzer.

mbasmanova · 2023-12-20T13:22:50Z

velox/exec/tests/UnnestTest.cpp

+        .planNode();
+  };
+
+  auto noUnnestPlan = [&](const VectorPtr& child1,


We should use the same plans but first use data generated by Fuzzer, then use same data but flattened. The results should be the same.

mbasmanova · 2023-12-20T13:23:02Z

velox/exec/tests/utils/PlanBuilder.cpp

@@ -1426,8 +1426,10 @@ PlanBuilder& PlanBuilder::nestedLoopJoin(
 PlanBuilder& PlanBuilder::unnest(
    const std::vector<std::string>& replicateColumns,
    const std::vector<std::string>& unnestColumns,
-    const std::optional<std::string>& ordinalColumn) {
+    const std::optional<std::string>& ordinalColumn,
+    const bool unnestArrayOfRows) {


remove const

velox/exec/tests/utils/QueryAssertions.h

stale · 2024-03-20T03:26:43Z

This pull request has been automatically marked as stale because it has not had recent activity. If you'd still like this PR merged, please comment on the PR, make sure you've addressed reviewer comments, and rebase on the latest main. Thank you for your contributions!

stale · 2024-07-26T05:40:19Z

This pull request has been automatically marked as stale because it has not had recent activity. If you'd still like this PR merged, please comment on the PR, make sure you've addressed reviewer comments, and rebase on the latest main. Thank you for your contributions!

stale · 2024-11-09T00:05:50Z

This pull request has been automatically marked as stale because it has not had recent activity. If you'd still like this PR merged, please comment on the PR, make sure you've addressed reviewer comments, and rebase on the latest main. Thank you for your contributions!

facebook-github-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 Nov 8, 2023

aditi-pandit marked this pull request as draft November 8, 2023 23:50

aditi-pandit force-pushed the unnest_rows branch from 664999b to bf83606 Compare November 14, 2023 23:33

aditi-pandit changed the title ~~Fix unnest for array of rows~~ Unnest array of rows based on legacyUnnest Nov 14, 2023

aditi-pandit force-pushed the unnest_rows branch 4 times, most recently from 6405b9e to 6221695 Compare November 15, 2023 06:07

aditi-pandit marked this pull request as ready for review November 15, 2023 06:07

aditi-pandit requested a review from mbasmanova November 15, 2023 06:08

aditi-pandit force-pushed the unnest_rows branch from 6221695 to 2dfc809 Compare November 15, 2023 06:21

aditi-pandit mentioned this pull request Nov 15, 2023

Fix unnest for array of rows #6880

Closed

mbasmanova reviewed Nov 15, 2023

View reviewed changes

velox/exec/Unnest.cpp Outdated Show resolved Hide resolved

aditi-pandit force-pushed the unnest_rows branch from 2dfc809 to f2d6429 Compare November 16, 2023 23:52

aditi-pandit force-pushed the unnest_rows branch from f2d6429 to 8ede58a Compare November 17, 2023 17:50

aditi-pandit force-pushed the unnest_rows branch 2 times, most recently from e8fad87 to 22a676e Compare December 1, 2023 20:11

mbasmanova reviewed Dec 5, 2023

View reviewed changes

velox/core/PlanNode.h Outdated Show resolved Hide resolved

mbasmanova reviewed Dec 5, 2023

View reviewed changes

aditi-pandit force-pushed the unnest_rows branch from 22a676e to 884ff44 Compare December 8, 2023 21:21

aditi-pandit changed the title ~~Unnest array of rows based on legacyUnnest~~ Unnest array of rows based on unnestArrayOfRows Dec 8, 2023

aditi-pandit force-pushed the unnest_rows branch 3 times, most recently from 3aa76f7 to 7d91e78 Compare December 15, 2023 22:35

aditi-pandit force-pushed the unnest_rows branch 2 times, most recently from 435ff31 to 4efbdc6 Compare December 16, 2023 07:09

aditi-pandit force-pushed the unnest_rows branch from 4efbdc6 to 72d07d4 Compare December 19, 2023 19:22

mbasmanova reviewed Dec 20, 2023

View reviewed changes

stale bot added the stale label Mar 20, 2024

stale bot closed this Apr 11, 2024

aditi-pandit reopened this Apr 26, 2024

stale bot removed the stale label Apr 26, 2024

stale bot added the stale label Jul 26, 2024

stale bot closed this Aug 9, 2024

aditi-pandit reopened this Aug 10, 2024

stale bot removed the stale label Aug 10, 2024

stale bot added the stale label Nov 9, 2024

stale bot closed this Nov 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unnest array of rows based on unnestArrayOfRows #7477

Unnest array of rows based on unnestArrayOfRows #7477

aditi-pandit commented Nov 8, 2023 •

edited

Loading

netlify bot commented Nov 8, 2023 •

edited

Loading

aditi-pandit commented Nov 17, 2023

mbasmanova left a comment

aditi-pandit commented Dec 15, 2023

mbasmanova commented Dec 20, 2023

mbasmanova left a comment

mbasmanova Dec 20, 2023

mbasmanova Dec 20, 2023

mbasmanova Dec 20, 2023

mbasmanova Dec 20, 2023

mbasmanova Dec 20, 2023

mbasmanova Dec 20, 2023

mbasmanova Dec 20, 2023

mbasmanova Dec 20, 2023

mbasmanova Dec 20, 2023

stale bot commented Mar 20, 2024

stale bot commented Jul 26, 2024

stale bot commented Nov 9, 2024

Unnest array of rows based on unnestArrayOfRows #7477

Unnest array of rows based on unnestArrayOfRows #7477

Conversation

aditi-pandit commented Nov 8, 2023 • edited Loading

netlify bot commented Nov 8, 2023 • edited Loading

✅ Deploy Preview for meta-velox canceled.

aditi-pandit commented Nov 17, 2023

mbasmanova left a comment

Choose a reason for hiding this comment

aditi-pandit commented Dec 15, 2023

mbasmanova commented Dec 20, 2023

mbasmanova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Mar 20, 2024

stale bot commented Jul 26, 2024

stale bot commented Nov 9, 2024

aditi-pandit commented Nov 8, 2023 •

edited

Loading

netlify bot commented Nov 8, 2023 •

edited

Loading