mihaibudiu commented on code in PR #3904:
URL: https://github.com/apache/calcite/pull/3904#discussion_r1705827512
##########
core/src/test/java/org/apache/calcite/test/SqlFunctionsTest.java:
##########
@@ -172,6 +173,58 @@ static <E> List<E> list() {
assertThat(concat(null, "b"), is("nullb"));
}
+ @Test void testSubString() {
+ // str vs single param
+ assertThat(substring("string", -1), is("string"));
+ assertThat(substring("string", -1L), is("string"));
+ assertThat(substring("string", 2), is("tring"));
+ assertThat(substring("string", 2L), is("tring"));
+ assertThat(substring("string", Integer.MIN_VALUE), is("string"));
+ assertThat(substring("string", Long.MIN_VALUE), is("string"));
+ assertThat(substring("string", Integer.MIN_VALUE + 10), is("string"));
+ assertThat(substring("string", Integer.MAX_VALUE), is(""));
+ assertThat(substring("string", Long.MAX_VALUE), is(""));
+ assertThat(substring("string", Integer.MAX_VALUE - 10), is(""));
+ assertThat(substring("string", Integer.MIN_VALUE - 10L), is("string"));
+ assertThat(substring("string", Integer.MAX_VALUE + 10L), is(""));
+
+ // str vs multi params
+ assertThat(substring("string", -1, 1), is(""));
Review Comment:
is this behavior documented?
it's not obvious whether clamping happens first or the index computation
happens first.
##########
core/src/test/java/org/apache/calcite/test/SqlFunctionsTest.java:
##########
@@ -172,6 +173,58 @@ static <E> List<E> list() {
assertThat(concat(null, "b"), is("nullb"));
}
+ @Test void testSubString() {
+ // str vs single param
+ assertThat(substring("string", -1), is("string"));
+ assertThat(substring("string", -1L), is("string"));
+ assertThat(substring("string", 2), is("tring"));
+ assertThat(substring("string", 2L), is("tring"));
+ assertThat(substring("string", Integer.MIN_VALUE), is("string"));
+ assertThat(substring("string", Long.MIN_VALUE), is("string"));
+ assertThat(substring("string", Integer.MIN_VALUE + 10), is("string"));
+ assertThat(substring("string", Integer.MAX_VALUE), is(""));
+ assertThat(substring("string", Long.MAX_VALUE), is(""));
+ assertThat(substring("string", Integer.MAX_VALUE - 10), is(""));
+ assertThat(substring("string", Integer.MIN_VALUE - 10L), is("string"));
+ assertThat(substring("string", Integer.MAX_VALUE + 10L), is(""));
+
+ // str vs multi params
+ assertThat(substring("string", -1, 1), is(""));
+ assertThat(substring("string", -1, 1L), is(""));
+ assertThat(substring("string", -1L, 1), is(""));
+ assertThat(substring("string", -1L, 1L), is(""));
+
+ assertThat(substring("string", 1, 2), is("st"));
+ assertThat(substring("string", 1, 2L), is("st"));
+ assertThat(substring("string", 1L, 2), is("st"));
+ assertThat(substring("string", 1L, 2L), is("st"));
+
+ assertThat(substring("string", -1, 2), is(""));
+ assertThat(substring("string", -1L, 2), is(""));
+ assertThat(substring("string", -1, 2L), is(""));
+ assertThat(substring("string", -1L, 2L), is(""));
+
+ assertThat(substring("string", -1, 3), is("s"));
+ assertThat(substring("string", -1L, 3), is("s"));
+ assertThat(substring("string", -1, 3L), is("s"));
+ assertThat(substring("string", -1L, 3L), is("s"));
+
+ assertThat(substring("string", -10, 12), is("s"));
+ assertThat(substring("string", -10L, 12), is("s"));
+ assertThat(substring("string", -10, 12L), is("s"));
+ assertThat(substring("string", -10L, 12L), is("s"));
+
+ assertThat(substring("string", -1, Integer.MAX_VALUE), is("string"));
+ assertThat(substring("string", -1L, Integer.MAX_VALUE), is("string"));
+ assertThat(substring("string", -1, Long.MAX_VALUE), is("string"));
+
+ assertThat(substring("string", Integer.MIN_VALUE, Integer.MAX_VALUE),
is(""));
+ assertThat(substring("string", Integer.MIN_VALUE, Integer.MAX_VALUE +
10L), is("string"));
+ assertThat(substring("string", Long.MIN_VALUE, Integer.MAX_VALUE), is(""));
+ assertThat(substring("string", Integer.MIN_VALUE, Long.MAX_VALUE),
is("string"));
+ assertThat(substring("string", Integer.MIN_VALUE - 10L, Long.MAX_VALUE),
is("string"));
Review Comment:
how about a few tests with other numeric types, to make sure that
conversions work.
I would add a tinyint, a decimal, a double. The decimal is particularly
interesting.
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -1076,15 +1076,39 @@ public static String substring(String c, int s, int l) {
}
public static String substring(String c, int s, long l) {
- return substring(c, s, clamp(l));
+ if (s < 0) {
+ if (l > 0 && l + s > 0) {
+ l += s;
+ return substring(c, 0, l);
+ }
+ }
+ int l0 = clamp(l);
+ return SqlFunctions.substring(c, s, l0);
}
public static String substring(String c, long s, int l) {
- return substring(c, clamp(s), l);
+ if (s < 0) {
+ long s0 = s + l;
Review Comment:
it makes it easier to review if you are consistent in your implementations
elsewhere you used l += s
##########
core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java:
##########
@@ -1054,6 +1042,18 @@ static int clamp(long s) {
return (int) s;
}
+ /** SQL SUBSTRING(string FROM ...) function. */
Review Comment:
why move this function which seems unchanged?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
