address comments

facebookincubator · Feb 10, 2025 · cca4e68 · cca4e68
1 parent 7ea46d8
commit cca4e68
Show file tree

Hide file tree

Showing 4 changed files with 21 additions and 42 deletions.
diff --git a/velox/core/PlanNode.h b/velox/core/PlanNode.h
@@ -1959,16 +1959,7 @@ class UnnestNode : public PlanNode {
       const std::vector<std::string>& unnestNames,
       const std::optional<std::string>& ordinalityName,
       const PlanNodePtr& source,
-      bool isOuter);
-
-  UnnestNode(
-      const PlanNodeId& id,
-      std::vector<FieldAccessTypedExprPtr> replicateVariables,
-      std::vector<FieldAccessTypedExprPtr> unnestVariables,
-      const std::vector<std::string>& unnestNames,
-      const std::optional<std::string>& ordinalityName,
-      const PlanNodePtr& source):
-      UnnestNode(id, replicateVariables, unnestVariables, unnestNames, ordinalityName, source, false) {}
+      bool isOuter = false);
 
   /// The order of columns in the output is: replicated columns (in the order
   /// specified), unnested columns (in the order specified, for maps: key

diff --git a/velox/exec/Unnest.cpp b/velox/exec/Unnest.cpp
@@ -103,12 +103,8 @@ RowVectorPtr Unnest::getOutput() {
     return nullptr;
   }
 
-  RowVectorPtr output = nullptr;
-  if (isOuter_) {
-    output = generateOutput<true>(rowRange);
-  } else {
-    output = generateOutput<false>(rowRange);
-  }
+  const auto output = isOuter_ ? generateOutput<true>(rowRange)
+                               : generateOutput<false>(rowRange);
 
   if (rowRange.lastRowEnd.has_value()) {
     // The last row is not processed completely.
@@ -255,7 +251,7 @@ VectorPtr Unnest::generateOrdinalityVector(const RowRange& range) {
   vector_size_t index = 0;
   range.forEachRow(
       [&](vector_size_t row, vector_size_t start, vector_size_t size) {
-        if constexpr(isOuter) {
+        if constexpr (isOuter) {
           if (rawOrdinalityIsNull_[row]) {
             ordinalityVector->setNull(index, true);
           }
@@ -312,9 +308,8 @@ RowVectorPtr Unnest::generateOutput(const RowRange& range) {
       std::move(outputs));
 }
 
-
 template <bool isOuter>
-void Unnest::countMaxNumElementsPerRow(int32_t size) {
+void Unnest::countMaxNumElementsPerRow(vector_size_t numRows) {
   for (auto channel = 0; channel < unnestChannels_.size(); ++channel) {
     const auto& unnestVector = input_->childAt(unnestChannels_[channel]);
 
@@ -337,7 +332,7 @@ void Unnest::countMaxNumElementsPerRow(int32_t size) {
     // Count max number of elements per row.
     auto* currentSizes = rawSizes_[channel];
     auto* currentIndices = rawIndices_[channel];
-    for (auto row = 0; row < size; ++row) {
+    for (auto row = 0; row < numRows; ++row) {
       if (!currentDecoded.isNullAt(row)) {
         auto unnestSize = currentSizes[currentIndices[row]];
         if constexpr (isOuter) {

diff --git a/velox/exec/Unnest.h b/velox/exec/Unnest.h
@@ -108,11 +108,11 @@ class Unnest : public Operator {
       std::vector<VectorPtr>& outputs);
 
   // Calculate the max number of elements of each row after unnested.
-  // @param size The number of input rows
-  // @param isOuter Whether we should generate a null element
-  // if the array/map is null or empty then null is produced
+  // @param numRows The number of input rows.
+  // @param isOuter Whether we should generate a null element.
+  // if the array/map is null or empty then null is produced.
   template <bool isOuter>
-  void countMaxNumElementsPerRow(int32_t size);
+  void countMaxNumElementsPerRow(vector_size_t numRows);
 
   struct UnnestChannelEncoding {
     BufferPtr indices;

diff --git a/velox/exec/tests/UnnestTest.cpp b/velox/exec/tests/UnnestTest.cpp
@@ -477,35 +477,28 @@ TEST_P(UnnestTest, batchSize) {
 TEST_P(UnnestTest, basicArrayWithOuter) {
   auto vector = makeRowVector({
       makeFlatVector<int64_t>({1, 2, 3}),
-      makeNullableArrayVector<int64_t>({
-        {1, 2, std::nullopt},
-        {},
-        {3}
-      }),
+      makeNullableArrayVector<int64_t>({{1, 2, std::nullopt}, {}, {3}}),
   });
 
   createDuckDbTable({vector});
 
-  // isOuter = false
-  auto op1 = PlanBuilder().values({vector}).unnest({"c0"}, {"c1"}, std::nullopt, /* isOuter = */ false).planNode();
-  auto params1 = makeCursorParameters(op1);
-  auto expected1 = makeRowVector({
-      makeFlatVector<int64_t>({1, 1, 1, 3}),
-      makeNullableFlatVector<int64_t>({1, 2, std::nullopt, 3}),
-  });
-  assertQuery(params1, expected1);
-
-  // isOuter = true
-  auto op2 = PlanBuilder().values({vector}).unnest({"c0"}, {"c1"}, std::nullopt, /* isOuter = */ true).planNode();
+  // isOuter = true.
+  auto op2 = PlanBuilder()
+                 .values({vector})
+                 .unnest({"c0"}, {"c1"}, std::nullopt, /* isOuter = */ true)
+                 .planNode();
   auto params2 = makeCursorParameters(op2);
   auto expected2 = makeRowVector({
       makeFlatVector<int64_t>({1, 1, 1, 2, 3}),
       makeNullableFlatVector<int64_t>({1, 2, std::nullopt, std::nullopt, 3}),
   });
   assertQuery(params2, expected2);
 
-  // ordinal = true && isOuter = true
-  auto op3 = PlanBuilder().values({vector}).unnest({"c0"}, {"c1"}, "ordinal", /* isOuter = */ true).planNode();
+  // ordinal = true && isOuter = true.
+  auto op3 = PlanBuilder()
+                 .values({vector})
+                 .unnest({"c0"}, {"c1"}, "ordinal", /* isOuter = */ true)
+                 .planNode();
   auto params3 = makeCursorParameters(op3);
   auto expected3 = makeRowVector({
       makeFlatVector<int64_t>({1, 1, 1, 2, 3}),