[GitHub] [calcite] ILuffZhe commented on a diff in pull request #3292: [CALCITE-5807] Add SUBSTRING_INDEX function (enabled in Spark library)

["ILuffZhe (via GitHub)" <gi...@apache.org>]

ILuffZhe commented on code in PR #3292:
URL: https://github.com/apache/calcite/pull/3292#discussion_r1259805158


##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -6178,6 +6178,34 @@ private static void checkIf(SqlOperatorFixture f) {
     f.checkNull("str_to_map('a:1,b:2,c:3', ',',null)");
   }
 
+  /** Tests {@code SUBSTRING_INDEX} function from Spark. */
+  @Test void testSubstringIndexFunc() {
+    final SqlOperatorFixture f0 = fixture();
+    f0.setFor(SqlLibraryOperators.SUBSTRING_INDEX);
+    f0.checkFails("^substring_index('a', ',')^",
+        "No match found for function signature SUBSTRING_INDEX\\("
+            + "<CHARACTER>, <CHARACTER>\\)", false);
+
+    final SqlOperatorFixture f = f0.withLibrary(SqlLibrary.SPARK);
+    f.checkString("substring_index('www.apache.org', '.', 2)",
+        "www.apache", "VARCHAR(14) NOT NULL");
+    f.checkString("substring_index('www.apache.org', '.', 1)",
+        "www", "VARCHAR(14) NOT NULL");
+    f.checkString("substring_index('www.apache.org', '.', 3)",
+        "www.apache.org", "VARCHAR(14) NOT NULL");
+    f.checkString("substring_index('www.apache.org', '.', -1)",
+        "org", "VARCHAR(14) NOT NULL");
+
+    f.checkNull("substring_index(cast(null as varchar(1)),"

Review Comment:
   We need test cases for those two senarios:
   
   1. `count` is null while `str` and `delimeter` is not null
   2. `delimeter` is null but `str` and `count` is not null



##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -6178,6 +6178,34 @@ private static void checkIf(SqlOperatorFixture f) {
     f.checkNull("str_to_map('a:1,b:2,c:3', ',',null)");
   }
 
+  /** Tests {@code SUBSTRING_INDEX} function from Spark. */

Review Comment:
   You can take a look at other java docs in this file. We normally use format like:
   `/** Test case for...`.
   IMO, it helps others to locate the corresponding JIRA ticket quickly.



##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -6178,6 +6178,34 @@ private static void checkIf(SqlOperatorFixture f) {
     f.checkNull("str_to_map('a:1,b:2,c:3', ',',null)");
   }
 
+  /** Tests {@code SUBSTRING_INDEX} function from Spark. */
+  @Test void testSubstringIndexFunc() {

Review Comment:
   This function performs a case-sensitive match when searching for delim. I think we should add a test to make a comparison.



##########
core/src/main/java/org/apache/calcite/sql/fun/SqlLibraryOperators.java:
##########
@@ -1260,6 +1260,14 @@ private static RelDataType deriveTypeMapFromEntries(SqlOperatorBinding opBinding
           ReturnTypes.IDENTITY_TO_MAP_NULLABLE,
           OperandTypes.STRING_OPTIONAL_STRING_OPTIONAL_STRING);
 
+  /** The "SUBSTRING_INDEX(string, delimiter, count)" function. */
+  @LibraryOperator(libraries = {SPARK})
+  public static final SqlFunction SUBSTRING_INDEX =
+      SqlBasicFunction.create(SqlKind.SUBSTRING_INDEX,
+          ReturnTypes.ARG0_NULLABLE_VARYING,

Review Comment:
   If any of the argument is null, this returns null. 
   I'm not very familiar with Spark, does this behave like Spark?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org