feat(function): Add Spark locate function #8863

rui-mo · 2024-02-27T03:27:53Z

A function that returns the position of the first occurrence of substring in
given string after the start position.

Doc: https://spark.apache.org/docs/latest/api/sql/index.html#locate
Spark implementation: https://github.com/apache/spark/blob/v3.5.1/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala#L1420

netlify · 2024-02-27T03:28:17Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`5f8e1c2`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/673c53c4c7a95d000853a4ed

stale · 2024-05-27T12:02:57Z

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!

stale · 2024-10-05T07:58:21Z

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!

zhli1142015 · 2024-11-07T05:49:18Z

@rui-mo , will you continue this PR?

rui-mo · 2024-11-07T06:17:17Z

@zhli1142015 Yes, this PR is ready overall. Would you like to take a review? Thanks.

zhli1142015 · 2024-11-07T06:25:38Z

velox/functions/sparksql/String.h

+      const arg_type<Varchar>& subString,
+      const arg_type<Varchar>& string,
+      const arg_type<int32_t>& start) {
+    result = stringImpl::stringPosition<true /*isAscii*/>(


maybe it's better to call findNthInstanceByteIndexFromStart here, which support start position parameter.

stringPosition calls findNthInstanceByteIndexFromStart, and it also handles the edge cases like empty substr, invalid start, so we can avoid duplicate code.

I see your point

rui-mo · 2024-11-08T03:28:26Z

Passes the 1-minute spark expression fuzzer test.

./velox/expression/fuzzer/spark_expression_fuzzer_test --seed ${RANDOM} --enable_variadic_signatures --lazy_vector_generation_ratio 0.2 --velox_fuzzer_enable_column_reuse --velox_fuzzer_enable_expression_reuse --max_expression_trees_per_step 2 --retry_with_try --enable_dereference --duration_sec 60 --minloglevel=1 --stderrthreshold=2 --only locate
E1108 18:29:23.069322 3621308 ExpressionFuzzerVerifier.cpp:408] Total iterations: 305566
E1108 18:29:23.069486 3621308 ExpressionFuzzerVerifier.cpp:409] Total failed: 0

velox/functions/sparksql/String.h

zhli1142015 · 2024-11-08T03:41:45Z

velox/functions/lib/string/StringImpl.h

+  }
+
+  // The string to search for substring.
+  auto view = std::string_view(


maybe we can pass the startByteIndex to findNthInstanceByteIndexFromStart, instead of creating a new string_view object.

findNthInstanceByteIndexFromEnd does not accept the startByteIndex parameter, and we also use view at L255.

Ok, stringPosition also supports to serach from end.

But it's little wired, this function searches from end but with start parameter as value 1.

I understand your point. Updated this PR to make the start the end position when searching from the end. Thanks.

zhli1142015 · 2024-11-11T06:27:15Z

velox/functions/lib/string/StringImpl.h

+  }
+
+  // The string to search for substring.
+  auto view = std::string_view(


Ok, stringPosition also supports to serach from end.

zhli1142015 · 2024-11-11T06:29:03Z

velox/functions/lib/string/StringImpl.h

+  }
+
+  // The string to search for substring.
+  auto view = std::string_view(


But it's little wired, this function searches from end but with start parameter as value 1.

rui-mo · 2024-11-13T13:12:45Z

Hi @mbasmanova, would you like to take a review? Thanks!

mbasmanova

@rui-mo Some questions.

mbasmanova · 2024-11-13T16:02:40Z

velox/docs/functions/spark/string.rst

+    after position ``start``. The search is from the beginning of ``string`` to the end.
+    The given ``start`` and return value are 1-based. The following rules are listed in order of priority:
+
+    Returns 0 if ``start`` is NULL. Returns NULL if ``substring`` or ``string`` is NULL.


Do these rules match Spark? Any particular version or all versions are having the same behavior?

Yes, it matches Spark, and we are testing against Spark-3.5.1. The versions before Spark-3.5.1 have the same behavior.

https://github.com/apache/spark/blame/v3.5.1/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/stringExpressions.scala#L1434-L1456

velox/functions/lib/string/StringImpl.h

mbasmanova · 2024-11-13T16:16:25Z

velox/functions/lib/string/StringImpl.h

+    const T& string,
+    const T& subString,
+    int64_t instance = 0,
+    std::optional<int64_t> startPosition = std::nullopt) {


Wondering if it makes sense to replace (string, startPosition) arguments with a single std::string_view argument.

Thanks for the suggestion. Updated to use std::string_view argument type.

velox/functions/lib/string/StringImpl.h

mbasmanova

@rui-mo Thanks.

mbasmanova · 2024-11-14T19:55:45Z

velox/functions/sparksql/String.h

+    // Unicode string "😋😋😋", each character occupies 4 bytes. When 'start' is
+    // 2, the 'startByteIndex' is 4 which specifies the start of the second
+    // character.
+    const char* pos = string->data();


I thought we have a helper function to find the location of the N-th char.

Yes, I found the helper function 'stringCore::cappedByteLengthUnicode' and changed to use it. Thanks for the remainder.

rui-mo · 2024-11-15T06:26:11Z

Opened #11549 to track the failure found in Presto fuzzer test.

facebook-github-bot · 2024-11-19T23:47:42Z

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

facebook-github-bot · 2024-11-20T19:10:43Z

@kagamiori merged this pull request in c286451.

conbench-facebook · 2024-11-20T19:42:15Z

Conbench analyzed the 1 benchmark run on commit c286451a.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

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

rui-mo changed the title ~~Support Spark locate function~~ Add Spark locate function Feb 27, 2024

stale bot added the stale label May 27, 2024

rui-mo force-pushed the wip_locate branch from abc61b2 to b5ea868 Compare May 28, 2024 00:16

stale bot removed the stale label May 28, 2024

rui-mo force-pushed the wip_locate branch 3 times, most recently from aa854a2 to 391b3b3 Compare July 5, 2024 03:01

stale bot added the stale label Oct 5, 2024

rui-mo force-pushed the wip_locate branch from 391b3b3 to 369c00c Compare October 9, 2024 02:04

stale bot removed the stale label Oct 9, 2024

zhli1142015 reviewed Nov 7, 2024

View reviewed changes

rui-mo force-pushed the wip_locate branch from 369c00c to 82dad84 Compare November 8, 2024 02:13

zhli1142015 reviewed Nov 8, 2024

View reviewed changes

rui-mo force-pushed the wip_locate branch 2 times, most recently from 3ced56f to ae30a78 Compare November 11, 2024 05:57

zhli1142015 approved these changes Nov 11, 2024

View reviewed changes

rui-mo force-pushed the wip_locate branch 3 times, most recently from b002b11 to 327a582 Compare November 12, 2024 05:28

mbasmanova reviewed Nov 13, 2024

View reviewed changes

rui-mo force-pushed the wip_locate branch from 327a582 to 13326aa Compare November 14, 2024 14:12

mbasmanova approved these changes Nov 14, 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 Nov 14, 2024

rui-mo force-pushed the wip_locate branch 2 times, most recently from 83de5d7 to 2db439c Compare November 15, 2024 03:04

rui-mo changed the title ~~Add Spark locate function~~ feat(function): Add Spark locate function Nov 15, 2024

rui-mo force-pushed the wip_locate branch from 2db439c to 6f10821 Compare November 15, 2024 03:08

feat(function): Add Spark locate function

5f8e1c2

rui-mo force-pushed the wip_locate branch from 6f10821 to 5f8e1c2 Compare November 19, 2024 09:00

facebook-github-bot closed this in c286451 Nov 20, 2024

facebook-github-bot added the Merged label Nov 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(function): Add Spark locate function #8863

feat(function): Add Spark locate function #8863

rui-mo commented Feb 27, 2024 •

edited

Loading

netlify bot commented Feb 27, 2024 •

edited

Loading

stale bot commented May 27, 2024

stale bot commented Oct 5, 2024

zhli1142015 commented Nov 7, 2024

rui-mo commented Nov 7, 2024

zhli1142015 Nov 7, 2024

rui-mo Nov 8, 2024

zhli1142015 Nov 8, 2024

rui-mo commented Nov 8, 2024

zhli1142015 Nov 8, 2024 •

edited

Loading

rui-mo Nov 11, 2024

zhli1142015 Nov 11, 2024

zhli1142015 Nov 11, 2024

rui-mo Nov 12, 2024

zhli1142015 Nov 11, 2024

zhli1142015 Nov 11, 2024

rui-mo commented Nov 13, 2024

mbasmanova left a comment

mbasmanova Nov 13, 2024

rui-mo Nov 14, 2024

mbasmanova Nov 13, 2024

rui-mo Nov 14, 2024

mbasmanova left a comment

mbasmanova Nov 14, 2024

rui-mo Nov 15, 2024

rui-mo commented Nov 15, 2024

facebook-github-bot commented Nov 19, 2024

facebook-github-bot commented Nov 20, 2024

conbench-facebook bot commented Nov 20, 2024

feat(function): Add Spark locate function #8863

feat(function): Add Spark locate function #8863

Conversation

rui-mo commented Feb 27, 2024 • edited Loading

netlify bot commented Feb 27, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

stale bot commented May 27, 2024

stale bot commented Oct 5, 2024

zhli1142015 commented Nov 7, 2024

rui-mo commented Nov 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rui-mo commented Nov 8, 2024

zhli1142015 Nov 8, 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

rui-mo commented Nov 13, 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

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

rui-mo commented Nov 15, 2024

facebook-github-bot commented Nov 19, 2024

facebook-github-bot commented Nov 20, 2024

conbench-facebook bot commented Nov 20, 2024

rui-mo commented Feb 27, 2024 •

edited

Loading

netlify bot commented Feb 27, 2024 •

edited

Loading

zhli1142015 Nov 8, 2024 •

edited

Loading