Enhance split Spark function to support regex #6155

unigof · 2023-08-18T12:51:02Z

Description

Split function just support split string by a single character for now.
This pr to support split by regex string.

split(str, regex)
    Splits `str` around occurrences that match `regex` and returns an array as many times as possible
    Arguments:
      * str - a string expression to split.
      * regex - a string representing a regular expression. The regex string should be a regular expression. It supported by RE2.

Examples:

 SELECT split('oneAtwoBthreeC', '[ABC]'); -- ["one","two","three",""]
 SELECT split('one', ''); -- ["o", "n", "e", ""]
 SELECT split('one', '1'); -- ["one"]

Notice:

There are some semantic diff between Java regex and re2 regex. And the lookahead/lookbehind patterns are not supported by RE2. If used, there will be a runtime exception (compile pattern error). e.g. :

Positive Lookahead: (?=regex)
Negative Lookahead: (?!regex)
Positive Lookbehind: （?<=regex）
Negative Lookbehind:（?<!regex)

#6017

netlify · 2023-08-18T12:51:08Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`9d97c73`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/665e75cb46ae0100083afe7c

unigof · 2023-08-25T06:44:12Z

Hi @mbasmanova @beroyfb ，could you help me review this pr? Thank you very much

mbasmanova

@unigof Please, review contributing guide and coding style docs and update the PR to comply.

unigof · 2023-09-14T11:49:32Z

Hi @mbasmanova @PHILO-HE @jackylee-ch , can you review this pr? Thank you

mbasmanova · 2023-09-14T12:05:34Z

velox/docs/functions/spark/string.rst

@@ -131,21 +131,20 @@ Unless specified otherwise, all functions return NULL if at least one of the arg

 .. spark:function:: split(string, delimiter) -> array(string)

-    Splits ``string`` on ``delimiter`` and returns an array. ::
+    Splits ``string`` on ``delimiter`` as much as possible and returns an array. 


Was the original implementation wrong? It treated 'delimiter' as a strings, but it should have treated it as regular expression.

Does Spark use Joni for regex or something else?

Hi @mbasmanova, let me help clarify. The original version only implements the basic split version in which the pattern is a single character. So this PR will address the limitations.

I just found spark uses JDK's regex.

In velox, split function treated 'delimiter' as a char, but in spark it treated 'delimiter' as a strings and support regular expression.

velox/docs/functions/spark/string.rst

PHILO-HE · 2023-09-15T02:03:31Z

velox/docs/functions/spark/string.rst

@@ -131,21 +131,20 @@ Unless specified otherwise, all functions return NULL if at least one of the arg

 .. spark:function:: split(string, delimiter) -> array(string)

-    Splits ``string`` on ``delimiter`` and returns an array. ::
+    Splits ``string`` on ``delimiter`` as much as possible and returns an array. 


Hi @mbasmanova, let me help clarify. The original version only implements the basic split version in which the pattern is a single character. So this PR will address the limitations.

I just found spark uses JDK's regex.

velox/functions/lib/Re2Functions.cpp

mbasmanova · 2023-09-15T11:31:20Z

Hi @mbasmanova, let me help clarify. The original version only implements the basic split version in which the pattern is a single character. So this PR will address the limitations.

Let's update PR description to clarify this.

I just found spark uses JDK's regex.

I see. Do you know whether it uses Joni, RE2 or some other regex engine?

PHILO-HE · 2023-09-15T12:50:41Z

I just found spark uses JDK's regex.

I see. Do you know whether it uses Joni, RE2 or some other regex engine?

Neither Joni nor RE2. It looks JDK's own regex engine. See link.

mbasmanova · 2023-09-15T12:53:28Z

Neither Joni nor RE2. It looks JDK's own regex engine.

Interesting. Isn't it very slow (especially compared with RE2)?

PHILO-HE · 2023-09-15T13:10:42Z

Neither Joni nor RE2. It looks JDK's own regex engine.

Interesting. Isn't it very slow (especially compared with RE2)?

Yes, I also guess it is very slow. And RE2 claimed here.

unigof · 2023-09-20T00:29:55Z

hi @mbasmanova , could you merge this pr, if no other problems?

mbasmanova · 2023-09-20T00:34:18Z

@rui-mo Rui, can you help review this PR?

PHILO-HE · 2023-09-20T00:46:41Z

velox/docs/functions/spark/string.rst

@@ -131,14 +131,24 @@ Unless specified otherwise, all functions return NULL if at least one of the arg

 .. spark:function:: split(string, delimiter) -> array(string)

-    Splits ``string`` on ``delimiter`` and returns an array. ::
+    Splits ``string`` on ``delimiter`` and returns an array. 


I think it is better to use regex instead of delimiter.

Suggested description:
Returns an array by splitting string as many times as possible. The delimiter is any string matching regex.

PHILO-HE · 2023-09-20T00:50:38Z

velox/docs/functions/spark/string.rst


        SELECT split('oneAtwoBthreeC', '[ABC]'); -- ["one","two","three",""]
        SELECT split('one', ''); -- ["o", "n", "e", ""]
        SELECT split('one', '1'); -- ["one"]

-.. spark:function:: split(string, delimiter, limit) -> array(string)
-   :noindex:
+ .. spark:function:: split(string, delimiter, limit) -> array(string)


No extra space.

Use regex instead.

PHILO-HE · 2023-09-20T00:52:01Z

velox/docs/functions/spark/string.rst

-.. spark:function:: split(string, delimiter, limit) -> array(string)
-   :noindex:
+ .. spark:function:: split(string, delimiter, limit) -> array(string)
+    :noindex:


PHILO-HE · 2023-09-20T00:59:03Z

velox/docs/functions/spark/string.rst

-.. spark:function:: split(string, delimiter, limit) -> array(string)
-   :noindex:
+ .. spark:function:: split(string, delimiter, limit) -> array(string)
+    :noindex:

    Splits ``string`` on ``delimiter`` and returns an array of size at most ``limit``. ::


This description should also be refined like the 1st function version. And it is better to tell user the first version is equivalent to -1 is used for limit.

PHILO-HE · 2023-09-20T01:16:34Z

velox/functions/lib/Re2Functions.cpp

+      EvalCtx& context,
+      VectorPtr& resultRef) const final {
+    try {
+      checkForBadPattern(re_);


I think it is better to move this check to makeRe2SplitAll to get bad pattern checked one time.

Thanks, I understand your means, but I think may it is better to keep it up now, because there is context in apply method, it can record exception for bad pattern.
If move checkForBadPattern to makeRe2SplitAll, maybe unable to find problems conveniently

It looks not a strong reason. As the pattern is always constant, I think it may be not necessary to do this check for each incoming batch. cc @mbasmanova

If pattern is constant, then let's make sure to check it once during function creation and use AlwaysFailingVectorFunction.

PHILO-HE · 2023-09-20T01:28:35Z

velox/docs/functions/spark/string.rst

+        ``string`` - a string expression to split, constant
+        ``delimiter`` - a string representing a regular expression, supported by re2.
+    Notice:
+        There are some usage not supported by re2 regex.  If used, there will be 


I think we can remove this clarification. RE2 indeed has a few incompatible patterns with other libs. It may be better to just clarify them at some common place, instead of for only a certain regex function.

rui-mo

Can we add Spark's implementation to the PR description? Thanks.
https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/regexpExpressions.scala#L526

rui-mo · 2023-09-21T00:47:54Z

velox/docs/functions/spark/string.rst

+        ``string`` - a string expression to split, constant
+        ``delimiter`` - a string representing a regular expression, supported by re2.
+    Notice:
+        There are some usage not supported by re2 regex.  If used, there will be 


Are these unsupported patterns needed by Spark? We may add unit tests for them if exception is expected.

Spark not support these patterns, just split string limit times

Is there any pattern supported by JDK's regex in Spark, but not supported by RE2?

rui-mo · 2023-09-21T00:56:41Z

velox/docs/functions/spark/string.rst

-.. spark:function:: split(string, delimiter, limit) -> array(string)
-   :noindex:
+ .. spark:function:: split(string, delimiter, limit) -> array(string)
+    :noindex:

    Splits ``string`` on ``delimiter`` and returns an array of size at most ``limit``. ::


Can we also document the behavior when limit is negative? We may also need to add corresponding tests.

unigof · 2023-09-26T05:45:18Z

hi @rui-mo , can you review this pr again? thanks~

PHILO-HE · 2023-10-09T05:52:57Z

velox/docs/functions/spark/string.rst


-    Splits ``string`` on ``delimiter`` and returns an array. ::
+    Returns an array by splitting ``string`` as many times as possible. 
+    The ``regex`` is any string matching regex, supported by re2.


Suggested statement:
The delimiter is any string matching regex

PHILO-HE · 2023-10-09T06:04:27Z

velox/functions/lib/Re2Functions.cpp

+      EvalCtx& context,
+      VectorPtr& resultRef) const final {
+    try {
+      checkForBadPattern(re_);


It looks not a strong reason. As the pattern is always constant, I think it may be not necessary to do this check for each incoming batch. cc @mbasmanova

PHILO-HE · 2023-10-09T06:13:29Z

velox/functions/lib/Re2Functions.h

@@ -135,6 +135,21 @@ std::shared_ptr<exec::VectorFunction> makeRe2ExtractAll(

 std::vector<std::shared_ptr<exec::FunctionSignature>> re2ExtractAllSignatures();

+/// re2SplitAll(string, pattern) → array<string>
+///
+/// If string has a substring that matches the given pattern, then will be split


It may be better to re-use some statements in string.rst to document it.

velox/functions/lib/Re2Functions.cpp

+    arrayWriter.add_item().setNoCopy(
+        StringView(remaining.data(), remaining.size()));
+  } else if (pos == input.size()) {
+    arrayWriter.add_item().setNoCopy(StringView(nullptr, 0));


rui-mo · 2023-10-09T08:44:22Z

velox/functions/lib/Re2Functions.cpp

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


Maybe use VELOX_USER_CHECK_EQ.

rui-mo · 2023-10-09T08:45:37Z

velox/functions/lib/tests/Re2FunctionsTest.cpp

+        },
+        [&inputs](vector_size_t row) { return !inputs[row].has_value(); });
+
+    // Constant pattern


Please add . at the end of all comments.

rui-mo · 2023-10-09T08:46:17Z

velox/functions/lib/tests/Re2FunctionsTest.cpp

+
+TEST_F(Re2FunctionsTest, regexSplitAllNonAscii) {
+  testRe2SplitAll(
+      // split('苹果香蕉velox橘子', '香蕉')


Are these commented cases needed?

PHILO-HE · 2023-10-12T13:26:37Z

velox/functions/lib/Re2Functions.cpp

@@ -730,8 +730,6 @@ void re2SplitAll(
    const re2::StringPiece remaining = input.substr(pos);
    arrayWriter.add_item().setNoCopy(
        StringView(remaining.data(), remaining.size()));
-  } else if (pos == input.size()) {


Looks relevant to test failure. Looks this if branch is necessary. I thought wrongly.

FelixYBW · 2023-12-23T02:34:56Z

Is the function still tracked?

stale · 2024-03-22T13:26:33Z

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!

jackylee-ch · 2024-04-11T12:12:14Z

cc @PHILO-HE @rui-mo

PHILO-HE

Basically looks good! Some comments. Please check whether they make sense. Thanks!

BTW, please squash historical commits into one or fewer ones (the community prefers that).

PHILO-HE · 2024-04-12T05:39:03Z

velox/functions/lib/Re2Functions.cpp

+    const re2::StringPiece remaining = input.substr(pos);
+    arrayWriter.add_item().setNoCopy(
+        StringView(remaining.data(), remaining.size()));
+  } else if (pos == input.size()) {


else if -> else？

PHILO-HE · 2024-04-12T05:42:04Z

velox/docs/functions/spark/string.rst

   :noindex:

-    Splits ``string`` on ``delimiter`` and returns an array of size at most ``limit``. ::
+    Splits ``string`` on ``regex`` and returns an array of size at most ``limit``.
+    If limit is negative, ``string`` will be split as many times as possible. ::


Nit: add "``" to surround word "limit".

If limit is zero, the behavior is like negative. So let's clarify zero case.
See spark doc.

Seems split with limit has not been supported and it will be covered by another PR: #8825. Maybe, we can remove the above relevant changes, and let that PR fix (if you agree, please ignore the above relevant comments).

Seems split with limit has not been supported and it will be covered by another PR: #8825. Maybe, we can remove the above relevant changes, and let that PR fix (if you agree, please ignore the above relevant comments).

It's ok to me

PHILO-HE · 2024-04-12T05:48:03Z

velox/docs/functions/spark/string.rst

   :noindex:

-    Splits ``string`` on ``delimiter`` and returns an array of size at most ``limit``. ::
+    Splits ``string`` on ``regex`` and returns an array of size at most ``limit``.


Suggested statement:
** returns an array whose size is ``limit`` at most.

PHILO-HE · 2024-04-12T05:55:39Z

velox/functions/lib/Re2Functions.cpp

+  try {
+    return std::make_shared<Re2SplitAllConstantPattern>(pattern);
+  } catch (...) {
+    return std::make_shared<exec::AlwaysFailingVectorFunction>(


Looks the exception only comes from checkForBadPattern. I remember makeRe2SplitAll can be called from Gluten's plan validation path. So it's better to remove catch to let exception thrown, then to make fallback happen on Gluten side.

We already have regex check in Gluten, thus we can use AlwaysFailingVectorFunction here.

PHILO-HE · 2024-04-12T05:56:26Z

velox/functions/lib/Re2Functions.h

+/// The pattern is any string matching regex.
+///
+/// If the pattern is invalid or not constant, throws an exception.
+/// If the pattern does not match, returns original string as array.


Please document these cases in string.rst.

jackylee-ch · 2024-04-16T09:21:55Z

cc @PHILO-HE @rui-mo

PHILO-HE

@jackylee-ch, I meant we can only update the doc for this function without limit arg, as I note limit arg is not considered in this pr. Right?
Except this, the pr looks good! Thanks!

jackylee-ch · 2024-04-19T04:44:42Z

@jackylee-ch, I meant we can only update the doc for this function without limit arg, as I note limit arg is not considered in this pr. Right? Except this, the pr looks good! Thanks!

The limit in split is not supported in current main branch, thus we need remove the split doc that contains limit.

FelixYBW · 2024-04-19T06:24:49Z

@PHILO-HE is the PR related to Cody's re2 support?

gaoyangxiaozhu · 2024-06-19T08:57:31Z

velox/functions/lib/Re2Functions.cpp

+  auto pattern = constantPattern->as<ConstantVector<StringView>>()->valueAt(0);
+
+  try {
+    return std::make_shared<Re2SplitAllConstantPattern>(pattern);


so looks current implement only support constant pattern, how about if no constant ?

@unigof , I have a PR #10248 to cover any (constant or non-constant) regex pattern and also limit parameter , but since you already started this part work, does it ok you apply the PR change into your PR ?

@gaoyangxiaozhu Just notice non-constant vector is also supported in your PR. Please feel free to continue its support in it if needed. The handling of some corner cases could be referred from mine https://github.com/facebookincubator/velox/pull/8825/files#diff-ebc054da2ecc11ac3cf20d47cbc112f9c3b9cf3d5f1784b2149fb0f4e997a2d8R127-R132. Thanks.

stale · 2024-09-18T01:17:33Z

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 Aug 18, 2023

unigof force-pushed the dev_split branch from 8bd8062 to 5028f79 Compare August 25, 2023 07:28

mbasmanova reviewed Aug 25, 2023

View reviewed changes

unigof changed the title ~~velox split function support pattern of string type~~ Enhance split Spark function to support regex Sep 14, 2023

mbasmanova reviewed Sep 14, 2023

View reviewed changes

PHILO-HE reviewed Sep 15, 2023

View reviewed changes

PHILO-HE reviewed Sep 20, 2023

View reviewed changes

rui-mo reviewed Sep 21, 2023

View reviewed changes

PHILO-HE reviewed Oct 9, 2023

View reviewed changes

rui-mo reviewed Oct 9, 2023

View reviewed changes

PHILO-HE reviewed Oct 12, 2023

View reviewed changes

stale bot added stale and removed stale labels Mar 22, 2024

PHILO-HE reviewed Apr 12, 2024

View reviewed changes

jackylee-ch force-pushed the dev_split branch 3 times, most recently from fbcec03 to 926b2e9 Compare April 14, 2024 16:33

PHILO-HE reviewed Apr 17, 2024

View reviewed changes

jackylee-ch force-pushed the dev_split branch from 926b2e9 to c98fb56 Compare April 19, 2024 04:40

jackylee-ch force-pushed the dev_split branch 2 times, most recently from d513611 to 5b627d6 Compare April 29, 2024 02:33

unigof and others added 2 commits June 4, 2024 10:00

velox split function support pattern of string type

721c670

change comments

9d97c73

jackylee-ch force-pushed the dev_split branch from 5b627d6 to 9d97c73 Compare June 4, 2024 02:02

rui-mo mentioned this pull request Jun 19, 2024

Support regex delimiter and limit argument for Spark split function #10248

Closed

gaoyangxiaozhu reviewed Jun 19, 2024

View reviewed changes

stale bot added the stale label Sep 18, 2024

stale bot closed this Oct 5, 2024

Enhance split Spark function to support regex #6155

Enhance split Spark function to support regex #6155

Conversation

unigof commented Aug 18, 2023 • edited Loading

Description

Notice:

netlify bot commented Aug 18, 2023 • edited Loading

✅ Deploy Preview for meta-velox canceled.

unigof commented Aug 25, 2023 • edited Loading

mbasmanova left a comment

Choose a reason for hiding this comment

unigof commented Sep 14, 2023

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

mbasmanova commented Sep 15, 2023

PHILO-HE commented Sep 15, 2023

mbasmanova commented Sep 15, 2023

PHILO-HE commented Sep 15, 2023

unigof commented Sep 20, 2023

mbasmanova commented Sep 20, 2023

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

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

Choose a reason for hiding this comment

unigof commented Sep 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

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

FelixYBW commented Dec 23, 2023

stale bot commented Mar 22, 2024

jackylee-ch commented Apr 11, 2024

PHILO-HE left a comment • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackylee-ch commented Apr 16, 2024

PHILO-HE left a comment

Choose a reason for hiding this comment

jackylee-ch commented Apr 19, 2024

FelixYBW commented Apr 19, 2024

gaoyangxiaozhu Jun 19, 2024 • edited Loading

Choose a reason for hiding this comment

rui-mo Jun 19, 2024 • edited Loading

Choose a reason for hiding this comment

stale bot commented Sep 18, 2024

unigof commented Aug 18, 2023 •

edited

Loading

netlify bot commented Aug 18, 2023 •

edited

Loading

unigof commented Aug 25, 2023 •

edited

Loading

PHILO-HE left a comment •

edited

Loading

gaoyangxiaozhu Jun 19, 2024 •

edited

Loading

rui-mo Jun 19, 2024 •

edited

Loading