zabetak commented on code in PR #3733:
URL: https://github.com/apache/calcite/pull/3733#discussion_r1575859391
##########
druid/src/test/java/org/apache/calcite/test/DruidAdapter2IT.java:
##########
@@ -1909,10 +1909,14 @@ private void checkGroupBySingleSortLimit(boolean
approx) {
+ "end as b from \"foodmart\" group by \"store_state\" order by a
desc";
final String postAggString =
"'postAggregations':[{'type':'expression','name':'A',"
+ "'expression':'(\\'$f1\\' /
\\'$f2\\')'},{'type':'expression','name':'B',"
- + "'expression':'case_searched((\\'$f3\\' == 0),1.0,CAST(\\'$f3\\'";
+ + "'expression':'case_searched((\\'$f3\\' == 0),1,CAST(\\'$f3\\'";
final String plan = "PLAN="
+ "EnumerableInterpreter\n"
- + " DruidQuery(table=[[foodmart, foodmart]],
intervals=[[1900-01-09T00:00:00.000Z/2992-01-10T00:00:00.000Z]],
projects=[[$63, $90, $91, $89]], groups=[{0}], aggs=[[SUM($1), SUM($2),
SUM($3)]], post_projects=[[$0, /($1, $2), CASE(=($3, 0), 1.0:DECIMAL(19, 0),
CAST($3):DECIMAL(19, 0))]], sort0=[1], dir0=[DESC])\n";
+ + " DruidQuery(table=[[foodmart, foodmart]],"
+ + " intervals=[[1900-01-09T00:00:00.000Z/2992-01-10T00:00:00.000Z]],"
+ + " projects=[[$63, $90, $91, $89]], groups=[{0}], aggs=[[SUM($1),
SUM($2), SUM($3)]],"
+ + " post_projects=[[$0, /($1, $2), CASE(=($3, 0), CAST(1:DECIMAL(19,
0)):DECIMAL(19, 0),"
Review Comment:
The new CAST appeared as part of the changes in this PR so even though it
may be a missing optimization in Druid it might have an impact on other Calcite
users if for some reasons CAST are added redundantly. It may also be
reproducible outside Druid but maybe we are missing the right test case.
##########
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;
Review Comment:
Before this PR was there a loss of precision in the results? Was the old
result of this query incorrect?
Does the same query in Oracle fail without adding the explicit CAST?
##########
core/src/main/java/org/apache/calcite/sql/type/SqlTypeUtil.java:
##########
@@ -1836,7 +1836,7 @@ public static boolean isValidDecimalValue(@Nullable
BigDecimal value, RelDataTyp
case DECIMAL:
final int intDigits = value.precision() - value.scale();
final int maxIntDigits = toType.getPrecision() - toType.getScale();
- return intDigits <= maxIntDigits;
+ return (intDigits <= maxIntDigits) && (value.scale() <=
toType.getScale());
Review Comment:
If we don't know what it does and why it exists better not modify it. Like
this we avoid accidental changes that are not directly related to this PR.
##########
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:
From a user perspective the old results for this query seem to be rather
fine. With the changes in this PR it seems that there will be an
`ArithmeticException` if there is no explicit `CAST` added by the user.
Moreover the extra `CAST` changes the results which doesn't seem desired.
Is the new behavior better than the old one and why?
--
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]
