[ https://issues.apache.org/jira/browse/FLINK-8903?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16401737#comment-16401737 ]
ASF GitHub Bot commented on FLINK-8903: --------------------------------------- Github user fhueske commented on a diff in the pull request: https://github.com/apache/flink/pull/5706#discussion_r175055646 --- Diff: flink-libraries/flink-table/src/main/java/org/apache/calcite/rel/rules/AggregateReduceFunctionsRule.java --- @@ -0,0 +1,590 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to you under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.calcite.rel.rules; + +import org.apache.calcite.plan.RelOptCluster; +import org.apache.calcite.plan.RelOptRule; +import org.apache.calcite.plan.RelOptRuleCall; +import org.apache.calcite.plan.RelOptRuleOperand; +import org.apache.calcite.rel.RelNode; +import org.apache.calcite.rel.core.Aggregate; +import org.apache.calcite.rel.core.AggregateCall; +import org.apache.calcite.rel.core.RelFactories; +import org.apache.calcite.rel.logical.LogicalAggregate; +import org.apache.calcite.rel.type.RelDataType; +import org.apache.calcite.rel.type.RelDataTypeFactory; +import org.apache.calcite.rel.type.RelDataTypeField; +import org.apache.calcite.rex.RexBuilder; +import org.apache.calcite.rex.RexLiteral; +import org.apache.calcite.rex.RexNode; +import org.apache.calcite.sql.SqlAggFunction; +import org.apache.calcite.sql.SqlKind; +import org.apache.calcite.sql.fun.SqlStdOperatorTable; +import org.apache.calcite.sql.type.SqlTypeUtil; +import org.apache.calcite.tools.RelBuilder; +import org.apache.calcite.tools.RelBuilderFactory; +import org.apache.calcite.util.CompositeList; +import org.apache.calcite.util.ImmutableIntList; +import org.apache.calcite.util.Util; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.Lists; +import com.google.common.collect.Maps; + +import java.math.BigDecimal; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Map; + +/* + * THIS FILE HAS BEEN COPIED FROM THE APACHE CALCITE PROJECT TO MAKE IT MORE EXTENSIBLE. --- End diff -- Yes, I agree. I would be much better to have this in code in Calcite. However, the changes are very Flink specific (we need to add a few fields to the projection). OTOH its just moving some code in a protected function, so no change in functionality and only few lines touched. I'll create a JIRA in Calcite and reference the issue. In case, Calcite does not want the change, we can keep the class in Flink. > Built-in agg functions VAR_POP, VAR_SAMP, STDEV_POP, STDEV_SAMP are broken in > Group Windows > ------------------------------------------------------------------------------------------- > > Key: FLINK-8903 > URL: https://issues.apache.org/jira/browse/FLINK-8903 > Project: Flink > Issue Type: Bug > Components: Table API & SQL > Affects Versions: 1.3.2, 1.5.0, 1.4.2 > Reporter: lilizhao > Assignee: Fabian Hueske > Priority: Critical > Fix For: 1.5.0 > > Attachments: QQ图片20180312180143.jpg, TableAndSQLTest.java > > > The built-in aggregation functions VAR_POP, VAR_SAMP, STDEV_POP, STDEV_SAMP > are translated into regular AVG functions if they are applied in the context > of a Group Window aggregation (\{{GROUP BY TUMBLE/HOP/SESSION}}). > The reason is that these functions are internally represented as > {{SqlAvgAggFunction}} but with different {{SqlKind}}. When translating > Calcite aggregation functions to Flink Table agg functions, we only look at > the type of the class, not at the value of the {{kind}} field. We did not > notice that before, because in all other cases (regular {{GROUP BY}} without > windows or {{OVER}} windows, we have a translation rule > {{AggregateReduceFunctionsRule}} that decomposes the more complex functions > into expressions of {{COUNT}} and {{SUM}} functions such that we never > execute an {{AVG}} Flink function. That rule can only be applied on > {{LogicalAggregate}}, however, we represent group windows as > {{LogicalWindowAggregate}}, so the rule does not match. > We should fix this by: > 1. restrict the translation to Flink avg functions in {{AggregateUtil}} to > {{SqlKind.AVG}}. > 2. implement a rule (hopefully based on {{AggregateReduceFunctionsRule}}) > that decomposes the complex agg functions into the {{SUM}} and {{COUNT}}. > Step 1. is easy and a quick fix but we would get an exception "Unsupported > Function" if {{VAR_POP}} is used in a {{GROUP BY}} window. > Step 2. might be more involved, depending on how difficult it is to port the > rule. > -- This message was sent by Atlassian JIRA (v7.6.3#76005)