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/10/10 14:55:42 UTC
[PR] [CALCITE-5979] Add REGEXP_REPLACE function (enabled in BigQuery library) [calcite]
Anthrino opened a new pull request, #3460:
URL: https://github.com/apache/calcite/pull/3460
(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
Re: [PR] [CALCITE-5979] Add REGEXP_REPLACE function (enabled in BigQuery library) [calcite]
Posted by "Anthrino (via GitHub)" <gi...@apache.org>.
Anthrino commented on code in PR #3460:
URL: https://github.com/apache/calcite/pull/3460#discussion_r1355420195
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlConformanceEnum.java:
##########
@@ -240,6 +240,15 @@ public enum SqlConformanceEnum implements SqlConformance {
}
}
+ @Override public boolean isRegexReplaceCaptureGroupDollarIndexed() {
Review Comment:
Hi @julianhyde, we came up with this approach to identify calls from Big Query by adding an `isRegexReplaceCaptureGroupDollarIndexed()` method in `SqlConformanceEnum`.
Like Tanner mentioned, it is a function specific use case so would it be better to create a new BQ specific operator (`REGEXP_REPLACE_BIG_QUERY`) in `StandardConvertletTable` instead and point that to the added custom preprocessing method in `SqlFunctions`
--
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
Re: [PR] [CALCITE-5979] Add REGEXP_REPLACE function (enabled in BigQuery library) [calcite]
Posted by "Anthrino (via GitHub)" <gi...@apache.org>.
Anthrino commented on code in PR #3460:
URL: https://github.com/apache/calcite/pull/3460#discussion_r1353386336
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -4810,12 +4810,34 @@ private static void checkIf(SqlOperatorFixture f) {
"VARCHAR NOT NULL");
f.checkString("regexp_replace('abc\t\ndef\t\nghi', '\\w+', '+')", "+\t\n+\t\n+",
"VARCHAR NOT NULL");
+
f.checkQuery("select regexp_replace('a b c', 'b', 'X')");
f.checkQuery("select regexp_replace('a b c', 'b', 'X', 1)");
f.checkQuery("select regexp_replace('a b c', 'b', 'X', 1, 3)");
f.checkQuery("select regexp_replace('a b c', 'b', 'X', 1, 3, 'i')");
};
- f0.forEachLibrary(list(SqlLibrary.MYSQL, SqlLibrary.ORACLE), consumer);
+ f0.forEachLibrary(list(SqlLibrary.MYSQL, SqlLibrary.ORACLE, SqlLibrary.BIG_QUERY), consumer);
+
Review Comment:
Yes these existing tests are for generic regexp_replace functionality so using them across all dialects, added some tests for backslash and $ syntax below on [line 4821](https://github.com/apache/calcite/blob/166efa4152cd448b9618aba39944215955f2d10f/testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java#L4821)
--
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
Re: [PR] [CALCITE-5979] Add REGEXP_REPLACE function (enabled in BigQuery library) [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3460:
URL: https://github.com/apache/calcite/pull/3460#issuecomment-1778157882
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3460)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3460&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3460&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3460&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3460&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3460&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3460&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3460&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3460&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3460&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3460&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3460&resolved=false&types=CODE_SMELL) [3 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3460&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3460&metric=new_coverage&view=list) [95.0% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3460&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3460&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3460&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
Re: [PR] [CALCITE-5979] Add REGEXP_REPLACE function (enabled in BigQuery library) [calcite]
Posted by "tanclary (via GitHub)" <gi...@apache.org>.
tanclary commented on code in PR #3460:
URL: https://github.com/apache/calcite/pull/3460#discussion_r1353072272
##########
babel/src/test/resources/sql/big-query.iq:
##########
@@ -707,7 +707,7 @@ SELECT SAFE_DIVIDE(CAST(1.7e308 as DOUBLE), CAST(1.7e-308 as DOUBLE)) as double_
!ok
-SELECT SAFE_DIVIDE(CAST(-3.5e75 AS DECIMAL(76, 0)), CAST(3.5e-75 AS DECIMAL(76, 0)))
Review Comment:
Oops thanks for fixing
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -431,7 +433,7 @@ private static boolean checkPosOccurrenceParamValues(int position,
throw RESOURCE.invalidIntegerInputForRegexpFunctions(Integer.toString(occurrencePosition),
"occurrence_position", methodName).ex();
}
- if (position <= value.length()) {
+ if (position > value.length()) {
Review Comment:
Was this wrong before? Or why the change?
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlConformanceEnum.java:
##########
@@ -240,6 +240,15 @@ public enum SqlConformanceEnum implements SqlConformance {
}
}
+ @Override public boolean isRegexReplaceCaptureGroupDollarIndexed() {
Review Comment:
I know I suggested this but reading it now I'm wondering whether this belongs in https://github.com/apache/calcite/blob/main/core/src/main/java/org/apache/calcite/sql2rel/StandardConvertletTable.java#L1288 especially since it is so function-specific. Could be a good question for @julianhyde
##########
core/src/test/java/org/apache/calcite/test/SqlFunctionsTest.java:
##########
@@ -482,6 +482,35 @@ static <E> List<E> list() {
}
}
+ @Test void testRegexpReplaceNonDollarIndexed() {
+ final SqlFunctions.RegexFunction f = new SqlFunctions.RegexFunction();
+ assertThat(f.regexpReplaceNonDollarIndexed("abascusB", "b", "X"), is("aXascusB"));
+ assertThat(f.regexpReplaceNonDollarIndexed("abc01def02ghi", "[a-z]+", "X"), is("X01X02X"));
+ assertThat(f.regexpReplaceNonDollarIndexed("a0b1c2d3", "0|2", "X"), is("aXb1cXd3"));
+
Review Comment:
remove blank line?
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -603,6 +605,38 @@ public String regexpReplace(String s, String regex, String replacement,
return Unsafe.regexpReplace(s, pattern, replacement, pos, occurrence);
}
+ /** SQL {@code REGEXP_REPLACE} function with 3 arguments with
+ * {@code \\} based indexing for capturing groups.
+ */
+ public String regexpReplaceNonDollarIndexed(String s, String regex,
+ String replacement) {
+ // Preprocessing to convert double-backslash based indexing for capturing
+ // groups into $ based indices recognized by java regex.
Review Comment:
nit: "$-based" and "double-backslash-based"
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java:
##########
@@ -2428,6 +2425,33 @@ private static class ContainsSubstrImplementor extends AbstractRexCallImplemento
}
}
+ /** Implementor for the {@code REGEXP_REPLACE} function. */
+ private static class RegexpReplaceImplementor extends AbstractRexCallImplementor {
+ RegexpReplaceImplementor() {
+ super("regexp_replace", NullPolicy.STRICT, false);
+ }
+
+ @Override Expression implementSafe(final RexToLixTranslator translator,
+ final RexCall call, final List<Expression> argValueList) {
+
+ final boolean dollarIndexed =
Review Comment:
maybe a small comment here? When I read `dollarIndexed` I'm not sure what that means without outside context.
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java:
##########
@@ -2428,6 +2425,33 @@ private static class ContainsSubstrImplementor extends AbstractRexCallImplemento
}
}
+ /** Implementor for the {@code REGEXP_REPLACE} function. */
+ private static class RegexpReplaceImplementor extends AbstractRexCallImplementor {
+ RegexpReplaceImplementor() {
+ super("regexp_replace", NullPolicy.STRICT, false);
+ }
+
+ @Override Expression implementSafe(final RexToLixTranslator translator,
+ final RexCall call, final List<Expression> argValueList) {
+
Review Comment:
can we remove this newline
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -603,6 +605,38 @@ public String regexpReplace(String s, String regex, String replacement,
return Unsafe.regexpReplace(s, pattern, replacement, pos, occurrence);
}
+ /** SQL {@code REGEXP_REPLACE} function with 3 arguments with
+ * {@code \\} based indexing for capturing groups.
+ */
+ public String regexpReplaceNonDollarIndexed(String s, String regex,
+ String replacement) {
+ // Preprocessing to convert double-backslash based indexing for capturing
+ // groups into $ based indices recognized by java regex.
+
+ // Explicitly escaping any $ symbols coming from input
+ // to ignore them from being considered as capturing group index.
+ String indexedReplacement = replacement.replace("\\\\", "\\")
+ .replace("$", "\\$");
+
+ // Check each occurrence of escaped chars, convert \<n> integers into
+ // $<n> indices, keep \\ and \$, throw an error for any other invalid escapes.
+ int lastOccIdx = 0;
+ while (lastOccIdx != -1) {
Review Comment:
This is a bit nested is there a way to flatten it some?
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlLibraryOperators.java:
##########
@@ -515,7 +515,9 @@ static RelDataType deriveTypeSplit(SqlOperatorBinding operatorBinding,
OperandTypes.STRING_STRING_OPTIONAL_INTEGER_OPTIONAL_INTEGER_OPTIONAL_INTEGER,
SqlFunctionCategory.STRING);
- @LibraryOperator(libraries = {MYSQL, ORACLE})
+ /** The "REGEXP_REPLACE(value, regexp, rep [, pos [, occurrence [, matchType]]])" function.
+ * Replaces all substrings of value that match regexp with rep and returns modified value. */
+ @LibraryOperator(libraries = {MYSQL, ORACLE, BIG_QUERY})
Review Comment:
Make this alphabetical if you don't mind
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -546,7 +548,7 @@ public int regexpInstr(String value, String regex, int position,
final String methodName = "REGEXP_INSTR";
final Pattern pattern = validateRegexPattern(regex, methodName);
- if (checkPosOccurrenceParamValues(position, occurrence, occurrencePosition, value,
+ if (!validatePosOccurrenceParamValues(position, occurrence, occurrencePosition, value,
methodName) || regex.isEmpty()) {
Review Comment:
Is it more optimal to check `regex.isEmpty()` first? Like if that is a cheaper operation maybe we should do it first? What do you think?
##########
site/_docs/reference.md:
##########
@@ -2791,7 +2791,7 @@ BigQuery's type system uses confusingly different names for types and functions:
| b | REGEXP_EXTRACT(string, regexp [, position [, occurrence]]) | Returns the substring in *string* that matches the *regexp*, starting search at *position* (default 1), and until locating the nth *occurrence* (default 1). Returns NULL if there is no match
| b | REGEXP_EXTRACT_ALL(string, regexp) | Returns an array of all substrings in *string* that matches the *regexp*. Returns an empty array if there is no match
| b | REGEXP_INSTR(string, regexp [, position [, occurrence [, occurrence_position]]]) | Returns the lowest 1-based position of the substring in *string* that matches the *regexp*, starting search at *position* (default 1), and until locating the nth *occurrence* (default 1). Setting occurrence_position (default 0) to 1 returns the end position of substring + 1. Returns 0 if there is no match
-| m o | REGEXP_REPLACE(string, regexp, rep [, pos [, occurrence [, matchType]]]) | Replaces all substrings of *string* that match *regexp* with *rep* at the starting *pos* in expr (if omitted, the default is 1), *occurrence* means which occurrence of a match to search for (if omitted, the default is 1), *matchType* specifies how to perform matching
+| m o b | REGEXP_REPLACE(string, regexp, rep [, pos [, occurrence [, matchType]]]) | Replaces all substrings of *string* that match *regexp* with *rep* at the starting *pos* in expr (if omitted, the default is 1), *occurrence* means which occurrence of a match to search for (if omitted, the default is 1), *matchType* specifies how to perform matching
Review Comment:
nit: change "occurrence means which ..." to "occurrence specifies which ..." to be consistent with the rest of the description
##########
site/_docs/reference.md:
##########
@@ -2791,7 +2791,7 @@ BigQuery's type system uses confusingly different names for types and functions:
| b | REGEXP_EXTRACT(string, regexp [, position [, occurrence]]) | Returns the substring in *string* that matches the *regexp*, starting search at *position* (default 1), and until locating the nth *occurrence* (default 1). Returns NULL if there is no match
| b | REGEXP_EXTRACT_ALL(string, regexp) | Returns an array of all substrings in *string* that matches the *regexp*. Returns an empty array if there is no match
| b | REGEXP_INSTR(string, regexp [, position [, occurrence [, occurrence_position]]]) | Returns the lowest 1-based position of the substring in *string* that matches the *regexp*, starting search at *position* (default 1), and until locating the nth *occurrence* (default 1). Setting occurrence_position (default 0) to 1 returns the end position of substring + 1. Returns 0 if there is no match
-| m o | REGEXP_REPLACE(string, regexp, rep [, pos [, occurrence [, matchType]]]) | Replaces all substrings of *string* that match *regexp* with *rep* at the starting *pos* in expr (if omitted, the default is 1), *occurrence* means which occurrence of a match to search for (if omitted, the default is 1), *matchType* specifies how to perform matching
+| m o b | REGEXP_REPLACE(string, regexp, rep [, pos [, occurrence [, matchType]]]) | Replaces all substrings of *string* that match *regexp* with *rep* at the starting *pos* in expr (if omitted, the default is 1), *occurrence* means which occurrence of a match to search for (if omitted, the default is 1), *matchType* specifies how to perform matching
Review Comment:
Make alphabetical as well
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -4810,12 +4810,34 @@ private static void checkIf(SqlOperatorFixture f) {
"VARCHAR NOT NULL");
f.checkString("regexp_replace('abc\t\ndef\t\nghi', '\\w+', '+')", "+\t\n+\t\n+",
"VARCHAR NOT NULL");
+
f.checkQuery("select regexp_replace('a b c', 'b', 'X')");
f.checkQuery("select regexp_replace('a b c', 'b', 'X', 1)");
f.checkQuery("select regexp_replace('a b c', 'b', 'X', 1, 3)");
f.checkQuery("select regexp_replace('a b c', 'b', 'X', 1, 3, 'i')");
};
- f0.forEachLibrary(list(SqlLibrary.MYSQL, SqlLibrary.ORACLE), consumer);
+ f0.forEachLibrary(list(SqlLibrary.MYSQL, SqlLibrary.ORACLE, SqlLibrary.BIG_QUERY), consumer);
+
Review Comment:
Do any of these tests check what happens if you use the wrong syntax for either dialect?
--
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
Re: [PR] [CALCITE-5979] Add REGEXP_REPLACE function (enabled in BigQuery library) [calcite]
Posted by "Anthrino (via GitHub)" <gi...@apache.org>.
Anthrino commented on code in PR #3460:
URL: https://github.com/apache/calcite/pull/3460#discussion_r1369267807
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlConformanceEnum.java:
##########
@@ -240,6 +240,15 @@ public enum SqlConformanceEnum implements SqlConformance {
}
}
+ @Override public boolean isRegexReplaceCaptureGroupDollarIndexed() {
Review Comment:
Thanks for the review @julianhyde, could you please elaborate a bit on which caching issues you're indicating here. If you meant the caching mechanism you added to the `RegexFunction` class to reuse previously built `Regex Patterns`, it's being used by the new function as well through the existing `regexpReplace` implementation.
Edit: Apologies missed your comment above, will look into that!
--
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
Re: [PR] [CALCITE-5979] Add REGEXP_REPLACE function (enabled in BigQuery library) [calcite]
Posted by "julianhyde (via GitHub)" <gi...@apache.org>.
julianhyde commented on code in PR #3460:
URL: https://github.com/apache/calcite/pull/3460#discussion_r1355659197
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -546,8 +548,8 @@ public int regexpInstr(String value, String regex, int position,
final String methodName = "REGEXP_INSTR";
final Pattern pattern = validateRegexPattern(regex, methodName);
- if (checkPosOccurrenceParamValues(position, occurrence, occurrencePosition, value,
- methodName) || regex.isEmpty()) {
+ if (regex.isEmpty() || !validatePosOccurrenceParamValues(position, occurrence,
+ occurrencePosition, value, methodName)) {
Review Comment:
Put `||` on a new line.
(Multi-line expressions shouldn't cross 'branches' of the expression tree in the middle of a line.)
--
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
Re: [PR] [CALCITE-5979] Add REGEXP_REPLACE function (enabled in BigQuery library) [calcite]
Posted by "julianhyde (via GitHub)" <gi...@apache.org>.
julianhyde commented on code in PR #3460:
URL: https://github.com/apache/calcite/pull/3460#discussion_r1369159033
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -405,19 +406,20 @@ private Pattern validateRegexPattern(String regex, String methodName) {
}
}
- /** Helper for multiple capturing group regex check in REGEXP_* fns. */
+ /** Helper to check for multiple capturing groups in REGEXP_* fns, throws an exception
+ * if regex pattern has more than 1 capturing group, else nothing is returned. */
private static void checkMultipleCapturingGroupsInRegex(Matcher matcher,
String methodName) {
if (matcher.groupCount() > 1) {
- throw RESOURCE.multipleCapturingGroupsForRegexpExtract(
+ throw RESOURCE.multipleCapturingGroupsForRegexpFunctions(
Integer.toString(matcher.groupCount()), methodName).ex();
}
}
/** Helper for checking values of position and occurrence arguments in REGEXP_* fns.
- * Regex fns not using occurrencePosition param pass a default value of 0.
- * Throws an exception or returns true in case of failed value checks. */
- private static boolean checkPosOccurrenceParamValues(int position,
+ * Functions not using occurrencePosition param pass a default value of 0. Throws an exception
+ * or returns false if any values are beyond accepted range, else returns true. */
+ private static boolean validatePosOccurrenceParamValues(int position,
Review Comment:
I think you should remove 'Helper for...'. Javadoc guidelines say that method javadoc should start with a verb.
--
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
Re: [PR] [CALCITE-5979] Add REGEXP_REPLACE function (enabled in BigQuery library) [calcite]
Posted by "julianhyde (via GitHub)" <gi...@apache.org>.
julianhyde commented on code in PR #3460:
URL: https://github.com/apache/calcite/pull/3460#discussion_r1369166613
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -603,6 +605,38 @@ public String regexpReplace(String s, String regex, String replacement,
return Unsafe.regexpReplace(s, pattern, replacement, pos, occurrence);
}
+ /** SQL {@code REGEXP_REPLACE} function with 3 arguments with
+ * {@code \\} based indexing for capturing groups.
+ */
+ public String regexpReplaceNonDollarIndexed(String s, String regex,
+ String replacement) {
+ // Preprocessing to convert double-backslash based indexing for capturing
+ // groups into $ based indices recognized by java regex.
+
+ // Explicitly escaping any $ symbols coming from input
+ // to ignore them from being considered as capturing group index.
+ String indexedReplacement = replacement.replace("\\\\", "\\")
+ .replace("$", "\\$");
+
+ // Check each occurrence of escaped chars, convert \<n> integers into
+ // $<n> indices, keep \\ and \$, throw an error for any other invalid escapes.
+ int lastOccIdx = 0;
+ while (lastOccIdx != -1) {
Review Comment:
The code to convert $-format to \-format can be a static utility method.
That utility method can be tested directly in `SqlFunctionsTest`.
You should add a (new) cache in `SqlRegexFunction`. Its type will be a `LoadingCache<String, String>`. Don't worry that most instances of `SqlRegexFunction` won't use this cache. The overhead of creating the cache is small.
--
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
Re: [PR] [CALCITE-5979] Add REGEXP_REPLACE function (enabled in BigQuery library) [calcite]
Posted by "Anthrino (via GitHub)" <gi...@apache.org>.
Anthrino commented on code in PR #3460:
URL: https://github.com/apache/calcite/pull/3460#discussion_r1369264548
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -405,19 +406,20 @@ private Pattern validateRegexPattern(String regex, String methodName) {
}
}
- /** Helper for multiple capturing group regex check in REGEXP_* fns. */
+ /** Helper to check for multiple capturing groups in REGEXP_* fns, throws an exception
+ * if regex pattern has more than 1 capturing group, else nothing is returned. */
private static void checkMultipleCapturingGroupsInRegex(Matcher matcher,
String methodName) {
if (matcher.groupCount() > 1) {
- throw RESOURCE.multipleCapturingGroupsForRegexpExtract(
+ throw RESOURCE.multipleCapturingGroupsForRegexpFunctions(
Integer.toString(matcher.groupCount()), methodName).ex();
}
}
/** Helper for checking values of position and occurrence arguments in REGEXP_* fns.
- * Regex fns not using occurrencePosition param pass a default value of 0.
- * Throws an exception or returns true in case of failed value checks. */
- private static boolean checkPosOccurrenceParamValues(int position,
+ * Functions not using occurrencePosition param pass a default value of 0. Throws an exception
+ * or returns false if any values are beyond accepted range, else returns true. */
+ private static boolean validatePosOccurrenceParamValues(int position,
Review Comment:
Thanks for suggesting Julian, based that on the pattern followed by other Helpers but yes aligned to Javadoc guidelines.
--
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
Re: [PR] [CALCITE-5979] Add REGEXP_REPLACE function (enabled in BigQuery library) [calcite]
Posted by "Anthrino (via GitHub)" <gi...@apache.org>.
Anthrino commented on code in PR #3460:
URL: https://github.com/apache/calcite/pull/3460#discussion_r1353050970
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -405,19 +406,20 @@ private Pattern validateRegexPattern(String regex, String methodName) {
}
}
- /** Helper for multiple capturing group regex check in REGEXP_* fns. */
+ /** Helper to check for multiple capturing groups in REGEXP_* fns, throws an exception
+ * if regex pattern has more than 1 capturing group, else nothing is returned. */
private static void checkMultipleCapturingGroupsInRegex(Matcher matcher,
String methodName) {
if (matcher.groupCount() > 1) {
- throw RESOURCE.multipleCapturingGroupsForRegexpExtract(
+ throw RESOURCE.multipleCapturingGroupsForRegexpFunctions(
Integer.toString(matcher.groupCount()), methodName).ex();
}
}
/** Helper for checking values of position and occurrence arguments in REGEXP_* fns.
- * Regex fns not using occurrencePosition param pass a default value of 0.
- * Throws an exception or returns true in case of failed value checks. */
- private static boolean checkPosOccurrenceParamValues(int position,
+ * Functions not using occurrencePosition param pass a default value of 0. Throws an exception
+ * or returns false if any values are beyond accepted range, else returns true. */
+ private static boolean validatePosOccurrenceParamValues(int position,
Review Comment:
Making helper method name and docs more descriptive based on Julian's feedback (ref: [CALCITE-5978](https://issues.apache.org/jira/browse/CALCITE-5978))
--
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
Re: [PR] [CALCITE-5979] Add REGEXP_REPLACE function (enabled in BigQuery library) [calcite]
Posted by "Anthrino (via GitHub)" <gi...@apache.org>.
Anthrino commented on code in PR #3460:
URL: https://github.com/apache/calcite/pull/3460#discussion_r1355420195
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlConformanceEnum.java:
##########
@@ -240,6 +240,15 @@ public enum SqlConformanceEnum implements SqlConformance {
}
}
+ @Override public boolean isRegexReplaceCaptureGroupDollarIndexed() {
Review Comment:
Hi @julianhyde, we came up with this approach to identify calls from Big Query by adding an `isDollarIndexed()` method in the `SqlConformanceEnum`.
Like Tanner mentioned, it is a function specific use case so would it be better to create a new BQ specific operator (`REGEXP_REPLACE_BIG_QUERY`) in `StandardConvertletTable` instead and point that to the added custom preprocessing method in `SqlFunctions`
--
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
Re: [PR] [CALCITE-5979] Add REGEXP_REPLACE function (enabled in BigQuery library) [calcite]
Posted by "Anthrino (via GitHub)" <gi...@apache.org>.
Anthrino commented on code in PR #3460:
URL: https://github.com/apache/calcite/pull/3460#discussion_r1357246795
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -546,8 +548,8 @@ public int regexpInstr(String value, String regex, int position,
final String methodName = "REGEXP_INSTR";
final Pattern pattern = validateRegexPattern(regex, methodName);
- if (checkPosOccurrenceParamValues(position, occurrence, occurrencePosition, value,
- methodName) || regex.isEmpty()) {
+ if (regex.isEmpty() || !validatePosOccurrenceParamValues(position, occurrence,
+ occurrencePosition, value, methodName)) {
Review Comment:
Thanks for the feedback Julian, corrected that.
--
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
Re: [PR] [CALCITE-5979] Add REGEXP_REPLACE function (enabled in BigQuery library) [calcite]
Posted by "Anthrino (via GitHub)" <gi...@apache.org>.
Anthrino commented on code in PR #3460:
URL: https://github.com/apache/calcite/pull/3460#discussion_r1353303095
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -431,7 +433,7 @@ private static boolean checkPosOccurrenceParamValues(int position,
throw RESOURCE.invalidIntegerInputForRegexpFunctions(Integer.toString(occurrencePosition),
"occurrence_position", methodName).ex();
}
- if (position <= value.length()) {
+ if (position > value.length()) {
Review Comment:
This wasn't wrong before, I just inverted the condition to make the return boolean more intuitive wrt helper function name: validatePosOccurrenceParamValues(), now true if everything is valid and false otherwise.
I've inverted the boolean checks for this method usages below, if you think this is more confusing we can revert to original.
--
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
Re: [PR] [CALCITE-5979] Add REGEXP_REPLACE function (enabled in BigQuery library) [calcite]
Posted by "Anthrino (via GitHub)" <gi...@apache.org>.
Anthrino commented on code in PR #3460:
URL: https://github.com/apache/calcite/pull/3460#discussion_r1353373965
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -603,6 +605,38 @@ public String regexpReplace(String s, String regex, String replacement,
return Unsafe.regexpReplace(s, pattern, replacement, pos, occurrence);
}
+ /** SQL {@code REGEXP_REPLACE} function with 3 arguments with
+ * {@code \\} based indexing for capturing groups.
+ */
+ public String regexpReplaceNonDollarIndexed(String s, String regex,
+ String replacement) {
+ // Preprocessing to convert double-backslash based indexing for capturing
+ // groups into $ based indices recognized by java regex.
+
+ // Explicitly escaping any $ symbols coming from input
+ // to ignore them from being considered as capturing group index.
+ String indexedReplacement = replacement.replace("\\\\", "\\")
+ .replace("$", "\\$");
+
+ // Check each occurrence of escaped chars, convert \<n> integers into
+ // $<n> indices, keep \\ and \$, throw an error for any other invalid escapes.
+ int lastOccIdx = 0;
+ while (lastOccIdx != -1) {
Review Comment:
I agree and would love to reduce the complexity here, but we need to check every occurrence of double-backslashes left to right to identify if they are used for indexing or escaping other chars ahead. The replacement to $ can only be made if subsequent escaped char is an integer, instead of blindly replacing all double-backslashes.
We can go over it and check if there is an easier path but this is the logic that aligns with how BQ evaluates input expressions, especially in cases where we have multiple backslashes together (example [test case](https://github.com/apache/calcite/blob/166efa4152cd448b9618aba39944215955f2d10f/testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java#L4828))
--
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
Re: [PR] [CALCITE-5979] Add REGEXP_REPLACE function (enabled in BigQuery library) [calcite]
Posted by "Anthrino (via GitHub)" <gi...@apache.org>.
Anthrino commented on code in PR #3460:
URL: https://github.com/apache/calcite/pull/3460#discussion_r1369267807
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlConformanceEnum.java:
##########
@@ -240,6 +240,15 @@ public enum SqlConformanceEnum implements SqlConformance {
}
}
+ @Override public boolean isRegexReplaceCaptureGroupDollarIndexed() {
Review Comment:
Thanks for the review @julianhyde, could you please elaborate a bit on which caching issues you're indicating here. If you meant the caching mechanism you added to the `RegexFunction` class to reuse previously built `Regex Patterns`, it's being used by the new function as well through the existing `regexpReplace` implementation.
Apologies missed your comment above, will look into that!
--
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
Re: [PR] [CALCITE-5979] Add REGEXP_REPLACE function (enabled in BigQuery library) [calcite]
Posted by "Anthrino (via GitHub)" <gi...@apache.org>.
Anthrino commented on code in PR #3460:
URL: https://github.com/apache/calcite/pull/3460#discussion_r1369267807
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlConformanceEnum.java:
##########
@@ -240,6 +240,15 @@ public enum SqlConformanceEnum implements SqlConformance {
}
}
+ @Override public boolean isRegexReplaceCaptureGroupDollarIndexed() {
Review Comment:
Thanks for the review @julianhyde, could you please elaborate a bit on which caching issues you're indicating here. If you meant the caching mechanism you added to the `RegexFunction` class to reuse previously built `Regex Patterns`, it's being used by the new function as well through the existing `regexpReplace` implementation.
--
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
Re: [PR] [CALCITE-5979] Add REGEXP_REPLACE function (enabled in BigQuery library) [calcite]
Posted by "julianhyde (via GitHub)" <gi...@apache.org>.
julianhyde commented on code in PR #3460:
URL: https://github.com/apache/calcite/pull/3460#discussion_r1369424094
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlConformanceEnum.java:
##########
@@ -240,6 +240,15 @@ public enum SqlConformanceEnum implements SqlConformance {
}
}
+ @Override public boolean isRegexReplaceCaptureGroupDollarIndexed() {
Review Comment:
You inherit the compiled regex from cache, and that's good.
But you still replace `$` with `\` every time the function is called. That's a waste of effort if the replacement string is a literal.
--
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
Re: [PR] [CALCITE-5979] Add REGEXP_REPLACE function (enabled in BigQuery library) [calcite]
Posted by "julianhyde (via GitHub)" <gi...@apache.org>.
julianhyde commented on code in PR #3460:
URL: https://github.com/apache/calcite/pull/3460#discussion_r1369166613
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -603,6 +605,38 @@ public String regexpReplace(String s, String regex, String replacement,
return Unsafe.regexpReplace(s, pattern, replacement, pos, occurrence);
}
+ /** SQL {@code REGEXP_REPLACE} function with 3 arguments with
+ * {@code \\} based indexing for capturing groups.
+ */
+ public String regexpReplaceNonDollarIndexed(String s, String regex,
+ String replacement) {
+ // Preprocessing to convert double-backslash based indexing for capturing
+ // groups into $ based indices recognized by java regex.
+
+ // Explicitly escaping any $ symbols coming from input
+ // to ignore them from being considered as capturing group index.
+ String indexedReplacement = replacement.replace("\\\\", "\\")
+ .replace("$", "\\$");
+
+ // Check each occurrence of escaped chars, convert \<n> integers into
+ // $<n> indices, keep \\ and \$, throw an error for any other invalid escapes.
+ int lastOccIdx = 0;
+ while (lastOccIdx != -1) {
Review Comment:
The code to convert $-format to `\`-format can be a static utility method.
That utility method can be tested directly in `SqlFunctionsTest`.
You should add a (new) cache in `SqlRegexFunction`. Its type will be a `LoadingCache<String, String>`. Don't worry that most instances of `SqlRegexFunction` won't use this cache. The overhead of creating the cache is small.
--
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
Re: [PR] [CALCITE-5979] Add REGEXP_REPLACE function (enabled in BigQuery library) [calcite]
Posted by "julianhyde (via GitHub)" <gi...@apache.org>.
julianhyde commented on code in PR #3460:
URL: https://github.com/apache/calcite/pull/3460#discussion_r1369154225
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlConformanceEnum.java:
##########
@@ -240,6 +240,15 @@ public enum SqlConformanceEnum implements SqlConformance {
}
}
+ @Override public boolean isRegexReplaceCaptureGroupDollarIndexed() {
Review Comment:
A BQ-specific function would be better. Conformance is generally for the 'big stuff'.
But this change is probably good enough already.
Would a BQ-specific function make it easier to solve the caching issues?
--
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
Re: [PR] [CALCITE-5979] Add REGEXP_REPLACE function (enabled in BigQuery library) [calcite]
Posted by "julianhyde (via GitHub)" <gi...@apache.org>.
julianhyde commented on code in PR #3460:
URL: https://github.com/apache/calcite/pull/3460#discussion_r1369168024
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlConformanceEnum.java:
##########
@@ -240,6 +240,15 @@ public enum SqlConformanceEnum implements SqlConformance {
}
}
+ @Override public boolean isRegexReplaceCaptureGroupDollarIndexed() {
Review Comment:
I'll answer my own question. A BQ-specific function won't make it easier to solve caching issues. So I think you should stay with conformance.
--
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
Re: [PR] [CALCITE-5979] Add REGEXP_REPLACE function (enabled in BigQuery library) [calcite]
Posted by "Anthrino (via GitHub)" <gi...@apache.org>.
Anthrino commented on code in PR #3460:
URL: https://github.com/apache/calcite/pull/3460#discussion_r1361239481
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlConformanceEnum.java:
##########
@@ -240,6 +240,15 @@ public enum SqlConformanceEnum implements SqlConformance {
}
}
+ @Override public boolean isRegexReplaceCaptureGroupDollarIndexed() {
Review Comment:
Hi @julianhyde, just a gentle reminder on this. We wanted to get your feedback on this approach of checking BQ conformance against introducing a separate operator, would be great if you can have a look, thanks!
--
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
Re: [PR] [CALCITE-5979] Add REGEXP_REPLACE function (enabled in BigQuery library) [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3460:
URL: https://github.com/apache/calcite/pull/3460#issuecomment-1758396265
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3460)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3460&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3460&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3460&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3460&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3460&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3460&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3460&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3460&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3460&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3460&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3460&resolved=false&types=CODE_SMELL) [4 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3460&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3460&metric=new_coverage&view=list) [98.4% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3460&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3460&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3460&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
Re: [PR] [CALCITE-5979] Add REGEXP_REPLACE function (enabled in BigQuery library) [calcite]
Posted by "tanclary (via GitHub)" <gi...@apache.org>.
tanclary commented on code in PR #3460:
URL: https://github.com/apache/calcite/pull/3460#discussion_r1355459182
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -4810,12 +4810,34 @@ private static void checkIf(SqlOperatorFixture f) {
"VARCHAR NOT NULL");
f.checkString("regexp_replace('abc\t\ndef\t\nghi', '\\w+', '+')", "+\t\n+\t\n+",
"VARCHAR NOT NULL");
+
f.checkQuery("select regexp_replace('a b c', 'b', 'X')");
f.checkQuery("select regexp_replace('a b c', 'b', 'X', 1)");
f.checkQuery("select regexp_replace('a b c', 'b', 'X', 1, 3)");
f.checkQuery("select regexp_replace('a b c', 'b', 'X', 1, 3, 'i')");
};
- f0.forEachLibrary(list(SqlLibrary.MYSQL, SqlLibrary.ORACLE), consumer);
+ f0.forEachLibrary(list(SqlLibrary.MYSQL, SqlLibrary.ORACLE, SqlLibrary.BIG_QUERY), consumer);
+
Review Comment:
Sounds good!
--
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
Re: [PR] [CALCITE-5979] Add REGEXP_REPLACE function (enabled in BigQuery library) [calcite]
Posted by "julianhyde (via GitHub)" <gi...@apache.org>.
julianhyde commented on code in PR #3460:
URL: https://github.com/apache/calcite/pull/3460#discussion_r1355671788
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlConformance.java:
##########
@@ -256,6 +256,16 @@ public interface SqlConformance {
*/
boolean isMinusAllowed();
+ /**
+ * Whether dialect uses default syntax of {@code $} for indexing capturing groups in
+ * regular expressions or alternate syntax of {@code \\} for {@code REGEXP_REPLACE}.
Review Comment:
s/regular expressions/the replacement string of regular expression functions/
also add an example. Something like "For example BigQuery REGEXP_REPLACE(...) is equivalent to Postgres REGEXP_REPLACE(...)"
--
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
Re: [PR] [CALCITE-5979] Add REGEXP_REPLACE function (enabled in BigQuery library) [calcite]
Posted by "Anthrino (via GitHub)" <gi...@apache.org>.
Anthrino commented on code in PR #3460:
URL: https://github.com/apache/calcite/pull/3460#discussion_r1355648351
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -603,6 +605,38 @@ public String regexpReplace(String s, String regex, String replacement,
return Unsafe.regexpReplace(s, pattern, replacement, pos, occurrence);
}
+ /** SQL {@code REGEXP_REPLACE} function with 3 arguments with
+ * {@code \\} based indexing for capturing groups.
+ */
+ public String regexpReplaceNonDollarIndexed(String s, String regex,
+ String replacement) {
+ // Preprocessing to convert double-backslash based indexing for capturing
+ // groups into $ based indices recognized by java regex.
+
+ // Explicitly escaping any $ symbols coming from input
+ // to ignore them from being considered as capturing group index.
+ String indexedReplacement = replacement.replace("\\\\", "\\")
+ .replace("$", "\\$");
+
+ // Check each occurrence of escaped chars, convert \<n> integers into
+ // $<n> indices, keep \\ and \$, throw an error for any other invalid escapes.
+ int lastOccIdx = 0;
+ while (lastOccIdx != -1) {
Review Comment:
Thanks for the feedback Tanner, restructured the index lookup statement outside the loop to remove one if condition.
--
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
Re: [PR] [CALCITE-5979] Add REGEXP_REPLACE function (enabled in BigQuery library) [calcite]
Posted by "tanclary (via GitHub)" <gi...@apache.org>.
tanclary commented on code in PR #3460:
URL: https://github.com/apache/calcite/pull/3460#discussion_r1370652246
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -431,12 +438,41 @@ private static boolean checkPosOccurrenceParamValues(int position,
throw RESOURCE.invalidIntegerInputForRegexpFunctions(Integer.toString(occurrencePosition),
"occurrence_position", methodName).ex();
}
- if (position <= value.length()) {
+ if (position > value.length()) {
return false;
}
return true;
Review Comment:
Looking at the code smells, should we rewrite this as `return position <= value.length()` (maybe I did my logic table wrong but hopefully you get what I'm saying: Squash the `if` into a single return)
--
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
Re: [PR] [CALCITE-5979] Add REGEXP_REPLACE function (enabled in BigQuery library) [calcite]
Posted by "julianhyde (via GitHub)" <gi...@apache.org>.
julianhyde commented on code in PR #3460:
URL: https://github.com/apache/calcite/pull/3460#discussion_r1370825644
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -431,12 +438,41 @@ private static boolean checkPosOccurrenceParamValues(int position,
throw RESOURCE.invalidIntegerInputForRegexpFunctions(Integer.toString(occurrencePosition),
"occurrence_position", methodName).ex();
}
- if (position <= value.length()) {
+ if (position > value.length()) {
return false;
}
return true;
Review Comment:
Good point, Tanner. It vindicates my philosophy that method's name and javadoc should say what the method does and returns.
That a method is named `checkXxx` and returns `boolean` (or throws) is a code smell. Note that the other `checkXxx` methods are `void`. As soon as a method's return changes from `void` to `boolean` you should rename it and re-write its javadoc.
The 'Helper for ...' pattern is harmful because it takes you out of that discipline.
--
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
Re: [PR] [CALCITE-5979] Add REGEXP_REPLACE function (enabled in BigQuery library) [calcite]
Posted by "asfgit (via GitHub)" <gi...@apache.org>.
asfgit closed pull request #3460: [CALCITE-5979] Add REGEXP_REPLACE function (enabled in BigQuery library)
URL: https://github.com/apache/calcite/pull/3460
--
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
Re: [PR] [CALCITE-5979] Add REGEXP_REPLACE function (enabled in BigQuery library) [calcite]
Posted by "julianhyde (via GitHub)" <gi...@apache.org>.
julianhyde commented on code in PR #3460:
URL: https://github.com/apache/calcite/pull/3460#discussion_r1370825644
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -431,12 +438,41 @@ private static boolean checkPosOccurrenceParamValues(int position,
throw RESOURCE.invalidIntegerInputForRegexpFunctions(Integer.toString(occurrencePosition),
"occurrence_position", methodName).ex();
}
- if (position <= value.length()) {
+ if (position > value.length()) {
return false;
}
return true;
Review Comment:
Good point, Tanner. It vindicates my philosophy that method's name and javadoc should say what the method does and returns.
That a method is named `checkXxx` and returns `boolean` (or throws) is a code smell. Note that the other `checkXxx` methods are `void`. As soon as a method's return changes from `void` to `boolean` you should rename it and re-write its javadoc.
--
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
Re: [PR] [CALCITE-5979] Add REGEXP_REPLACE function (enabled in BigQuery library) [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3460:
URL: https://github.com/apache/calcite/pull/3460#issuecomment-1760211560
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3460)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3460&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3460&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3460&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3460&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3460&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3460&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3460&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3460&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3460&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3460&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3460&resolved=false&types=CODE_SMELL) [4 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3460&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3460&metric=new_coverage&view=list) [98.4% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3460&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3460&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3460&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
Re: [PR] [CALCITE-5979] Add REGEXP_REPLACE function (enabled in BigQuery library) [calcite]
Posted by "Anthrino (via GitHub)" <gi...@apache.org>.
Anthrino commented on code in PR #3460:
URL: https://github.com/apache/calcite/pull/3460#discussion_r1357247804
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlConformance.java:
##########
@@ -256,6 +256,16 @@ public interface SqlConformance {
*/
boolean isMinusAllowed();
+ /**
+ * Whether dialect uses default syntax of {@code $} for indexing capturing groups in
+ * regular expressions or alternate syntax of {@code \\} for {@code REGEXP_REPLACE}.
Review Comment:
Updated the docs with an example, thanks for the suggestion!
--
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
Re: [PR] [CALCITE-5979] Add REGEXP_REPLACE function (enabled in BigQuery library) [calcite]
Posted by "Anthrino (via GitHub)" <gi...@apache.org>.
Anthrino commented on code in PR #3460:
URL: https://github.com/apache/calcite/pull/3460#discussion_r1355420195
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlConformanceEnum.java:
##########
@@ -240,6 +240,15 @@ public enum SqlConformanceEnum implements SqlConformance {
}
}
+ @Override public boolean isRegexReplaceCaptureGroupDollarIndexed() {
Review Comment:
Hi @julianhyde, we came up with this approach to identify calls from Big Query by adding an `isRegexReplaceCaptureGroupDollarIndexed()` method in the `SqlConformanceEnum`.
Like Tanner mentioned, it is a function specific use case so would it be better to create a new BQ specific operator (`REGEXP_REPLACE_BIG_QUERY`) in `StandardConvertletTable` instead and point that to the added custom preprocessing method in `SqlFunctions`
--
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
Re: [PR] [CALCITE-5979] Add REGEXP_REPLACE function (enabled in BigQuery library) [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3460:
URL: https://github.com/apache/calcite/pull/3460#issuecomment-1755775780
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3460)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3460&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3460&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3460&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3460&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3460&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3460&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3460&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3460&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3460&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3460&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3460&resolved=false&types=CODE_SMELL) [4 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3460&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3460&metric=new_coverage&view=list) [98.5% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3460&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3460&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3460&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
Re: [PR] [CALCITE-5979] Add REGEXP_REPLACE function (enabled in BigQuery library) [calcite]
Posted by "Anthrino (via GitHub)" <gi...@apache.org>.
Anthrino commented on code in PR #3460:
URL: https://github.com/apache/calcite/pull/3460#discussion_r1353373965
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -603,6 +605,38 @@ public String regexpReplace(String s, String regex, String replacement,
return Unsafe.regexpReplace(s, pattern, replacement, pos, occurrence);
}
+ /** SQL {@code REGEXP_REPLACE} function with 3 arguments with
+ * {@code \\} based indexing for capturing groups.
+ */
+ public String regexpReplaceNonDollarIndexed(String s, String regex,
+ String replacement) {
+ // Preprocessing to convert double-backslash based indexing for capturing
+ // groups into $ based indices recognized by java regex.
+
+ // Explicitly escaping any $ symbols coming from input
+ // to ignore them from being considered as capturing group index.
+ String indexedReplacement = replacement.replace("\\\\", "\\")
+ .replace("$", "\\$");
+
+ // Check each occurrence of escaped chars, convert \<n> integers into
+ // $<n> indices, keep \\ and \$, throw an error for any other invalid escapes.
+ int lastOccIdx = 0;
+ while (lastOccIdx != -1) {
Review Comment:
I agree and would love to reduce the complexity here, but we need to check every occurrence of double-backslashes left to right to identify if they are used for indexing or escaping other chars ahead. The replacement to $ can only be made if subsequent escaped char is an integer, instead of blindly replacing all double-backslashes in one go.
We can go over it and check if there is an easier path but this is the logic that aligns with how BQ evaluates input expressions, especially in cases where we have multiple backslashes together (example [test case](https://github.com/apache/calcite/blob/166efa4152cd448b9618aba39944215955f2d10f/testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java#L4828))
--
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
Re: [PR] [CALCITE-5979] Add REGEXP_REPLACE function (enabled in BigQuery library) [calcite]
Posted by "Anthrino (via GitHub)" <gi...@apache.org>.
Anthrino commented on code in PR #3460:
URL: https://github.com/apache/calcite/pull/3460#discussion_r1353378275
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -546,7 +548,7 @@ public int regexpInstr(String value, String regex, int position,
final String methodName = "REGEXP_INSTR";
final Pattern pattern = validateRegexPattern(regex, methodName);
- if (checkPosOccurrenceParamValues(position, occurrence, occurrencePosition, value,
+ if (!validatePosOccurrenceParamValues(position, occurrence, occurrencePosition, value,
methodName) || regex.isEmpty()) {
Review Comment:
Yes that makes sense, there isn't a very costly operation hidden behind the function but you're right as to using this pattern, corrected.
--
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
Re: [PR] [CALCITE-5979] Add REGEXP_REPLACE function (enabled in BigQuery library) [calcite]
Posted by "Anthrino (via GitHub)" <gi...@apache.org>.
Anthrino commented on code in PR #3460:
URL: https://github.com/apache/calcite/pull/3460#discussion_r1355420195
##########
core/src/main/java/org/apache/calcite/sql/validate/SqlConformanceEnum.java:
##########
@@ -240,6 +240,15 @@ public enum SqlConformanceEnum implements SqlConformance {
}
}
+ @Override public boolean isRegexReplaceCaptureGroupDollarIndexed() {
Review Comment:
Hi @julianhyde, we came up with this approach to identify calls from Big Query by adding an `isRegexReplaceCaptureGroupDollarIndexed()` method in `SqlConformanceEnum`.
Like Tanner mentioned, it is a function specific use case so would it be better instead to create a new BQ specific operator (`REGEXP_REPLACE_BIG_QUERY`) in `StandardConvertletTable` and point that to the added custom preprocessing method in `SqlFunctions`
--
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
Re: [PR] [CALCITE-5979] Add REGEXP_REPLACE function (enabled in BigQuery library) [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3460:
URL: https://github.com/apache/calcite/pull/3460#issuecomment-1777774977
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3460)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3460&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3460&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3460&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3460&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3460&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3460&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3460&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3460&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3460&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3460&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3460&resolved=false&types=CODE_SMELL) [4 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3460&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3460&metric=new_coverage&view=list) [95.0% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3460&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3460&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3460&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