[
https://issues.apache.org/jira/browse/FLINK-12249?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16887834#comment-16887834
]
godfrey he edited comment on FLINK-12249 at 7/19/19 1:23 AM:
-------------------------------------------------------------
there is another big issue: is {{WindowAggregate}} inherited from {{Aggregate}}
correct? My answer is NO.
for {{WindowAggregate}}, the group keys are window group and normal fields (may
be empty), while {{Aggregate}} only has normal group keys part, and know
nothing about window group key. currently, many planner rules match and apply
transformation on {{Aggregate}}, however some of them does not applicable to
{{WindowAggregate}}, e.g. {{AggregateJoinTransposeRule}},
{{AggregateProjectMergeRule}}, etc. I think the design violates the Liskov
Substitution Principle.
there are three solutions:
1. make {{Aggregate}}'s group key supports expressions(such as RexCall), not
field reference only. and then the window group expression could be as a part
of {{Aggregate}}'s group key. the disadvantage is we must update all existing
aggregate rules, metadata handlers, etc.
2. make {{WindowAggregate}} extends from {{SingleRel}}, not from {{Aggregate}}.
the disadvantage is we must implement related planner rules about
WindowAggregate.
3. in logical phase, we does not merge {{Aggregate}} and {{Project}} (with
window group) into {{WindowAggregate}}, and convert the {{Project}} to a new
kind of node named {{WindowAssigner}}, which could prevent {{Project}} from
being pushed down/merged. and in physical phase, we merge them into
{{WindowAggregate}}. the advantage is we could reuse current aggregate rules,
and the disadvantage is we should add new rules about {{WindowAssigner}}.
i think solution3 is a more easier approach, which could make sure all rules
are correct.
if this refactor is finished, i think the above bug is fixed too.
thank~
was (Author: godfreyhe):
there is another big issue: is {{WindowAggregate}} inherited from {{Aggregate}}
correct? My answer is NO.
for {{WindowAggregate}}, the group keys are window group and normal fields (may
be empty), while {{Aggregate}} only has normal group keys part, and know
nothing about window group key. currently, many planner rules match and apply
transformation on {{Aggregate}}, however some of them does not applicable to
{{WindowAggregate}}, e.g. {{AggregateJoinTransposeRule}},
{{AggregateProjectMergeRule}}, etc. I think the design violates the Liskov
Substitution Principle.
there are two solutions:
1. make {{Aggregate}}'s group key supports expressions(such as RexCall), not
field reference only. and then the window group expression could be as a part
of {{Aggregate}}'s group key. the disadvantage is we must update all existing
aggregate rules, metadata handlers, etc.
2. make {{WindowAggregate}} extends from {{SingleRel}}, not from {{Aggregate}}.
the disadvantage is we must implement related planner rules about
WindowAggregate.
3. in logical phase, we does not merge {{Aggregate}} and {{Project}} (with
window group) into {{WindowAggregate}}, and convert the {{Project}} to a new
kind of node named {{WindowAssigner}}, which could prevent {{Project}} from
being pushed down/merged. and in physical phase, we merge them into
{{WindowAggregate}}. the advantage is we could reuse current aggregate rules,
and the disadvantage is we should add new rules about {{WindowAssigner}}.
i think solution3 is a more easier approach, which could make sure all rules
are correct.
if this refactor is finished, i think the above bug is fixed too.
thank~
> Type equivalence check fails for Window Aggregates
> --------------------------------------------------
>
> Key: FLINK-12249
> URL: https://issues.apache.org/jira/browse/FLINK-12249
> Project: Flink
> Issue Type: Bug
> Components: Table SQL / Legacy Planner, Tests
> Affects Versions: 1.9.0
> Reporter: Dawid Wysakowicz
> Assignee: Hequn Cheng
> Priority: Critical
> Fix For: 1.9.0
>
>
> Creating Aggregate node fails in rules: {{LogicalWindowAggregateRule}} and
> {{ExtendedAggregateExtractProjectRule}} if the only grouping expression is a
> window and
> we compute aggregation on NON NULLABLE field.
> The root cause for that, is how return type inference strategies in calcite
> work and how we handle window aggregates. Take
> {{org.apache.calcite.sql.type.ReturnTypes#AGG_SUM}} as an example, based on
> {{groupCount}} it adjusts type nullability based on groupCount.
> Though we pass a false information as we strip down window aggregation from
> groupSet (in {{LogicalWindowAggregateRule}}).
> One can reproduce this problem also with a unit test like this:
> {code}
> @Test
> def testTumbleFunction2() = {
>
> val innerQuery =
> """
> |SELECT
> | CASE a WHEN 1 THEN 1 ELSE 99 END AS correct,
> | rowtime
> |FROM MyTable
> """.stripMargin
> val sql =
> "SELECT " +
> " SUM(correct) as cnt, " +
> " TUMBLE_START(rowtime, INTERVAL '15' MINUTE) as wStart " +
> s"FROM ($innerQuery) " +
> "GROUP BY TUMBLE(rowtime, INTERVAL '15' MINUTE)"
> val expected = ""
> streamUtil.verifySql(sql, expected)
> }
> {code}
> This causes e2e tests to fail:
> https://travis-ci.org/apache/flink/builds/521183361?utm_source=slack&utm_medium=notificationhttps://travis-ci.org/apache/flink/builds/521183361?utm_source=slack&utm_medium=notification
--
This message was sent by Atlassian JIRA
(v7.6.14#76016)
