godfreyhe commented on a change in pull request #15498:
URL: https://github.com/apache/flink/pull/15498#discussion_r610462034
##########
File path:
flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/plan/rules/physical/batch/BatchPhysicalOverAggregateRule.scala
##########
@@ -190,7 +190,8 @@ class BatchPhysicalOverAggregateRule
val inputNameList = inputType.getFieldNames
val inputTypeList = inputType.getFieldList.asScala.map(field =>
field.getType)
- val aggNames = aggCalls.map(_.getName)
+ // we should avoid duplicated names with input column names
+ val aggNames = aggCalls.map(c => RowTypeUtils.getUniqueName(c.getName,
inputNameList))
Review comment:
what if there are three duplicate names: "a, a, a", the result will be
"a, a0, a0".
the new unique name should be added to inputNameList.
please add an UT for this class
##########
File path:
flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/plan/rules/physical/batch/BatchPhysicalOverAggregateRule.scala
##########
@@ -190,7 +190,8 @@ class BatchPhysicalOverAggregateRule
val inputNameList = inputType.getFieldNames
val inputTypeList = inputType.getFieldList.asScala.map(field =>
field.getType)
- val aggNames = aggCalls.map(_.getName)
+ // we should avoid duplicated names with input column names
+ val aggNames = aggCalls.map(c => RowTypeUtils.getUniqueName(c.getName,
inputNameList))
Review comment:
please add a plan test for this change
##########
File path:
flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/plan/rules/physical/batch/BatchPhysicalOverAggregateRule.scala
##########
@@ -26,14 +26,14 @@ import
org.apache.flink.table.planner.plan.nodes.logical.FlinkLogicalOverAggrega
import
org.apache.flink.table.planner.plan.nodes.physical.batch.{BatchPhysicalOverAggregate,
BatchPhysicalOverAggregateBase, BatchPhysicalPythonOverAggregate}
import org.apache.flink.table.planner.plan.utils.PythonUtil.isPythonAggregate
import org.apache.flink.table.planner.plan.utils.{AggregateUtil,
OverAggregateUtil, SortUtil}
-
import org.apache.calcite.plan.RelOptRule._
import org.apache.calcite.plan.{RelOptCluster, RelOptRule, RelOptRuleCall}
import org.apache.calcite.rel._
import org.apache.calcite.rel.`type`.RelDataType
import org.apache.calcite.rel.core.Window.Group
import org.apache.calcite.rel.core.{AggregateCall, Window}
import org.apache.calcite.tools.ValidationException
+import org.apache.flink.table.planner.typeutils.RowTypeUtils
Review comment:
nit: reorder the imports
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
