This is an automated email from the ASF dual-hosted git repository. mbudiu pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/calcite.git
commit 1b36f07ce98f0e6d620b5ea1e1c773b376c3e81e Author: Mihai Budiu <[email protected]> AuthorDate: Thu Dec 7 05:20:35 2023 -0800 [CALCITE-5811] Error messages produced for constant out-of-bounds arguments are confusing Signed-off-by: Mihai Budiu <[email protected]> --- .../org/apache/calcite/runtime/SqlFunctions.java | 32 ++++++++++++++++++++++ .../calcite/sql/fun/SqlSubstringFunction.java | 5 ++-- .../org/apache/calcite/test/SqlValidatorTest.java | 14 ++++++---- .../org/apache/calcite/test/SqlOperatorTest.java | 27 ++++++++++++++++++ 4 files changed, 70 insertions(+), 8 deletions(-) diff --git a/core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java b/core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java index 4e11245ae3..adaf232add 100644 --- a/core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java +++ b/core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java @@ -986,6 +986,26 @@ public class SqlFunctions { return c.substring(s0); } + // Clamp very large long values to integer values. + // Used by the substring functions. + // Java strings do not support long indexes anyway, + // so this is most likely a safe approximation. + // But if a string has more than 2^31 characters + // the result of calling String.substring will be wrong anyway. + static int clamp(long s) { + if (s < Integer.MIN_VALUE) { + return Integer.MIN_VALUE; + } + if (s > Integer.MAX_VALUE) { + return Integer.MAX_VALUE; + } + return (int) s; + } + + public static String substring(String c, long s) { + return substring(c, clamp(s)); + } + /** SQL SUBSTRING(string FROM ... FOR ...) function. */ public static String substring(String c, int s, int l) { int lc = c.length(); @@ -1003,6 +1023,18 @@ public class SqlFunctions { return c.substring(s0, (int) e0); } + public static String substring(String c, int s, long l) { + return substring(c, s, clamp(l)); + } + + public static String substring(String c, long s, int l) { + return substring(c, clamp(s), l); + } + + public static String substring(String c, long s, long l) { + return substring(c, clamp(s), clamp(l)); + } + /** SQL SUBSTRING(binary FROM ...) function for binary. */ public static ByteString substring(ByteString c, int s) { final int s0 = s - 1; diff --git a/core/src/main/java/org/apache/calcite/sql/fun/SqlSubstringFunction.java b/core/src/main/java/org/apache/calcite/sql/fun/SqlSubstringFunction.java index 53cc96fa83..2730e89c0c 100644 --- a/core/src/main/java/org/apache/calcite/sql/fun/SqlSubstringFunction.java +++ b/core/src/main/java/org/apache/calcite/sql/fun/SqlSubstringFunction.java @@ -47,8 +47,9 @@ public class SqlSubstringFunction extends SqlFunction { /** Type checker for 3 argument calls. Put the STRING_INTEGER_INTEGER checker * first because almost every other type can be coerced to STRING. */ private static final SqlSingleOperandTypeChecker CHECKER3 = - OperandTypes.STRING_INTEGER_INTEGER - .or(OperandTypes.STRING_STRING_STRING); + OperandTypes.STRING_INTEGER_INTEGER; + // Not yet implemented + // .or(OperandTypes.STRING_STRING_STRING) //~ Constructors ----------------------------------------------------------- diff --git a/core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java b/core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java index 1f6bafae61..c5947df813 100644 --- a/core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java +++ b/core/src/test/java/org/apache/calcite/test/SqlValidatorTest.java @@ -1051,17 +1051,19 @@ public class SqlValidatorTest extends SqlValidatorTestCase { } @Test void testSubstringFails() { + String error = "(?s).*Cannot apply 'SUBSTRING' to arguments of type.*"; wholeExpr("substring('a' from 1 for 'b')") .withTypeCoercion(false) - .fails("(?s).*Cannot apply 'SUBSTRING' to arguments of type.*"); - expr("substring('a' from 1 for 'b')") - .columnType("VARCHAR(1) NOT NULL"); + .fails(error); wholeExpr("substring(_UTF16'10' FROM '0' FOR '\\')") - .fails("(?s).* not comparable to each other.*"); + .withTypeCoercion(false) + .fails(error); wholeExpr("substring('10' FROM _UTF16'0' FOR '\\')") - .fails("(?s).* not comparable to each other.*"); + .withTypeCoercion(false) + .fails(error); wholeExpr("substring('10' FROM '0' FOR _UTF16'\\')") - .fails("(?s).* not comparable to each other.*"); + .withTypeCoercion(false) + .fails(error); } @Test void testLikeAndSimilar() { diff --git a/testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java b/testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java index f17ceb2a7b..9578522bd8 100644 --- a/testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java +++ b/testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java @@ -8979,6 +8979,33 @@ public class SqlOperatorTest { + "requires extra delimiter argument", false); } + + /** Test case for + * <a href="https://issues.apache.org/jira/browse/CALCITE-5811";>[CALCITE-5811] + * Error messages produced for constant out-of-bounds arguments are confusing</a>. */ + @Test void testIndexOutOfBounds() { + final SqlOperatorFixture f = fixture(); + f.checkScalar("substring('abc' from 2 for 2147483650)", + "bc", "VARCHAR(3) NOT NULL"); + f.checkScalar("substring('abc' from 2147483650)", + "", "VARCHAR(3) NOT NULL"); + f.checkScalar("substring('abc' from 2147483650 for 2147483650)", + "", "VARCHAR(3) NOT NULL"); + f.checkScalar("substring('abc' from 2147483650 for 2)", + "", "VARCHAR(3) NOT NULL"); + f.checkFails("^substring('abc' from 2 for 2147483650.0)^", + "Cannot apply 'SUBSTRING' to arguments of type " + + "'SUBSTRING\\(<CHAR\\(3\\)> FROM <INTEGER> FOR <DECIMAL\\(11, 1\\)>\\)'\\. " + + "Supported form\\(s\\): 'SUBSTRING\\(<CHAR> FROM <INTEGER>\\)'\n" + + "'SUBSTRING\\(<CHAR> FROM <INTEGER> FOR <INTEGER>\\)'\n" + + "'SUBSTRING\\(<VARCHAR> FROM <INTEGER>\\)'\n" + + "'SUBSTRING\\(<VARCHAR> FROM <INTEGER> FOR <INTEGER>\\)'\n" + + "'SUBSTRING\\(<BINARY> FROM <INTEGER>\\)'\n" + + "'SUBSTRING\\(<BINARY> FROM <INTEGER> FOR <INTEGER>\\)'\n" + + "'SUBSTRING\\(<VARBINARY> FROM <INTEGER>\\)'\n" + + "'SUBSTRING\\(<VARBINARY> FROM <INTEGER> FOR <INTEGER>\\)'", false); + } + /** Tests the {@code SUBSTRING} operator. Many test cases that used to be * have been moved to {@link SubFunChecker#assertSubFunReturns}, and are * called for both {@code SUBSTRING} and {@code SUBSTR}. */
