You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "Anthrino (via GitHub)" <gi...@apache.org> on 2023/07/25 22:44:49 UTC

[GitHub] [calcite] Anthrino opened a new pull request, #3338: [CALCITE-5873/CALCITE-5858] Add REGEXP_CONTAINS function for BigQuery Library

Anthrino opened a new pull request, #3338:
URL: https://github.com/apache/calcite/pull/3338

   (no comment)


-- 
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

[GitHub] [calcite] Anthrino commented on a diff in pull request #3338: [CALCITE-5873] Add REGEXP_CONTAINS function (enabled in BigQuery library)

Posted by "Anthrino (via GitHub)" <gi...@apache.org>.
Anthrino commented on code in PR #3338:
URL: https://github.com/apache/calcite/pull/3338#discussion_r1296485930


##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -345,6 +346,17 @@ public static String sha512(ByteString string)  {
     return DigestUtils.sha512Hex(string.getBytes());
   }
 
+  /** SQL {@code REGEXP_CONTAINS(value, regexp)} function.
+   * Throws a runtime exception for invalid regular expressions.*/
+  public static boolean regexpContains(String value, String regex) {
+    try {
+      Pattern regexp = Pattern.compile(regex);

Review Comment:
   Sure I think it makes sense to add a comment in the method body too, just wanted to point out anything that works in RE2 should be processed identically by java regex as well, which should cover 99.9% of queries. Only some invalid regex errors caught by RE2 aren't caught here so Calcite might produce an incorrect output for a few corner cases, which are technically incorrect regex inputs so they shouldn't be used in a query anyway.



-- 
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

[GitHub] [calcite] julianhyde commented on a diff in pull request #3338: [CALCITE-5873] Add REGEXP_CONTAINS function (enabled in BigQuery library)

Posted by "julianhyde (via GitHub)" <gi...@apache.org>.
julianhyde commented on code in PR #3338:
URL: https://github.com/apache/calcite/pull/3338#discussion_r1294074788


##########
site/_docs/reference.md:
##########
@@ -2776,6 +2776,7 @@ BigQuery's type system uses confusingly different names for types and functions:
 | b | PARSE_TIMESTAMP(format, string[, timeZone])    | Uses format specified by *format* to convert *string* representation of timestamp to a TIMESTAMP WITH LOCAL TIME ZONE value in *timeZone*
 | h s | PARSE_URL(urlString, partToExtract [, keyToExtract] ) | Returns the specified *partToExtract* from the *urlString*. Valid values for *partToExtract* include HOST, PATH, QUERY, REF, PROTOCOL, AUTHORITY, FILE, and USERINFO. *keyToExtract* specifies which query to extract
 | b | POW(numeric1, numeric2)                        | Returns *numeric1* raised to the power *numeric2*
+| b | REGEXP_CONTAINS(string, regexp)                | Returns whether *string* is a partial match for the regular expression, *regexp*

Review Comment:
   remove 'the regular expression, '. It's clear from the variable name 'regexp'



-- 
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

[GitHub] [calcite] julianhyde commented on a diff in pull request #3338: [CALCITE-5873] Add REGEXP_CONTAINS function (enabled in BigQuery library)

Posted by "julianhyde (via GitHub)" <gi...@apache.org>.
julianhyde commented on code in PR #3338:
URL: https://github.com/apache/calcite/pull/3338#discussion_r1295274217


##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -345,6 +346,17 @@ public static String sha512(ByteString string)  {
     return DigestUtils.sha512Hex(string.getBytes());
   }
 
+  /** SQL {@code REGEXP_CONTAINS(value, regexp)} function.
+   * Throws a runtime exception for invalid regular expressions.*/
+  public static boolean regexpContains(String value, String regex) {
+    try {
+      Pattern regexp = Pattern.compile(regex);

Review Comment:
   I wouldn't link to jira from the source code. Comments in the source code should be self-contained.
   
   (I know we add jira links in bugs. But that's because we *removed* a bug, and we don't want to leave a trail of mistakes in the source code.)
   
   This is a decision affects the specification of the function, not just its implementation. So I think the right place for the comment is in `big-query.iq`. You could have another comment in the body of `regexpContains` if you think maintainers might be confused why we're using Java regex.
   



-- 
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

[GitHub] [calcite] sonarcloud[bot] commented on pull request #3338: [CALCITE-5873] Add REGEXP_CONTAINS function (enabled in BigQuery library)

Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3338:
URL: https://github.com/apache/calcite/pull/3338#issuecomment-1679685151

   Kudos, SonarCloud Quality Gate passed!&nbsp; &nbsp; [![Quality Gate passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/passed-16px.png 'Quality Gate passed')](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3338)
   
   [![Bug](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug-16px.png 'Bug')](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=BUG) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=BUG)  
   [![Vulnerability](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability-16px.png 'Vulnerability')](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=VULNERABILITY) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=VULNERABILITY)  
   [![Security Hotspot](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot-16px.png 'Security Hotspot')](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3338&resolved=false&types=SECURITY_HOTSPOT) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3338&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3338&resolved=false&types=SECURITY_HOTSPOT)  
   [![Code Smell](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell-16px.png 'Code Smell')](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=CODE_SMELL) [![D](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/D-16px.png 'D')](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=CODE_SMELL) [6 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=CODE_SMELL)
   
   [![100.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/100-16px.png '100.0%')](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3338&metric=new_coverage&view=list) [100.0% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3338&metric=new_coverage&view=list)  
   [![2.1%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3-16px.png '2.1%')](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3338&metric=new_duplicated_lines_density&view=list) [2.1% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3338&metric=new_duplicated_lines_density&view=list)
   
   


-- 
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

[GitHub] [calcite] tanclary commented on a diff in pull request #3338: [CALCITE-5873/CALCITE-5858] Add REGEXP_CONTAINS function for BigQuery Library

Posted by "tanclary (via GitHub)" <gi...@apache.org>.
tanclary commented on code in PR #3338:
URL: https://github.com/apache/calcite/pull/3338#discussion_r1274192744


##########
babel/src/test/resources/sql/big-query.iq:
##########
@@ -3451,5 +3451,60 @@ FROM items;
 
 !ok
 
+#####################################################################
+# REGEXP_CONTAINS(value, regexp)
+#
+# Takes two STRING values. Returns TRUE if value is a partial match
+# for the regular expression, regexp.
+# If the regexp argument is invalid, the function returns an error.
+
+SELECT
+  email,
+  REGEXP_CONTAINS(email, r'@[a-zA-Z0-9-]+\.[a-zA-Z0-9-.]+') AS is_valid
+FROM
+  (SELECT
+    ['foo@example.com', 'bar@example.org', 'www.example.net']
+    AS addresses),
+  UNNEST(addresses) AS email;
+

Review Comment:
   Does it pass with this blank line between the query and the results? We may want to remove it to be consistent with other tests.



##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java:
##########
@@ -684,6 +685,8 @@ Builder populate2() {
       defineMethod(PARSE_TIME, "parseTime", NullPolicy.STRICT);
       defineMethod(PARSE_TIMESTAMP, "parseTimestamp", NullPolicy.STRICT);
 
+      defineMethod(REGEXP_CONTAINS, "regexpContains", NullPolicy.STRICT);

Review Comment:
   I would move to this around line 545 so it is with the other BigQuery string functions like SPLIT, etc



##########
core/src/main/java/org/apache/calcite/runtime/CalciteResource.java:
##########
@@ -1008,6 +1008,9 @@ ExInstWithCause<CalciteException> failedToAccessField(
   @BaseMessage("Invalid input for REGEXP_REPLACE: ''{0}''")
   ExInst<CalciteException> invalidInputForRegexpReplace(String value);
 
+  @BaseMessage("Invalid regex input for REGEXP_CONTAINS: ''{0}''")
+  ExInst<CalciteException> invalidInputForRegexpContains(String value);

Review Comment:
   nit (up to you): move this above RegexpReplace (especially if you anticipate adding other Regexp Errors to this, might make sense to alphabetize them)



##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -4438,6 +4438,31 @@ private static void checkIf(SqlOperatorFixture f) {
     f0.forEachLibrary(list(SqlLibrary.MYSQL, SqlLibrary.ORACLE), consumer);
   }
 
+  @Test
+  void testRegexpContainsFunc() {
+    final SqlOperatorFixture f = fixture().setFor(SqlLibraryOperators.REGEXP_CONTAINS)
+        .withLibrary(SqlLibrary.BIG_QUERY);
+    f.checkBoolean("regexp_contains('abc def ghi', 'abc')", true);
+    f.checkBoolean("regexp_contains('abc def ghi', '[a-z]+')", true);
+    f.checkBoolean("regexp_contains('foo@bar.com', '@[a-zA-Z0-9-]+\\.[a-zA-Z0-9-.]+')", true);
+    f.checkBoolean("regexp_contains('foo@.com', '@[a-zA-Z0-9-]+\\.[a-zA-Z0-9-.]+')", false);
+    f.checkBoolean("regexp_contains('5556664422', '^\\d{10}$')", true);
+    f.checkBoolean("regexp_contains('11555666442233', '^\\d{10}$')", false);
+    f.checkBoolean("regexp_contains('55566644221133', '\\d{10}')", true);
+    f.checkBoolean("regexp_contains('55as56664as422', '\\d{10}')", false);
+
+    f.checkQuery("select regexp_contains('abc def ghi', 'abc')");
+    f.checkQuery("select regexp_contains('foo@bar.com', '@[a-zA-Z0-9-]+\\\\.[a-zA-Z0-9-.]+')");
+    f.checkQuery("select regexp_contains('55as56664as422', '\\d{10}')");
+
+    f.checkFails("regexp_contains('abc def ghi', '(abc')",
+        "Invalid regex input for REGEXP_CONTAINS: '(abc'", true);
+    f.checkFails("regexp_contains('abc def ghi', '{3}')",
+        "Invalid regex input for REGEXP_CONTAINS: '{3}'", true);
+    f.checkFails("regexp_contains('abc def ghi', '[z-a]')",

Review Comment:
   add a couple more tests for null values "f.checkNull" might be useful



##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -394,6 +394,15 @@ private static int makeRegexpFlags(@Nullable String stringFlags) {
     return flags;
   }
 
+  /** SQL {@code REGEXP_CONTAINS(value, regexp)} function. */
+  public static boolean regexpContains(String value, String regex) {
+    try {
+      return Pattern.compile(regex, Pattern.CASE_INSENSITIVE).matcher(value).matches();
+    } catch (Exception exception) {

Review Comment:
   nit: change this to 'e' to match other caught exception names



##########
core/src/main/java/org/apache/calcite/sql/fun/SqlLibraryOperators.java:
##########
@@ -467,6 +467,16 @@ static RelDataType deriveTypeSplit(SqlOperatorBinding operatorBinding,
   @LibraryOperator(libraries = {MYSQL, ORACLE})
   public static final SqlFunction REGEXP_REPLACE = new SqlRegexpReplaceFunction();
 
+  /**
+   * The "REGEXP_CONTAINS(value, regexp)" function.
+   * Returns TRUE if value is a partial match for the regular expression, regexp.
+   */
+  @LibraryOperator(libraries = { BIG_QUERY })

Review Comment:
   Also can you fix the javadoc so that it matches the format of the other ones? There are some good examples starting around line 511



##########
core/src/main/java/org/apache/calcite/sql/fun/SqlLibraryOperators.java:
##########
@@ -467,6 +467,16 @@ static RelDataType deriveTypeSplit(SqlOperatorBinding operatorBinding,
   @LibraryOperator(libraries = {MYSQL, ORACLE})
   public static final SqlFunction REGEXP_REPLACE = new SqlRegexpReplaceFunction();
 
+  /**
+   * The "REGEXP_CONTAINS(value, regexp)" function.
+   * Returns TRUE if value is a partial match for the regular expression, regexp.
+   */
+  @LibraryOperator(libraries = { BIG_QUERY })

Review Comment:
   Can remove the spaces around 'BIG_QUERY' 



##########
site/_docs/reference.md:
##########
@@ -2774,6 +2774,7 @@ BigQuery's type system uses confusingly different names for types and functions:
 | b | PARSE_TIME(format, string)                     | Uses format specified by *format* to convert *string* representation of time to a TIME value
 | b | PARSE_TIMESTAMP(format, string[, timeZone])    | Uses format specified by *format* to convert *string* representation of timestamp to a TIMESTAMP WITH LOCAL TIME ZONE value in *timeZone*
 | b | POW(numeric1, numeric2)                        | Returns *numeric1* raised to the power *numeric2*
+| b | REGEXP_CONTAINS(value, regexp)                 | Returns TRUE if *value* is a partial match for the regular expression, *regexp*.

Review Comment:
   Don't need period here. 
   
   nit: Would it be easier to say something like "Returns whether \*value\* is a partial match for the regular expression, \*regexp\*? This is up to you



##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -394,6 +394,15 @@ private static int makeRegexpFlags(@Nullable String stringFlags) {
     return flags;
   }
 
+  /** SQL {@code REGEXP_CONTAINS(value, regexp)} function. */
+  public static boolean regexpContains(String value, String regex) {
+    try {
+      return Pattern.compile(regex, Pattern.CASE_INSENSITIVE).matcher(value).matches();
+    } catch (Exception exception) {

Review Comment:
   This is inconsistent though because there is 'e', 'ex', etc



-- 
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

[GitHub] [calcite] Anthrino commented on a diff in pull request #3338: [CALCITE-5873] Add REGEXP_CONTAINS function (enabled in BigQuery library)

Posted by "Anthrino (via GitHub)" <gi...@apache.org>.
Anthrino commented on code in PR #3338:
URL: https://github.com/apache/calcite/pull/3338#discussion_r1293843362


##########
babel/src/test/resources/sql/big-query.iq:
##########
@@ -3520,5 +3520,71 @@ FROM items;
 
 !ok
 
+#####################################################################

Review Comment:
   Thanks for pointing out Julian, moved the tests to appropriate location.



-- 
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

[GitHub] [calcite] tanclary commented on a diff in pull request #3338: [CALCITE-5873] Add REGEXP_CONTAINS function (enabled in BigQuery library)

Posted by "tanclary (via GitHub)" <gi...@apache.org>.
tanclary commented on code in PR #3338:
URL: https://github.com/apache/calcite/pull/3338#discussion_r1295388897


##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -345,6 +346,17 @@ public static String sha512(ByteString string)  {
     return DigestUtils.sha512Hex(string.getBytes());
   }
 
+  /** SQL {@code REGEXP_CONTAINS(value, regexp)} function.
+   * Throws a runtime exception for invalid regular expressions.*/
+  public static boolean regexpContains(String value, String regex) {
+    try {
+      Pattern regexp = Pattern.compile(regex);

Review Comment:
   I think it makes sense to have one in both place because If someone were to write a query using regex that was valid in the re2 library and not Java, if it used Calcite's implementation then the error would get thrown from `regexpContains` and the user/developer would be able to reference that comment for clarification on the library decision.



##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -345,6 +346,17 @@ public static String sha512(ByteString string)  {
     return DigestUtils.sha512Hex(string.getBytes());
   }
 
+  /** SQL {@code REGEXP_CONTAINS(value, regexp)} function.
+   * Throws a runtime exception for invalid regular expressions.*/
+  public static boolean regexpContains(String value, String regex) {
+    try {
+      Pattern regexp = Pattern.compile(regex);

Review Comment:
   I think it makes sense to have one in both places because If someone were to write a query using regex that was valid in the re2 library and not Java, if it used Calcite's implementation then the error would get thrown from `regexpContains` and the user/developer would be able to reference that comment for clarification on the library decision.



-- 
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

[GitHub] [calcite] Anthrino commented on a diff in pull request #3338: [CALCITE-5873/CALCITE-5858] Add REGEXP_CONTAINS function for BigQuery Library

Posted by "Anthrino (via GitHub)" <gi...@apache.org>.
Anthrino commented on code in PR #3338:
URL: https://github.com/apache/calcite/pull/3338#discussion_r1275552731


##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -4438,6 +4438,31 @@ private static void checkIf(SqlOperatorFixture f) {
     f0.forEachLibrary(list(SqlLibrary.MYSQL, SqlLibrary.ORACLE), consumer);
   }
 
+  @Test
+  void testRegexpContainsFunc() {
+    final SqlOperatorFixture f = fixture().setFor(SqlLibraryOperators.REGEXP_CONTAINS)
+        .withLibrary(SqlLibrary.BIG_QUERY);
+    f.checkBoolean("regexp_contains('abc def ghi', 'abc')", true);
+    f.checkBoolean("regexp_contains('abc def ghi', '[a-z]+')", true);
+    f.checkBoolean("regexp_contains('foo@bar.com', '@[a-zA-Z0-9-]+\\.[a-zA-Z0-9-.]+')", true);
+    f.checkBoolean("regexp_contains('foo@.com', '@[a-zA-Z0-9-]+\\.[a-zA-Z0-9-.]+')", false);
+    f.checkBoolean("regexp_contains('5556664422', '^\\d{10}$')", true);
+    f.checkBoolean("regexp_contains('11555666442233', '^\\d{10}$')", false);
+    f.checkBoolean("regexp_contains('55566644221133', '\\d{10}')", true);
+    f.checkBoolean("regexp_contains('55as56664as422', '\\d{10}')", false);
+
+    f.checkQuery("select regexp_contains('abc def ghi', 'abc')");
+    f.checkQuery("select regexp_contains('foo@bar.com', '@[a-zA-Z0-9-]+\\\\.[a-zA-Z0-9-.]+')");
+    f.checkQuery("select regexp_contains('55as56664as422', '\\d{10}')");
+
+    f.checkFails("regexp_contains('abc def ghi', '(abc')",
+        "Invalid regex input for REGEXP_CONTAINS: '(abc'", true);
+    f.checkFails("regexp_contains('abc def ghi', '{3}')",
+        "Invalid regex input for REGEXP_CONTAINS: '{3}'", true);
+    f.checkFails("regexp_contains('abc def ghi', '[z-a]')",

Review Comment:
   Got it, thanks Tanner! The documentation didn't mention NULL returns so I assumed it returns bools, will run some test queries for additional NULL scenarios.



-- 
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

[GitHub] [calcite] Anthrino commented on a diff in pull request #3338: [CALCITE-5873] Add REGEXP_CONTAINS function (enabled in BigQuery library)

Posted by "Anthrino (via GitHub)" <gi...@apache.org>.
Anthrino commented on code in PR #3338:
URL: https://github.com/apache/calcite/pull/3338#discussion_r1293844571


##########
core/src/test/java/org/apache/calcite/test/SqlFunctionsTest.java:
##########
@@ -234,6 +235,34 @@ static <E> List<E> list() {
     assertThat(posixRegex("abcq", "[[:xdigit:]]", false), is(true));
   }
 
+  @Test void testRegexpContains() {
+    try {
+      regexpContains("abc def ghi", "(abc");
+      fail("'regexp_contains' on an invalid regex input '(abc' is not possible");
+    } catch (CalciteException e) {
+      assertThat(e.getMessage(),
+          is("Invalid regex input for REGEXP_CONTAINS: 'Unclosed group near index 4\n(abc'"));

Review Comment:
   Converted into Runtime Exceptions and corrected \n formatting.



-- 
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

[GitHub] [calcite] tanclary commented on a diff in pull request #3338: [CALCITE-5873] Add REGEXP_CONTAINS function (enabled in BigQuery library)

Posted by "tanclary (via GitHub)" <gi...@apache.org>.
tanclary commented on code in PR #3338:
URL: https://github.com/apache/calcite/pull/3338#discussion_r1295388897


##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -345,6 +346,17 @@ public static String sha512(ByteString string)  {
     return DigestUtils.sha512Hex(string.getBytes());
   }
 
+  /** SQL {@code REGEXP_CONTAINS(value, regexp)} function.
+   * Throws a runtime exception for invalid regular expressions.*/
+  public static boolean regexpContains(String value, String regex) {
+    try {
+      Pattern regexp = Pattern.compile(regex);

Review Comment:
   I think it makes sense to have one in both places because If someone were to write a query using regex that was valid in the re2 library and not Java, if it used Calcite's implementation then the error would get thrown from `regexpContains` and the user/developer would be able to reference that comment for clarification on the library decision. This comes to mind given the recent bugs with split and other functions regarding differing behavior between Calcite & BigQuery.



##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -345,6 +346,17 @@ public static String sha512(ByteString string)  {
     return DigestUtils.sha512Hex(string.getBytes());
   }
 
+  /** SQL {@code REGEXP_CONTAINS(value, regexp)} function.
+   * Throws a runtime exception for invalid regular expressions.*/
+  public static boolean regexpContains(String value, String regex) {
+    try {
+      Pattern regexp = Pattern.compile(regex);

Review Comment:
   I think it makes sense to have one in both places because If someone were to write a query using regex that was valid in the re2 library and not Java, if it used Calcite's implementation then the error would get thrown from `regexpContains` and the user/developer would be able to reference that comment for clarification on the library decision. This comes to mind given the recent bugs with `split` and other functions regarding differing behavior between Calcite & BigQuery.



-- 
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

[GitHub] [calcite] tanclary commented on a diff in pull request #3338: [CALCITE-5873/CALCITE-5858] Add REGEXP_CONTAINS function for BigQuery Library

Posted by "tanclary (via GitHub)" <gi...@apache.org>.
tanclary commented on code in PR #3338:
URL: https://github.com/apache/calcite/pull/3338#discussion_r1275537196


##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -4438,6 +4438,31 @@ private static void checkIf(SqlOperatorFixture f) {
     f0.forEachLibrary(list(SqlLibrary.MYSQL, SqlLibrary.ORACLE), consumer);
   }
 
+  @Test
+  void testRegexpContainsFunc() {
+    final SqlOperatorFixture f = fixture().setFor(SqlLibraryOperators.REGEXP_CONTAINS)
+        .withLibrary(SqlLibrary.BIG_QUERY);
+    f.checkBoolean("regexp_contains('abc def ghi', 'abc')", true);
+    f.checkBoolean("regexp_contains('abc def ghi', '[a-z]+')", true);
+    f.checkBoolean("regexp_contains('foo@bar.com', '@[a-zA-Z0-9-]+\\.[a-zA-Z0-9-.]+')", true);
+    f.checkBoolean("regexp_contains('foo@.com', '@[a-zA-Z0-9-]+\\.[a-zA-Z0-9-.]+')", false);
+    f.checkBoolean("regexp_contains('5556664422', '^\\d{10}$')", true);
+    f.checkBoolean("regexp_contains('11555666442233', '^\\d{10}$')", false);
+    f.checkBoolean("regexp_contains('55566644221133', '\\d{10}')", true);
+    f.checkBoolean("regexp_contains('55as56664as422', '\\d{10}')", false);
+
+    f.checkQuery("select regexp_contains('abc def ghi', 'abc')");
+    f.checkQuery("select regexp_contains('foo@bar.com', '@[a-zA-Z0-9-]+\\\\.[a-zA-Z0-9-.]+')");
+    f.checkQuery("select regexp_contains('55as56664as422', '\\d{10}')");
+
+    f.checkFails("regexp_contains('abc def ghi', '(abc')",
+        "Invalid regex input for REGEXP_CONTAINS: '(abc'", true);
+    f.checkFails("regexp_contains('abc def ghi', '{3}')",
+        "Invalid regex input for REGEXP_CONTAINS: '{3}'", true);
+    f.checkFails("regexp_contains('abc def ghi', '[z-a]')",

Review Comment:
   In other cases where you are expecting a non-non value you should use .checkBoolean (at least for this function given that the return type is boolean).



-- 
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

[GitHub] [calcite] julianhyde commented on a diff in pull request #3338: [CALCITE-5873] Add REGEXP_CONTAINS function (enabled in BigQuery library)

Posted by "julianhyde (via GitHub)" <gi...@apache.org>.
julianhyde commented on code in PR #3338:
URL: https://github.com/apache/calcite/pull/3338#discussion_r1289331928


##########
site/_docs/reference.md:
##########
@@ -2776,6 +2776,7 @@ BigQuery's type system uses confusingly different names for types and functions:
 | b | PARSE_TIMESTAMP(format, string[, timeZone])    | Uses format specified by *format* to convert *string* representation of timestamp to a TIMESTAMP WITH LOCAL TIME ZONE value in *timeZone*
 | h s | PARSE_URL(urlString, partToExtract [, keyToExtract] ) | Returns the specified *partToExtract* from the *urlString*. Valid values for *partToExtract* include HOST, PATH, QUERY, REF, PROTOCOL, AUTHORITY, FILE, and USERINFO. *keyToExtract* specifies which query to extract
 | b | POW(numeric1, numeric2)                        | Returns *numeric1* raised to the power *numeric2*
+| b | REGEXP_CONTAINS(value, regexp)                 | Returns TRUE if *value* is a partial match for the regular expression, *regexp*

Review Comment:
   would 'string' be better than 'value'?



##########
core/src/main/java/org/apache/calcite/runtime/CalciteResource.java:
##########
@@ -1005,6 +1005,9 @@ ExInstWithCause<CalciteException> failedToAccessField(
   @BaseMessage("Invalid input for JSON_STORAGE_SIZE: ''{0}''")
   ExInst<CalciteException> invalidInputForJsonStorageSize(String value);
 
+  @BaseMessage("Invalid regex input for REGEXP_CONTAINS: ''{0}''")

Review Comment:
   'Invalid regular expression' might be better



##########
core/src/main/java/org/apache/calcite/runtime/CalciteResource.java:
##########
@@ -1005,6 +1005,9 @@ ExInstWithCause<CalciteException> failedToAccessField(
   @BaseMessage("Invalid input for JSON_STORAGE_SIZE: ''{0}''")
   ExInst<CalciteException> invalidInputForJsonStorageSize(String value);
 
+  @BaseMessage("Invalid regex input for REGEXP_CONTAINS: ''{0}''")
+  ExInst<CalciteException> invalidInputForRegexpContains(String value);

Review Comment:
   Change `ExInst<CalciteException>` to `ExInst<RuntimeException>` and then I think you will be able to remove the use of `Util.toUnchecked`.
   
   `CalciteException` is for exceptions detected at prepare time (generally validation) whereas you need a runtime exception.



##########
site/_docs/reference.md:
##########
@@ -2776,6 +2776,7 @@ BigQuery's type system uses confusingly different names for types and functions:
 | b | PARSE_TIMESTAMP(format, string[, timeZone])    | Uses format specified by *format* to convert *string* representation of timestamp to a TIMESTAMP WITH LOCAL TIME ZONE value in *timeZone*
 | h s | PARSE_URL(urlString, partToExtract [, keyToExtract] ) | Returns the specified *partToExtract* from the *urlString*. Valid values for *partToExtract* include HOST, PATH, QUERY, REF, PROTOCOL, AUTHORITY, FILE, and USERINFO. *keyToExtract* specifies which query to extract
 | b | POW(numeric1, numeric2)                        | Returns *numeric1* raised to the power *numeric2*
+| b | REGEXP_CONTAINS(value, regexp)                 | Returns TRUE if *value* is a partial match for the regular expression, *regexp*

Review Comment:
   I prefer 'Returns whether' to 'Returns TRUE if'. The former is more direct, and the latter doesn't actually say under what conditions the method returns FALSE.



##########
babel/src/test/resources/sql/big-query.iq:
##########
@@ -3520,5 +3520,71 @@ FROM items;
 
 !ok
 
+#####################################################################

Review Comment:
   Don't put stuff at the end of files. Put it in the best place. (You'll have to figure out how the file is organized - e.g. alphabetical or by topic.)
   
   (If everyone puts stuff at the end, we get lots of conflicts.)



##########
core/src/test/java/org/apache/calcite/test/SqlFunctionsTest.java:
##########
@@ -234,6 +235,34 @@ static <E> List<E> list() {
     assertThat(posixRegex("abcq", "[[:xdigit:]]", false), is(true));
   }
 
+  @Test void testRegexpContains() {
+    try {
+      regexpContains("abc def ghi", "(abc");
+      fail("'regexp_contains' on an invalid regex input '(abc' is not possible");
+    } catch (CalciteException e) {
+      assertThat(e.getMessage(),
+          is("Invalid regex input for REGEXP_CONTAINS: 'Unclosed group near index 4\n(abc'"));

Review Comment:
   Break strings after each `\n`. It makes them easier to read.



##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -401,6 +402,16 @@ private static int makeRegexpFlags(@Nullable String stringFlags) {
     return flags;
   }
 
+  /** SQL {@code REGEXP_CONTAINS(value, regexp)} function. */

Review Comment:
   Can you briefly document what happens if the regex is invalid.



##########
core/src/test/java/org/apache/calcite/test/SqlFunctionsTest.java:
##########
@@ -234,6 +235,34 @@ static <E> List<E> list() {
     assertThat(posixRegex("abcq", "[[:xdigit:]]", false), is(true));
   }
 
+  @Test void testRegexpContains() {
+    try {
+      regexpContains("abc def ghi", "(abc");
+      fail("'regexp_contains' on an invalid regex input '(abc' is not possible");
+    } catch (CalciteException e) {
+      assertThat(e.getMessage(),
+          is("Invalid regex input for REGEXP_CONTAINS: 'Unclosed group near index 4\n(abc'"));

Review Comment:
   I'm surprised that this test works. Does `regexpContains` throw a `CalciteException`? Looks like it throws a `RuntimeException`.



-- 
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

[GitHub] [calcite] julianhyde commented on a diff in pull request #3338: [CALCITE-5873] Add REGEXP_CONTAINS function (enabled in BigQuery library)

Posted by "julianhyde (via GitHub)" <gi...@apache.org>.
julianhyde commented on code in PR #3338:
URL: https://github.com/apache/calcite/pull/3338#discussion_r1291828491


##########
site/_docs/reference.md:
##########
@@ -2776,6 +2776,7 @@ BigQuery's type system uses confusingly different names for types and functions:
 | b | PARSE_TIMESTAMP(format, string[, timeZone])    | Uses format specified by *format* to convert *string* representation of timestamp to a TIMESTAMP WITH LOCAL TIME ZONE value in *timeZone*
 | h s | PARSE_URL(urlString, partToExtract [, keyToExtract] ) | Returns the specified *partToExtract* from the *urlString*. Valid values for *partToExtract* include HOST, PATH, QUERY, REF, PROTOCOL, AUTHORITY, FILE, and USERINFO. *keyToExtract* specifies which query to extract
 | b | POW(numeric1, numeric2)                        | Returns *numeric1* raised to the power *numeric2*
+| b | REGEXP_CONTAINS(value, regexp)                 | Returns TRUE if *value* is a partial match for the regular expression, *regexp*

Review Comment:
   I understand your dilemma. I think it's better to make the Calcite descriptions uniform, even if they come from many different DBs.
   
   In `big-query.iq`, feel free to use the doc copy-pasted from BigQuery.



-- 
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

[GitHub] [calcite] Anthrino commented on a diff in pull request #3338: [CALCITE-5873] Add REGEXP_CONTAINS function (enabled in BigQuery library)

Posted by "Anthrino (via GitHub)" <gi...@apache.org>.
Anthrino commented on code in PR #3338:
URL: https://github.com/apache/calcite/pull/3338#discussion_r1291725341


##########
core/src/test/java/org/apache/calcite/test/SqlFunctionsTest.java:
##########
@@ -234,6 +235,34 @@ static <E> List<E> list() {
     assertThat(posixRegex("abcq", "[[:xdigit:]]", false), is(true));
   }
 
+  @Test void testRegexpContains() {
+    try {
+      regexpContains("abc def ghi", "(abc");
+      fail("'regexp_contains' on an invalid regex input '(abc' is not possible");
+    } catch (CalciteException e) {
+      assertThat(e.getMessage(),
+          is("Invalid regex input for REGEXP_CONTAINS: 'Unclosed group near index 4\n(abc'"));

Review Comment:
   > Break strings after each `\n`. It makes them easier to read.
   
   Thanks for the pointer @julianhyde, the \n is included only as it is part of the exception message returned, I can break the string for legibility but this was easier to match with the returned string.
   
   
   
   > I'm surprised that this test works. Does `regexpContains` throw a `CalciteException`? Looks like it throws a `RuntimeException`.
   
   Yes we were returning a `CalciteException` previously, saw your comment on the other file will modify it to a `RuntimeException`.



-- 
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

[GitHub] [calcite] tanclary commented on a diff in pull request #3338: [CALCITE-5873] Add REGEXP_CONTAINS function (enabled in BigQuery library)

Posted by "tanclary (via GitHub)" <gi...@apache.org>.
tanclary commented on code in PR #3338:
URL: https://github.com/apache/calcite/pull/3338#discussion_r1295097215


##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -345,6 +346,17 @@ public static String sha512(ByteString string)  {
     return DigestUtils.sha512Hex(string.getBytes());
   }
 
+  /** SQL {@code REGEXP_CONTAINS(value, regexp)} function.
+   * Throws a runtime exception for invalid regular expressions.*/
+  public static boolean regexpContains(String value, String regex) {
+    try {
+      Pattern regexp = Pattern.compile(regex);

Review Comment:
   If you're going to stick with Java library, I think it would help to add a comment here or elsewhere linking to the case where that decision was made for future users/devs



-- 
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

[GitHub] [calcite] tanclary commented on a diff in pull request #3338: [CALCITE-5873/CALCITE-5858] Add REGEXP_CONTAINS function for BigQuery Library

Posted by "tanclary (via GitHub)" <gi...@apache.org>.
tanclary commented on code in PR #3338:
URL: https://github.com/apache/calcite/pull/3338#discussion_r1275535907


##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -4438,6 +4438,31 @@ private static void checkIf(SqlOperatorFixture f) {
     f0.forEachLibrary(list(SqlLibrary.MYSQL, SqlLibrary.ORACLE), consumer);
   }
 
+  @Test
+  void testRegexpContainsFunc() {
+    final SqlOperatorFixture f = fixture().setFor(SqlLibraryOperators.REGEXP_CONTAINS)
+        .withLibrary(SqlLibrary.BIG_QUERY);
+    f.checkBoolean("regexp_contains('abc def ghi', 'abc')", true);
+    f.checkBoolean("regexp_contains('abc def ghi', '[a-z]+')", true);
+    f.checkBoolean("regexp_contains('foo@bar.com', '@[a-zA-Z0-9-]+\\.[a-zA-Z0-9-.]+')", true);
+    f.checkBoolean("regexp_contains('foo@.com', '@[a-zA-Z0-9-]+\\.[a-zA-Z0-9-.]+')", false);
+    f.checkBoolean("regexp_contains('5556664422', '^\\d{10}$')", true);
+    f.checkBoolean("regexp_contains('11555666442233', '^\\d{10}$')", false);
+    f.checkBoolean("regexp_contains('55566644221133', '\\d{10}')", true);
+    f.checkBoolean("regexp_contains('55as56664as422', '\\d{10}')", false);
+
+    f.checkQuery("select regexp_contains('abc def ghi', 'abc')");
+    f.checkQuery("select regexp_contains('foo@bar.com', '@[a-zA-Z0-9-]+\\\\.[a-zA-Z0-9-.]+')");
+    f.checkQuery("select regexp_contains('55as56664as422', '\\d{10}')");
+
+    f.checkFails("regexp_contains('abc def ghi', '(abc')",
+        "Invalid regex input for REGEXP_CONTAINS: '(abc'", true);
+    f.checkFails("regexp_contains('abc def ghi', '{3}')",
+        "Invalid regex input for REGEXP_CONTAINS: '{3}'", true);
+    f.checkFails("regexp_contains('abc def ghi', '[z-a]')",

Review Comment:
   That's right. It's still helpful to add .checkNull() tests to ensure that if either (or both) arguments is null that, that null is returned. This is useful because not all functions follow that behavior. In this case however, based on a couple of quick queries I ran against BigQuery, if either or both arguments is null the function should return null. Hope this helps.



##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -4438,6 +4438,31 @@ private static void checkIf(SqlOperatorFixture f) {
     f0.forEachLibrary(list(SqlLibrary.MYSQL, SqlLibrary.ORACLE), consumer);
   }
 
+  @Test
+  void testRegexpContainsFunc() {
+    final SqlOperatorFixture f = fixture().setFor(SqlLibraryOperators.REGEXP_CONTAINS)
+        .withLibrary(SqlLibrary.BIG_QUERY);
+    f.checkBoolean("regexp_contains('abc def ghi', 'abc')", true);
+    f.checkBoolean("regexp_contains('abc def ghi', '[a-z]+')", true);
+    f.checkBoolean("regexp_contains('foo@bar.com', '@[a-zA-Z0-9-]+\\.[a-zA-Z0-9-.]+')", true);
+    f.checkBoolean("regexp_contains('foo@.com', '@[a-zA-Z0-9-]+\\.[a-zA-Z0-9-.]+')", false);
+    f.checkBoolean("regexp_contains('5556664422', '^\\d{10}$')", true);
+    f.checkBoolean("regexp_contains('11555666442233', '^\\d{10}$')", false);
+    f.checkBoolean("regexp_contains('55566644221133', '\\d{10}')", true);
+    f.checkBoolean("regexp_contains('55as56664as422', '\\d{10}')", false);
+
+    f.checkQuery("select regexp_contains('abc def ghi', 'abc')");
+    f.checkQuery("select regexp_contains('foo@bar.com', '@[a-zA-Z0-9-]+\\\\.[a-zA-Z0-9-.]+')");
+    f.checkQuery("select regexp_contains('55as56664as422', '\\d{10}')");
+
+    f.checkFails("regexp_contains('abc def ghi', '(abc')",
+        "Invalid regex input for REGEXP_CONTAINS: '(abc'", true);
+    f.checkFails("regexp_contains('abc def ghi', '{3}')",
+        "Invalid regex input for REGEXP_CONTAINS: '{3}'", true);
+    f.checkFails("regexp_contains('abc def ghi', '[z-a]')",

Review Comment:
   That's right. It's still helpful to add .checkNull() tests to ensure that if either (or both) arguments is null that null is returned. This is useful because not all functions follow that behavior. In this case however, based on a couple of quick queries I ran against BigQuery, if either or both arguments is null the function should return null. Hope this helps.



-- 
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

[GitHub] [calcite] Anthrino commented on a diff in pull request #3338: [CALCITE-5873] Add REGEXP_CONTAINS function (enabled in BigQuery library)

Posted by "Anthrino (via GitHub)" <gi...@apache.org>.
Anthrino commented on code in PR #3338:
URL: https://github.com/apache/calcite/pull/3338#discussion_r1293844008


##########
core/src/main/java/org/apache/calcite/runtime/CalciteResource.java:
##########
@@ -1005,6 +1005,9 @@ ExInstWithCause<CalciteException> failedToAccessField(
   @BaseMessage("Invalid input for JSON_STORAGE_SIZE: ''{0}''")
   ExInst<CalciteException> invalidInputForJsonStorageSize(String value);
 
+  @BaseMessage("Invalid regex input for REGEXP_CONTAINS: ''{0}''")

Review Comment:
   Updated error statement as suggested.



-- 
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

[GitHub] [calcite] sonarcloud[bot] commented on pull request #3338: [CALCITE-5873] Add REGEXP_CONTAINS function (enabled in BigQuery library)

Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3338:
URL: https://github.com/apache/calcite/pull/3338#issuecomment-1679793467

   Kudos, SonarCloud Quality Gate passed!&nbsp; &nbsp; [![Quality Gate passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/passed-16px.png 'Quality Gate passed')](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3338)
   
   [![Bug](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug-16px.png 'Bug')](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=BUG) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=BUG)  
   [![Vulnerability](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability-16px.png 'Vulnerability')](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=VULNERABILITY) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=VULNERABILITY)  
   [![Security Hotspot](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot-16px.png 'Security Hotspot')](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3338&resolved=false&types=SECURITY_HOTSPOT) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3338&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3338&resolved=false&types=SECURITY_HOTSPOT)  
   [![Code Smell](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell-16px.png 'Code Smell')](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=CODE_SMELL) [![D](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/D-16px.png 'D')](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=CODE_SMELL) [6 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=CODE_SMELL)
   
   [![100.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/100-16px.png '100.0%')](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3338&metric=new_coverage&view=list) [100.0% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3338&metric=new_coverage&view=list)  
   [![2.1%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3-16px.png '2.1%')](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3338&metric=new_duplicated_lines_density&view=list) [2.1% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3338&metric=new_duplicated_lines_density&view=list)
   
   


-- 
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

[GitHub] [calcite] tanclary merged pull request #3338: [CALCITE-5873] Add REGEXP_CONTAINS function (enabled in BigQuery library)

Posted by "tanclary (via GitHub)" <gi...@apache.org>.
tanclary merged PR #3338:
URL: https://github.com/apache/calcite/pull/3338


-- 
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

[GitHub] [calcite] sonarcloud[bot] commented on pull request #3338: [CALCITE-5873] Add REGEXP_CONTAINS function (enabled in BigQuery library)

Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3338:
URL: https://github.com/apache/calcite/pull/3338#issuecomment-1679687642

   Kudos, SonarCloud Quality Gate passed!&nbsp; &nbsp; [![Quality Gate passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/passed-16px.png 'Quality Gate passed')](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3338)
   
   [![Bug](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug-16px.png 'Bug')](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=BUG) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=BUG)  
   [![Vulnerability](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability-16px.png 'Vulnerability')](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=VULNERABILITY) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=VULNERABILITY)  
   [![Security Hotspot](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot-16px.png 'Security Hotspot')](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3338&resolved=false&types=SECURITY_HOTSPOT) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3338&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3338&resolved=false&types=SECURITY_HOTSPOT)  
   [![Code Smell](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell-16px.png 'Code Smell')](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=CODE_SMELL) [![D](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/D-16px.png 'D')](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=CODE_SMELL) [6 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=CODE_SMELL)
   
   [![100.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/100-16px.png '100.0%')](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3338&metric=new_coverage&view=list) [100.0% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3338&metric=new_coverage&view=list)  
   [![2.1%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3-16px.png '2.1%')](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3338&metric=new_duplicated_lines_density&view=list) [2.1% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3338&metric=new_duplicated_lines_density&view=list)
   
   


-- 
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

[GitHub] [calcite] mihaibudiu commented on a diff in pull request #3338: [CALCITE-5873] Add REGEXP_CONTAINS function (enabled in BigQuery library)

Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #3338:
URL: https://github.com/apache/calcite/pull/3338#discussion_r1289341615


##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -401,6 +402,16 @@ private static int makeRegexpFlags(@Nullable String stringFlags) {
     return flags;
   }
 
+  /** SQL {@code REGEXP_CONTAINS(value, regexp)} function. */
+  public static boolean regexpContains(String value, String regex) {
+    try {
+      Pattern regexp = Pattern.compile(regex, Pattern.CASE_INSENSITIVE);

Review Comment:
   Does this pattern handle Java regular expressions or BigQuery regular expressions?



-- 
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

[GitHub] [calcite] Anthrino commented on a diff in pull request #3338: [CALCITE-5873] Add REGEXP_CONTAINS function (enabled in BigQuery library)

Posted by "Anthrino (via GitHub)" <gi...@apache.org>.
Anthrino commented on code in PR #3338:
URL: https://github.com/apache/calcite/pull/3338#discussion_r1293847273


##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -401,6 +402,16 @@ private static int makeRegexpFlags(@Nullable String stringFlags) {
     return flags;
   }
 
+  /** SQL {@code REGEXP_CONTAINS(value, regexp)} function. */

Review Comment:
   Added a statement, please check if this is what you had in mind.



-- 
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

[GitHub] [calcite] tanclary commented on a diff in pull request #3338: [CALCITE-5873] Add REGEXP_CONTAINS function (enabled in BigQuery library)

Posted by "tanclary (via GitHub)" <gi...@apache.org>.
tanclary commented on code in PR #3338:
URL: https://github.com/apache/calcite/pull/3338#discussion_r1295127698


##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -345,6 +346,17 @@ public static String sha512(ByteString string)  {
     return DigestUtils.sha512Hex(string.getBytes());
   }
 
+  /** SQL {@code REGEXP_CONTAINS(value, regexp)} function.
+   * Throws a runtime exception for invalid regular expressions.*/
+  public static boolean regexpContains(String value, String regex) {
+    try {
+      Pattern regexp = Pattern.compile(regex);

Review Comment:
   you can use "<a href >" to link to a specific case if you text-search you should find some good examples



##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -345,6 +346,17 @@ public static String sha512(ByteString string)  {
     return DigestUtils.sha512Hex(string.getBytes());
   }
 
+  /** SQL {@code REGEXP_CONTAINS(value, regexp)} function.
+   * Throws a runtime exception for invalid regular expressions.*/
+  public static boolean regexpContains(String value, String regex) {
+    try {
+      Pattern regexp = Pattern.compile(regex);

Review Comment:
   you can use <a href > to link to a specific case if you text-search you should find some good examples



-- 
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

[GitHub] [calcite] julianhyde commented on a diff in pull request #3338: [CALCITE-5873] Add REGEXP_CONTAINS function (enabled in BigQuery library)

Posted by "julianhyde (via GitHub)" <gi...@apache.org>.
julianhyde commented on code in PR #3338:
URL: https://github.com/apache/calcite/pull/3338#discussion_r1294074398


##########
core/src/test/java/org/apache/calcite/test/SqlFunctionsTest.java:
##########
@@ -234,6 +235,44 @@ static <E> List<E> list() {
     assertThat(posixRegex("abcq", "[[:xdigit:]]", false), is(true));
   }
 
+  @Test void testRegexpContains() {
+    try {
+      regexpContains("abc def ghi", "(abc");
+      fail("'regexp_contains' on an invalid regex input '(abc' is not possible");
+    } catch (RuntimeException e) {
+      assertThat(e.getMessage(),
+          is("Invalid regular expression for REGEXP_CONTAINS: 'Unclosed group near index 4\n"
+              +

Review Comment:
   no newline after `+`



-- 
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

[GitHub] [calcite] Anthrino commented on a diff in pull request #3338: [CALCITE-5873] Add REGEXP_CONTAINS function (enabled in BigQuery library)

Posted by "Anthrino (via GitHub)" <gi...@apache.org>.
Anthrino commented on code in PR #3338:
URL: https://github.com/apache/calcite/pull/3338#discussion_r1295165098


##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -345,6 +346,17 @@ public static String sha512(ByteString string)  {
     return DigestUtils.sha512Hex(string.getBytes());
   }
 
+  /** SQL {@code REGEXP_CONTAINS(value, regexp)} function.
+   * Throws a runtime exception for invalid regular expressions.*/
+  public static boolean regexpContains(String value, String regex) {
+    try {
+      Pattern regexp = Pattern.compile(regex);

Review Comment:
   Thanks for the pointer Tanner, added a note with issue link to the method description.



-- 
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

[GitHub] [calcite] sonarcloud[bot] commented on pull request #3338: [CALCITE-5873] Add REGEXP_CONTAINS function (enabled in BigQuery library)

Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3338:
URL: https://github.com/apache/calcite/pull/3338#issuecomment-1682741902

   Kudos, SonarCloud Quality Gate passed!&nbsp; &nbsp; [![Quality Gate passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/passed-16px.png 'Quality Gate passed')](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3338)
   
   [![Bug](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug-16px.png 'Bug')](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=BUG) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=BUG)  
   [![Vulnerability](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability-16px.png 'Vulnerability')](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=VULNERABILITY) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=VULNERABILITY)  
   [![Security Hotspot](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot-16px.png 'Security Hotspot')](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3338&resolved=false&types=SECURITY_HOTSPOT) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3338&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3338&resolved=false&types=SECURITY_HOTSPOT)  
   [![Code Smell](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell-16px.png 'Code Smell')](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=CODE_SMELL) [![D](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/D-16px.png 'D')](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=CODE_SMELL) [6 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=CODE_SMELL)
   
   [![100.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/100-16px.png '100.0%')](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3338&metric=new_coverage&view=list) [100.0% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3338&metric=new_coverage&view=list)  
   [![2.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3-16px.png '2.0%')](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3338&metric=new_duplicated_lines_density&view=list) [2.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3338&metric=new_duplicated_lines_density&view=list)
   
   


-- 
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

[GitHub] [calcite] Anthrino commented on a diff in pull request #3338: [CALCITE-5873] Add REGEXP_CONTAINS function (enabled in BigQuery library)

Posted by "Anthrino (via GitHub)" <gi...@apache.org>.
Anthrino commented on code in PR #3338:
URL: https://github.com/apache/calcite/pull/3338#discussion_r1293845335


##########
site/_docs/reference.md:
##########
@@ -2776,6 +2776,7 @@ BigQuery's type system uses confusingly different names for types and functions:
 | b | PARSE_TIMESTAMP(format, string[, timeZone])    | Uses format specified by *format* to convert *string* representation of timestamp to a TIMESTAMP WITH LOCAL TIME ZONE value in *timeZone*
 | h s | PARSE_URL(urlString, partToExtract [, keyToExtract] ) | Returns the specified *partToExtract* from the *urlString*. Valid values for *partToExtract* include HOST, PATH, QUERY, REF, PROTOCOL, AUTHORITY, FILE, and USERINFO. *keyToExtract* specifies which query to extract
 | b | POW(numeric1, numeric2)                        | Returns *numeric1* raised to the power *numeric2*
+| b | REGEXP_CONTAINS(value, regexp)                 | Returns TRUE if *value* is a partial match for the regular expression, *regexp*

Review Comment:
   Updated ref docs as per suggestions.



-- 
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

[GitHub] [calcite] tanclary commented on a diff in pull request #3338: [CALCITE-5873] Add REGEXP_CONTAINS function (enabled in BigQuery library)

Posted by "tanclary (via GitHub)" <gi...@apache.org>.
tanclary commented on code in PR #3338:
URL: https://github.com/apache/calcite/pull/3338#discussion_r1295127698


##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -345,6 +346,17 @@ public static String sha512(ByteString string)  {
     return DigestUtils.sha512Hex(string.getBytes());
   }
 
+  /** SQL {@code REGEXP_CONTAINS(value, regexp)} function.
+   * Throws a runtime exception for invalid regular expressions.*/
+  public static boolean regexpContains(String value, String regex) {
+    try {
+      Pattern regexp = Pattern.compile(regex);

Review Comment:
   you can use "<a" to link to a specific case if you text-search you should find some good examples



-- 
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

[GitHub] [calcite] sonarcloud[bot] commented on pull request #3338: [CALCITE-5873] Add REGEXP_CONTAINS function (enabled in BigQuery library)

Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3338:
URL: https://github.com/apache/calcite/pull/3338#issuecomment-1679719742

   Kudos, SonarCloud Quality Gate passed!&nbsp; &nbsp; [![Quality Gate passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/passed-16px.png 'Quality Gate passed')](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3338)
   
   [![Bug](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug-16px.png 'Bug')](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=BUG) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=BUG)  
   [![Vulnerability](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability-16px.png 'Vulnerability')](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=VULNERABILITY) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=VULNERABILITY)  
   [![Security Hotspot](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot-16px.png 'Security Hotspot')](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3338&resolved=false&types=SECURITY_HOTSPOT) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3338&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3338&resolved=false&types=SECURITY_HOTSPOT)  
   [![Code Smell](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell-16px.png 'Code Smell')](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=CODE_SMELL) [![D](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/D-16px.png 'D')](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=CODE_SMELL) [6 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=CODE_SMELL)
   
   [![100.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/100-16px.png '100.0%')](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3338&metric=new_coverage&view=list) [100.0% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3338&metric=new_coverage&view=list)  
   [![2.1%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3-16px.png '2.1%')](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3338&metric=new_duplicated_lines_density&view=list) [2.1% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3338&metric=new_duplicated_lines_density&view=list)
   
   


-- 
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

[GitHub] [calcite] Anthrino commented on pull request #3338: [CALCITE-5873] Add REGEXP_CONTAINS function (enabled in BigQuery library)

Posted by "Anthrino (via GitHub)" <gi...@apache.org>.
Anthrino commented on PR #3338:
URL: https://github.com/apache/calcite/pull/3338#issuecomment-1677898074

   > I've made a few cosmetic suggestions. Hopefully they are easy to fix.
   > 
   
   Thanks for the review @julianhyde, I've made the changes. Unfortunately I ran into some conflicts while rebasing this branch which led to duplicate changes from other merged PRs. Had to force push some changes to the remote branch when resolving the conflicts, hence your comments are tagged to outdated files but the changes are added to the latest versions. Sorry for the confusion, please review and let me know.
   
   > Also, you need to add to `SqlOperatorTest`. There are currently no tests for scenarios where REGEXP_CONTAINS returns null (null first or second argument).
   
   I had added three checkNull tests to test these null arg scenarios in Operator tests, am I missing any other means of testing this?


-- 
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

[GitHub] [calcite] wnob commented on a diff in pull request #3338: [CALCITE-5873] Add REGEXP_CONTAINS function (enabled in BigQuery library)

Posted by "wnob (via GitHub)" <gi...@apache.org>.
wnob commented on code in PR #3338:
URL: https://github.com/apache/calcite/pull/3338#discussion_r1295062657


##########
core/src/main/java/org/apache/calcite/sql/fun/SqlLibraryOperators.java:
##########
@@ -475,6 +475,14 @@ static RelDataType deriveTypeSplit(SqlOperatorBinding operatorBinding,
   @LibraryOperator(libraries = {MYSQL, ORACLE})
   public static final SqlFunction REGEXP_REPLACE = new SqlRegexpReplaceFunction();
 
+  /** The "REGEXP_CONTAINS(value, regexp)" function.
+   * Returns TRUE if value is a partial match for the regular expression, regexp. */
+  @LibraryOperator(libraries = {BIG_QUERY})

Review Comment:
   BigQuery [uses re2][1] for their regex engine. Google has an [implementation for Java][2]. We should use that here, both as a matter of syntax conformance (since this is a BigQuery-specific function) and for the security reasons outlined in the RE2J readme. This could be a follow-up, but I don't think we should close the bug until that's done.
   
   [1]: https://cloud.google.com/bigquery/docs/reference/standard-sql/string_functions#regexp_contains
   [2]: https://github.com/google/re2j



-- 
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

[GitHub] [calcite] Anthrino commented on a diff in pull request #3338: [CALCITE-5873] Add REGEXP_CONTAINS function (enabled in BigQuery library)

Posted by "Anthrino (via GitHub)" <gi...@apache.org>.
Anthrino commented on code in PR #3338:
URL: https://github.com/apache/calcite/pull/3338#discussion_r1291720323


##########
site/_docs/reference.md:
##########
@@ -2776,6 +2776,7 @@ BigQuery's type system uses confusingly different names for types and functions:
 | b | PARSE_TIMESTAMP(format, string[, timeZone])    | Uses format specified by *format* to convert *string* representation of timestamp to a TIMESTAMP WITH LOCAL TIME ZONE value in *timeZone*
 | h s | PARSE_URL(urlString, partToExtract [, keyToExtract] ) | Returns the specified *partToExtract* from the *urlString*. Valid values for *partToExtract* include HOST, PATH, QUERY, REF, PROTOCOL, AUTHORITY, FILE, and USERINFO. *keyToExtract* specifies which query to extract
 | b | POW(numeric1, numeric2)                        | Returns *numeric1* raised to the power *numeric2*
+| b | REGEXP_CONTAINS(value, regexp)                 | Returns TRUE if *value* is a partial match for the regular expression, *regexp*

Review Comment:
   Sure @julianhyde, that sounds more clear I just thought it would be best to align it to BQ documentation as they use *value* everywhere.



-- 
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

[GitHub] [calcite] tanclary commented on a diff in pull request #3338: [CALCITE-5873] Add REGEXP_CONTAINS function for BigQuery Library

Posted by "tanclary (via GitHub)" <gi...@apache.org>.
tanclary commented on code in PR #3338:
URL: https://github.com/apache/calcite/pull/3338#discussion_r1280829992


##########
core/src/main/resources/org/apache/calcite/runtime/CalciteResource.properties:
##########
@@ -329,6 +329,7 @@ InvalidInputForJsonKeys=Invalid input for JSON_KEYS: ''{0}''
 InvalidInputForJsonRemove=Invalid input for JSON_REMOVE: document: ''{0}'', jsonpath expressions: ''{1}''
 InvalidInputForJsonStorageSize=Invalid input for JSON_STORAGE_SIZE: ''{0}''
 InvalidInputForRegexpReplace=Invalid input for REGEXP_REPLACE: ''{0}''
+InvalidInputForRegexpContains=Invalid regex input for REGEXP_CONTAINS: ''{0}''

Review Comment:
   Sorry for the nit but maybe contains should go above replace to preserve alphabetic order for the group, it seems like the other sets of functions follow it which is why I suggest it.



##########
core/src/main/java/org/apache/calcite/sql/fun/SqlLibraryOperators.java:
##########
@@ -467,6 +467,14 @@ static RelDataType deriveTypeSplit(SqlOperatorBinding operatorBinding,
   @LibraryOperator(libraries = {MYSQL, ORACLE})
   public static final SqlFunction REGEXP_REPLACE = new SqlRegexpReplaceFunction();
 
+  /** The "REGEXP_CONTAINS(value, regexp)" function.
+   * Returns TRUE if value is a partial match for the regular expression, regexp. */
+  @LibraryOperator(libraries = {BIG_QUERY})
+  public static final SqlFunction REGEXP_CONTAINS =
+          SqlBasicFunction.create("REGEXP_CONTAINS", ReturnTypes.BOOLEAN_NULLABLE,
+          OperandTypes.STRING_STRING,

Review Comment:
   Is the indentation off here? Look at below functions for other examples



-- 
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

[GitHub] [calcite] Anthrino commented on a diff in pull request #3338: [CALCITE-5873] Add REGEXP_CONTAINS function (enabled in BigQuery library)

Posted by "Anthrino (via GitHub)" <gi...@apache.org>.
Anthrino commented on code in PR #3338:
URL: https://github.com/apache/calcite/pull/3338#discussion_r1291719259


##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -401,6 +402,16 @@ private static int makeRegexpFlags(@Nullable String stringFlags) {
     return flags;
   }
 
+  /** SQL {@code REGEXP_CONTAINS(value, regexp)} function. */
+  public static boolean regexpContains(String value, String regex) {
+    try {
+      Pattern regexp = Pattern.compile(regex, Pattern.CASE_INSENSITIVE);

Review Comment:
   Hi @mihaibudiu, this Pattern object is an instance of the java.util.regex library, it has almost complete overlap with BQ regex except for a few negative scenarios as highlighted on this ticket: [CALCITE-5910](https://issues.apache.org/jira/browse/CALCITE-5910)



-- 
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

[GitHub] [calcite] Anthrino commented on a diff in pull request #3338: [CALCITE-5873] Add REGEXP_CONTAINS function (enabled in BigQuery library)

Posted by "Anthrino (via GitHub)" <gi...@apache.org>.
Anthrino commented on code in PR #3338:
URL: https://github.com/apache/calcite/pull/3338#discussion_r1293846017


##########
core/src/main/java/org/apache/calcite/runtime/CalciteResource.java:
##########
@@ -1005,6 +1005,9 @@ ExInstWithCause<CalciteException> failedToAccessField(
   @BaseMessage("Invalid input for JSON_STORAGE_SIZE: ''{0}''")
   ExInst<CalciteException> invalidInputForJsonStorageSize(String value);
 
+  @BaseMessage("Invalid regex input for REGEXP_CONTAINS: ''{0}''")
+  ExInst<CalciteException> invalidInputForRegexpContains(String value);

Review Comment:
   Changed into RuntimeException, thanks for the correction.



-- 
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

[GitHub] [calcite] sonarcloud[bot] commented on pull request #3338: [CALCITE-5873] Add REGEXP_CONTAINS function (enabled in BigQuery library)

Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3338:
URL: https://github.com/apache/calcite/pull/3338#issuecomment-1682806351

   Kudos, SonarCloud Quality Gate passed!&nbsp; &nbsp; [![Quality Gate passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/passed-16px.png 'Quality Gate passed')](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3338)
   
   [![Bug](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug-16px.png 'Bug')](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=BUG) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=BUG)  
   [![Vulnerability](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability-16px.png 'Vulnerability')](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=VULNERABILITY) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=VULNERABILITY)  
   [![Security Hotspot](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot-16px.png 'Security Hotspot')](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3338&resolved=false&types=SECURITY_HOTSPOT) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3338&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3338&resolved=false&types=SECURITY_HOTSPOT)  
   [![Code Smell](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell-16px.png 'Code Smell')](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=CODE_SMELL) [![D](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/D-16px.png 'D')](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=CODE_SMELL) [6 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=CODE_SMELL)
   
   [![100.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/100-16px.png '100.0%')](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3338&metric=new_coverage&view=list) [100.0% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3338&metric=new_coverage&view=list)  
   [![2.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3-16px.png '2.0%')](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3338&metric=new_duplicated_lines_density&view=list) [2.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3338&metric=new_duplicated_lines_density&view=list)
   
   


-- 
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

[GitHub] [calcite] Anthrino commented on a diff in pull request #3338: [CALCITE-5873/CALCITE-5858] Add REGEXP_CONTAINS function for BigQuery Library

Posted by "Anthrino (via GitHub)" <gi...@apache.org>.
Anthrino commented on code in PR #3338:
URL: https://github.com/apache/calcite/pull/3338#discussion_r1275523077


##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -4438,6 +4438,31 @@ private static void checkIf(SqlOperatorFixture f) {
     f0.forEachLibrary(list(SqlLibrary.MYSQL, SqlLibrary.ORACLE), consumer);
   }
 
+  @Test
+  void testRegexpContainsFunc() {
+    final SqlOperatorFixture f = fixture().setFor(SqlLibraryOperators.REGEXP_CONTAINS)
+        .withLibrary(SqlLibrary.BIG_QUERY);
+    f.checkBoolean("regexp_contains('abc def ghi', 'abc')", true);
+    f.checkBoolean("regexp_contains('abc def ghi', '[a-z]+')", true);
+    f.checkBoolean("regexp_contains('foo@bar.com', '@[a-zA-Z0-9-]+\\.[a-zA-Z0-9-.]+')", true);
+    f.checkBoolean("regexp_contains('foo@.com', '@[a-zA-Z0-9-]+\\.[a-zA-Z0-9-.]+')", false);
+    f.checkBoolean("regexp_contains('5556664422', '^\\d{10}$')", true);
+    f.checkBoolean("regexp_contains('11555666442233', '^\\d{10}$')", false);
+    f.checkBoolean("regexp_contains('55566644221133', '\\d{10}')", true);
+    f.checkBoolean("regexp_contains('55as56664as422', '\\d{10}')", false);
+
+    f.checkQuery("select regexp_contains('abc def ghi', 'abc')");
+    f.checkQuery("select regexp_contains('foo@bar.com', '@[a-zA-Z0-9-]+\\\\.[a-zA-Z0-9-.]+')");
+    f.checkQuery("select regexp_contains('55as56664as422', '\\d{10}')");
+
+    f.checkFails("regexp_contains('abc def ghi', '(abc')",
+        "Invalid regex input for REGEXP_CONTAINS: '(abc'", true);
+    f.checkFails("regexp_contains('abc def ghi', '{3}')",
+        "Invalid regex input for REGEXP_CONTAINS: '{3}'", true);
+    f.checkFails("regexp_contains('abc def ghi', '[z-a]')",

Review Comment:
   Quick clarification @tanclary , f.checkNull() verifies if we get back a SQL NULL value from the expression right? What should be done in case we are not expecting a NULL return, as in this function always returns a boolean value if the required arguments are passed or an exception for invalid regex.



-- 
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

[GitHub] [calcite] sonarcloud[bot] commented on pull request #3338: [CALCITE-5873] Add REGEXP_CONTAINS function for BigQuery Library

Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3338:
URL: https://github.com/apache/calcite/pull/3338#issuecomment-1656417135

   Kudos, SonarCloud Quality Gate passed!&nbsp; &nbsp; [![Quality Gate passed](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/QualityGateBadge/passed-16px.png 'Quality Gate passed')](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3338)
   
   [![Bug](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug-16px.png 'Bug')](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=BUG) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=BUG)  
   [![Vulnerability](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability-16px.png 'Vulnerability')](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=VULNERABILITY) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=VULNERABILITY)  
   [![Security Hotspot](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot-16px.png 'Security Hotspot')](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3338&resolved=false&types=SECURITY_HOTSPOT) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3338&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3338&resolved=false&types=SECURITY_HOTSPOT)  
   [![Code Smell](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell-16px.png 'Code Smell')](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=CODE_SMELL) [![A](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A-16px.png 'A')](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=CODE_SMELL) [2 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3338&resolved=false&types=CODE_SMELL)
   
   [![100.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/100-16px.png '100.0%')](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3338&metric=new_coverage&view=list) [100.0% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3338&metric=new_coverage&view=list)  
   [![0.0%](https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3-16px.png '0.0%')](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3338&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3338&metric=new_duplicated_lines_density&view=list)
   
   


-- 
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