Add substrings search path for constant like pattern #10731

skadilover · 2024-08-13T02:47:38Z

We noticed that some like constant-patterns can be equivalent to substrings search such as tpch queries Q13 and Q16: "%special%requests%" "%Customer%Complaints%"

Add substring-search fast path for like with search pattern : "((%+[^%#_\\]+)+%+)"
Some OLAP engine supports "_" for substring-search. In order to obtain the best performance, "_" is not supported here, '#' is not support for the same reason too.
This PR will also affect single substring-search like tpch Q9, if a fast-simd-strstr to speedup string-search will be added as follow-up, changes nothing right now.

benchmark result :
before:

tpchQuery9 23.96ms 41.73
tpchQuery13 156.02ms 6.41
tpchQuery16Part 9.02ms 110.83
tpchQuery16Supplier 192.13ms 5.20

after:

tpchQuery9 24.30ms 41.14
tpchQuery13 55.03ms 18.17
tpchQuery16Part 8.92ms 112.07
tpchQuery16Supplier 17.64ms 56.69

netlify · 2024-08-13T02:47:54Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`2ee154c`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/66f0df2d24d7f70008067085

skadilover · 2024-08-13T02:52:46Z

@mbasmanova @Yuhta

Could you help to have a look at this PR ? Is it reasonable to add a fast-path here?
And could you help to look at if the "load_unaligned" method usage in smid_strstr_memcmp is safe as it may load some unkown-memory address here.

Yuhta

Can you add a benchmark to show the performance gain?

velox/common/base/FixedMemCompare.h

velox/common/base/SimdUtil-inl.h

Yuhta · 2024-08-13T14:50:59Z

velox/common/base/SimdUtil-inl.h

+    size_t n,
+    const char* needle,
+    MEMCMP memcmp_fun) {
+  assert(k > 0);


VELOX_DCHECK

velox/common/base/FixedMemCompare.h

velox/common/base/SimdUtil-inl.h

skadilover · 2024-08-14T13:07:11Z

@Yuhta update code , could you have a look at this once more ?

i + CharVector::size <= n otherwise it's overreading, you need to handle trailing characters separately

I think this is the most interesting thing in this PR:

I add pageSafe check here now , like ClickHouse does : https://github.com/ClickHouse/ClickHouse/blob/a51f867ce014a4cb8f97c46e004b90f5feafd80f/src/Common/StringSearcher.h#L31

    bool isPageSafe(const void * const ptr) const
    {
        return ((page_size - 1) & reinterpret_cast<std::uintptr_t>(ptr)) <= page_size - N;
    }

By this way, load_unaligned is safe when overreading for they are in same memory page.

For StringSearch, ClickHouse use Volnitsky https://github.com/ClickHouse/ClickHouse/blob/1513eda3efe3a24b7a87b9f0e7803bb8693d1bc8/src/Common/Volnitsky.h#L373

Howevever, performance seems not best, Doris delete Volnitsky when they copy the StringSearch implement : https://github.com/apache/doris/blob/4e326e5f496109edb253d094869d057573ab7a59/be/src/vec/common/string_searcher.h#L44

And use first-two char compare, however, I test it , about 2x slow than this PR`s implement.

mbasmanova · 2024-09-13T11:21:28Z

velox/docs/functions/presto/regexp.rst

@@ -32,6 +32,12 @@ limited to 20 different expressions per instance and thread of execution.
        SELECT like('abc', '%b%'); -- true
        SELECT like('a_c', '%#_%', '#'); -- true

+    String sequence search: There are some patterns that are equivalent to simple


This paragraph doesn't fit with the existing "Patterns 'hello',..." docs. It would be nice to make the whole doc consistent.

updated with "hello velox"

mbasmanova · 2024-09-13T11:22:13Z

velox/functions/lib/Re2Functions.cpp

@@ -14,6 +14,7 @@
 * limitations under the License.
 */
 #include "velox/functions/lib/Re2Functions.h"
+#include "velox/common/base/SimdUtil.h"


Is this needed? Would you point me to where it is used?

er...., forget this after removed simdstrstr

skadilover · 2024-09-19T02:33:35Z

@mbasmanova ping ～

mbasmanova · 2024-09-20T08:03:13Z

velox/docs/functions/presto/regexp.rst

@@ -32,6 +32,12 @@ limited to 20 different expressions per instance and thread of execution.
        SELECT like('abc', '%b%'); -- true
        SELECT like('a_c', '%#_%', '#'); -- true

+    String sequence search: There are some patterns that are equivalent to simple


It would be better to embed the new information into the existing flow.

Not all patterns require compilation of regular expressions. Patterns ....

The new pattern needs to added to this list. The separate paragraph then can be about the limitations that apply to these special patterns. I.e. We would clarify separately whether special patterns allow for custom character or not.

Also, please, generate the docs to make sure they look nice.

I am not sure if I understand your meaning, add a list for all fast path in velox.

@mbasmanova Could you look at this again ?

mbasmanova · 2024-09-20T10:40:54Z

velox/docs/functions/presto/regexp.rst

@@ -32,6 +32,22 @@ limited to 20 different expressions per instance and thread of execution.
        SELECT like('abc', '%b%'); -- true
        SELECT like('a_c', '%#_%', '#'); -- true

+Not all patterns require compilation of regular expressions. For the patterns without custom escape:


There is existing text

Not all patterns require compilation of regular expressions. Patterns 'hello', 'hello%', '_hello__%',....

We need to update it to add the new pattern. Currently, you are adding new text as if existing text doesn't exist or doesn't talk about similar thing.

Current:

Patterns 'hello', 'hello%', '_hello__%', '%hello', '%__hello_', '%hello%', where

New (proposed):

Patterns 'hello', 'hello%', '_hello__%', '%hello', '%__hello_', '%hello%', '%hello%velox%' where

mbasmanova · 2024-09-20T10:42:42Z

velox/docs/functions/presto/regexp.rst

@@ -32,6 +32,22 @@ limited to 20 different expressions per instance and thread of execution.
        SELECT like('abc', '%b%'); -- true
        SELECT like('a_c', '%#_%', '#'); -- true

+Not all patterns require compilation of regular expressions. For the patterns without custom escape:
+
+| patterns  | description  | fast search mothod |


This table might be a nice alternative to current doc, but we'll need to iterate on columns, formatting and user-friendly descriptions. I suggest to do that in a separate PR.

skadilover · 2024-09-20T11:59:37Z

@mbasmanova
Got it, thanks for detail the comment, updated code, would you check it again?

mbasmanova

Thanks.

Yuhta · 2024-09-20T18:40:37Z

Is the regression in benchmark caused by the change? If so we probably want to merge this after #10858

skadilover · 2024-09-20T18:53:54Z

Is the regression in benchmark caused by the change? If so we probably want to merge this after #10858

It`s strange because Q2 and suffix is not supported by this change.

  // Not supports prefix.
  test("aa%bb%%", {});
  // Not supports sufix.
  test("%aa%bb", {});

@Yuhta I do not see regression on my machine

before 9851a4e
wildcardExactlyN 3.88ms 257.73
wildcardAtLeastN 3.79ms 263.72
fixedPattern 22.03ms 45.39
prefixPattern 21.83ms 45.80
suffixPattern 7.35ms 136.12
tpchQuery2 10.11ms 98.95
tpchQuery9 23.54ms 42.48
tpchQuery13 154.68ms 6.46
tpchQuery14 9.49ms 105.40
tpchQuery16Part 8.78ms 113.85
tpchQuery16Supplier 180.59ms 5.54
tpchQuery20 7.34ms 136.22

after
wildcardExactlyN 3.88ms 257.82
wildcardAtLeastN 3.79ms 263.84
fixedPattern 22.10ms 45.24
prefixPattern 21.92ms 45.61
suffixPattern 7.36ms 135.85
tpchQuery2 10.06ms 99.36
tpchQuery9 25.06ms 39.90
tpchQuery13 56.61ms 17.66
tpchQuery14 9.59ms 104.28
tpchQuery16Part 8.74ms 114.41
tpchQuery16Supplier 18.43ms 54.26
tpchQuery20 7.53ms 132.89

@Yuhta Maybe input data is not same ? what`s 'data' means in the cicd result? Would you help to check if the cicd result not stable, because I could not reproduced on my machine ?

remove simd_strstr to a seperate PR fix format

facebook-github-bot · 2024-09-23T15:15:23Z

@Yuhta has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Yuhta · 2024-09-23T15:15:40Z

@skadilover It's just flakiness, disappeared after rerun

skadilover · 2024-09-24T10:54:52Z

It's just flakiness, disappeared after rerun

I think it`s nothing to do with this change.

facebook-github-bot · 2024-09-24T17:19:36Z

@Yuhta merged this pull request in 11e4047.

JkSelf · 2024-09-25T05:19:23Z

velox/functions/lib/Re2Functions.cpp

@@ -2154,6 +2201,14 @@ std::shared_ptr<exec::VectorFunction> makeLike(

  PatternMetadata patternMetadata = PatternMetadata::generic();
  try {
+    // Fast path for substrings search.
+    auto substrings =


After picking this PR, Gluten will be a unit test that fails.

Incorrect evaluation: ///__ LIKE %//%/% ESCAPE '/', actual: true, expected: false

It seems that this implementation did not take into account the ESCAPE. In this example, the last slash (/) is the ESCAPE, which causes the last percent (%) to be a regular character, not a wildcard. Therefore, the result should be false, but Velox returns true.
@skadilover @mbasmanova @Yuhta @xiaoxmeng Do you have any input?

So sorry for the mistake, I mis-understand the code here:

BaseVector* escape = inputArgs[2].constantValue.get(); if (!escape) { return std::make_shared<LikeGeneric>(); }

and the ut just skiped by '#'

testLike("a%c", "%#%%", '#', true); testLike("%cd", "%#%%", '#', true);

fix it here : #11091

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 13, 2024

skadilover force-pushed the like_fast_path branch 18 times, most recently from 91983d0 to ee87495 Compare August 13, 2024 10:22

Yuhta reviewed Aug 13, 2024

View reviewed changes

velox/common/base/SimdUtil-inl.h Outdated Show resolved Hide resolved

skadilover force-pushed the like_fast_path branch 2 times, most recently from 1ac4f0f to 60b2015 Compare August 14, 2024 12:58

skadilover force-pushed the like_fast_path branch 2 times, most recently from 20d70bf to 079c6b1 Compare August 14, 2024 13:22

skadilover requested a review from Yuhta August 14, 2024 13:30

skadilover force-pushed the like_fast_path branch from 079c6b1 to 4718337 Compare August 14, 2024 13:53

mbasmanova reviewed Sep 13, 2024

View reviewed changes

skadilover force-pushed the like_fast_path branch from fcfd2e4 to 2d91717 Compare September 13, 2024 13:56

skadilover requested a review from mbasmanova September 13, 2024 14:05

mbasmanova reviewed Sep 20, 2024

View reviewed changes

skadilover requested a review from mbasmanova September 20, 2024 10:13

skadilover force-pushed the like_fast_path branch 2 times, most recently from b963239 to 97822e5 Compare September 20, 2024 10:27

mbasmanova reviewed Sep 20, 2024

View reviewed changes

skadilover force-pushed the like_fast_path branch from 97822e5 to 383166b Compare September 20, 2024 11:57

skadilover requested a review from mbasmanova September 20, 2024 11:59

mbasmanova approved these changes Sep 20, 2024

View reviewed changes

mbasmanova added the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label Sep 20, 2024

skadilover requested a review from Yuhta September 20, 2024 19:22

skadilover force-pushed the like_fast_path branch from 383166b to 88e78a4 Compare September 23, 2024 01:54

opt like

2ee154c

remove simd_strstr to a seperate PR fix format

skadilover force-pushed the like_fast_path branch from 88e78a4 to 2ee154c Compare September 23, 2024 03:23

facebook-github-bot closed this in 11e4047 Sep 24, 2024

facebook-github-bot added the Merged label Sep 24, 2024

zhouyuan mentioned this pull request Sep 25, 2024

[GLUTEN-6887][VL] Daily Update Velox Version (2024_09_25) apache/incubator-gluten#7338

Merged

JkSelf reviewed Sep 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add substrings search path for constant like pattern #10731

Add substrings search path for constant like pattern #10731

skadilover commented Aug 13, 2024 •

edited

Loading

netlify bot commented Aug 13, 2024 •

edited

Loading

skadilover commented Aug 13, 2024 •

edited

Loading

Yuhta left a comment

Yuhta Aug 13, 2024

skadilover commented Aug 14, 2024

mbasmanova Sep 13, 2024

skadilover Sep 13, 2024

mbasmanova Sep 13, 2024

skadilover Sep 13, 2024

skadilover commented Sep 19, 2024

mbasmanova Sep 20, 2024

skadilover Sep 20, 2024

mbasmanova Sep 20, 2024

mbasmanova Sep 20, 2024

skadilover commented Sep 20, 2024

mbasmanova left a comment

Yuhta commented Sep 20, 2024

skadilover commented Sep 20, 2024 •

edited

Loading

facebook-github-bot commented Sep 23, 2024

Yuhta commented Sep 23, 2024

skadilover commented Sep 24, 2024

facebook-github-bot commented Sep 24, 2024

JkSelf Sep 25, 2024

skadilover Sep 25, 2024

Add substrings search path for constant like pattern #10731

Add substrings search path for constant like pattern #10731

Conversation

skadilover commented Aug 13, 2024 • edited Loading

netlify bot commented Aug 13, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

skadilover commented Aug 13, 2024 • edited Loading

Yuhta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skadilover commented Aug 14, 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

skadilover commented Sep 19, 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

skadilover commented Sep 20, 2024

mbasmanova left a comment

Choose a reason for hiding this comment

Yuhta commented Sep 20, 2024

skadilover commented Sep 20, 2024 • edited Loading

facebook-github-bot commented Sep 23, 2024

Yuhta commented Sep 23, 2024

skadilover commented Sep 24, 2024

facebook-github-bot commented Sep 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skadilover commented Aug 13, 2024 •

edited

Loading

netlify bot commented Aug 13, 2024 •

edited

Loading

skadilover commented Aug 13, 2024 •

edited

Loading

skadilover commented Sep 20, 2024 •

edited

Loading