Skip to content

Commit

Permalink
[CALCITE-5990] Explicit cast to numeric type doesn't check overflow
Browse files Browse the repository at this point in the history 
Signed-off-by: Mihai Budiu <mbudiu@gmail.com>
  • Loading branch information
@mihaibudiu
mihaibudiu committed Oct 31, 2023
1 parent 782d327 commit 620058f
Show file tree
Hide file tree
Showing 7 changed files with 118 additions and 95 deletions.
8 changes: 0 additions & 8 deletions core/src/main/java/org/apache/calcite/util/Bug.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down
7 changes: 1 addition & 6 deletions core/src/test/java/org/apache/calcite/test/SqlOperatorUnparseTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ TesterImpl withFactory(UnaryOperator<SqlTestFactory> transform) {
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(
Expand All @@ -99,11 +99,6 @@ String rewrite(String sql) throws SqlParseException {
}
}

@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.

Expand Down
52 changes: 47 additions & 5 deletions linq4j/src/main/java/org/apache/calcite/linq4j/tree/Primitive.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
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;
Expand Down Expand Up @@ -367,28 +369,68 @@ public static List<Double> asList(double[] elements) {
}

/**
* 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;
Expand Down
2 changes: 1 addition & 1 deletion site/_docs/reference.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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*
Expand Down
37 changes: 13 additions & 24 deletions testkit/src/main/java/org/apache/calcite/sql/test/SqlOperatorFixture.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,30 +68,19 @@
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.
Expand Down
4 changes: 4 additions & 0 deletions testkit/src/main/java/org/apache/calcite/sql/test/SqlTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,10 @@ public static void checkEx(@Nullable Throwable ex,
}
} 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);
Expand Down
103 changes: 52 additions & 51 deletions testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -624,7 +624,7 @@ void testCastExactNumericLimits(CastType castType, SqlOperatorFixture f) {
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,
Expand Down Expand Up @@ -921,35 +921,36 @@ void testCastWithRoundingToScalar(CastType castType, SqlOperatorFixture f) {

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);
Expand All @@ -962,13 +963,10 @@ void testCastDecimalToDoubleToInteger(CastType castType, SqlOperatorFixture f) {

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
Expand Down Expand Up @@ -1124,15 +1122,24 @@ void testCastInvalid(CastType castType, SqlOperatorFixture f) {
// 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)");
}
}

Expand Down Expand Up @@ -1458,7 +1465,7 @@ void testCastToBoolean(CastType castType, SqlOperatorFixture f) {
/** 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);
Expand Down Expand Up @@ -13191,10 +13198,7 @@ private static void checkLogicalOrFunc(SqlOperatorFixture f) {
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);
}
Expand Down Expand Up @@ -13300,9 +13304,6 @@ private static void checkLogicalOrFunc(SqlOperatorFixture f) {
@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) {
Expand Down

0 comments on commit 620058f

Please sign in to comment.