mihaibudiu commented on code in PR #3733: URL: https://github.com/apache/calcite/pull/3733#discussion_r1576685078
########## babel/src/test/resources/sql/redshift.iq: ########## @@ -523,78 +523,78 @@ select deptno, ratio_to_report(sal) over (partition by deptno) from emp; !} # STDDEV_POP -select empno, stddev_pop(comm) over (order by empno rows unbounded preceding) from emp where deptno = 30 order by 1; +select empno, stddev_pop(CAST(comm AS DECIMAL(12, 4))) over (order by empno rows unbounded preceding) from emp where deptno = 30 order by 1; EMPNO, EXPR$1 -7499, 0 -7521, 100 -7654, 478.42333648024424519462627358734607696533203125 -7698, 478.42333648024424519462627358734607696533203125 -7844, 522.0153254455275373402400873601436614990234375 -7900, 522.0153254455275373402400873601436614990234375 +7499, 0.0000 +7521, 100.0000 +7654, 478.4233 +7698, 478.4233 +7844, 522.0153 +7900, 522.0153 !ok # STDDEV_SAMP (synonym for STDDEV) -select empno, stddev_samp(comm) over (order by empno rows unbounded preceding) from emp where deptno = 30 order by 1; +select empno, stddev_samp(CAST(comm AS DECIMAL(12, 4))) over (order by empno rows unbounded preceding) from emp where deptno = 30 order by 1; EMPNO, EXPR$1 7499, null -7521, 141.421356237309510106570087373256683349609375 -7654, 585.9465277082316561063635163009166717529296875 -7698, 585.9465277082316561063635163009166717529296875 -7844, 602.7713773341707792496890760958194732666015625 -7900, 602.7713773341707792496890760958194732666015625 +7521, 141.4213 +7654, 585.9465 +7698, 585.9465 +7844, 602.7713 +7900, 602.7713 !ok -select empno, stddev(comm) over (order by empno rows unbounded preceding) from emp where deptno = 30 order by 1; +select empno, stddev(CAST(comm AS DECIMAL(12, 4))) over (order by empno rows unbounded preceding) from emp where deptno = 30 order by 1; EMPNO, EXPR$1 7499, null -7521, 141.421356237309510106570087373256683349609375 -7654, 585.9465277082316561063635163009166717529296875 -7698, 585.9465277082316561063635163009166717529296875 -7844, 602.7713773341707792496890760958194732666015625 -7900, 602.7713773341707792496890760958194732666015625 +7521, 141.4213 +7654, 585.9465 +7698, 585.9465 +7844, 602.7713 +7900, 602.7713 !ok # SUM -select empno, sum(comm) over (order by empno rows unbounded preceding) from emp where deptno = 30 order by 1; +select empno, sum(CAST(comm AS DECIMAL(12, 4))) over (order by empno rows unbounded preceding) from emp where deptno = 30 order by 1; EMPNO, EXPR$1 -7499, 300.00 -7521, 800.00 -7654, 2200.00 -7698, 2200.00 -7844, 2200.00 -7900, 2200.00 +7499, 300.0000 +7521, 800.0000 +7654, 2200.0000 +7698, 2200.0000 +7844, 2200.0000 +7900, 2200.0000 Review Comment: If I use `(CAST(comm as DECIMAL(12, 2))` the results will be unchanged. I will make that change to make the diff smaller. The implementation of casts to decimal in Calcite treats them as noops, which is clearly wrong. This bug was masking many other potential problems, which now will be apparent. For example, aggregation results were also incorrectly computed. There is no clear spec in Calcite how aggregation results are computed (i.e., the precision of intermediate results, or the precision of the final results). SQL dialects differ in this respect, and the standard does not mandate a specific implementation. The current Calcite implementation is a reasonable one, but it won't work for this example without the manually-inserted cast, because overflows in decimal computations will cause the computation to fail. If we think that the aggregation implementation in Calcite is wrong, we should file a separate issue. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
