From c4dd224ea8e5ef2f12b6f4022bc25a0963a94eb2 Mon Sep 17 00:00:00 2001
From: "joey.ljy" <joey.ljy@alibaba-inc.com>
Date: Sat, 30 Mar 2024 09:52:08 +0800
Subject: [PATCH] Address comments and rebase

---
 velox/docs/functions/spark/aggregate.rst                 | 5 +++++
 .../sparksql/aggregates/CollectListAggregate.cpp         | 9 +++++----
 .../functions/sparksql/aggregates/CollectListAggregate.h | 3 +--
 3 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/velox/docs/functions/spark/aggregate.rst b/velox/docs/functions/spark/aggregate.rst
index 18c501ff99ac..d0d2f319858a 100644
--- a/velox/docs/functions/spark/aggregate.rst
+++ b/velox/docs/functions/spark/aggregate.rst
@@ -53,6 +53,11 @@ General Aggregate Functions
 
     ``hash`` cannot be null.
 
+.. spark:function:: collect_list(x) -> array<[same as x]>
+
+    Returns an array created from the input ``x`` elements. Ignores null
+    inputs, and return an empty array when all inputs are null.
+
 .. spark:function:: first(x) -> x
 
     Returns the first value of `x`.
diff --git a/velox/functions/sparksql/aggregates/CollectListAggregate.cpp b/velox/functions/sparksql/aggregates/CollectListAggregate.cpp
index 101eee9d8a11..e2c14cfa7969 100644
--- a/velox/functions/sparksql/aggregates/CollectListAggregate.cpp
+++ b/velox/functions/sparksql/aggregates/CollectListAggregate.cpp
@@ -16,6 +16,9 @@
 
 #include "velox/functions/sparksql/aggregates/CollectListAggregate.h"
 
+#include "velox/exec/SimpleAggregateAdapter.h"
+#include "velox/functions/lib/aggregates/ValueList.h"
+
 using namespace facebook::velox::aggregate;
 using namespace facebook::velox::exec;
 
@@ -48,8 +51,6 @@ class CollectListAggregate {
   struct AccumulatorType {
     ValueList elements_;
 
-    AccumulatorType() = delete;
-
     explicit AccumulatorType(HashStringAllocator* /*allocator*/)
         : elements_{} {}
 
@@ -81,7 +82,7 @@ class CollectListAggregate {
         bool /*nonNullGroup*/,
         exec::out_type<IntermediateType>& out) {
       // If the group's accumulator is null, the corresponding intermediate
-      // result is an empty list.
+      // result is an empty array.
       copyValueListToArrayWriter(out, elements_);
       return true;
     }
@@ -90,7 +91,7 @@ class CollectListAggregate {
         bool /*nonNullGroup*/,
         exec::out_type<OutputType>& out) {
       // If the group's accumulator is null, the corresponding result is an
-      // empty list.
+      // empty array.
       copyValueListToArrayWriter(out, elements_);
       return true;
     }
diff --git a/velox/functions/sparksql/aggregates/CollectListAggregate.h b/velox/functions/sparksql/aggregates/CollectListAggregate.h
index 5696d68e1f0d..3c32023db95e 100644
--- a/velox/functions/sparksql/aggregates/CollectListAggregate.h
+++ b/velox/functions/sparksql/aggregates/CollectListAggregate.h
@@ -16,8 +16,7 @@
 
 #pragma once
 
-#include "velox/exec/SimpleAggregateAdapter.h"
-#include "velox/functions/lib/aggregates/ValueList.h"
+#include <string>
 
 namespace facebook::velox::functions::aggregate::sparksql {