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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
