You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by mb...@apache.org on 2023/10/31 21:33:49 UTC
(calcite) branch main updated: [CALCITE-5990] Explicit cast to numeric type doesn't check overflow
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
The following commit(s) were added to refs/heads/main by this push:
new 620058f94b [CALCITE-5990] Explicit cast to numeric type doesn't check overflow
620058f94b is described below
commit 620058f94bd2dbaae019b85e9de842721fd70623
Author: Mihai Budiu <mb...@gmail.com>
AuthorDate: Tue Oct 31 13:49:12 2023 -0700
[CALCITE-5990] Explicit cast to numeric type doesn't check overflow
Signed-off-by: Mihai Budiu <mb...@gmail.com>
---
.../src/main/java/org/apache/calcite/util/Bug.java | 8 --
.../calcite/test/SqlOperatorUnparseTest.java | 7 +-
.../org/apache/calcite/linq4j/tree/Primitive.java | 52 ++++++++++-
site/_docs/reference.md | 2 +-
.../calcite/sql/test/SqlOperatorFixture.java | 37 +++-----
.../java/org/apache/calcite/sql/test/SqlTests.java | 4 +
.../org/apache/calcite/test/SqlOperatorTest.java | 103 +++++++++++----------
7 files changed, 118 insertions(+), 95 deletions(-)
diff --git a/core/src/main/java/org/apache/calcite/util/Bug.java b/core/src/main/java/org/apache/calcite/util/Bug.java
index b2f14d76ef..8f36d07eee 100644
--- a/core/src/main/java/org/apache/calcite/util/Bug.java
+++ b/core/src/main/java/org/apache/calcite/util/Bug.java
@@ -56,14 +56,6 @@ public abstract class Bug {
public static final boolean DT785_FIXED = false;
- // jhyde
-
- /**
- * Whether <a href="http://issues.eigenbase.org/browse/FNL-3">issue
- * Fnl-3</a> is fixed.
- */
- public static final boolean FNL3_FIXED = false;
-
/**
* Whether <a href="http://issues.eigenbase.org/browse/FRG-327">issue
* FRG-327: AssertionError while translating IN list that contains null</a>
diff --git a/core/src/test/java/org/apache/calcite/test/SqlOperatorUnparseTest.java b/core/src/test/java/org/apache/calcite/test/SqlOperatorUnparseTest.java
index 49872ef988..645642068b 100644
--- a/core/src/test/java/org/apache/calcite/test/SqlOperatorUnparseTest.java
+++ b/core/src/test/java/org/apache/calcite/test/SqlOperatorUnparseTest.java
@@ -80,7 +80,7 @@ public class SqlOperatorUnparseTest extends CalciteSqlOperatorTest {
String rewrite(String sql) throws SqlParseException {
final SqlParser parser = factory.createParser(sql);
final SqlNode sqlNode = parser.parseStmt();
- return sqlNode.toSqlString(c -> c).getSql();
+ return sqlNode.toSqlString(c -> c.withClauseStartsLine(false)).getSql();
}
@Override public void forEachQuery(
@@ -99,11 +99,6 @@ public class SqlOperatorUnparseTest extends CalciteSqlOperatorTest {
}
}
- @Override @Disabled("Runtime error message differs after parsing and unparsing")
- void testBitAndFuncRuntimeFails() {
- super.testContainsSubstrFunc();
- }
-
// Every test that is Disabled below corresponds to a bug.
// These tests should just be deleted when the corresponding bugs are fixed.
diff --git a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Primitive.java b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Primitive.java
index 066b1a5f00..b3a43f1ae2 100644
--- a/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Primitive.java
+++ b/linq4j/src/main/java/org/apache/calcite/linq4j/tree/Primitive.java
@@ -22,6 +22,8 @@ import org.checkerframework.checker.nullness.qual.Nullable;
import java.lang.reflect.Array;
import java.lang.reflect.Field;
import java.lang.reflect.Type;
+import java.math.BigDecimal;
+import java.math.RoundingMode;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.util.AbstractList;
@@ -367,28 +369,68 @@ public enum Primitive {
}
/**
- * Converts a number into a value of the type
- * specified by this primitive.
+ * Check if a value after rounding falls within a specified range.
+ *
+ * @param value Value to compare.
+ * @param min Minimum value allowed.
+ * @param max Maximum value allowed.
+ */
+ static void checkRoundedRange(Number value, double min, double max) {
+ double dbl = value.doubleValue();
+ // The equivalent of DOWN rounding for BigDecimal
+ dbl = dbl > 0 ? Math.floor(dbl) : Math.ceil(dbl);
+ if (dbl < min || dbl > max) {
+ throw new ArithmeticException("Value " + value + " out of range");
+ }
+ }
+
+ /**
+ * Converts a number into a value of the type specified by this primitive
+ * using the SQL CAST rules. If the value conversion causes loss of significant digits,
+ * an exception is thrown.
*
* @param value Value to convert.
- * @return The converted value, or null if the
- * conversion cannot be performed.
+ * @return The converted value, or null if the type of the result is not a number.
*/
public @Nullable Object numberValue(Number value) {
switch (this) {
case BYTE:
+ checkRoundedRange(value, Byte.MIN_VALUE, Byte.MAX_VALUE);
return value.byteValue();
case CHAR:
+ // No overflow checks for char values.
+ // For example, Postgres has this behavior.
return (char) value.intValue();
case SHORT:
+ checkRoundedRange(value, Short.MIN_VALUE, Short.MAX_VALUE);
return value.shortValue();
case INT:
+ checkRoundedRange(value, Integer.MIN_VALUE, Integer.MAX_VALUE);
return value.intValue();
case LONG:
- return value.longValue();
+ if (value instanceof Byte
+ || value instanceof Short
+ || value instanceof Integer
+ || value instanceof Long) {
+ return value.longValue();
+ }
+ if (value instanceof Float
+ || value instanceof Double) {
+ // The value Long.MAX_VALUE cannot be represented exactly as a double,
+ // so we cannot use checkRoundedRange.
+ BigDecimal decimal = BigDecimal.valueOf(value.doubleValue())
+ // Round to an integer
+ .setScale(0, RoundingMode.DOWN);
+ // longValueExact will throw ArithmeticException if out of range
+ return decimal.longValueExact();
+ }
+ throw new AssertionError("Unexpected Number type "
+ + value.getClass().getSimpleName());
case FLOAT:
+ // out of range values will be represented as infinities
return value.floatValue();
case DOUBLE:
+ // out of range values will be represented as infinities
return value.doubleValue();
default:
return null;
diff --git a/site/_docs/reference.md b/site/_docs/reference.md
index eef28ad1cf..e279e87339 100644
--- a/site/_docs/reference.md
+++ b/site/_docs/reference.md
@@ -1470,7 +1470,7 @@ Algorithms for implicit conversion are subject to change across Calcite releases
| Operator syntax | Description
|:----------------------------------------| :----------
-| CAST(value AS type) | Converts a value to a given type
+| CAST(value AS type) | Converts a value to a given type. Casts between integer types truncate towards 0
| CONVERT(string, charSet1, charSet2) | Converts *string* from *charSet1* to *charSet2*
| CONVERT(value USING transcodingName) | Alter *value* from one base character set to *transcodingName*
| TRANSLATE(value USING transcodingName) | Alter *value* from one base character set to *transcodingName*
diff --git a/testkit/src/main/java/org/apache/calcite/sql/test/SqlOperatorFixture.java b/testkit/src/main/java/org/apache/calcite/sql/test/SqlOperatorFixture.java
index 3ae3a3e2bb..7bcfcb1cb0 100644
--- a/testkit/src/main/java/org/apache/calcite/sql/test/SqlOperatorFixture.java
+++ b/testkit/src/main/java/org/apache/calcite/sql/test/SqlOperatorFixture.java
@@ -68,30 +68,19 @@ import static org.apache.calcite.sql.test.ResultCheckers.isSingle;
public interface SqlOperatorFixture extends AutoCloseable {
//~ Enums ------------------------------------------------------------------
- // TODO: Change message when Fnl3Fixed to something like
- // "Invalid character for cast: PC=0 Code=22018"
- String INVALID_CHAR_MESSAGE =
- Bug.FNL3_FIXED ? null : "(?s).*";
-
- // TODO: Change message when Fnl3Fixed to something like
- // "Overflow during calculation or cast: PC=0 Code=22003"
- String OUT_OF_RANGE_MESSAGE =
- Bug.FNL3_FIXED ? null : "(?s).*";
-
- // TODO: Change message when Fnl3Fixed to something like
- // "Division by zero: PC=0 Code=22012"
- String DIVISION_BY_ZERO_MESSAGE =
- Bug.FNL3_FIXED ? null : "(?s).*";
-
- // TODO: Change message when Fnl3Fixed to something like
- // "String right truncation: PC=0 Code=22001"
- String STRING_TRUNC_MESSAGE =
- Bug.FNL3_FIXED ? null : "(?s).*";
-
- // TODO: Change message when Fnl3Fixed to something like
- // "Invalid datetime format: PC=0 Code=22007"
- String BAD_DATETIME_MESSAGE =
- Bug.FNL3_FIXED ? null : "(?s).*";
+ // TODO: Change message
+ String INVALID_CHAR_MESSAGE = "(?s).*";
+
+ String OUT_OF_RANGE_MESSAGE = ".* out of range";
+
+ // TODO: Change message
+ String DIVISION_BY_ZERO_MESSAGE = "(?s).*";
+
+ // TODO: Change message
+ String STRING_TRUNC_MESSAGE = "(?s).*";
+
+ // TODO: Change message
+ String BAD_DATETIME_MESSAGE = "(?s).*";
// Error messages when an invalid time unit is given as
// input to extract for a particular input type.
diff --git a/testkit/src/main/java/org/apache/calcite/sql/test/SqlTests.java b/testkit/src/main/java/org/apache/calcite/sql/test/SqlTests.java
index 3c5deb5216..bb33212d97 100644
--- a/testkit/src/main/java/org/apache/calcite/sql/test/SqlTests.java
+++ b/testkit/src/main/java/org/apache/calcite/sql/test/SqlTests.java
@@ -299,6 +299,10 @@ public abstract class SqlTests {
}
} else {
actualMessage = ex.getMessage();
+ if (ex instanceof NumberFormatException) {
+ // The message from NumberFormatException is not very usable
+ actualMessage = "Number has wrong format " + actualMessage;
+ }
if (actualMessage != null) {
java.util.regex.Matcher matcher =
LINE_COL_TWICE_PATTERN.matcher(actualMessage);
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 b0338bc759..013843fd8f 100644
--- a/testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java
+++ b/testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java
@@ -624,7 +624,7 @@ public class SqlOperatorTest {
f.checkCastFails(numeric.minOverflowNumericString,
type, LITERAL_OUT_OF_RANGE_MESSAGE, false, castType);
} else {
- if (Bug.CALCITE_2539_FIXED) {
+ if (numeric != Numeric.DECIMAL5_2 || Bug.CALCITE_2539_FIXED) {
f.checkCastFails(numeric.maxOverflowNumericString,
type, OUT_OF_RANGE_MESSAGE, true, castType);
f.checkCastFails(numeric.minOverflowNumericString,
@@ -921,35 +921,36 @@ public class SqlOperatorTest {
f.checkScalar("cast(1.25 as int)", 1, "INTEGER NOT NULL");
f.checkScalar("cast(1.25E0 as int)", 1, "INTEGER NOT NULL");
+ // Calcite's simplifier uses BigDecimal.intValue(), which rounds down
+ f.checkScalar("cast(1.5 as int)", 1, "INTEGER NOT NULL");
+ f.checkScalar("cast(5E-1 as int)", 0, "INTEGER NOT NULL");
+ f.checkScalar("cast(1.75 as int)", 1, "INTEGER NOT NULL");
+ f.checkScalar("cast(1.75E0 as int)", 1, "INTEGER NOT NULL");
+
+ f.checkScalar("cast(-1.25 as int)", -1, "INTEGER NOT NULL");
+ f.checkScalar("cast(-1.25E0 as int)", -1, "INTEGER NOT NULL");
+ f.checkScalar("cast(-1.5 as int)", -1, "INTEGER NOT NULL");
+ f.checkScalar("cast(-5E-1 as int)", 0, "INTEGER NOT NULL");
+ f.checkScalar("cast(-1.75 as int)", -1, "INTEGER NOT NULL");
+ f.checkScalar("cast(-1.75E0 as int)", -1, "INTEGER NOT NULL");
+
+ f.checkScalar("cast(1.23454 as int)", 1, "INTEGER NOT NULL");
+ f.checkScalar("cast(1.23454E0 as int)", 1, "INTEGER NOT NULL");
+ f.checkScalar("cast(1.23455 as int)", 1, "INTEGER NOT NULL");
+ f.checkScalar("cast(5E-5 as int)", 0, "INTEGER NOT NULL");
+ f.checkScalar("cast(1.99995 as int)", 1, "INTEGER NOT NULL");
+ f.checkScalar("cast(1.99995E0 as int)", 1, "INTEGER NOT NULL");
+
+ f.checkScalar("cast(-1.23454 as int)", -1, "INTEGER NOT NULL");
+ f.checkScalar("cast(-1.23454E0 as int)", -1, "INTEGER NOT NULL");
+ f.checkScalar("cast(-1.23455 as int)", -1, "INTEGER NOT NULL");
+ f.checkScalar("cast(-5E-5 as int)", 0, "INTEGER NOT NULL");
+ f.checkScalar("cast(-1.99995 as int)", -1, "INTEGER NOT NULL");
+ f.checkScalar("cast(-1.99995E0 as int)", -1, "INTEGER NOT NULL");
+
if (!f.brokenTestsEnabled()) {
return;
}
- f.checkFails("cast(1.5 as int)", OUT_OF_RANGE_MESSAGE, true);
- f.checkFails("cast(5E-1 as int)", OUT_OF_RANGE_MESSAGE, true);
- f.checkFails("cast(1.75 as int)", OUT_OF_RANGE_MESSAGE, true);
- f.checkFails("cast(1.75E0 as int)", OUT_OF_RANGE_MESSAGE, true);
-
- f.checkFails("cast(-1.25 as int)", OUT_OF_RANGE_MESSAGE, true);
- f.checkFails("cast(-1.25E0 as int)", OUT_OF_RANGE_MESSAGE, true);
- f.checkFails("cast(-1.5 as int)", OUT_OF_RANGE_MESSAGE, true);
- f.checkFails("cast(-5E-1 as int)", OUT_OF_RANGE_MESSAGE, true);
- f.checkFails("cast(-1.75 as int)", OUT_OF_RANGE_MESSAGE, true);
- f.checkFails("cast(-1.75E0 as int)", OUT_OF_RANGE_MESSAGE, true);
-
- f.checkFails("cast(1.23454 as int)", OUT_OF_RANGE_MESSAGE, true);
- f.checkFails("cast(1.23454E0 as int)", OUT_OF_RANGE_MESSAGE, true);
- f.checkFails("cast(1.23455 as int)", OUT_OF_RANGE_MESSAGE, true);
- f.checkFails("cast(5E-5 as int)", OUT_OF_RANGE_MESSAGE, true);
- f.checkFails("cast(1.99995 as int)", OUT_OF_RANGE_MESSAGE, true);
- f.checkFails("cast(1.99995E0 as int)", OUT_OF_RANGE_MESSAGE, true);
-
- f.checkFails("cast(-1.23454 as int)", OUT_OF_RANGE_MESSAGE, true);
- f.checkFails("cast(-1.23454E0 as int)", OUT_OF_RANGE_MESSAGE, true);
- f.checkFails("cast(-1.23455 as int)", OUT_OF_RANGE_MESSAGE, true);
- f.checkFails("cast(-5E-5 as int)", OUT_OF_RANGE_MESSAGE, true);
- f.checkFails("cast(-1.99995 as int)", OUT_OF_RANGE_MESSAGE, true);
- f.checkFails("cast(-1.99995E0 as int)", OUT_OF_RANGE_MESSAGE, true);
-
// 9.99 round to 10.0, should give out of range error
f.checkFails("cast(9.99 as decimal(2,1))", OUT_OF_RANGE_MESSAGE,
true);
@@ -962,13 +963,10 @@ public class SqlOperatorTest {
f.checkScalar("cast( cast(1.25 as double) as integer)", 1, "INTEGER NOT NULL");
f.checkScalar("cast( cast(-1.25 as double) as integer)", -1, "INTEGER NOT NULL");
- if (!f.brokenTestsEnabled()) {
- return;
- }
- f.checkFails("cast( cast(1.75 as double) as integer)", OUT_OF_RANGE_MESSAGE, true);
- f.checkFails("cast( cast(-1.75 as double) as integer)", OUT_OF_RANGE_MESSAGE, true);
- f.checkFails("cast( cast(1.5 as double) as integer)", OUT_OF_RANGE_MESSAGE, true);
- f.checkFails("cast( cast(-1.5 as double) as integer)", OUT_OF_RANGE_MESSAGE, true);
+ f.checkScalar("cast( cast(1.75 as double) as integer)", 1, "INTEGER NOT NULL");
+ f.checkScalar("cast( cast(-1.75 as double) as integer)", -1, "INTEGER NOT NULL");
+ f.checkScalar("cast( cast(1.5 as double) as integer)", 1, "INTEGER NOT NULL");
+ f.checkScalar("cast( cast(-1.5 as double) as integer)", -1, "INTEGER NOT NULL");
}
@ParameterizedTest
@@ -1124,15 +1122,24 @@ public class SqlOperatorTest {
// generate Java constants that throw when the class is loaded, thus
// ExceptionInInitializerError.
f.checkScalarExact("cast('15' as integer)", "INTEGER NOT NULL", "15");
- if (Bug.CALCITE_2539_FIXED) {
- f.checkFails("cast('15.4' as integer)", "xxx", true);
- f.checkFails("cast('15.6' as integer)", "xxx", true);
- f.checkFails("cast('ue' as boolean)", "xxx", true);
- f.checkFails("cast('' as boolean)", "xxx", true);
- f.checkFails("cast('' as integer)", "xxx", true);
- f.checkFails("cast('' as real)", "xxx", true);
- f.checkFails("cast('' as double)", "xxx", true);
- f.checkFails("cast('' as smallint)", "xxx", true);
+ if (castType == CastType.CAST) { // Safe casts should not fail
+ f.checkFails("cast('15.4' as integer)", "Number has wrong format.*", true);
+ f.checkFails("cast('15.6' as integer)", "Number has wrong format.*", true);
+ f.checkFails("cast('ue' as boolean)", "Invalid character for cast.*", true);
+ f.checkFails("cast('' as boolean)", "Invalid character for cast.*", true);
+ f.checkFails("cast('' as integer)", "Number has wrong format.*", true);
+ f.checkFails("cast('' as real)", "Number has wrong format.*", true);
+ f.checkFails("cast('' as double)", "Number has wrong format.*", true);
+ f.checkFails("cast('' as smallint)", "Number has wrong format.*", true);
+ } else {
+ f.checkNull("cast('15.4' as integer)");
+ f.checkNull("cast('15.6' as integer)");
+ f.checkNull("cast('ue' as boolean)");
+ f.checkNull("cast('' as boolean)");
+ f.checkNull("cast('' as integer)");
+ f.checkNull("cast('' as real)");
+ f.checkNull("cast('' as double)");
+ f.checkNull("cast('' as smallint)");
}
}
@@ -1458,7 +1465,7 @@ public class SqlOperatorTest {
/** Test case for
* <a href="https://issues.apache.org/jira/browse/CALCITE-4861">[CALCITE-4861]
* Optimization of chained CAST calls leads to unexpected behavior</a>. */
- @Test @Disabled("CALCITE-5990") void testChainedCast() {
+ @Test void testChainedCast() {
final SqlOperatorFixture f = fixture();
f.checkFails("CAST(CAST(CAST(123456 AS TINYINT) AS INT) AS BIGINT)",
".*Value 123456 out of range", true);
@@ -13191,10 +13198,7 @@ public class SqlOperatorTest {
final SqlOperatorFixture f = fixture();
f.checkAggFails("bit_and(x)",
new String[]{"CAST(x'0201' AS VARBINARY)", "CAST(x'02' AS VARBINARY)"},
- "Error while executing SQL"
- + " \"SELECT bit_and\\(x\\)"
- + " FROM \\(SELECT CAST\\(x'0201' AS VARBINARY\\) AS x FROM \\(VALUES \\(1\\)\\)"
- + " UNION ALL SELECT CAST\\(x'02' AS VARBINARY\\) AS x FROM \\(VALUES \\(1\\)\\)\\)\":"
+ "Error while executing SQL .*"
+ " Different length for bitwise operands: the first: 2, the second: 1",
true);
}
@@ -13300,9 +13304,6 @@ public class SqlOperatorTest {
@Test void testLiteralAtLimit() {
final SqlOperatorFixture f = fixture();
f.setFor(SqlStdOperatorTable.CAST, VmName.EXPAND);
- if (!f.brokenTestsEnabled()) {
- return;
- }
final List<RelDataType> types =
SqlTests.getTypes(f.getFactory().getTypeFactory());
for (RelDataType type : types) {