-
Notifications
You must be signed in to change notification settings - Fork 93
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
IGNITE-23192 Sql. Arithmetic operations failed with "out of range" exception #4422
base: main
Are you sure you want to change the base?
Changes from 10 commits
08ae199
a218ef3
f083c47
6f94308
ec4fa7b
9d11dec
d8fcd85
4d67f48
3b04c93
6b6118a
5e23b85
420c596
7fe6e44
2d174e7
0eb57e8
a23fa0b
84b46c9
f0144b1
d91c7ad
7f5d2b2
eda56e4
c375808
876b38a
775e35f
24a8014
42a42b5
da50be3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -519,7 +519,7 @@ public async Task TestCustomDecimalScale() | |
await using var resultSet = await Client.Sql.ExecuteAsync(null, "select cast((10 / ?) as decimal(20, 5))", 3m); | ||
IIgniteTuple res = await resultSet.SingleAsync(); | ||
|
||
Assert.AreEqual(3.33333m, res[0]); | ||
Assert.AreEqual(3.3m, res[0]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will be fixed soon |
||
} | ||
|
||
[Test] | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -217,7 +217,7 @@ void testExecuteAsyncDdlDml() { | |||||
assertEquals(10, rows.size()); | ||||||
assertEquals("hello 1", rows.get(1).stringValue(0)); | ||||||
assertEquals(1, rows.get(1).intValue(1)); | ||||||
assertEquals(2, rows.get(1).intValue(2)); | ||||||
assertEquals(2, rows.get(1).longValue(2)); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||||||
|
||||||
// Update data. | ||||||
AsyncResultSet updateRes = sql | ||||||
|
@@ -300,7 +300,7 @@ void testExecuteDdlDml() { | |||||
assertEquals(10, rows.size()); | ||||||
assertEquals("hello 1", rows.get(1).stringValue(0)); | ||||||
assertEquals(1, rows.get(1).intValue(1)); | ||||||
assertEquals(2, rows.get(1).intValue(2)); | ||||||
assertEquals(2L, rows.get(1).longValue(2)); | ||||||
|
||||||
// Update data. | ||||||
ResultSet updateRes = sql.execute(null, "UPDATE testExecuteDdlDml SET VAL='upd' WHERE ID < 5"); | ||||||
|
@@ -398,18 +398,18 @@ void testResultSetMapping(boolean useStatement) { | |||||
@ValueSource(booleans = {true, false}) | ||||||
void testResultSetMappingAsync(boolean useStatement) { | ||||||
IgniteSql sql = client().sql(); | ||||||
String query = "select 1 as num, concat('hello ', ?) as str"; | ||||||
String query = "select 1 + ? as num, concat('hello ', ?) as str"; | ||||||
korlov42 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
AsyncResultSet<Pojo> resultSet = useStatement | ||||||
? sql.executeAsync(null, Mapper.of(Pojo.class), client().sql().statementBuilder().query(query).build(), "world").join() | ||||||
: sql.executeAsync(null, Mapper.of(Pojo.class), query, "world").join(); | ||||||
? sql.executeAsync(null, Mapper.of(Pojo.class), client().sql().statementBuilder().query(query).build(), 10, "world").join() | ||||||
: sql.executeAsync(null, Mapper.of(Pojo.class), query, 10, "world").join(); | ||||||
|
||||||
assertTrue(resultSet.hasRowSet()); | ||||||
assertEquals(1, resultSet.currentPageSize()); | ||||||
|
||||||
Pojo row = resultSet.currentPage().iterator().next(); | ||||||
|
||||||
assertEquals(1, row.num); | ||||||
assertEquals(11L, row.num); | ||||||
assertEquals("hello world", row.str); | ||||||
} | ||||||
|
||||||
|
@@ -522,7 +522,7 @@ public void testExecuteScriptFail() { | |||||
} | ||||||
|
||||||
private static class Pojo { | ||||||
public int num; | ||||||
public long num; | ||||||
|
||||||
public String str; | ||||||
} | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -61,8 +61,6 @@ public class ItDataTypesTest extends BaseSqlIntegrationTest { | |
|
||
private static final String NUMERIC_FORMAT_ERROR = "neither a decimal digit number"; | ||
|
||
private static final Object EMPTY_PARAM = new Object(); | ||
|
||
/** | ||
* Drops all created tables. | ||
*/ | ||
|
@@ -606,64 +604,51 @@ public void testDecimalCastsFromNumeric(RelDataType inputType, Object input, | |
@ParameterizedTest(name = "{1} {2}") | ||
@MethodSource("decimalOverflows") | ||
public void testCalcOpOverflow(SqlTypeName type, String expr, Object param) { | ||
if (param == EMPTY_PARAM) { | ||
assertThrowsSqlException(RUNTIME_ERR, type.getName() + " out of range", () -> sql(expr)); | ||
} else { | ||
assertThrowsSqlException(RUNTIME_ERR, type.getName() + " out of range", () -> sql(expr, param)); | ||
} | ||
assertThrowsSqlException(RUNTIME_ERR, type.getName() + " out of range", () -> sql(expr, param)); | ||
} | ||
|
||
private static Stream<Arguments> decimalOverflows() { | ||
return Stream.of( | ||
// BIGINT | ||
arguments(SqlTypeName.BIGINT, "SELECT 9223372036854775807 + 1", EMPTY_PARAM), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why did you decide to remove such cases? It looks as valid testcases |
||
arguments(SqlTypeName.BIGINT, "SELECT 9223372036854775807 * 2", EMPTY_PARAM), | ||
arguments(SqlTypeName.BIGINT, "SELECT -9223372036854775808 - 1", EMPTY_PARAM), | ||
arguments(SqlTypeName.BIGINT, "SELECT -(-9223372036854775807 - 1)", EMPTY_PARAM), | ||
arguments(SqlTypeName.BIGINT, "SELECT -CAST(-9223372036854775808 AS BIGINT)", EMPTY_PARAM), | ||
arguments(SqlTypeName.BIGINT, "SELECT -(?)", -9223372036854775808L), | ||
arguments(SqlTypeName.BIGINT, "SELECT -9223372036854775808/-1", EMPTY_PARAM), | ||
arguments(SqlTypeName.BIGINT, "SELECT -CAST(? AS BIGINT)", -9223372036854775808L), | ||
|
||
// INTEGER | ||
arguments(SqlTypeName.INTEGER, "SELECT 2147483647 + 1", EMPTY_PARAM), | ||
arguments(SqlTypeName.INTEGER, "SELECT CAST(CAST(2147483648 AS BIGINT) AS INTEGER)", EMPTY_PARAM), | ||
arguments(SqlTypeName.INTEGER, "SELECT 2147483647 * 2", EMPTY_PARAM), | ||
arguments(SqlTypeName.INTEGER, "SELECT -2147483648 - 1", EMPTY_PARAM), | ||
arguments(SqlTypeName.INTEGER, "SELECT -(-2147483647 - 1)", EMPTY_PARAM), | ||
arguments(SqlTypeName.INTEGER, "SELECT -CAST(-2147483648 AS INTEGER)", EMPTY_PARAM), | ||
arguments(SqlTypeName.INTEGER, "SELECT -(?)", -2147483648), | ||
arguments(SqlTypeName.INTEGER, "SELECT -2147483648/-1", EMPTY_PARAM), | ||
arguments(SqlTypeName.INTEGER, "select CAST(9223372036854775807.5 + 9223372036854775807.5 AS INTEGER)", | ||
EMPTY_PARAM), | ||
arguments(SqlTypeName.INTEGER, "SELECT -CAST(? AS INTEGER)", -2147483648), | ||
|
||
// SMALLINT | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does it make sense to have similar test cases for all 4 types ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. derived type for SMALLINT and TINYINT tests without explicit casting is INTEGER so - no overflow will raised here |
||
arguments(SqlTypeName.SMALLINT, "SELECT 32000::SMALLINT + 1000::SMALLINT", EMPTY_PARAM), | ||
arguments(SqlTypeName.SMALLINT, "select CAST(9223372036854775807.5 + 9223372036854775807.5 AS SMALLINT)", | ||
EMPTY_PARAM), | ||
arguments(SqlTypeName.SMALLINT, "SELECT CAST(CAST(33000 AS BIGINT) AS SMALLINT)", EMPTY_PARAM), | ||
arguments(SqlTypeName.SMALLINT, "SELECT CAST(CAST(33000 AS FLOAT) AS SMALLINT)", EMPTY_PARAM), | ||
arguments(SqlTypeName.SMALLINT, "SELECT CAST(CAST(33000 + 1 AS FLOAT) AS SMALLINT)", EMPTY_PARAM), | ||
arguments(SqlTypeName.SMALLINT, "SELECT 17000::SMALLINT * 2::SMALLINT", EMPTY_PARAM), | ||
arguments(SqlTypeName.SMALLINT, "SELECT -32000::SMALLINT - 1000::SMALLINT", EMPTY_PARAM), | ||
arguments(SqlTypeName.SMALLINT, "SELECT -(-32767::SMALLINT - 1::SMALLINT)", EMPTY_PARAM), | ||
arguments(SqlTypeName.SMALLINT, "SELECT -CAST(-32768 AS SMALLINT)", EMPTY_PARAM), | ||
arguments(SqlTypeName.SMALLINT, "SELECT -CAST(? AS SMALLINT)", -32768), | ||
arguments(SqlTypeName.SMALLINT, "SELECT CAST (-32768 AS SMALLINT)/-1::SMALLINT", EMPTY_PARAM), | ||
|
||
// TINYINT | ||
arguments(SqlTypeName.TINYINT, "SELECT 2::TINYINT + 127::TINYINT", EMPTY_PARAM), | ||
arguments(SqlTypeName.TINYINT, "select CAST(9223372036854775807.5 + 9223372036854775807.5 AS TINYINT)", | ||
EMPTY_PARAM), | ||
arguments(SqlTypeName.TINYINT, "SELECT CAST(CAST(200 AS BIGINT) AS TINYINT)", EMPTY_PARAM), | ||
arguments(SqlTypeName.TINYINT, "SELECT CAST(CAST(200 AS FLOAT) AS TINYINT)", EMPTY_PARAM), | ||
arguments(SqlTypeName.TINYINT, "SELECT CAST(CAST(200 + 1 AS FLOAT) AS TINYINT)", EMPTY_PARAM), | ||
arguments(SqlTypeName.TINYINT, "SELECT 2::TINYINT * 127::TINYINT", EMPTY_PARAM), | ||
arguments(SqlTypeName.TINYINT, "SELECT -2::TINYINT - 127::TINYINT", EMPTY_PARAM), | ||
arguments(SqlTypeName.TINYINT, "SELECT -(-127::TINYINT - 1::TINYINT)", EMPTY_PARAM), | ||
arguments(SqlTypeName.TINYINT, "SELECT -CAST(-128 AS TINYINT)", EMPTY_PARAM), | ||
arguments(SqlTypeName.TINYINT, "SELECT -CAST(? AS TINYINT)", -128), | ||
arguments(SqlTypeName.TINYINT, "SELECT CAST(-128 AS TINYINT)/-1::TINYINT", EMPTY_PARAM), | ||
arguments(SqlTypeName.TINYINT, "SELECT CAST(CAST(200 + 1 AS FLOAT) AS TINYINT)", EMPTY_PARAM) | ||
arguments(SqlTypeName.TINYINT, "SELECT -CAST(? AS TINYINT)", -128) | ||
); | ||
} | ||
|
||
@ParameterizedTest | ||
@MethodSource("decimalOpTypeExtension") | ||
public void testCalcOpDynParamOverflow(String expr, String expect, Object param) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why did you name this test 'overflow' when no overflow is expected? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
||
assertEquals(sql(expr, param).get(0).get(0).toString(), expect); | ||
} | ||
|
||
private static Stream<Arguments> decimalOpTypeExtension() { | ||
return Stream.of( | ||
// TODO: https://issues.apache.org/jira/browse/IGNITE-23232 | ||
// arguments("SELECT -9223372036854775808::BIGINT/-1::BIGINT", "9223372036854775808"), | ||
// arguments("SELECT -?::BIGINT/-1::BIGINT", "9223372036854775808", "9223372036854775808"), | ||
// arguments("SELECT -2147483648::INTEGER/-1::INTEGER", "2147483648")/*, | ||
// arguments("SELECT -32768::SMALLINT/-1::SMALLINT", "32768"), | ||
// arguments("SELECT -128::TINYINT/-1::TINYINT", "128") | ||
|
||
arguments("SELECT CAST(-? AS BIGINT)/-1", "9223372036854775808", "9223372036854775808"), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why the test doesn't fail with overflow? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i not understand why it need to fail ? Overflow need to operate with type, i didn`t see ant type here, did i miss smth ? |
||
arguments("SELECT CAST(-? AS INTEGER)/-1", "2147483648", "2147483648"), | ||
arguments("SELECT CAST(-? AS SMALLINT)/-1", "32768", "32768"), | ||
arguments("SELECT CAST(-? AS TINYINT)/-1", "128", "128"), | ||
|
||
arguments("SELECT CAST(-? AS BIGINT) * -1", "9223372036854775808", "9223372036854775808"), | ||
arguments("SELECT CAST(-? AS INTEGER) * -1", "2147483648", "2147483648"), | ||
arguments("SELECT CAST(-? AS SMALLINT) * -1", "32768", "32768"), | ||
arguments("SELECT CAST(-? AS TINYINT) * -1", "128", "128") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does it make sense to add test cases not only with cast from string? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. other tests are moved into "cast_to_integer.test" |
||
); | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's should be either 3.00000 or 3.33333, but not 3.3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed