[GitHub] [flink] cshuo commented on a change in pull request #17634: [FLINK-24691][table-common] Fix precision for sum on decimal types

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

cshuo commented on a change in pull request #17634:
URL: https://github.com/apache/flink/pull/17634#discussion_r743368304



##########
File path: flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/functions/aggfunctions/SumAggFunction.java
##########
@@ -66,7 +66,10 @@ public int operandCount() {
                     ifThenElse(
                             isNull(operand(0)),
                             sum,
-                            ifThenElse(isNull(sum), operand(0), plus(sum, operand(0)))))
+                            ifThenElse(

Review comment:
       This fix probably does not work. You can check by the following test case, 
   ```
   @Test
     def testDecimalSum(): Unit = {
       val data = new mutable.MutableList[(Double, Int)]
       data .+= ((1.11111111, 1))
       data .+= ((1.11111111, 2))
   
       env.setParallelism(1)
       val t = failingDataSource(data).toTable(tEnv, 'a, 'b)
       tEnv.registerTable("T", t)
   
       val t1 = tEnv.sqlQuery("select sum(cast(a as decimal(32, 8))) from T")
       val sink = new TestingRetractSink
       t1.toRetractStream[Row].addSink(sink)
       env.execute()
       val expected = List("2.22222220")
       assertEquals(expected, sink.getRetractResults)
     }
   ```
   As shown above, the result is also finally rounded to (38, 7), then cast to (38, 8) which also have the precision losing problem. 
   This is because that during codegen of DeclarativeAggregateFunction, 'plus' expression is convert to RexNode `PLUS` firstly, and during the converting there also exists type inference(..ReturnTypes.NULLABLE_SUM) which infers the return type of 'PLUS' as decimal(38, 7)...
   
   So, maybe we should introduce an internal built-in SqlFunction, e.g, 'SUM_PLUS' for SUM, which has a different return type inference (maybe ReturnTypes.LEAST_RESTRICTIVE is enough), and infers return type of SUM just same as the type of operand.
   
   Besides, there is same problem for AVG.




-- 
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: issues-unsubscribe@flink.apache.org

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