[CALCITE-5826] Add FIND_IN_SET function (enabled in Hive and Spark libraries) #3317

herunkang2018 · 2023-07-14T01:14:11Z

No description provided.

sonarcloud · 2023-07-14T01:39:23Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

LakeShen · 2023-09-20T14:56:03Z

core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java

@@ -1076,6 +1076,32 @@ public static int levenshtein(String string1, String string2) {
    return LEVENSHTEIN_DISTANCE.apply(string1, string2);
  }

+  /** SQL FIND_IN_SET(string, stringArray) function. */
+  public static int findInSet(String matchStr, String textStr) {
+    if (matchStr.contains(",")) {


Why you add this check? Could you give the code comments to explain?

This check is from the function specification, which refers to Spark's doc. I will add function annotation to address the meaning of this function.

LakeShen · 2023-09-20T14:57:24Z

core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java

@@ -1076,6 +1076,32 @@ public static int levenshtein(String string1, String string2) {
    return LEVENSHTEIN_DISTANCE.apply(string1, string2);
  }

+  /** SQL FIND_IN_SET(string, stringArray) function. */
+  public static int findInSet(String matchStr, String textStr) {


Because this method is public, I think we should handle the cases where matchStr and textStr are null

For null check issue, we can define the null policy of the function, and null policy will handle null properly in code generation phase, so I think there is no need to implement null check in each function implementation additionally.

Agree with @LakeShen.

Check null for public method always valuable.

And your unit test need have null parameter case. @herunkang2018

I have added the null check. Thanks for the review.

LakeShen · 2023-09-20T14:57:56Z

core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java

+    if (matchStr.contains(",")) {
+      return 0;
+    }
+    int textStrLen = textStr.length();


final int textStrLen = textStr.length();

Thanks for catching this, I will improve it.

LakeShen · 2023-09-20T14:58:06Z

core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java

+      return 0;
+    }
+    int textStrLen = textStr.length();
+    int matchStrLen = matchStr.length();


final int matchStrLen = matchStr.length();

LakeShen · 2023-09-20T14:59:05Z

core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java

+    int textStrLen = textStr.length();
+    int matchStrLen = matchStr.length();
+    int n = 1;
+    int lastComma = -1;


lastComma -> lastCommaIndex or lastCommaPosition maybe better.

LakeShen · 2023-09-20T15:00:33Z

core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java

+    int n = 1;
+    int lastComma = -1;
+    for (int i = 0; i < textStrLen; i++) {
+      if (textStr.charAt(i) == ',') {


Now ',' is hard coding,are there some character constants that represent this?

Thanks for the comment. Updated.

LakeShen · 2023-09-20T15:02:57Z

core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java

+      }
+    }
+    if (textStrLen - (lastComma + 1) == matchStrLen
+        && textStr.substring(lastComma + 1, textStrLen).equals(matchStr)) {


If there are more code comments would help people understand better,I think you could give more code comments in this method:)

The function annotation is added.

LakeShen · 2023-09-20T15:03:36Z

@herunkang2018 ,thank you for opening this PR, I left a couple of comments,overall looks good.

herunkang2018 · 2023-10-01T14:31:09Z

@LakeShen Thanks a lot for the review. All comments resolved, please help to review again if you have time.

LakeShen · 2023-10-02T09:33:44Z

@herunkang2018 ,LGTM.

And need the second people to review it:)

mihaibudiu · 2023-10-13T17:29:34Z

site/_docs/reference.md

@@ -2729,6 +2729,7 @@ BigQuery's type system uses confusingly different names for types and functions:
 | o | EXTRACT(xml, xpath, [, namespaces ])           | Returns the XML fragment of the element or elements matched by the XPath expression. The optional namespace value that specifies a default mapping or namespace mapping for prefixes, which is used when evaluating the XPath expression
 | o | EXISTSNODE(xml, xpath, [, namespaces ])        | Determines whether traversal of a XML document using a specified xpath results in any nodes. Returns 0 if no nodes remain after applying the XPath traversal on the document fragment of the element or elements matched by the XPath expression. Returns 1 if any nodes remain. The optional namespace value that specifies a default mapping or namespace mapping for prefixes, which is used when evaluating the XPath expression.
 | m | EXTRACTVALUE(xml, xpathExpr))                  | Returns the text of the first text node which is a child of the element or elements matched by the XPath expression.
+| h s | FIND_IN_SET(string, stringArray)             | Returns the index (1-based) of the given *string* in the comma-delimited *stringArray*. Returns 0, if the given *string* was not found or if *string* contains a comma


From this description I infer that stringArray is of type ARRAY STRING, however it seems to be just a string which may contain commas. (I also think that the function name is not a very good choice, but that cannot be changed.)

Good advice, the parameter name can be improved to a more concise one, like matchStr and textStr, to avoid ambiguity.

I think we can add an example to clarify it, like FIND_IN_SET('bc', 'a,bc,def') returns 2.

JiajunBernoulli · 2023-10-28T06:01:32Z

site/_docs/reference.md

@@ -2729,6 +2729,7 @@ BigQuery's type system uses confusingly different names for types and functions:
 | o | EXTRACT(xml, xpath, [, namespaces ])           | Returns the XML fragment of the element or elements matched by the XPath expression. The optional namespace value that specifies a default mapping or namespace mapping for prefixes, which is used when evaluating the XPath expression
 | o | EXISTSNODE(xml, xpath, [, namespaces ])        | Determines whether traversal of a XML document using a specified xpath results in any nodes. Returns 0 if no nodes remain after applying the XPath traversal on the document fragment of the element or elements matched by the XPath expression. Returns 1 if any nodes remain. The optional namespace value that specifies a default mapping or namespace mapping for prefixes, which is used when evaluating the XPath expression.
 | m | EXTRACTVALUE(xml, xpathExpr))                  | Returns the text of the first text node which is a child of the element or elements matched by the XPath expression.
+| h s | FIND_IN_SET(string, stringArray)             | Returns the index (1-based) of the given *string* in the comma-delimited *stringArray*. Returns 0, if the given *string* was not found or if *string* contains a comma. For example, FIND_IN_SET('bc', 'a,bc,def') returns 2


*stringArray* is string not array. Perhaps it's confusing.

How about *strings*, *mkStrings* or *joinStrings*?

Thanks for the advice. Updated.

JiajunBernoulli · 2023-10-30T01:55:45Z

core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java

+    int n = 1;
+    int lastCommaIndex = -1;
+    for (int i = 0; i < textStrLen; i++) {
+      if (textStr.charAt(i) == COMMA_DELIMITER) {


Why not use textStr.split(",") ?
It will be easier to understand.

Thanks for the advice. Current implementation is consistent with Spark's. For better code readability, change to use split is good, but some performance may be reduced since for loop can early exit when matched and split cannot.

…braries)

sonarcloud · 2023-10-31T02:29:05Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

92.1% Coverage
0.0% Duplication

libenchao · 2023-11-01T02:21:25Z

This PR has received 1 approval from @mihaibudiu . Do you have more concerns @LakeShen @JiajunBernoulli ? Let's try to get it in 1.36.0

LakeShen · 2023-11-01T04:03:52Z

This PR has received 1 approval from @mihaibudiu . Do you have more concerns @LakeShen @JiajunBernoulli ? Let's try to get it in 1.36.0

LGTM.

tanclary force-pushed the main branch from 4804912 to 00db001 Compare September 6, 2023 00:42

herunkang2018 force-pushed the CALCITE-5826 branch 2 times, most recently from 15006fe to 1866952 Compare September 20, 2023 00:23

LakeShen reviewed Sep 20, 2023

View reviewed changes

mihaibudiu reviewed Oct 13, 2023

View reviewed changes

JiajunBernoulli reviewed Oct 28, 2023

View reviewed changes

herunkang2018 force-pushed the CALCITE-5826 branch from f01dbfe to c4c593d Compare October 29, 2023 03:35

herunkang2018 changed the title ~~[CALCITE-5826] Add FIND_IN_SET function (enabled in Hive and Spark library)~~ [CALCITE-5826] Add FIND_IN_SET function (enabled in Hive and Spark libraries) Oct 29, 2023

JiajunBernoulli reviewed Oct 30, 2023

View reviewed changes

mihaibudiu approved these changes Oct 30, 2023

View reviewed changes

[CALCITE-5826] Add FIND_IN_SET function (enabled in Hive and Spark li…

e81547f

…braries)

herunkang2018 force-pushed the CALCITE-5826 branch from c4c593d to e81547f Compare October 31, 2023 01:36

LakeShen approved these changes Nov 1, 2023

View reviewed changes

JiajunBernoulli approved these changes Nov 1, 2023

View reviewed changes

JiajunBernoulli merged commit 2ddc605 into apache:main Nov 1, 2023
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CALCITE-5826] Add FIND_IN_SET function (enabled in Hive and Spark libraries) #3317

[CALCITE-5826] Add FIND_IN_SET function (enabled in Hive and Spark libraries) #3317

herunkang2018 commented Jul 14, 2023

sonarcloud bot commented Jul 14, 2023

LakeShen Sep 20, 2023

herunkang2018 Oct 1, 2023 •

edited

Loading

LakeShen Sep 20, 2023

herunkang2018 Oct 1, 2023

JiajunBernoulli Oct 28, 2023 •

edited

Loading

herunkang2018 Oct 28, 2023

LakeShen Sep 20, 2023

herunkang2018 Oct 1, 2023 •

edited

Loading

herunkang2018 Oct 1, 2023

LakeShen Sep 20, 2023

herunkang2018 Oct 1, 2023

LakeShen Sep 20, 2023

herunkang2018 Oct 1, 2023

LakeShen Sep 20, 2023 •

edited

Loading

herunkang2018 Oct 1, 2023

LakeShen Sep 20, 2023

herunkang2018 Oct 1, 2023 •

edited

Loading

LakeShen commented Sep 20, 2023

herunkang2018 commented Oct 1, 2023

LakeShen commented Oct 2, 2023

mihaibudiu Oct 13, 2023

herunkang2018 Oct 15, 2023

herunkang2018 Oct 26, 2023 •

edited

Loading

herunkang2018 Oct 26, 2023

JiajunBernoulli Oct 28, 2023 •

edited

Loading

herunkang2018 Oct 28, 2023

JiajunBernoulli Oct 30, 2023

herunkang2018 Oct 30, 2023

herunkang2018 Oct 31, 2023

sonarcloud bot commented Oct 31, 2023

libenchao commented Nov 1, 2023

LakeShen commented Nov 1, 2023

[CALCITE-5826] Add FIND_IN_SET function (enabled in Hive and Spark libraries) #3317

[CALCITE-5826] Add FIND_IN_SET function (enabled in Hive and Spark libraries) #3317

Conversation

herunkang2018 commented Jul 14, 2023

sonarcloud bot commented Jul 14, 2023

Choose a reason for hiding this comment

herunkang2018 Oct 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JiajunBernoulli Oct 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

herunkang2018 Oct 1, 2023 • 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

LakeShen Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

herunkang2018 Oct 1, 2023 • edited Loading

Choose a reason for hiding this comment

LakeShen commented Sep 20, 2023

herunkang2018 commented Oct 1, 2023

LakeShen commented Oct 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

herunkang2018 Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JiajunBernoulli Oct 28, 2023 • 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

sonarcloud bot commented Oct 31, 2023

libenchao commented Nov 1, 2023

LakeShen commented Nov 1, 2023

herunkang2018 Oct 1, 2023 •

edited

Loading

JiajunBernoulli Oct 28, 2023 •

edited

Loading

herunkang2018 Oct 1, 2023 •

edited

Loading

LakeShen Sep 20, 2023 •

edited

Loading

herunkang2018 Oct 1, 2023 •

edited

Loading

herunkang2018 Oct 26, 2023 •

edited

Loading

JiajunBernoulli Oct 28, 2023 •

edited

Loading