You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "jhugomoore (via GitHub)" <gi...@apache.org> on 2023/05/01 19:22:26 UTC
[GitHub] [calcite] jhugomoore opened a new pull request, #3184: [CALCITE-3959] Implement INSTR function (Enabled for BigQuery, MySql, Oracle)
jhugomoore opened a new pull request, #3184:
URL: https://github.com/apache/calcite/pull/3184
[BiqQuery](https://cloud.google.com/bigquery/docs/reference/standard-sql/string_functions#instr) and [Oracle](https://docs.oracle.com/cd/B13789_01/server.101/b10759/functions058.htm) both support functionally identical INSTR(source_value, search_value[, position[, occurrence]]) functions which accepts 2 (character strings or binary strings), 1 optional int representing position, and 1 optional int representing occurrence.
Occurrence and position are assigned a default value of 1 if not specified.
The function returns the 1-based position of the nth occurrence of the 2nd operand in the 1st operand where n is defined by the 4th operand. The function begins searching at the 1-based position specified in the 3rd operand.
The function also supports negative position values, with -1 indicating the last character, and will search backwards from the position specified in that case.
Returns 0 if:
No match is found.
If occurrence is greater than the number of matches found.
If position is greater than the length of source_value.
Returns NULL if:
Any input argument is NULL.
Returns an error if:
position is 0.
occurrence is 0 or negative.
EXAMPLE: INSTR("abc", "bc") would return 2.
EXAMPLE: INSTR("abcabc", "bc", 3) would return 5.
EXAMPLE: INSTR("abcabc", "bc", -1, 1) would return 5.
EXAMPLE: INSTR("abcabc", "bc", -1, 2) would return 2.
MySQL also has an [INSTR](https://www.w3schools.com/sql/func_mysql_instr.asp) function, the functionality of which is a subset of the INSTR present in BQ and Oracle. MySQL INSTR only takes 2 parameters and returns the first occurrence of the search value in the source value.
--
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] olivrlee commented on a diff in pull request #3184: [CALCITE-3959] Implement INSTR function (Enabled for BigQuery, MySql, Oracle)
Posted by "olivrlee (via GitHub)" <gi...@apache.org>.
olivrlee commented on code in PR #3184:
URL: https://github.com/apache/calcite/pull/3184#discussion_r1181854516
##########
core/src/main/java/org/apache/calcite/sql2rel/StandardConvertletTable.java:
##########
@@ -402,6 +407,32 @@ private static RexNode convertNvl(SqlRexContext cx, SqlCall call) {
operand1)));
}
+ /** Converts a call to the INSTR function. */
+ private static RexNode convertInstr(SqlRexContext cx, SqlCall call) {
+ final RexBuilder rexBuilder = cx.getRexBuilder();
+ final List<RexNode> operands =
+ convertOperands(cx, call, SqlOperandTypeChecker.Consistency.NONE);
+ final RelDataType type =
+ cx.getValidator().getValidatedNodeType(call);
+ final List<RexNode> exprs = new ArrayList<>();
+ if (call.operandCount() == 2) {
Review Comment:
consider a
```
switch (call.operandCount())
case 2
case 3
...
```
--
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] jhugomoore commented on a diff in pull request #3184: [CALCITE-3959] Implement INSTR function (Enabled for BigQuery, MySql, Oracle)
Posted by "jhugomoore (via GitHub)" <gi...@apache.org>.
jhugomoore commented on code in PR #3184:
URL: https://github.com/apache/calcite/pull/3184#discussion_r1182819017
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -3104,23 +3104,106 @@ public static int position(ByteString seek, ByteString s) {
/** SQL {@code POSITION(seek IN string FROM integer)} function. */
public static int position(String seek, String s, int from) {
- final int from0 = from - 1; // 0-based
- if (from0 > s.length() || from0 < 0) {
+ if (from == 0) {
+ throw RESOURCE.fromNotZero().ex();
+ }
+ // Case when from is positive
+ if (from > 0) {
+ final int from0 = from - 1; // 0-based
+ if (from0 >= s.length() || from0 < 0) {
+ return 0;
+ } else {
+ return s.indexOf(seek, from0) + 1;
+ }
+ }
+ // Case when from is negative
+ final int rightIndex = from + s.length(); // negative position to positive index
+ if (rightIndex <= 0) {
return 0;
}
-
- return s.indexOf(seek, from0) + 1;
+ return s.substring(0, rightIndex + 1).lastIndexOf(seek) + 1;
}
/** SQL {@code POSITION(seek IN string FROM integer)} function for byte
* strings. */
public static int position(ByteString seek, ByteString s, int from) {
- final int from0 = from - 1;
- if (from0 > s.length() || from0 < 0) {
+ if (from == 0) {
+ throw RESOURCE.fromNotZero().ex();
+ }
+ // Case when from is positive
+ if (from > 0) {
+ final int from0 = from - 1; // 0-based
+ if (from0 >= s.length() || from0 < 0) {
+ return 0;
+ } else {
+ return s.indexOf(seek, from0) + 1;
+ }
+ }
+ // Case when from is negative
+ final int rightIndex = from + s.length();
+ if (rightIndex <= 0) {
return 0;
}
+ int lastIndex = 0;
+ while (lastIndex < rightIndex) {
+ int indexOf = s.substring(lastIndex, rightIndex + 1).indexOf(seek) + 1;
+ if (indexOf == 0) {
+ break;
+ }
+ lastIndex += indexOf;
+ }
+ return lastIndex;
+ }
- return s.indexOf(seek, from0) + 1;
+ /** SQL {@code POSITION(seek, string, from, occurrence)} function. */
+ public static int position(String seek, String s, int from, int occurrence) {
+ if (occurrence == 0) {
+ throw RESOURCE.occurrenceNotZero().ex();
+ }
+ for (int i = 0; i < occurrence; i++) {
Review Comment:
Not sure I understand what you mean. From gets updated every time the for loop runs, so we have to check whether to use the logic for from being positive or negative each time. I think checking if from is 0 after assignment is necessary because it will error if we continue running
--
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 #3184: [CALCITE-3959] Implement INSTR function (Enabled for BigQuery, MySql, Oracle)
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3184:
URL: https://github.com/apache/calcite/pull/3184#issuecomment-1530116627
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3184)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3184&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3184&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3184&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3184&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3184&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3184&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3184&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3184&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3184&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3184&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3184&resolved=false&types=CODE_SMELL) [3 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3184&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3184&metric=new_coverage&view=list) [88.6% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3184&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3184&metric=new_duplicated_lines_density&view=list) [34.7% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3184&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] jhugomoore commented on a diff in pull request #3184: [CALCITE-3959] Implement INSTR function (Enabled for BigQuery, MySql, Oracle)
Posted by "jhugomoore (via GitHub)" <gi...@apache.org>.
jhugomoore commented on code in PR #3184:
URL: https://github.com/apache/calcite/pull/3184#discussion_r1182903685
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -3104,23 +3104,106 @@ public static int position(ByteString seek, ByteString s) {
/** SQL {@code POSITION(seek IN string FROM integer)} function. */
public static int position(String seek, String s, int from) {
- final int from0 = from - 1; // 0-based
- if (from0 > s.length() || from0 < 0) {
+ if (from == 0) {
+ throw RESOURCE.fromNotZero().ex();
+ }
+ // Case when from is positive
+ if (from > 0) {
+ final int from0 = from - 1; // 0-based
+ if (from0 >= s.length() || from0 < 0) {
+ return 0;
+ } else {
+ return s.indexOf(seek, from0) + 1;
+ }
+ }
+ // Case when from is negative
+ final int rightIndex = from + s.length(); // negative position to positive index
Review Comment:
oooh boy great catch! fixing that now.
##########
core/src/main/java/org/apache/calcite/runtime/CalciteResource.java:
##########
@@ -1020,6 +1020,12 @@ ExInstWithCause<CalciteException> failedToAccessField(
@BaseMessage("No operator for ''{0}'' with kind: ''{1}'', syntax: ''{2}'' during JSON deserialization")
ExInst<CalciteException> noOperator(String name, String kind, String syntax);
+ @BaseMessage("Invalid input for position function: from operand value must not be zero")
Review Comment:
in the standard syntax in calcite it was Position(a IN b FROM c) so the c parameter was referred to as from in a couple spots. Decided to keep that especially in the function logic so there isn't a bunch of position variables in the position function.
--
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 #3184: [CALCITE-3959] Implement INSTR function (Enabled for BigQuery, MySql, Oracle)
Posted by "julianhyde (via GitHub)" <gi...@apache.org>.
julianhyde commented on code in PR #3184:
URL: https://github.com/apache/calcite/pull/3184#discussion_r1182955655
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -3104,23 +3104,106 @@ public static int position(ByteString seek, ByteString s) {
/** SQL {@code POSITION(seek IN string FROM integer)} function. */
public static int position(String seek, String s, int from) {
- final int from0 = from - 1; // 0-based
- if (from0 > s.length() || from0 < 0) {
+ if (from == 0) {
+ throw RESOURCE.fromNotZero().ex();
+ }
+ // Case when from is positive
+ if (from > 0) {
+ final int from0 = from - 1; // 0-based
+ if (from0 >= s.length() || from0 < 0) {
+ return 0;
+ } else {
+ return s.indexOf(seek, from0) + 1;
+ }
+ }
+ // Case when from is negative
+ final int rightIndex = from + s.length(); // negative position to positive index
+ if (rightIndex <= 0) {
return 0;
}
-
- return s.indexOf(seek, from0) + 1;
+ return s.substring(0, rightIndex + 1).lastIndexOf(seek) + 1;
}
/** SQL {@code POSITION(seek IN string FROM integer)} function for byte
* strings. */
public static int position(ByteString seek, ByteString s, int from) {
- final int from0 = from - 1;
- if (from0 > s.length() || from0 < 0) {
+ if (from == 0) {
+ throw RESOURCE.fromNotZero().ex();
+ }
+ // Case when from is positive
+ if (from > 0) {
+ final int from0 = from - 1; // 0-based
+ if (from0 >= s.length() || from0 < 0) {
+ return 0;
+ } else {
+ return s.indexOf(seek, from0) + 1;
+ }
+ }
+ // Case when from is negative
+ final int rightIndex = from + s.length();
+ if (rightIndex <= 0) {
return 0;
}
+ int lastIndex = 0;
+ while (lastIndex < rightIndex) {
+ int indexOf = s.substring(lastIndex, rightIndex + 1).indexOf(seek) + 1;
+ if (indexOf == 0) {
+ break;
+ }
+ lastIndex += indexOf;
+ }
+ return lastIndex;
+ }
- return s.indexOf(seek, from0) + 1;
+ /** SQL {@code POSITION(seek, string, from, occurrence)} function. */
+ public static int position(String seek, String s, int from, int occurrence) {
+ if (occurrence == 0) {
+ throw RESOURCE.occurrenceNotZero().ex();
+ }
+ for (int i = 0; i < occurrence; i++) {
Review Comment:
Ok, `from` isn't constant. But is `from > 0` constant? Or, if it isn't, should it be? That is, if we go into the `else` branch of the `if` on i = 0, we also want to go into the `else` branch for i = 1 and i = 2.
--
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] jhugomoore commented on a diff in pull request #3184: [CALCITE-3959] Implement INSTR function (Enabled for BigQuery, MySql, Oracle)
Posted by "jhugomoore (via GitHub)" <gi...@apache.org>.
jhugomoore commented on code in PR #3184:
URL: https://github.com/apache/calcite/pull/3184#discussion_r1183031472
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -3104,23 +3104,106 @@ public static int position(ByteString seek, ByteString s) {
/** SQL {@code POSITION(seek IN string FROM integer)} function. */
public static int position(String seek, String s, int from) {
- final int from0 = from - 1; // 0-based
- if (from0 > s.length() || from0 < 0) {
+ if (from == 0) {
+ throw RESOURCE.fromNotZero().ex();
+ }
+ // Case when from is positive
+ if (from > 0) {
+ final int from0 = from - 1; // 0-based
+ if (from0 >= s.length() || from0 < 0) {
+ return 0;
+ } else {
+ return s.indexOf(seek, from0) + 1;
+ }
+ }
+ // Case when from is negative
+ final int rightIndex = from + s.length(); // negative position to positive index
+ if (rightIndex <= 0) {
return 0;
}
-
- return s.indexOf(seek, from0) + 1;
+ return s.substring(0, rightIndex + 1).lastIndexOf(seek) + 1;
}
/** SQL {@code POSITION(seek IN string FROM integer)} function for byte
* strings. */
public static int position(ByteString seek, ByteString s, int from) {
- final int from0 = from - 1;
- if (from0 > s.length() || from0 < 0) {
+ if (from == 0) {
+ throw RESOURCE.fromNotZero().ex();
+ }
+ // Case when from is positive
+ if (from > 0) {
+ final int from0 = from - 1; // 0-based
+ if (from0 >= s.length() || from0 < 0) {
+ return 0;
+ } else {
+ return s.indexOf(seek, from0) + 1;
+ }
+ }
+ // Case when from is negative
+ final int rightIndex = from + s.length();
+ if (rightIndex <= 0) {
return 0;
}
+ int lastIndex = 0;
+ while (lastIndex < rightIndex) {
Review Comment:
https://issues.apache.org/jira/browse/CALCITE-5682?filter=-2
Done! let me know if anything isn't correct there
--
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] jhugomoore commented on a diff in pull request #3184: [CALCITE-3959] Implement INSTR function (Enabled for BigQuery, MySql, Oracle)
Posted by "jhugomoore (via GitHub)" <gi...@apache.org>.
jhugomoore commented on code in PR #3184:
URL: https://github.com/apache/calcite/pull/3184#discussion_r1181868315
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -3677,7 +3677,7 @@ static void checkRlikeFails(SqlOperatorFixture f) {
f.checkScalarExact("position('b' in 'abcabc' FROM 3)", 5);
f.checkScalarExact("position('b' in 'abcabc' FROM 5)", 5);
f.checkScalarExact("position('b' in 'abcabc' FROM 6)", 0);
- f.checkScalarExact("position('b' in 'abcabc' FROM -5)", 0);
+ f.checkScalarExact("position('b' in 'abcabc' FROM -5)", 2);
Review Comment:
These tests were from before a negative position value was accepted as valid input, so it would just return 0.
--
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 closed pull request #3184: [CALCITE-3959] Implement INSTR function (Enabled for BigQuery, MySql, Oracle)
Posted by "julianhyde (via GitHub)" <gi...@apache.org>.
julianhyde closed pull request #3184: [CALCITE-3959] Implement INSTR function (Enabled for BigQuery, MySql, Oracle)
URL: https://github.com/apache/calcite/pull/3184
--
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] jhugomoore commented on a diff in pull request #3184: [CALCITE-3959] Implement INSTR function (Enabled for BigQuery, MySql, Oracle)
Posted by "jhugomoore (via GitHub)" <gi...@apache.org>.
jhugomoore commented on code in PR #3184:
URL: https://github.com/apache/calcite/pull/3184#discussion_r1181885117
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -3104,23 +3104,105 @@ public static int position(ByteString seek, ByteString s) {
/** SQL {@code POSITION(seek IN string FROM integer)} function. */
public static int position(String seek, String s, int from) {
- final int from0 = from - 1; // 0-based
- if (from0 > s.length() || from0 < 0) {
+ if (from == 0) {
+ throw RESOURCE.fromNotZero().ex();
+ }
+ // Case when from is positive
+ if (from > 0) {
+ final int from0 = from - 1; // 0-based
+ if (from0 >= s.length() || from0 < 0) {
+ return 0;
+ }
+
+ return s.indexOf(seek, from0) + 1;
+ }
+ // Case when from is negative
+ final int rightIndex = from + s.length(); // negative position to positive index
+ if (rightIndex <= 0) {
return 0;
}
-
- return s.indexOf(seek, from0) + 1;
+ return s.substring(0, rightIndex + 1).lastIndexOf(seek) + 1;
}
/** SQL {@code POSITION(seek IN string FROM integer)} function for byte
* strings. */
public static int position(ByteString seek, ByteString s, int from) {
- final int from0 = from - 1;
- if (from0 > s.length() || from0 < 0) {
+ if (from == 0) {
+ throw RESOURCE.fromNotZero().ex();
+ }
+ // Case when from is positive
+ if (from > 0) {
+ final int from0 = from - 1; // 0-based
+ if (from0 >= s.length() || from0 < 0) {
+ return 0;
+ }
+ return s.indexOf(seek, from0) + 1;
+ }
+ // Case when from is negative
+ final int rightIndex = from + s.length();
+ if (rightIndex <= 0) {
return 0;
}
+ int lastIndex = 0;
+ while (lastIndex < rightIndex) {
+ int indexOf = s.substring(lastIndex, rightIndex + 1).indexOf(seek) + 1;
+ if (indexOf == 0) {
+ break;
+ }
+ lastIndex += indexOf;
+ }
+ return lastIndex;
+ }
+
+ /** SQL {@code POSITION(seek, string, from, occurrence)} function. */
+ public static int position(String seek, String s, int from, int occurrence) {
+ if (occurrence == 0) {
+ throw RESOURCE.occurrenceNotZero().ex();
+ }
+ for (int i = 0; i < occurrence; i++) {
+ if (from > 0) {
+ from = position(seek, s, from + (i == 0 ? 0 : 1));
+ if (from == 0) {
+ return 0;
+ }
+ } else {
+ from = position(seek, s, from);
+ if (from == 0) {
+ return 0;
+ }
+ from -= s.length() + 2;
+ }
+ }
+ if (from < 0) {
+ from += s.length() + 2;
+ }
+ return from;
+ }
- return s.indexOf(seek, from0) + 1;
+ /** SQL {@code POSITION(seek, string, from, occurrence)} function for byte
+ * strings. */
+ public static int position(ByteString seek, ByteString s, int from, int occurrence) {
+ if (occurrence == 0) {
+ throw RESOURCE.occurrenceNotZero().ex();
+ }
+ for (int i = 0; i < occurrence; i++) {
Review Comment:
Definitely had a sneaking feeling there's a much more elegant solution than what I implemented, will keep brainstorming.
--
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 #3184: [CALCITE-3959] Implement INSTR function (Enabled for BigQuery, MySql, Oracle)
Posted by "julianhyde (via GitHub)" <gi...@apache.org>.
julianhyde commented on code in PR #3184:
URL: https://github.com/apache/calcite/pull/3184#discussion_r1184449263
##########
core/src/main/resources/org/apache/calcite/runtime/CalciteResource.properties:
##########
@@ -336,6 +336,6 @@ InvalidPartitionKeys=Only tables with set semantics may be partitioned. Invalid
InvalidOrderBy=Only tables with set semantics may be ordered. Invalid ORDER BY clause in the {0,number,#}-th operand of table function ''{1}''
MultipleRowSemanticsTables=A table function at most has one input table with row semantics. Table function ''{0}'' has multiple input tables with row semantics
NoOperator=No operator for ''{0}'' with kind: ''{1}'', syntax: ''{2}'' during JSON deserialization
-FromNotZero=Invalid input for position function: from operand value must not be zero
-OccurrenceNotZero=Invalid input for position function: occurrence operand value must not be zero
+FromNotZero=Invalid input for POSITION function: from operand value must not be zero
+OccurrenceNotZero=Invalid input for POSITION function: occurrence operand value must be positive
Review Comment:
Don't put stuff at the end of a file. Open source maintainers will hate you.
--
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] jhugomoore commented on a diff in pull request #3184: [CALCITE-3959] Implement INSTR function (Enabled for BigQuery, MySql, Oracle)
Posted by "jhugomoore (via GitHub)" <gi...@apache.org>.
jhugomoore commented on code in PR #3184:
URL: https://github.com/apache/calcite/pull/3184#discussion_r1181919606
##########
core/src/main/java/org/apache/calcite/sql2rel/StandardConvertletTable.java:
##########
@@ -402,6 +407,32 @@ private static RexNode convertNvl(SqlRexContext cx, SqlCall call) {
operand1)));
}
+ /** Converts a call to the INSTR function. */
+ private static RexNode convertInstr(SqlRexContext cx, SqlCall call) {
+ final RexBuilder rexBuilder = cx.getRexBuilder();
+ final List<RexNode> operands =
+ convertOperands(cx, call, SqlOperandTypeChecker.Consistency.NONE);
+ final RelDataType type =
+ cx.getValidator().getValidatedNodeType(call);
+ final List<RexNode> exprs = new ArrayList<>();
+ if (call.operandCount() == 2) {
Review Comment:
added some but might've gone overboard, let me know if it's too verbose
--
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] jhugomoore commented on a diff in pull request #3184: [CALCITE-3959] Implement INSTR function (Enabled for BigQuery, MySql, Oracle)
Posted by "jhugomoore (via GitHub)" <gi...@apache.org>.
jhugomoore commented on code in PR #3184:
URL: https://github.com/apache/calcite/pull/3184#discussion_r1181870690
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlPositionFunction.java:
##########
@@ -53,12 +57,23 @@ public SqlPositionFunction(String name) {
int leftPrec,
int rightPrec) {
final SqlWriter.Frame frame = writer.startFunCall(getName());
- call.operand(0).unparse(writer, leftPrec, rightPrec);
- writer.sep("IN");
- call.operand(1).unparse(writer, leftPrec, rightPrec);
- if (3 == call.operandCount()) {
- writer.sep("FROM");
+ if (2 == call.operandCount() || 3 == call.operandCount()) {
+ call.operand(0).unparse(writer, leftPrec, rightPrec);
+ writer.sep("IN");
+ call.operand(1).unparse(writer, leftPrec, rightPrec);
+ if (3 == call.operandCount()) {
+ writer.sep("FROM");
+ call.operand(2).unparse(writer, leftPrec, rightPrec);
+ }
+ }
+ if (4 == call.operandCount()) {
Review Comment:
As far as my understanding, this 4 argument position function doesn't exist outside of calcite. I was basically left with the choice of adding another keyword in the function (would get confusing and doesn't have a clear best identifier), or to just default to comma separated operands (introduces inconsistent syntax in calcite which isn't ideal), wasn't sure the best choice. Let me know if you think I should switch it over.
--
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] jhugomoore commented on a diff in pull request #3184: [CALCITE-3959] Implement INSTR function (Enabled for BigQuery, MySql, Oracle)
Posted by "jhugomoore (via GitHub)" <gi...@apache.org>.
jhugomoore commented on code in PR #3184:
URL: https://github.com/apache/calcite/pull/3184#discussion_r1181868635
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -3686,7 +3686,7 @@ static void checkRlikeFails(SqlOperatorFixture f) {
f.checkScalarExact("position(x'bb' in x'aabbccaabbcc' FROM 3)", 5);
f.checkScalarExact("position(x'bb' in x'aabbccaabbcc' FROM 5)", 5);
f.checkScalarExact("position(x'bb' in x'aabbccaabbcc' FROM 6)", 0);
- f.checkScalarExact("position(x'bb' in x'aabbccaabbcc' FROM -5)", 0);
Review Comment:
same reasoning^ also had me confused for a while
--
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] jhugomoore commented on a diff in pull request #3184: [CALCITE-3959] Implement INSTR function (Enabled for BigQuery, MySql, Oracle)
Posted by "jhugomoore (via GitHub)" <gi...@apache.org>.
jhugomoore commented on code in PR #3184:
URL: https://github.com/apache/calcite/pull/3184#discussion_r1182912039
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -3104,23 +3104,106 @@ public static int position(ByteString seek, ByteString s) {
/** SQL {@code POSITION(seek IN string FROM integer)} function. */
public static int position(String seek, String s, int from) {
- final int from0 = from - 1; // 0-based
- if (from0 > s.length() || from0 < 0) {
+ if (from == 0) {
+ throw RESOURCE.fromNotZero().ex();
+ }
+ // Case when from is positive
+ if (from > 0) {
+ final int from0 = from - 1; // 0-based
+ if (from0 >= s.length() || from0 < 0) {
+ return 0;
+ } else {
+ return s.indexOf(seek, from0) + 1;
+ }
+ }
+ // Case when from is negative
+ final int rightIndex = from + s.length(); // negative position to positive index
Review Comment:
Added some tests and a fix? It's definitely not particularly pretty though.
--
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 #3184: [CALCITE-3959] Implement INSTR function (Enabled for BigQuery, MySql, Oracle)
Posted by "julianhyde (via GitHub)" <gi...@apache.org>.
julianhyde commented on code in PR #3184:
URL: https://github.com/apache/calcite/pull/3184#discussion_r1182950607
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -3104,23 +3104,106 @@ public static int position(ByteString seek, ByteString s) {
/** SQL {@code POSITION(seek IN string FROM integer)} function. */
public static int position(String seek, String s, int from) {
- final int from0 = from - 1; // 0-based
- if (from0 > s.length() || from0 < 0) {
+ if (from == 0) {
+ throw RESOURCE.fromNotZero().ex();
+ }
+ // Case when from is positive
+ if (from > 0) {
+ final int from0 = from - 1; // 0-based
+ if (from0 >= s.length() || from0 < 0) {
+ return 0;
+ } else {
+ return s.indexOf(seek, from0) + 1;
+ }
+ }
+ // Case when from is negative
+ final int rightIndex = from + s.length(); // negative position to positive index
+ if (rightIndex <= 0) {
return 0;
}
-
- return s.indexOf(seek, from0) + 1;
+ return s.substring(0, rightIndex + 1).lastIndexOf(seek) + 1;
}
/** SQL {@code POSITION(seek IN string FROM integer)} function for byte
* strings. */
public static int position(ByteString seek, ByteString s, int from) {
- final int from0 = from - 1;
- if (from0 > s.length() || from0 < 0) {
+ if (from == 0) {
+ throw RESOURCE.fromNotZero().ex();
+ }
+ // Case when from is positive
+ if (from > 0) {
+ final int from0 = from - 1; // 0-based
+ if (from0 >= s.length() || from0 < 0) {
+ return 0;
+ } else {
+ return s.indexOf(seek, from0) + 1;
+ }
+ }
+ // Case when from is negative
+ final int rightIndex = from + s.length();
+ if (rightIndex <= 0) {
return 0;
}
+ int lastIndex = 0;
+ while (lastIndex < rightIndex) {
Review Comment:
Ah, good to know. Please log an Avatica bug (similar to https://issues.apache.org/jira/browse/CALCITE-5415) for that. No urgency to fix.
--
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 #3184: [CALCITE-3959] Implement INSTR function (Enabled for BigQuery, MySql, Oracle)
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3184:
URL: https://github.com/apache/calcite/pull/3184#issuecomment-1530443103
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3184)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3184&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3184&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3184&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3184&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3184&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3184&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3184&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3184&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3184&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3184&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3184&resolved=false&types=CODE_SMELL) [4 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3184&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3184&metric=new_coverage&view=list) [86.9% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3184&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3184&metric=new_duplicated_lines_density&view=list) [35.2% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3184&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] wnob commented on a diff in pull request #3184: [CALCITE-3959] Implement INSTR function (Enabled for BigQuery, MySql, Oracle)
Posted by "wnob (via GitHub)" <gi...@apache.org>.
wnob commented on code in PR #3184:
URL: https://github.com/apache/calcite/pull/3184#discussion_r1182852333
##########
core/src/main/java/org/apache/calcite/runtime/CalciteResource.java:
##########
@@ -1020,6 +1020,12 @@ ExInstWithCause<CalciteException> failedToAccessField(
@BaseMessage("No operator for ''{0}'' with kind: ''{1}'', syntax: ''{2}'' during JSON deserialization")
ExInst<CalciteException> noOperator(String name, String kind, String syntax);
+ @BaseMessage("Invalid input for position function: from operand value must not be zero")
+ ExInst<CalciteException> fromNotZero();
+
+ @BaseMessage("Invalid input for position function: occurrence operand value must not be zero")
Review Comment:
`occurence` must be positive. Update the message and condition.
##########
core/src/main/java/org/apache/calcite/runtime/CalciteResource.java:
##########
@@ -1020,6 +1020,12 @@ ExInstWithCause<CalciteException> failedToAccessField(
@BaseMessage("No operator for ''{0}'' with kind: ''{1}'', syntax: ''{2}'' during JSON deserialization")
ExInst<CalciteException> noOperator(String name, String kind, String syntax);
+ @BaseMessage("Invalid input for position function: from operand value must not be zero")
Review Comment:
BQ and Oracle both call this `position`, not `from`. Is there another dialect that calls it `from`?
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -3104,23 +3104,106 @@ public static int position(ByteString seek, ByteString s) {
/** SQL {@code POSITION(seek IN string FROM integer)} function. */
public static int position(String seek, String s, int from) {
- final int from0 = from - 1; // 0-based
- if (from0 > s.length() || from0 < 0) {
+ if (from == 0) {
+ throw RESOURCE.fromNotZero().ex();
+ }
+ // Case when from is positive
+ if (from > 0) {
+ final int from0 = from - 1; // 0-based
+ if (from0 >= s.length() || from0 < 0) {
+ return 0;
+ } else {
+ return s.indexOf(seek, from0) + 1;
+ }
+ }
+ // Case when from is negative
+ final int rightIndex = from + s.length(); // negative position to positive index
Review Comment:
I don't think this is the correct semantics. BQ will actually search backward when `position` is negative. To see what I mean, try running `SELECT INSTR("fersher dude", "dude", -3, 1), INSTR("fersher dude", "dude", -4, 1), INSTR("fersher dude", "dude", -5, 1)` in http://pantheon/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] jhugomoore commented on a diff in pull request #3184: [CALCITE-3959] Implement INSTR function (Enabled for BigQuery, MySql, Oracle)
Posted by "jhugomoore (via GitHub)" <gi...@apache.org>.
jhugomoore commented on code in PR #3184:
URL: https://github.com/apache/calcite/pull/3184#discussion_r1182865341
##########
core/src/test/java/org/apache/calcite/test/SqlFunctionsTest.java:
##########
@@ -1054,6 +1055,54 @@ private void thereAndBack(byte[] bytes) {
// ok
}
}
+ @Test void testPosition() {
+ assertThat(3, is(position("c", "abcdec")));
+ assertThat(3, is(position("c", "abcdec", 2)));
Review Comment:
Ah great point! I had thought this felt a little off, 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
[GitHub] [calcite] sonarcloud[bot] commented on pull request #3184: [CALCITE-3959] Implement INSTR function (Enabled for BigQuery, MySql, Oracle)
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3184:
URL: https://github.com/apache/calcite/pull/3184#issuecomment-1532257572
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3184)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3184&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3184&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3184&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3184&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3184&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3184&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3184&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3184&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3184&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3184&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3184&resolved=false&types=CODE_SMELL) [6 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3184&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3184&metric=new_coverage&view=list) [86.6% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3184&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3184&metric=new_duplicated_lines_density&view=list) [35.3% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3184&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] jhugomoore commented on a diff in pull request #3184: [CALCITE-3959] Implement INSTR function (Enabled for BigQuery, MySql, Oracle)
Posted by "jhugomoore (via GitHub)" <gi...@apache.org>.
jhugomoore commented on code in PR #3184:
URL: https://github.com/apache/calcite/pull/3184#discussion_r1181870690
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlPositionFunction.java:
##########
@@ -53,12 +57,23 @@ public SqlPositionFunction(String name) {
int leftPrec,
int rightPrec) {
final SqlWriter.Frame frame = writer.startFunCall(getName());
- call.operand(0).unparse(writer, leftPrec, rightPrec);
- writer.sep("IN");
- call.operand(1).unparse(writer, leftPrec, rightPrec);
- if (3 == call.operandCount()) {
- writer.sep("FROM");
+ if (2 == call.operandCount() || 3 == call.operandCount()) {
+ call.operand(0).unparse(writer, leftPrec, rightPrec);
+ writer.sep("IN");
+ call.operand(1).unparse(writer, leftPrec, rightPrec);
+ if (3 == call.operandCount()) {
+ writer.sep("FROM");
+ call.operand(2).unparse(writer, leftPrec, rightPrec);
+ }
+ }
+ if (4 == call.operandCount()) {
Review Comment:
As far as my understanding, this 4 argument position function doesn't exist outside of calcite. I was basically left with the choice of adding another keyword in the function (would get confusing and doesn't have a clear best identifier), or to just default to comma separated operands, wasn't sure the best choice. Let me know if you think I should switch it over.
--
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] olivrlee commented on a diff in pull request #3184: [CALCITE-3959] Implement INSTR function (Enabled for BigQuery, MySql, Oracle)
Posted by "olivrlee (via GitHub)" <gi...@apache.org>.
olivrlee commented on code in PR #3184:
URL: https://github.com/apache/calcite/pull/3184#discussion_r1181853492
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlPositionFunction.java:
##########
@@ -53,12 +57,23 @@ public SqlPositionFunction(String name) {
int leftPrec,
int rightPrec) {
final SqlWriter.Frame frame = writer.startFunCall(getName());
- call.operand(0).unparse(writer, leftPrec, rightPrec);
- writer.sep("IN");
- call.operand(1).unparse(writer, leftPrec, rightPrec);
- if (3 == call.operandCount()) {
- writer.sep("FROM");
+ if (2 == call.operandCount() || 3 == call.operandCount()) {
Review Comment:
nit: switch the ordering like `call.operandCount() == 2`
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlPositionFunction.java:
##########
@@ -53,12 +57,23 @@ public SqlPositionFunction(String name) {
int leftPrec,
int rightPrec) {
final SqlWriter.Frame frame = writer.startFunCall(getName());
- call.operand(0).unparse(writer, leftPrec, rightPrec);
- writer.sep("IN");
- call.operand(1).unparse(writer, leftPrec, rightPrec);
- if (3 == call.operandCount()) {
- writer.sep("FROM");
+ if (2 == call.operandCount() || 3 == call.operandCount()) {
+ call.operand(0).unparse(writer, leftPrec, rightPrec);
+ writer.sep("IN");
+ call.operand(1).unparse(writer, leftPrec, rightPrec);
+ if (3 == call.operandCount()) {
+ writer.sep("FROM");
+ call.operand(2).unparse(writer, leftPrec, rightPrec);
+ }
+ }
+ if (4 == call.operandCount()) {
+ call.operand(0).unparse(writer, leftPrec, rightPrec);
+ writer.sep(",");
+ call.operand(1).unparse(writer, leftPrec, rightPrec);
+ writer.sep(",");
Review Comment:
nit: switch the ordering like `call.operandCount() == 2`
--
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 #3184: [CALCITE-3959] Implement INSTR function (Enabled for BigQuery, MySql, Oracle)
Posted by "julianhyde (via GitHub)" <gi...@apache.org>.
julianhyde commented on code in PR #3184:
URL: https://github.com/apache/calcite/pull/3184#discussion_r1182009899
##########
site/_docs/reference.md:
##########
@@ -2693,6 +2693,8 @@ BigQuery's type system uses confusingly different names for types and functions:
| b o | GREATEST(expr [, expr ]*) | Returns the greatest of the expressions
| b h s | IF(condition, value1, value2) | Returns *value1* if *condition* is TRUE, *value2* otherwise
| b | IFNULL(value1, value2) | Equivalent to `NVL(value1, value2)`
+| b o | INSTR(string, substring[, from, occurrence]) | Returns the position of substring in string, searching starting at from, and until locating the nth occurrence of substring.
Review Comment:
s/substring/\*substring\*/ etc.
s/from/from (default 1)/
s/occurrence/occurrence (default 1)/
remove '.' at end of line
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -3104,23 +3104,106 @@ public static int position(ByteString seek, ByteString s) {
/** SQL {@code POSITION(seek IN string FROM integer)} function. */
public static int position(String seek, String s, int from) {
- final int from0 = from - 1; // 0-based
- if (from0 > s.length() || from0 < 0) {
+ if (from == 0) {
+ throw RESOURCE.fromNotZero().ex();
+ }
+ // Case when from is positive
+ if (from > 0) {
+ final int from0 = from - 1; // 0-based
+ if (from0 >= s.length() || from0 < 0) {
+ return 0;
+ } else {
+ return s.indexOf(seek, from0) + 1;
+ }
+ }
+ // Case when from is negative
+ final int rightIndex = from + s.length(); // negative position to positive index
+ if (rightIndex <= 0) {
return 0;
}
-
- return s.indexOf(seek, from0) + 1;
+ return s.substring(0, rightIndex + 1).lastIndexOf(seek) + 1;
}
/** SQL {@code POSITION(seek IN string FROM integer)} function for byte
* strings. */
public static int position(ByteString seek, ByteString s, int from) {
- final int from0 = from - 1;
- if (from0 > s.length() || from0 < 0) {
+ if (from == 0) {
+ throw RESOURCE.fromNotZero().ex();
+ }
+ // Case when from is positive
+ if (from > 0) {
+ final int from0 = from - 1; // 0-based
+ if (from0 >= s.length() || from0 < 0) {
+ return 0;
+ } else {
+ return s.indexOf(seek, from0) + 1;
+ }
+ }
+ // Case when from is negative
+ final int rightIndex = from + s.length();
+ if (rightIndex <= 0) {
return 0;
}
+ int lastIndex = 0;
+ while (lastIndex < rightIndex) {
Review Comment:
Is there a method we could add to `ByteString` to allow you to write the same code in this method as for `position(String, String, int)`? (`ByteString` is in Avatica, so we would have to wait a release.)
##########
core/src/main/resources/org/apache/calcite/runtime/CalciteResource.properties:
##########
@@ -336,4 +336,6 @@ InvalidPartitionKeys=Only tables with set semantics may be partitioned. Invalid
InvalidOrderBy=Only tables with set semantics may be ordered. Invalid ORDER BY clause in the {0,number,#}-th operand of table function ''{1}''
MultipleRowSemanticsTables=A table function at most has one input table with row semantics. Table function ''{0}'' has multiple input tables with row semantics
NoOperator=No operator for ''{0}'' with kind: ''{1}'', syntax: ''{2}'' during JSON deserialization
+FromNotZero=Invalid input for position function: from operand value must not be zero
Review Comment:
s/position/POSITION/
Other error messages have the function in upper case. It reinforces that it's a function name.
##########
core/src/test/java/org/apache/calcite/test/SqlFunctionsTest.java:
##########
@@ -1054,6 +1055,54 @@ private void thereAndBack(byte[] bytes) {
// ok
}
}
+ @Test void testPosition() {
+ assertThat(3, is(position("c", "abcdec")));
+ assertThat(3, is(position("c", "abcdec", 2)));
Review Comment:
These tests are all backwards! For example, rather than
```
assertThat(3, is(position("c", "abcdec", 2)));
```
you should write
```
assertThat(position("c", "abcdec", 2), is(3)));
```
Here, `position("c", "abcdec", 2)` is the thing you are testing, and `3` is the expected result. In fact `is(3)` is the assertion about the expected result.
Why does it matter? If the test fails - say the position call returns 57 - then the framework should print 'got 57, expected 3'. Also, the test reads from left-to-right like an English sentence.
Maybe you're familiar with `assertEquals`, and earlier method where you were SUPPOSED to write the arguments backwards. `assertEquals(3, position("c", "abcdec", 2));` would be the correct way to write this test using `assertEquals`.
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -3104,23 +3104,105 @@ public static int position(ByteString seek, ByteString s) {
/** SQL {@code POSITION(seek IN string FROM integer)} function. */
public static int position(String seek, String s, int from) {
- final int from0 = from - 1; // 0-based
- if (from0 > s.length() || from0 < 0) {
+ if (from == 0) {
+ throw RESOURCE.fromNotZero().ex();
+ }
+ // Case when from is positive
+ if (from > 0) {
+ final int from0 = from - 1; // 0-based
+ if (from0 >= s.length() || from0 < 0) {
+ return 0;
+ }
+
+ return s.indexOf(seek, from0) + 1;
+ }
+ // Case when from is negative
+ final int rightIndex = from + s.length(); // negative position to positive index
+ if (rightIndex <= 0) {
return 0;
}
-
- return s.indexOf(seek, from0) + 1;
+ return s.substring(0, rightIndex + 1).lastIndexOf(seek) + 1;
}
/** SQL {@code POSITION(seek IN string FROM integer)} function for byte
* strings. */
public static int position(ByteString seek, ByteString s, int from) {
- final int from0 = from - 1;
- if (from0 > s.length() || from0 < 0) {
+ if (from == 0) {
+ throw RESOURCE.fromNotZero().ex();
+ }
+ // Case when from is positive
+ if (from > 0) {
+ final int from0 = from - 1; // 0-based
+ if (from0 >= s.length() || from0 < 0) {
+ return 0;
+ }
+ return s.indexOf(seek, from0) + 1;
+ }
+ // Case when from is negative
+ final int rightIndex = from + s.length();
+ if (rightIndex <= 0) {
return 0;
}
+ int lastIndex = 0;
+ while (lastIndex < rightIndex) {
+ int indexOf = s.substring(lastIndex, rightIndex + 1).indexOf(seek) + 1;
+ if (indexOf == 0) {
+ break;
+ }
+ lastIndex += indexOf;
+ }
+ return lastIndex;
+ }
+
+ /** SQL {@code POSITION(seek, string, from, occurrence)} function. */
+ public static int position(String seek, String s, int from, int occurrence) {
+ if (occurrence == 0) {
+ throw RESOURCE.occurrenceNotZero().ex();
+ }
+ for (int i = 0; i < occurrence; i++) {
+ if (from > 0) {
+ from = position(seek, s, from + (i == 0 ? 0 : 1));
+ if (from == 0) {
+ return 0;
+ }
+ } else {
+ from = position(seek, s, from);
+ if (from == 0) {
+ return 0;
+ }
+ from -= s.length() + 2;
+ }
+ }
+ if (from < 0) {
+ from += s.length() + 2;
+ }
+ return from;
+ }
- return s.indexOf(seek, from0) + 1;
+ /** SQL {@code POSITION(seek, string, from, occurrence)} function for byte
+ * strings. */
+ public static int position(ByteString seek, ByteString s, int from, int occurrence) {
+ if (occurrence == 0) {
+ throw RESOURCE.occurrenceNotZero().ex();
+ }
+ for (int i = 0; i < occurrence; i++) {
Review Comment:
The call to `position` isn't recursive, thank goodness.
I'd be inclined to inline the `position` function, and move the `if` outside the `for`. I think you can skip a lot of checks.
I was wondering how to implement this without calling `String.substring`, but I can't find an easy solution. So let's leave it as is.
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -3104,23 +3104,106 @@ public static int position(ByteString seek, ByteString s) {
/** SQL {@code POSITION(seek IN string FROM integer)} function. */
public static int position(String seek, String s, int from) {
- final int from0 = from - 1; // 0-based
- if (from0 > s.length() || from0 < 0) {
+ if (from == 0) {
+ throw RESOURCE.fromNotZero().ex();
+ }
+ // Case when from is positive
+ if (from > 0) {
+ final int from0 = from - 1; // 0-based
+ if (from0 >= s.length() || from0 < 0) {
+ return 0;
+ } else {
+ return s.indexOf(seek, from0) + 1;
+ }
+ }
+ // Case when from is negative
+ final int rightIndex = from + s.length(); // negative position to positive index
+ if (rightIndex <= 0) {
return 0;
}
-
- return s.indexOf(seek, from0) + 1;
+ return s.substring(0, rightIndex + 1).lastIndexOf(seek) + 1;
}
/** SQL {@code POSITION(seek IN string FROM integer)} function for byte
* strings. */
public static int position(ByteString seek, ByteString s, int from) {
- final int from0 = from - 1;
- if (from0 > s.length() || from0 < 0) {
+ if (from == 0) {
+ throw RESOURCE.fromNotZero().ex();
+ }
+ // Case when from is positive
+ if (from > 0) {
+ final int from0 = from - 1; // 0-based
+ if (from0 >= s.length() || from0 < 0) {
+ return 0;
+ } else {
+ return s.indexOf(seek, from0) + 1;
+ }
+ }
+ // Case when from is negative
+ final int rightIndex = from + s.length();
+ if (rightIndex <= 0) {
return 0;
}
+ int lastIndex = 0;
+ while (lastIndex < rightIndex) {
+ int indexOf = s.substring(lastIndex, rightIndex + 1).indexOf(seek) + 1;
+ if (indexOf == 0) {
+ break;
+ }
+ lastIndex += indexOf;
+ }
+ return lastIndex;
+ }
- return s.indexOf(seek, from0) + 1;
+ /** SQL {@code POSITION(seek, string, from, occurrence)} function. */
+ public static int position(String seek, String s, int from, int occurrence) {
+ if (occurrence == 0) {
+ throw RESOURCE.occurrenceNotZero().ex();
+ }
+ for (int i = 0; i < occurrence; i++) {
Review Comment:
`from` is constant and `i` is not. So would it make sense to move the `if` outside the `for`, so that it's only checked once?
##########
core/src/test/java/org/apache/calcite/test/SqlFunctionsTest.java:
##########
@@ -1054,6 +1055,54 @@ private void thereAndBack(byte[] bytes) {
// ok
}
}
+ @Test void testPosition() {
Review Comment:
add blankline before
--
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] jhugomoore commented on a diff in pull request #3184: [CALCITE-3959] Implement INSTR function (Enabled for BigQuery, MySql, Oracle)
Posted by "jhugomoore (via GitHub)" <gi...@apache.org>.
jhugomoore commented on code in PR #3184:
URL: https://github.com/apache/calcite/pull/3184#discussion_r1182917529
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -3104,23 +3104,105 @@ public static int position(ByteString seek, ByteString s) {
/** SQL {@code POSITION(seek IN string FROM integer)} function. */
public static int position(String seek, String s, int from) {
- final int from0 = from - 1; // 0-based
- if (from0 > s.length() || from0 < 0) {
+ if (from == 0) {
+ throw RESOURCE.fromNotZero().ex();
+ }
+ // Case when from is positive
+ if (from > 0) {
+ final int from0 = from - 1; // 0-based
+ if (from0 >= s.length() || from0 < 0) {
+ return 0;
+ }
+
+ return s.indexOf(seek, from0) + 1;
+ }
+ // Case when from is negative
+ final int rightIndex = from + s.length(); // negative position to positive index
+ if (rightIndex <= 0) {
return 0;
}
-
- return s.indexOf(seek, from0) + 1;
+ return s.substring(0, rightIndex + 1).lastIndexOf(seek) + 1;
}
/** SQL {@code POSITION(seek IN string FROM integer)} function for byte
* strings. */
public static int position(ByteString seek, ByteString s, int from) {
- final int from0 = from - 1;
- if (from0 > s.length() || from0 < 0) {
+ if (from == 0) {
+ throw RESOURCE.fromNotZero().ex();
+ }
+ // Case when from is positive
+ if (from > 0) {
+ final int from0 = from - 1; // 0-based
+ if (from0 >= s.length() || from0 < 0) {
+ return 0;
+ }
+ return s.indexOf(seek, from0) + 1;
+ }
+ // Case when from is negative
+ final int rightIndex = from + s.length();
+ if (rightIndex <= 0) {
return 0;
}
+ int lastIndex = 0;
+ while (lastIndex < rightIndex) {
+ int indexOf = s.substring(lastIndex, rightIndex + 1).indexOf(seek) + 1;
+ if (indexOf == 0) {
+ break;
+ }
+ lastIndex += indexOf;
+ }
+ return lastIndex;
+ }
+
+ /** SQL {@code POSITION(seek, string, from, occurrence)} function. */
+ public static int position(String seek, String s, int from, int occurrence) {
+ if (occurrence == 0) {
+ throw RESOURCE.occurrenceNotZero().ex();
+ }
+ for (int i = 0; i < occurrence; i++) {
+ if (from > 0) {
+ from = position(seek, s, from + (i == 0 ? 0 : 1));
+ if (from == 0) {
+ return 0;
+ }
+ } else {
+ from = position(seek, s, from);
+ if (from == 0) {
+ return 0;
+ }
+ from -= s.length() + 2;
+ }
+ }
+ if (from < 0) {
+ from += s.length() + 2;
+ }
+ return from;
+ }
- return s.indexOf(seek, from0) + 1;
+ /** SQL {@code POSITION(seek, string, from, occurrence)} function for byte
+ * strings. */
+ public static int position(ByteString seek, ByteString s, int from, int occurrence) {
+ if (occurrence == 0) {
+ throw RESOURCE.occurrenceNotZero().ex();
+ }
+ for (int i = 0; i < occurrence; i++) {
Review Comment:
Not sure I understand what you mean by this, see my comment below as well.
--
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] olivrlee commented on pull request #3184: [CALCITE-3959] Implement INSTR function (Enabled for BigQuery, MySql, Oracle)
Posted by "olivrlee (via GitHub)" <gi...@apache.org>.
olivrlee commented on PR #3184:
URL: https://github.com/apache/calcite/pull/3184#issuecomment-1530157208
Left some minor comments, great if someone else takes a look too.
Awesome job and first PR!
--
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] olivrlee commented on a diff in pull request #3184: [CALCITE-3959] Implement INSTR function (Enabled for BigQuery, MySql, Oracle)
Posted by "olivrlee (via GitHub)" <gi...@apache.org>.
olivrlee commented on code in PR #3184:
URL: https://github.com/apache/calcite/pull/3184#discussion_r1181845432
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -3104,23 +3104,105 @@ public static int position(ByteString seek, ByteString s) {
/** SQL {@code POSITION(seek IN string FROM integer)} function. */
public static int position(String seek, String s, int from) {
- final int from0 = from - 1; // 0-based
- if (from0 > s.length() || from0 < 0) {
+ if (from == 0) {
+ throw RESOURCE.fromNotZero().ex();
+ }
+ // Case when from is positive
+ if (from > 0) {
+ final int from0 = from - 1; // 0-based
+ if (from0 >= s.length() || from0 < 0) {
+ return 0;
+ }
+
+ return s.indexOf(seek, from0) + 1;
+ }
+ // Case when from is negative
Review Comment:
I'd probably add an explicit `else` for this case
##########
core/src/main/java/org/apache/calcite/runtime/CalciteResource.java:
##########
@@ -1028,4 +1034,6 @@ ExInstWithCause<CalciteException> failedToAccessField(
@BaseMessage("A table function at most has one input table with row semantics. Table function ''{0}'' has multiple input tables with row semantics")
ExInst<SqlValidatorException> multipleRowSemanticsTables(String funcName);
+
Review Comment:
These can be removed
--
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 #3184: [CALCITE-3959] Implement INSTR function (Enabled for BigQuery, MySql, Oracle)
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3184:
URL: https://github.com/apache/calcite/pull/3184#issuecomment-1530387922
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3184)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3184&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3184&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3184&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3184&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3184&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3184&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3184&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3184&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3184&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3184&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3184&resolved=false&types=CODE_SMELL) [5 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3184&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3184&metric=new_coverage&view=list) [87.6% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3184&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3184&metric=new_duplicated_lines_density&view=list) [36.1% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3184&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] jhugomoore commented on a diff in pull request #3184: [CALCITE-3959] Implement INSTR function (Enabled for BigQuery, MySql, Oracle)
Posted by "jhugomoore (via GitHub)" <gi...@apache.org>.
jhugomoore commented on code in PR #3184:
URL: https://github.com/apache/calcite/pull/3184#discussion_r1183021844
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -3104,23 +3104,106 @@ public static int position(ByteString seek, ByteString s) {
/** SQL {@code POSITION(seek IN string FROM integer)} function. */
public static int position(String seek, String s, int from) {
- final int from0 = from - 1; // 0-based
- if (from0 > s.length() || from0 < 0) {
+ if (from == 0) {
+ throw RESOURCE.fromNotZero().ex();
+ }
+ // Case when from is positive
+ if (from > 0) {
+ final int from0 = from - 1; // 0-based
+ if (from0 >= s.length() || from0 < 0) {
+ return 0;
+ } else {
+ return s.indexOf(seek, from0) + 1;
+ }
+ }
+ // Case when from is negative
+ final int rightIndex = from + s.length(); // negative position to positive index
+ if (rightIndex <= 0) {
return 0;
}
-
- return s.indexOf(seek, from0) + 1;
+ return s.substring(0, rightIndex + 1).lastIndexOf(seek) + 1;
}
/** SQL {@code POSITION(seek IN string FROM integer)} function for byte
* strings. */
public static int position(ByteString seek, ByteString s, int from) {
- final int from0 = from - 1;
- if (from0 > s.length() || from0 < 0) {
+ if (from == 0) {
+ throw RESOURCE.fromNotZero().ex();
+ }
+ // Case when from is positive
+ if (from > 0) {
+ final int from0 = from - 1; // 0-based
+ if (from0 >= s.length() || from0 < 0) {
+ return 0;
+ } else {
+ return s.indexOf(seek, from0) + 1;
+ }
+ }
+ // Case when from is negative
+ final int rightIndex = from + s.length();
+ if (rightIndex <= 0) {
return 0;
}
+ int lastIndex = 0;
+ while (lastIndex < rightIndex) {
+ int indexOf = s.substring(lastIndex, rightIndex + 1).indexOf(seek) + 1;
+ if (indexOf == 0) {
+ break;
+ }
+ lastIndex += indexOf;
+ }
+ return lastIndex;
+ }
- return s.indexOf(seek, from0) + 1;
+ /** SQL {@code POSITION(seek, string, from, occurrence)} function. */
+ public static int position(String seek, String s, int from, int occurrence) {
+ if (occurrence == 0) {
+ throw RESOURCE.occurrenceNotZero().ex();
+ }
+ for (int i = 0; i < occurrence; i++) {
Review Comment:
ahhh great point yes! We have to keep a negative from operand negative on future runs so it continues to search backwards. Will update
--
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 #3184: [CALCITE-3959] Implement INSTR function (Enabled for BigQuery, MySql, Oracle)
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3184:
URL: https://github.com/apache/calcite/pull/3184#issuecomment-1531997875
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3184)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3184&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3184&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3184&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3184&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3184&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3184&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3184&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3184&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3184&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3184&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3184&resolved=false&types=CODE_SMELL) [4 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3184&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3184&metric=new_coverage&view=list) [86.3% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3184&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3184&metric=new_duplicated_lines_density&view=list) [34.3% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3184&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] julianhyde commented on a diff in pull request #3184: [CALCITE-3959] Implement INSTR function (Enabled for BigQuery, MySql, Oracle)
Posted by "julianhyde (via GitHub)" <gi...@apache.org>.
julianhyde commented on code in PR #3184:
URL: https://github.com/apache/calcite/pull/3184#discussion_r1184449084
##########
core/src/main/java/org/apache/calcite/runtime/CalciteResource.java:
##########
@@ -1020,10 +1020,10 @@ ExInstWithCause<CalciteException> failedToAccessField(
@BaseMessage("No operator for ''{0}'' with kind: ''{1}'', syntax: ''{2}'' during JSON deserialization")
ExInst<CalciteException> noOperator(String name, String kind, String syntax);
- @BaseMessage("Invalid input for position function from operand value must not be zero")
+ @BaseMessage("Invalid input for POSITION function: from operand value must not be zero")
ExInst<CalciteException> fromNotZero();
- @BaseMessage("Invalid input for position function occurrence operand value must not be zero")
+ @BaseMessage("Invalid input for POSITION function: occurrence operand value must be positive")
Review Comment:
Don't put stuff at the end of a file. Open source maintainers will hate you.
--
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] tjbanghart commented on a diff in pull request #3184: [CALCITE-3959] Implement INSTR function (Enabled for BigQuery, MySql, Oracle)
Posted by "tjbanghart (via GitHub)" <gi...@apache.org>.
tjbanghart commented on code in PR #3184:
URL: https://github.com/apache/calcite/pull/3184#discussion_r1181838154
##########
core/src/main/java/org/apache/calcite/sql2rel/StandardConvertletTable.java:
##########
@@ -279,6 +279,11 @@ private StandardConvertletTable() {
SqlStdOperatorTable.POSITION.createCall(SqlParserPos.ZERO,
call.operand(1), call.operand(0))));
+ // "INSTR(string, substring, position, occurrence) is equivalent to
Review Comment:
We might want to add this comment to the `convertInstr` function since `POSITION` is not referenced in line 284 - we're explicitly registering a `convertInstr` method made for `INSTR`
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -3686,7 +3686,7 @@ static void checkRlikeFails(SqlOperatorFixture f) {
f.checkScalarExact("position(x'bb' in x'aabbccaabbcc' FROM 3)", 5);
f.checkScalarExact("position(x'bb' in x'aabbccaabbcc' FROM 5)", 5);
f.checkScalarExact("position(x'bb' in x'aabbccaabbcc' FROM 6)", 0);
- f.checkScalarExact("position(x'bb' in x'aabbccaabbcc' FROM -5)", 0);
Review Comment:
Same here, that seems odd
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -3677,7 +3677,7 @@ static void checkRlikeFails(SqlOperatorFixture f) {
f.checkScalarExact("position('b' in 'abcabc' FROM 3)", 5);
f.checkScalarExact("position('b' in 'abcabc' FROM 5)", 5);
f.checkScalarExact("position('b' in 'abcabc' FROM 6)", 0);
- f.checkScalarExact("position('b' in 'abcabc' FROM -5)", 0);
+ f.checkScalarExact("position('b' in 'abcabc' FROM -5)", 2);
Review Comment:
Why do we need to change this test?
##########
core/src/main/java/org/apache/calcite/sql2rel/StandardConvertletTable.java:
##########
@@ -402,6 +407,32 @@ private static RexNode convertNvl(SqlRexContext cx, SqlCall call) {
operand1)));
}
+ /** Converts a call to the INSTR function. */
+ private static RexNode convertInstr(SqlRexContext cx, SqlCall call) {
+ final RexBuilder rexBuilder = cx.getRexBuilder();
+ final List<RexNode> operands =
+ convertOperands(cx, call, SqlOperandTypeChecker.Consistency.NONE);
+ final RelDataType type =
+ cx.getValidator().getValidatedNodeType(call);
+ final List<RexNode> exprs = new ArrayList<>();
+ if (call.operandCount() == 2) {
Review Comment:
Comments here would also be helpful. It is not clear what each operand is and why their order matters.
##########
core/src/main/java/org/apache/calcite/sql/dialect/BigQuerySqlDialect.java:
##########
@@ -150,15 +150,36 @@ public BigQuerySqlDialect(SqlDialect.Context context) {
final int rightPrec) {
switch (call.getKind()) {
case POSITION:
- final SqlWriter.Frame frame = writer.startFunCall("STRPOS");
- writer.sep(",");
- call.operand(1).unparse(writer, leftPrec, rightPrec);
- writer.sep(",");
- call.operand(0).unparse(writer, leftPrec, rightPrec);
+ if (2 == call.operandCount()) {
Review Comment:
I think we could simplify the `if` expressions here and elsewhere when checking the `operandCount()`. Consider moving this into a helper function or starting the frame before the `if` blocks and ending the with the `writer.endFunCall(frame)` outside of a block.
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -3104,23 +3104,105 @@ public static int position(ByteString seek, ByteString s) {
/** SQL {@code POSITION(seek IN string FROM integer)} function. */
public static int position(String seek, String s, int from) {
- final int from0 = from - 1; // 0-based
- if (from0 > s.length() || from0 < 0) {
+ if (from == 0) {
+ throw RESOURCE.fromNotZero().ex();
+ }
+ // Case when from is positive
+ if (from > 0) {
+ final int from0 = from - 1; // 0-based
+ if (from0 >= s.length() || from0 < 0) {
+ return 0;
+ }
+
+ return s.indexOf(seek, from0) + 1;
+ }
+ // Case when from is negative
+ final int rightIndex = from + s.length(); // negative position to positive index
+ if (rightIndex <= 0) {
return 0;
}
-
- return s.indexOf(seek, from0) + 1;
+ return s.substring(0, rightIndex + 1).lastIndexOf(seek) + 1;
}
/** SQL {@code POSITION(seek IN string FROM integer)} function for byte
* strings. */
public static int position(ByteString seek, ByteString s, int from) {
- final int from0 = from - 1;
- if (from0 > s.length() || from0 < 0) {
+ if (from == 0) {
+ throw RESOURCE.fromNotZero().ex();
+ }
+ // Case when from is positive
+ if (from > 0) {
+ final int from0 = from - 1; // 0-based
+ if (from0 >= s.length() || from0 < 0) {
+ return 0;
+ }
+ return s.indexOf(seek, from0) + 1;
+ }
+ // Case when from is negative
+ final int rightIndex = from + s.length();
+ if (rightIndex <= 0) {
return 0;
}
+ int lastIndex = 0;
+ while (lastIndex < rightIndex) {
+ int indexOf = s.substring(lastIndex, rightIndex + 1).indexOf(seek) + 1;
+ if (indexOf == 0) {
+ break;
+ }
+ lastIndex += indexOf;
+ }
+ return lastIndex;
+ }
+
+ /** SQL {@code POSITION(seek, string, from, occurrence)} function. */
+ public static int position(String seek, String s, int from, int occurrence) {
+ if (occurrence == 0) {
+ throw RESOURCE.occurrenceNotZero().ex();
+ }
+ for (int i = 0; i < occurrence; i++) {
+ if (from > 0) {
+ from = position(seek, s, from + (i == 0 ? 0 : 1));
+ if (from == 0) {
+ return 0;
+ }
+ } else {
+ from = position(seek, s, from);
+ if (from == 0) {
+ return 0;
+ }
+ from -= s.length() + 2;
+ }
+ }
+ if (from < 0) {
+ from += s.length() + 2;
+ }
+ return from;
+ }
- return s.indexOf(seek, from0) + 1;
+ /** SQL {@code POSITION(seek, string, from, occurrence)} function for byte
+ * strings. */
+ public static int position(ByteString seek, ByteString s, int from, int occurrence) {
+ if (occurrence == 0) {
+ throw RESOURCE.occurrenceNotZero().ex();
+ }
+ for (int i = 0; i < occurrence; i++) {
Review Comment:
I think we should rely more heavily on String builtins here. For example rather than making a recursive call to `position` why not just take a substring with the `from` argument and then use a Pattern with grouping sets to count occurrences.
Also consider StringUtils: https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/StringUtils.html#countMatches(java.lang.CharSequence,%20java.lang.CharSequence)
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlPositionFunction.java:
##########
@@ -53,12 +57,23 @@ public SqlPositionFunction(String name) {
int leftPrec,
int rightPrec) {
final SqlWriter.Frame frame = writer.startFunCall(getName());
- call.operand(0).unparse(writer, leftPrec, rightPrec);
- writer.sep("IN");
- call.operand(1).unparse(writer, leftPrec, rightPrec);
- if (3 == call.operandCount()) {
- writer.sep("FROM");
+ if (2 == call.operandCount() || 3 == call.operandCount()) {
+ call.operand(0).unparse(writer, leftPrec, rightPrec);
+ writer.sep("IN");
+ call.operand(1).unparse(writer, leftPrec, rightPrec);
+ if (3 == call.operandCount()) {
+ writer.sep("FROM");
+ call.operand(2).unparse(writer, leftPrec, rightPrec);
+ }
+ }
+ if (4 == call.operandCount()) {
Review Comment:
What dialects support this 4 argument position without `IN` and `FROM`?
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -3104,23 +3104,105 @@ public static int position(ByteString seek, ByteString s) {
/** SQL {@code POSITION(seek IN string FROM integer)} function. */
public static int position(String seek, String s, int from) {
- final int from0 = from - 1; // 0-based
- if (from0 > s.length() || from0 < 0) {
+ if (from == 0) {
+ throw RESOURCE.fromNotZero().ex();
+ }
+ // Case when from is positive
+ if (from > 0) {
+ final int from0 = from - 1; // 0-based
+ if (from0 >= s.length() || from0 < 0) {
+ return 0;
+ }
+
+ return s.indexOf(seek, from0) + 1;
+ }
+ // Case when from is negative
+ final int rightIndex = from + s.length(); // negative position to positive index
+ if (rightIndex <= 0) {
return 0;
}
-
- return s.indexOf(seek, from0) + 1;
+ return s.substring(0, rightIndex + 1).lastIndexOf(seek) + 1;
}
/** SQL {@code POSITION(seek IN string FROM integer)} function for byte
* strings. */
public static int position(ByteString seek, ByteString s, int from) {
- final int from0 = from - 1;
- if (from0 > s.length() || from0 < 0) {
+ if (from == 0) {
+ throw RESOURCE.fromNotZero().ex();
+ }
+ // Case when from is positive
+ if (from > 0) {
+ final int from0 = from - 1; // 0-based
+ if (from0 >= s.length() || from0 < 0) {
+ return 0;
+ }
+ return s.indexOf(seek, from0) + 1;
+ }
+ // Case when from is negative
+ final int rightIndex = from + s.length();
+ if (rightIndex <= 0) {
return 0;
}
+ int lastIndex = 0;
+ while (lastIndex < rightIndex) {
+ int indexOf = s.substring(lastIndex, rightIndex + 1).indexOf(seek) + 1;
+ if (indexOf == 0) {
+ break;
+ }
+ lastIndex += indexOf;
+ }
+ return lastIndex;
+ }
+
+ /** SQL {@code POSITION(seek, string, from, occurrence)} function. */
+ public static int position(String seek, String s, int from, int occurrence) {
+ if (occurrence == 0) {
+ throw RESOURCE.occurrenceNotZero().ex();
+ }
+ for (int i = 0; i < occurrence; i++) {
+ if (from > 0) {
+ from = position(seek, s, from + (i == 0 ? 0 : 1));
+ if (from == 0) {
+ return 0;
+ }
+ } else {
+ from = position(seek, s, from);
+ if (from == 0) {
+ return 0;
+ }
+ from -= s.length() + 2;
+ }
+ }
+ if (from < 0) {
+ from += s.length() + 2;
+ }
+ return from;
+ }
- return s.indexOf(seek, from0) + 1;
+ /** SQL {@code POSITION(seek, string, from, occurrence)} function for byte
+ * strings. */
+ public static int position(ByteString seek, ByteString s, int from, int occurrence) {
+ if (occurrence == 0) {
+ throw RESOURCE.occurrenceNotZero().ex();
+ }
+ for (int i = 0; i < occurrence; i++) {
Review Comment:
^ I didn't think about overlapping patterns. I am not sure if those suggestions will work.
--
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] jhugomoore commented on a diff in pull request #3184: [CALCITE-3959] Implement INSTR function (Enabled for BigQuery, MySql, Oracle)
Posted by "jhugomoore (via GitHub)" <gi...@apache.org>.
jhugomoore commented on code in PR #3184:
URL: https://github.com/apache/calcite/pull/3184#discussion_r1182806771
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -3104,23 +3104,106 @@ public static int position(ByteString seek, ByteString s) {
/** SQL {@code POSITION(seek IN string FROM integer)} function. */
public static int position(String seek, String s, int from) {
- final int from0 = from - 1; // 0-based
- if (from0 > s.length() || from0 < 0) {
+ if (from == 0) {
+ throw RESOURCE.fromNotZero().ex();
+ }
+ // Case when from is positive
+ if (from > 0) {
+ final int from0 = from - 1; // 0-based
+ if (from0 >= s.length() || from0 < 0) {
+ return 0;
+ } else {
+ return s.indexOf(seek, from0) + 1;
+ }
+ }
+ // Case when from is negative
+ final int rightIndex = from + s.length(); // negative position to positive index
+ if (rightIndex <= 0) {
return 0;
}
-
- return s.indexOf(seek, from0) + 1;
+ return s.substring(0, rightIndex + 1).lastIndexOf(seek) + 1;
}
/** SQL {@code POSITION(seek IN string FROM integer)} function for byte
* strings. */
public static int position(ByteString seek, ByteString s, int from) {
- final int from0 = from - 1;
- if (from0 > s.length() || from0 < 0) {
+ if (from == 0) {
+ throw RESOURCE.fromNotZero().ex();
+ }
+ // Case when from is positive
+ if (from > 0) {
+ final int from0 = from - 1; // 0-based
+ if (from0 >= s.length() || from0 < 0) {
+ return 0;
+ } else {
+ return s.indexOf(seek, from0) + 1;
+ }
+ }
+ // Case when from is negative
+ final int rightIndex = from + s.length();
+ if (rightIndex <= 0) {
return 0;
}
+ int lastIndex = 0;
+ while (lastIndex < rightIndex) {
Review Comment:
Yes! Java strings have a nice [.lastIndexOf()](https://www.w3schools.com/java/ref_string_lastindexof.asp) function that isn't present in Avatica Bytestrings
--
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] jhugomoore commented on a diff in pull request #3184: [CALCITE-3959] Implement INSTR function (Enabled for BigQuery, MySql, Oracle)
Posted by "jhugomoore (via GitHub)" <gi...@apache.org>.
jhugomoore commented on code in PR #3184:
URL: https://github.com/apache/calcite/pull/3184#discussion_r1181876156
##########
core/src/main/java/org/apache/calcite/sql/dialect/BigQuerySqlDialect.java:
##########
@@ -150,15 +150,36 @@ public BigQuerySqlDialect(SqlDialect.Context context) {
final int rightPrec) {
switch (call.getKind()) {
case POSITION:
- final SqlWriter.Frame frame = writer.startFunCall("STRPOS");
- writer.sep(",");
- call.operand(1).unparse(writer, leftPrec, rightPrec);
- writer.sep(",");
- call.operand(0).unparse(writer, leftPrec, rightPrec);
+ if (2 == call.operandCount()) {
Review Comment:
Main reason I couldn't do that was due to wanting to unparse a position() with 2 operands into STRPOS, but 3 or 4 operands into INSTR. I think I'll just unparsing position into INSTR no matter what because it fully encompasses the functionality of STRPOS, let me know if that's an issue
--
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