[GitHub] [flink] wuchong commented on a diff in pull request #22990: [FLINK-32280][table-runtime] HashAgg support operator fusion codegen

["wuchong (via GitHub)" <gi...@apache.org>]

wuchong commented on code in PR #22990:
URL: https://github.com/apache/flink/pull/22990#discussion_r1267669642


##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/agg/batch/AggCodeGenHelper.scala:
##########
@@ -45,11 +45,16 @@ import scala.annotation.tailrec
 /** Batch aggregate code generate helper. */
 object AggCodeGenHelper {
 
+  /**
+   * The aggBufferPrefix is used to guarantee the variable name unique in OFCG case, so user should
+   * guarantee the aggBufferPrefix unique in this case.
+   */
   def getAggBufferNames(
+      aggBufferPrefix: String,

Review Comment:
   From my point of view, in terms of code, this uniqueness is very hard to guarantee (e.g. fusion 2 hash agg into fusion operator). Can we just use `newName(..)` to generate a unique one?



##########
flink-table/flink-table-planner/src/test/resources/org/apache/flink/table/planner/plan/batch/sql/join/ShuffledHashJoinTest.xml:
##########
@@ -129,14 +129,13 @@ LogicalProject(c=[$7], g=[$3])
     </Resource>
     <Resource name="optimized exec plan">
       <![CDATA[
-Calc(select=[c, g])
-+- HashJoin(joinType=[InnerJoin], where=[(a = d)], select=[d, g, a, c], build=[right])
-   :- Exchange(distribution=[hash[d]])
-   :  +- Calc(select=[d, g], where=[(d < 2)])
-   :     +- LegacyTableSourceScan(table=[[default_catalog, default_database, MyTable2, source: [TestTableSource(d, e, f, g, h)]]], fields=[d, e, f, g, h])
-   +- Exchange(distribution=[hash[a]])
-      +- Calc(select=[a, c], where=[(a < 2)])
-         +- LegacyTableSourceScan(table=[[default_catalog, default_database, MyTable1, source: [TestTableSource(a, b, c)]]], fields=[a, b, c])
+MultipleInput(readOrder=[1,0], members=[\nCalc(select=[c, g])\n+- HashJoin(joinType=[InnerJoin], where=[(a = d)], select=[d, g, a, c], build=[right])\n   :- [#1] Exchange(distribution=[hash[d]])\n   +- [#2] Exchange(distribution=[hash[a]])\n])
+:- Exchange(distribution=[hash[d]])
+:  +- Calc(select=[d, g], where=[(d < 2)])

Review Comment:
   Isn't `table.exec.operator-fusion-codegen.enabled` disabled by default? Why so many plans are still effected?



-- 
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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org