IGNITE-23192 Sql. Arithmetic operations failed with "out of range" exception #4422

zstan · 2024-09-19T07:19:14Z

Thank you for submitting the pull request.

To streamline the review process of the patch and ensure better code quality
we ask both an author and a reviewer to verify the following:

The Review Checklist

Formal criteria: TC status, codestyle, mandatory documentation. Also make sure to complete the following:
- There is a single JIRA ticket related to the pull request.
- The web-link to the pull request is attached to the JIRA ticket.
- The JIRA ticket has the Patch Available state.
- The description of the JIRA ticket explains WHAT was made, WHY and HOW.
- The pull request title is treated as the final commit message. The following pattern must be used: IGNITE-XXXX Change summary where XXXX - number of JIRA issue.
Design: new code conforms with the design principles of the components it is added to.
Patch quality: patch cannot be split into smaller pieces, its size must be reasonable.
Code quality: code is clean and readable, necessary developer documentation is added if needed.
Tests code quality: test set covers positive/negative scenarios, happy/edge cases. Tests are effective in terms of execution time and resources.

Notes

Apache Ignite Coding Guidelines

…ception

ygerzhedovich · 2024-09-20T08:55:00Z

modules/platforms/dotnet/Apache.Ignite.Tests/Sql/SqlTests.cs

@@ -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]);


it's should be either 3.00000 or 3.33333, but not 3.3

ygerzhedovich · 2024-09-20T09:10:48Z

...c/integrationTest/java/org/apache/ignite/internal/runner/app/client/ItThinClientSqlTest.java

@@ -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));


Suggested change

assertEquals(2, rows.get(1).longValue(2));

assertEquals(2L, rows.get(1).longValue(2));

?

zstan · 2024-09-20T09:18:49Z

modules/platforms/dotnet/Apache.Ignite.Tests/Sql/SqlTests.cs

@@ -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]);


will be fixed soon

ygerzhedovich · 2024-09-20T09:31:34Z

...l-engine/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItDataTypesTest.java

    }

    private static Stream<Arguments> decimalOverflows() {
        return Stream.of(
                // BIGINT
-                arguments(SqlTypeName.BIGINT, "SELECT 9223372036854775807 + 1", EMPTY_PARAM),


Why did you decide to remove such cases? It looks as valid testcases

ygerzhedovich · 2024-09-20T09:35:59Z

...l-engine/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItDataTypesTest.java

+                // arguments("SELECT -32768::SMALLINT/-1::SMALLINT", "32768"),
+                // arguments("SELECT -128::TINYINT/-1::TINYINT", "128")
+
+                arguments("SELECT CAST(-? AS BIGINT)/-1", "9223372036854775808", "9223372036854775808"),


Why the test doesn't fail with overflow?

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 ?

ygerzhedovich · 2024-09-20T09:41:23Z

modules/sql-engine/src/integrationTest/sql/types/integer/cast_to_integer.test

most of the cases here should fail due to overflow, it looks incorrect now

modules/sql-engine/src/integrationTest/sql/types/integer/cast_to_integer.test

ygerzhedovich · 2024-09-20T10:58:17Z

modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/util/IgniteMath.java

+
+    /** Returns the multiply of its arguments, extending result type for overflow avoidance. */
+    public static BigDecimal multiply(BigDecimal x, BigDecimal y) {
+        int maxPrecision = Commons.cluster().getTypeFactory().getTypeSystem().getMaxPrecision(SqlTypeName.DECIMAL);


Standard (see 6.29) says for the case the following:
The precision of the result of multiplication is implementation-defined, and the scale is S1 + S2.

korlov42

Let's add more tests to the patch:

planning tests to make sure derived result type for math operation is as expected for every type pair (you may extend NumericBinaryOperationsTypeCoercionTest fro this)
test on changes in TypeSystem
execution test covering all math operation for all type pairs

korlov42 · 2024-09-23T07:25:01Z

...es/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/type/IgniteTypeSystem.java

+        if (isIntType(type1) && isIntType(type2)) {
+            boolean typesNullability = type1.isNullable() || type2.isNullable();
+
+            switch (type1.getSqlTypeName()) {


instead of copy-pasting all these switch cases it's better to do following:

get least restrictive type among arguments (typeFactory.leastRestrictive(List.of(type1, type2)))

get next type for the result type from p1

korlov42 · 2024-09-23T07:26:53Z

...es/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/type/IgniteTypeSystem.java

changes in type systems must be covered IgniteTypeSystemTest test

append test, IgniteTypeSystemTest#deriveType

Pass the original address to `NettyClientConnection` to avoid using `channel.remoteAddress()`, which can return null on disconnect. We want have the address for logging and debugging purposes even after disconnect.

…pache#4394)

…pirationTime may unbox a null value and cause NPE (apache#4425)

zstan · 2024-09-23T14:51:09Z

@korlov42

I append tests into IgniteTypeSystemTest
Fix IgniteTypeSystem issue
But executions tests already implemented in IGNITE-23141 and just wait this issue to be resolved

korlov42

I append tests into IgniteTypeSystemTest

Fix IgniteTypeSystem issue

tests of IgniteTypeSystem was in p2. We need planner tests as well

korlov42 · 2024-09-24T07:37:09Z

modules/platforms/dotnet/Apache.Ignite.Tests/Sql/SqlTests.cs

@@ -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.00000m, res[0]);


can you explain why the result has changed?

Yes, i can, previously we call SqlFunctions#divide() and it implementation is a bit diff from current:
calcite : b0.divide(b1, MathContext.DECIMAL64)
current: x.divide(y, RoundingMode.HALF_DOWN)
i can change it like calcite but test "test_correlated_any_all.test" will fail in such a case and as for me this is not hack from my side, check: we create accumulator MinMaxAccumulator with RelDataType as a parameter, but also if scale of such a type is == 0 input data still can has some fractional data which brings errors

...c/integrationTest/java/org/apache/ignite/internal/runner/app/client/ItThinClientSqlTest.java

korlov42 · 2024-09-24T07:49:00Z

...l-engine/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItDataTypesTest.java

-                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


does it make sense to have similar test cases for all 4 types ?

derived type for SMALLINT and TINYINT tests without explicit casting is INTEGER so - no overflow will raised here

korlov42 · 2024-09-24T07:49:59Z

...l-engine/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItDataTypesTest.java

+
+    @ParameterizedTest
+    @MethodSource("decimalOpTypeExtension")
+    public void testCalcOpDynParamOverflow(String expr, String expect, Object param) {


why did you name this test 'overflow' when no overflow is expected?

korlov42 · 2024-09-24T07:51:48Z

...l-engine/src/integrationTest/java/org/apache/ignite/internal/sql/engine/ItDataTypesTest.java

+                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")


does it make sense to add test cases not only with cast from string?

other tests are moved into "cast_to_integer.test"

korlov42 · 2024-09-24T07:54:11Z

...es/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/type/IgniteTypeSystem.java

+                return typeFactory.createTypeWithNullability(
+                        typeFactory.createSqlType(
+                                SqlTypeName.DECIMAL,
+                                typeFactory.getTypeSystem().getMaxPrecision(SqlTypeName.DECIMAL),


does it make sense to use minimal yet sufficient precision rather then maximum possible value?

I change it and some tests will fail, for example "select1.test", if it not "MaxPrecision(SqlTypeName.DECIMAL)" results will return with non zero scale "177.00000" vs "177" and it not expected, i can`t found the problem for a shot time, probably we need additional issue here, wdyt ?

will return with non zero scale "177.00000" vs "177" and it not expected

why do you say "it's not expected"? it's literally what you've changed in this patch :) Before this patch chain of math operation results in a least restrictive type among operands, now result type is bumped every time. Given an expression like this (a+b+c+d+e)/5 (from the first failed statement from mentioned script), before this patch final operation was (int) / (int) which results in another int. Now it's (decimal) / (int), and to know resulting type you should check org.apache.calcite.rel.type.RelDataTypeSystem#deriveDecimalDivideType

...ql-engine/src/test/java/org/apache/ignite/internal/sql/engine/type/IgniteTypeSystemTest.java

zstan · 2024-09-25T06:07:55Z

@korlov42 planner tests extended, check NumericBinaryOperationsTypeCoercionTest -> mathResultMatcher

korlov42 · 2024-09-25T07:01:27Z

...ql-engine/src/test/java/org/apache/ignite/internal/sql/engine/type/IgniteTypeSystemTest.java

+                Arguments.of(NativeTypes.INT32, NativeTypes.INT64, NativeTypes.INT64),
+
+                Arguments.of(NativeTypes.INT64, NativeTypes.INT8,
+                        NativeTypes.decimalOf(typeSystem.getMaxPrecision(SqlTypeName.DECIMAL), 0)),


let's introduce constant for DECIMAL_MAX_0

korlov42 · 2024-09-25T07:02:28Z

...ql-engine/src/test/java/org/apache/ignite/internal/sql/engine/type/IgniteTypeSystemTest.java

+    }
+
+
+    private static Stream<Arguments> deriveMultTypeArguments() {


argument for multiplication and addition seems to be identical. Does it make sense leave only one set of arguments?

korlov42 · 2024-09-25T07:18:00Z

...es/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/type/IgniteTypeSystem.java

+                return typeFactory.createTypeWithNullability(
+                        typeFactory.createSqlType(
+                                SqlTypeName.DECIMAL,
+                                typeFactory.getTypeSystem().getMaxPrecision(SqlTypeName.DECIMAL),


will return with non zero scale "177.00000" vs "177" and it not expected

why do you say "it's not expected"? it's literally what you've changed in this patch :) Before this patch chain of math operation results in a least restrictive type among operands, now result type is bumped every time. Given an expression like this (a+b+c+d+e)/5 (from the first failed statement from mentioned script), before this patch final operation was (int) / (int) which results in another int. Now it's (decimal) / (int), and to know resulting type you should check org.apache.calcite.rel.type.RelDataTypeSystem#deriveDecimalDivideType

zstan added 2 commits September 19, 2024 13:45

IGNITE-23192 Sql. Arithmetic operations failed with "out of range" ex…

08ae199

…ception

fix tests

a218ef3

zstan force-pushed the ignite-23192 branch from e90a105 to a218ef3 Compare September 19, 2024 10:45

zstan added 8 commits September 19, 2024 14:28

fix

f083c47

fix

6f94308

fix

ec4fa7b

fix

9d11dec

fix

d8fcd85

fix

4d67f48

fix cs

3b04c93

fix

6b6118a

ygerzhedovich reviewed Sep 20, 2024

View reviewed changes

zstan commented Sep 20, 2024

View reviewed changes

ygerzhedovich reviewed Sep 20, 2024

View reviewed changes

modules/sql-engine/src/integrationTest/sql/types/integer/cast_to_integer.test Show resolved Hide resolved

fix after review

5e23b85

ygerzhedovich reviewed Sep 20, 2024

View reviewed changes

zstan added 2 commits September 20, 2024 14:23

mute cs test

420c596

mute cs test

7fe6e44

korlov42 requested changes Sep 23, 2024

View reviewed changes

ptupitsyn and others added 7 commits September 23, 2024 17:39

IGNITE-23133 Java thin: fix NPE in handshakeRes (apache#4423)

2d174e7

Pass the original address to `NettyClientConnection` to avoid using `channel.remoteAddress()`, which can return null on disconnect. We want have the address for logging and debugging purposes even after disconnect.

IGNITE-23181 Rename "affinity" to "partition distribution algorithm" (a…

0eb57e8

…pache#4394)

IGNITE-23229 Fixed StopLeaseProlongationMessageResponse#deniedLeaseEx…

a23fa0b

…pirationTime may unbox a null value and cause NPE (apache#4425)

disable cs test

84b46c9

fix cs style

f0144b1

fix cs style

d91c7ad

fix cs style

7f5d2b2

fix after review

eda56e4

zstan force-pushed the ignite-23192 branch from 6ff0424 to eda56e4 Compare September 23, 2024 14:41

Merge branch 'main' into ignite-23192

c375808

zstan added 2 commits September 24, 2024 08:53

fix

876b38a

fix

775e35f

korlov42 requested changes Sep 24, 2024

View reviewed changes

zstan added 3 commits September 24, 2024 15:56

divide tests

24a8014

rename test

42a42b5

fix

da50be3

korlov42 reviewed Sep 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IGNITE-23192 Sql. Arithmetic operations failed with "out of range" exception #4422

IGNITE-23192 Sql. Arithmetic operations failed with "out of range" exception #4422

zstan commented Sep 19, 2024

ygerzhedovich Sep 20, 2024

zstan Sep 20, 2024

ygerzhedovich Sep 20, 2024

zstan Sep 20, 2024

zstan Sep 20, 2024

ygerzhedovich Sep 20, 2024

ygerzhedovich Sep 20, 2024

zstan Sep 20, 2024

ygerzhedovich Sep 20, 2024

ygerzhedovich Sep 20, 2024

korlov42 left a comment

korlov42 Sep 23, 2024

zstan Sep 23, 2024

korlov42 Sep 23, 2024

zstan Sep 23, 2024

zstan commented Sep 23, 2024

korlov42 left a comment

korlov42 Sep 24, 2024

zstan Sep 24, 2024

korlov42 Sep 24, 2024

zstan Sep 24, 2024

korlov42 Sep 24, 2024

zstan Sep 24, 2024

korlov42 Sep 24, 2024

zstan Sep 24, 2024

korlov42 Sep 24, 2024

zstan Sep 24, 2024

korlov42 Sep 25, 2024

zstan commented Sep 25, 2024

korlov42 Sep 25, 2024

korlov42 Sep 25, 2024

korlov42 Sep 25, 2024

	assertEquals(2, rows.get(1).longValue(2));
	assertEquals(2L, rows.get(1).longValue(2));

		}


		private static Stream<Arguments> deriveMultTypeArguments() {

IGNITE-23192 Sql. Arithmetic operations failed with "out of range" exception #4422

Are you sure you want to change the base?

IGNITE-23192 Sql. Arithmetic operations failed with "out of range" exception #4422

Conversation

zstan commented Sep 19, 2024

The Review Checklist

Notes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

korlov42 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zstan commented Sep 23, 2024

korlov42 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zstan commented Sep 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment