[CALCITE-6020] SqlToRelConverter should not replace windowed SUM with… #3882
Conversation
|
This is supposedly fixing a bug. No test for that bug? |
| @@ -1475,6 +1475,16 @@ protected AggCall aggregateCall(SqlAggFunction aggFunction, boolean distinct, | |||
| filter, alias, preOperands, operands, distinctKeys, orderKeys); | |||
| } | |||
|
|
|||
| public SqlAggFunction windowedAggregateOperator(SqlAggFunction aggFunction, | |||
There was a problem hiding this comment.
shouldn't this be a SqlToRelConverter config option?
| @@ -6154,7 +6150,7 @@ private class HistogramShuttle extends RexShuttle { | |||
| : c.rangeBetween(lowerBound, upperBound)) | |||
| .exclude(exclude) | |||
| .allowPartial(allowPartial) | |||
| .nullWhenCountZero(needSum0) | |||
| .nullWhenCountZero(aggOp == SqlStdOperatorTable.SUM && type.isNullable()) | |||
There was a problem hiding this comment.
wouldn't this will still add the count stuff if the actual relBuilder.windowedAggregateOperator decided to not override it?
|
I've reworked this PR. Now the conversion of SUM to SUM0 happens in a new RelRule. The advantage is that a user can choose whether to include the new RelRule or not. |
| @@ -41,16 +43,17 @@ public class RexShuttle implements RexVisitor<RexNode> { | |||
| @Override public RexNode visitOver(RexOver over) { | |||
| boolean[] update = {false}; | |||
| List<RexNode> clonedOperands = visitList(over.operands, update); | |||
| SqlAggFunction op = visitOverAggFunction(over.getAggOperator()); | |||
There was a problem hiding this comment.
A more suggestive name for the variable would be overAggregator
| @@ -5790,7 +5790,7 @@ window w1 as (partition by job order by hiredate rows 2 preceding)]]> | |||
| </Resource> | |||
| <Resource name="plan"> | |||
| <![CDATA[ | |||
| LogicalProject(EXPR$0=[CASE(>(COUNT($5) OVER (PARTITION BY $2 ORDER BY $4 ROWS 2 PRECEDING), 0), $SUM0($5) OVER (PARTITION BY $2 ORDER BY $4 ROWS 2 PRECEDING), null:INTEGER)], EXPR$1=[/(CASE(>(COUNT(CAST($5):REAL NOT NULL) OVER (PARTITION BY $2 ORDER BY $4 ROWS 2 PRECEDING), 0), $SUM0(CAST($5):REAL NOT NULL) OVER (PARTITION BY $2 ORDER BY $4 ROWS 2 PRECEDING), null:REAL), COUNT(CAST($5):REAL NOT NULL) OVER (PARTITION BY $2 ORDER BY $4 ROWS 2 PRECEDING))]) | |||
| LogicalProject(EXPR$0=[CASE(>(COUNT($5) OVER (PARTITION BY $2 ORDER BY $4 ROWS 2 PRECEDING), 0), SUM($5) OVER (PARTITION BY $2 ORDER BY $4 ROWS 2 PRECEDING), null:INTEGER)], EXPR$1=[/(CASE(>(COUNT(CAST($5):REAL NOT NULL) OVER (PARTITION BY $2 ORDER BY $4 ROWS 2 PRECEDING), 0), SUM(CAST($5):REAL NOT NULL) OVER (PARTITION BY $2 ORDER BY $4 ROWS 2 PRECEDING), null:REAL), COUNT(CAST($5):REAL NOT NULL) OVER (PARTITION BY $2 ORDER BY $4 ROWS 2 PRECEDING))]) | |||
There was a problem hiding this comment.
these look like regressions to me. Do you expect this?
There was a problem hiding this comment.
Or maybe this is actually addressing the actual issue: you are saying that theses were incorrect to start with?
There was a problem hiding this comment.
This is sort of a regression. These tests only convert the SQL to Rel, but do not go through planning the query. As a result, they do not run the RelRules, so SUM is not converted to SUM0. There are other tests (in RelOptRulesTest) that also go through planning and verify that SUM becomes SUM0.
|
|
||
| /** | ||
| * Planner rule that converts SUM to SUM0 when it is used in an OVER clause inside | ||
| * the project list. |
There was a problem hiding this comment.
can you add an argument why this is always safe?
There was a problem hiding this comment.
I expanded on the Javadoc here to explain why this rule should be used. Let me know if more explanation is needed.
mihaibudiu
added
the
LGTM-will-merge-soon
A new RelRule was created that looks for OVER in a the project list. If the OVER contains SUM in the aggregate, it is changed to SUM0. The conversion now happens during planning rather than when SQL is converted to Rel.
normanj-bitquill
force-pushed
the
calcite-6020
branch
from
f58828a to
0e64feb
Compare
|
|
The commits have been squashed. |
… equivalent expression using SUM0