[GitHub] [ignite] alex-plekhanov commented on a change in pull request #9148: IGNITE-14681 Calcite engine. Extend return type of sum() aggregate function

[GitBox <gi...@apache.org>]

alex-plekhanov commented on a change in pull request #9148:
URL: https://github.com/apache/ignite/pull/9148#discussion_r702695844



##########
File path: modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/agg/Accumulators.java
##########
@@ -93,32 +92,38 @@
     /** */
     private static Supplier<Accumulator> sumFactory(AggregateCall call) {
         switch (call.type.getSqlTypeName()) {
+            case INTEGER:
+            case BIGINT:
+            case DECIMAL:
+                return () -> new Sum(new DecimalSumEmptyIsZero());
+
             case DOUBLE:
             case REAL:
             case FLOAT:
-                return DoubleSum.FACTORY;
-            case DECIMAL:
-                return DecimalSum.FACTORY;
-            case INTEGER:
-                return IntSum.FACTORY;
-            case BIGINT:
+                return () -> new Sum(new DoubleSumEmptyIsZero());
+
+            case TINYINT:
+            case SMALLINT:
             default:
-                return LongSum.FACTORY;
+                return () -> new Sum(new LongSumEmptyIsZero());
         }
     }
 
     /** */
     private static Supplier<Accumulator> sumEmptyIsZeroFactory(AggregateCall call) {
         switch (call.type.getSqlTypeName()) {
+            case INTEGER:

Review comment:
       Do we really need DECIMAL for INTEGER sums? Perhaps BIGINT will be enough (postgree approach). 

##########
File path: modules/calcite/src/test/java/org/apache/ignite/testsuites/ScriptTestSuite.java
##########
@@ -36,6 +35,5 @@
  */
 @RunWith(ScriptTestRunner.class)
 @ScriptRunnerTestsEnvironment(scriptsRoot = "src/test/sql")
-@Ignore

Review comment:
       Unignoring this suite will cause Travis checks to fail, let's keep it ignored for a while, it doesn't prevent to run this suite manually.

##########
File path: modules/calcite/src/test/sql/aggregate/aggregates/test_aggr_string.test
##########
@@ -2,12 +2,6 @@
 # description: Test aggregations on strings
 # group: [aggregates]
 
-query TTTTI

Review comment:
       Why this test was removed? Looks like it works well.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org