Add concat_ws Spark function #8854

PHILO-HE · 2024-02-26T09:01:59Z

Add concat_ws Spark function which returns the concatenation for the
input, separated by a separator (the first argument). It allows variable
number of VARCHAR or ARRAY<VARCHAR> arguments. And these two
types can be used in combination.

This function is a bit similar to ConcatFunction, except that concat_ws
requires separator and supports using ARRAY type and mixed types.

This PR is based on #6292 (author: @unigof). There are a few
improvements to align with Spark's behavior.

Doc link.

netlify · 2024-02-26T09:02:18Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`ca1250c`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/667930042952aa0008314f0b

PHILO-HE · 2024-02-28T08:03:56Z

Found Spark supports arguments with String/Array type mixed used. Needs to enhance this patch.

SELECT concat_ws(,, a, b, array(c, d), e);
"a,b,c,d,e"

PHILO-HE · 2024-04-10T03:08:04Z

Found Spark supports arguments with String/Array type mixed used. Needs to enhance this patch.
SELECT concat_ws(,, a, b, array(c, d), e);
"a,b,c,d,e"

Supported such case in the latest code.

PHILO-HE · 2024-04-10T03:08:27Z

@rui-mo, could you take a review?

rui-mo · 2024-04-10T09:06:54Z

velox/docs/functions/spark/string.rst

+.. spark:function:: concat_ws(separator, [string]/[array<string>], ...) -> varchar
+
+   Returns the concatenation for ``string`` & all elements in ``array<string>``, separated by
+   ``separator``. Only accepts constant ``separator``. It takes variable number of remaining


Does Spark allow non-constant separator?

Confirmed, yes. Just updated the code to support this. Thanks!

rui-mo · 2024-04-10T09:08:24Z

velox/docs/functions/spark/string.rst

@@ -20,6 +20,21 @@ Unless specified otherwise, all functions return NULL if at least one of the arg
    If ``n < 0``, the result is an empty string.
    If ``n >= 256``, the result is equivalent to chr(``n % 256``).

+.. spark:function:: concat_ws(separator, [string]/[array<string>], ...) -> varchar
+
+   Returns the concatenation for ``string`` & all elements in ``array<string>``, separated by


nit: & to and?

rui-mo · 2024-04-10T09:10:40Z

velox/docs/functions/spark/string.rst

+   ``separator``. Only accepts constant ``separator``. It takes variable number of remaining
+   arguments. And ``string`` & ``array<string>`` can be used in combination.  If ``separator``
+   is NULL, returns NULL, regardless of the following inputs. If only ``separator`` (not a
+   NULL) is provided or all remaining inputs are NULL, returns an empty string. ::


Would you clarify the behavior when the separator is not NULL, but string or array(string) contain NULL? Maybe also include an example.

Just clarified and added an example.

PHILO-HE · 2024-04-12T03:37:28Z

@rui-mo, could you review again? Thanks!

rui-mo

Thanks.

rui-mo · 2024-04-15T00:44:24Z

velox/functions/sparksql/String.cpp

+    const std::vector<exec::VectorFunctionArg>& inputArgs,
+    const core::QueryConfig& /*config*/) {
+  auto numArgs = inputArgs.size();
+  VELOX_USER_CHECK(


nit: VELOX_USER_CHECK_GE

rui-mo · 2024-04-15T00:44:53Z

velox/functions/sparksql/String.cpp

+      numArgs >= 1,
+      "concat_ws requires one arguments at least, but got {}.",
+      numArgs);
+  for (auto& arg : inputArgs) {


nit: const &

rui-mo · 2024-04-15T00:47:36Z

velox/functions/sparksql/String.cpp

+      }
+    }
+
+    // String arg.


This is a bit obscure, could we use a complete sentence as comment?

rui-mo · 2024-04-15T00:48:53Z

velox/functions/sparksql/String.cpp

@@ -103,6 +103,229 @@ class Length : public exec::VectorFunction {
  }
 };

+void doApply(


I feel we are lacking some description for the process of concating, maybe better to add some.

rui-mo

Thanks.

rui-mo · 2024-04-15T00:54:06Z

velox/functions/sparksql/String.cpp

+    separatorDecoded = exec::LocalDecodedVector(context, *args[0], rows);
+  }
+
+  // Calculate the total number of bytes in the result.


For better readability, it seems several helper functions could be extracted to achieve a single purpose.

PHILO-HE · 2024-04-16T02:28:13Z

@rui-mo, could you review this pr again? Thanks!

rui-mo

Thanks.

rui-mo · 2024-04-16T06:47:50Z

velox/docs/functions/spark/string.rst

+   of the following inputs. For non-NULL ``separator``, if no remaining input or all remaining
+   inputs are NULL, returns an empty string. ::
+
+        SELECT concat_ws('~', 'a', 'b', 'c'); -- 'a~b~c'


How does concat_ws behave when element is empty string or array, like concat_ws('~', 'a', 'b', ''), or concat_ws('~', [], ['d'])?

Just confirmed. Empty string & its neighbors are also separated in Spark. Just fixed this and updated doc.

rui-mo · 2024-04-16T06:48:22Z

velox/functions/sparksql/String.cpp

+            auto element = elementsDecoded.valueAt<StringView>(offset + j);
+            if (!element.empty()) {
+              allElements++;
+              totalResultBytes += element.size();


Here it seems empty element is skipped. Will empty element being concated in Spark?

Just fixed. Thanks!

rui-mo · 2024-04-16T06:52:47Z

velox/functions/sparksql/String.cpp

+        decodedSeparator);
+
+    // Allocate a string buffer.
+    auto rawBuffer = flatResult.getRawStringBufferWithSpace(totalResultBytes);


Is totalResultBytes the exact size? Maybe use getRawStringBufferWithSpace(totalResultBytes, true) if it is.

Fixed. Thanks!

rui-mo · 2024-04-16T06:53:21Z

velox/functions/sparksql/String.cpp

+
+    // Allocate a string buffer.
+    auto rawBuffer = flatResult.getRawStringBufferWithSpace(totalResultBytes);
+    rows.applyToSelected([&](int row) {


int -> vector_size_t or auto

PHILO-HE

@rui-mo, fixed your comments. Thanks!

PHILO-HE · 2024-04-16T09:59:04Z

velox/docs/functions/spark/string.rst

+   of the following inputs. For non-NULL ``separator``, if no remaining input or all remaining
+   inputs are NULL, returns an empty string. ::
+
+        SELECT concat_ws('~', 'a', 'b', 'c'); -- 'a~b~c'


Just confirmed. Empty string & its neighbors are also separated in Spark. Just fixed this and updated doc.

PHILO-HE · 2024-04-16T09:59:27Z

velox/functions/sparksql/String.cpp

+            auto element = elementsDecoded.valueAt<StringView>(offset + j);
+            if (!element.empty()) {
+              allElements++;
+              totalResultBytes += element.size();


Just fixed. Thanks!

PHILO-HE · 2024-04-16T10:01:42Z

velox/functions/sparksql/String.cpp

+        decodedSeparator);
+
+    // Allocate a string buffer.
+    auto rawBuffer = flatResult.getRawStringBufferWithSpace(totalResultBytes);


Fixed. Thanks!

PHILO-HE · 2024-04-16T13:29:13Z

Hi @mbasmanova, Rui has no more comment. Could you please spare some time to review? Thanks!

mbasmanova

@PHILO-HE Thank you for adding this function. Some comments.

mbasmanova · 2024-05-02T12:21:45Z

velox/docs/functions/spark/string.rst

+
+   Returns the concatenation for ``string`` and all elements in ``array<string>``, separated
+   by ``separator``. The type of ``separator`` is VARCHAR . It can take variable number of
+   remaining arguments, where ``string`` & ``array<string>`` can be used in combination. NULL


mbasmanova · 2024-05-02T12:22:00Z

velox/docs/functions/spark/string.rst

+   by ``separator``. The type of ``separator`` is VARCHAR . It can take variable number of
+   remaining arguments, where ``string`` & ``array<string>`` can be used in combination. NULL
+   element is skipped in the concatenation. If ``separator`` is NULL, returns NULL, regardless
+   of the followinginputs. For non-NULL ``separator``, if no remaining input or all remaining


followinginputs -> following inputs

mbasmanova · 2024-05-02T12:22:55Z

velox/expression/tests/SparkExpressionFuzzerTest.cpp

+      "unix_timestamp",
+      // Skip concat_ws due to the below issue:
+      // We use "any" type in its signature to allow mixed
+      // using of VARCHAR & ARRAY<VARCHAR>. But the fuzzer


& -> and

'mixed using' -> 'a mix of VARCHAR and ARRAY arguments'

mbasmanova · 2024-05-02T12:25:04Z

velox/functions/sparksql/String.cpp

+          exec::FunctionSignatureBuilder()
+              .returnType("varchar")
+              .constantArgumentType("varchar")
+              .argumentType("any")


I wonder if this function would be better defined as a special form.

mbasmanova · 2024-05-02T12:25:30Z

velox/functions/sparksql/String.cpp

@@ -103,6 +103,286 @@ class Length : public exec::VectorFunction {
  }
 };

+class ConcatWs : public exec::VectorFunction {
+ public:
+  explicit ConcatWs(const std::optional<std::string>& separator)


Can this function be implemented as a simple function? If not, let's move it into a separate file.

mbasmanova · 2024-05-02T12:26:40Z

velox/expression/tests/SparkExpressionFuzzerTest.cpp

@@ -54,7 +54,12 @@ int main(int argc, char** argv) {
      "chr",
      "replace",
      "might_contain",
-      "unix_timestamp"};
+      "unix_timestamp",
+      // Skip concat_ws due to the below issue:


Without fuzzer coverage it will be hard to ensure the implementation is correct.

mbasmanova · 2024-05-02T12:29:10Z

velox/functions/sparksql/String.cpp

+          // varchar, [varchar], [array(varchar)], ... -> varchar.
+          exec::FunctionSignatureBuilder()
+              .returnType("varchar")
+              .constantArgumentType("varchar")


This signature says that separator must be constant, but it seems that implementation also allows non-constant separator.

mbasmanova · 2024-05-02T12:30:44Z

velox/functions/sparksql/String.cpp

+        DecodedVector& arrayDecoded = *decodedArrays[i].get();
+        auto baseArray = arrayDecoded.base()->as<ArrayVector>();
+        auto rawSizes = baseArray->rawSizes();
+        auto rawOffsets = baseArray->rawOffsets();
+        auto indices = arrayDecoded.indices();
+        auto elements = baseArray->elements();
+        exec::LocalSelectivityVector nestedRows(context, elements->size());
+        nestedRows.get()->setAll();
+        exec::LocalDecodedVector elementsHolder(
+            context, *elements, *nestedRows.get());
+        auto& elementsDecoded = *elementsHolder.get();


Perhaps, rearrange the code to avoid doing this per row and do that per batch of rows instead.

jinchengchenghh · 2024-07-29T07:41:25Z

velox/functions/sparksql/ConcatWs.h

+
+namespace facebook::velox::functions::sparksql {
+
+class ConcatWsCallToSpecialForm : public exec::FunctionCallToSpecialForm {


I don't have many context here. can we register as simple function?

stale · 2024-10-28T19:41:44Z

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!

boneanxs · 2024-11-07T06:59:59Z

Hey @PHILO-HE any new progress for this pr? Looking forward to this function support!

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 Feb 26, 2024

PHILO-HE mentioned this pull request Feb 26, 2024

Add concat_ws Spark Function #6292

Closed

PHILO-HE force-pushed the concat_ws branch 3 times, most recently from b2bca03 to 7efd217 Compare March 26, 2024 14:49

PHILO-HE force-pushed the concat_ws branch 9 times, most recently from 3033eb8 to a1f105b Compare April 10, 2024 02:58

PHILO-HE force-pushed the concat_ws branch 2 times, most recently from 379108a to 8203b08 Compare April 10, 2024 03:11

rui-mo reviewed Apr 10, 2024

View reviewed changes

PHILO-HE force-pushed the concat_ws branch 2 times, most recently from c2638ed to bbfa05e Compare April 12, 2024 03:35

PHILO-HE mentioned this pull request Apr 12, 2024

[VL] Unsupported spark function list [please leave a comment if you plan to pick some] apache/incubator-gluten#4039

Open

98 tasks

PHILO-HE force-pushed the concat_ws branch 2 times, most recently from 6651c0d to 20df5f9 Compare April 12, 2024 09:39

rui-mo reviewed Apr 15, 2024

View reviewed changes

PHILO-HE force-pushed the concat_ws branch from b3ee1cc to cabeb65 Compare April 16, 2024 02:22

PHILO-HE force-pushed the concat_ws branch from 7433ee6 to d61d6f9 Compare April 16, 2024 02:43

rui-mo reviewed Apr 16, 2024

View reviewed changes

PHILO-HE commented Apr 16, 2024

View reviewed changes

PHILO-HE force-pushed the concat_ws branch from 5e1f752 to 5c89648 Compare April 16, 2024 12:21

mbasmanova reviewed May 2, 2024

View reviewed changes

PHILO-HE added 9 commits June 23, 2024 17:35

Initial commit

7d55d1b

Support non-constant separator

4f8c3ac

Refine comments

bd25c90

Fix comment

6bf3f8a

Consider empty string

c3fdc10

Allocate exact buffer size

3707161

Minor fix

6da7332

Fix comment

62355d0

Code refactor

c5f0ede

PHILO-HE force-pushed the concat_ws branch from 821cfd8 to 3953185 Compare June 23, 2024 09:36

Move to separate file

2260c9c

PHILO-HE force-pushed the concat_ws branch from 3953185 to 2260c9c Compare June 23, 2024 09:42

Use special form

b007b1e

PHILO-HE force-pushed the concat_ws branch from 249dd17 to ca233d2 Compare June 24, 2024 08:33

Fix column separator with NULLs

ca1250c

PHILO-HE force-pushed the concat_ws branch from ca233d2 to ca1250c Compare June 24, 2024 08:36

PHILO-HE mentioned this pull request Jul 23, 2024

Add Spark concat_ws function #10527

Closed

jinchengchenghh reviewed Jul 29, 2024

View reviewed changes

stale bot added the stale label Oct 28, 2024


		namespace facebook::velox::functions::sparksql {

		class ConcatWsCallToSpecialForm : public exec::FunctionCallToSpecialForm {

Add concat_ws Spark function #8854

Are you sure you want to change the base?

Add concat_ws Spark function #8854

Conversation

PHILO-HE commented Feb 26, 2024 • edited Loading

netlify bot commented Feb 26, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

PHILO-HE commented Feb 28, 2024

PHILO-HE commented Apr 10, 2024

PHILO-HE commented Apr 10, 2024

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

PHILO-HE commented Apr 12, 2024

rui-mo 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

rui-mo left a comment

Choose a reason for hiding this comment

rui-mo Apr 15, 2024 • edited Loading

Choose a reason for hiding this comment

PHILO-HE commented Apr 16, 2024

rui-mo left a comment

Choose a reason for hiding this comment

rui-mo Apr 16, 2024 • edited Loading

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

PHILO-HE 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

PHILO-HE commented Apr 16, 2024

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 Oct 28, 2024

boneanxs commented Nov 7, 2024

PHILO-HE commented Feb 26, 2024 •

edited

Loading

netlify bot commented Feb 26, 2024 •

edited

Loading

rui-mo Apr 15, 2024 •

edited

Loading

rui-mo Apr 16, 2024 •

edited

Loading