[GitHub] [flink] lsyldliu commented on a diff in pull request #22734: [FLINK-32277][table-runtime] Introduce the basic operator fusion codegen framework

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

lsyldliu commented on code in PR #22734:
URL: https://github.com/apache/flink/pull/22734#discussion_r1249314613


##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/GeneratedExpression.scala:
##########
@@ -39,7 +39,7 @@ import org.apache.flink.table.types.logical.LogicalType
 case class GeneratedExpression(
     resultTerm: String,
     nullTerm: String,
-    code: String,
+    var code: String,

Review Comment:
   I'm introducing a little more background, there are two ways of implementation here, the first is to keep the existing way, each time copy out a new  `GeneratedExpression` object, but there is a problem with using this way, that is, after copying out a new `GeneratedExpression`,  all have to call `ctx.addReusableInputUnboxingExprs` method to overwrite this expression in ctx cache, so that later in generating the relevant condition code which use this field, we can get the corresponding expression code correctly and avoid duplicate evaluation. For example
   ```
     def evaluateVariables(
         varExprs: Seq[GeneratedExpression],
         operatorCtx: CodeGeneratorContext,
         inputRowTerm: String): (String, Seq[GeneratedExpression]) = {
       val evaluate = varExprs.filter(_.code.nonEmpty).map(_.code).mkString("\n")
       val copiedVars = mutable.ArrayBuffer[GeneratedExpression]()
   
       varExprs.zipWithIndex.foreach { case (expr, i) =>
         val copyExpr = expr.copy(NO_CODE)
         operatorCtx.addReusableInputUnboxingExprs(inputRowTerm, i, copyExpr)
         copiedVars += copyExpr
       }
       (evaluate, copiedVars)
     }
   ```
   The second way is to set the code to var. The advantage of this is that you can change it at will without having to re-bind it after the change, which I borrowed from Spark, and I prefer this implementation. 
   ```
     def evaluateVariables(varExprs: Seq[GeneratedExpression]): String = {
       val evaluate = varExprs.filter(_.code.nonEmpty).map(_.code).mkString("\n")
       varExprs.foreach(_.code = NO_CODE)
       evaluate
     }
   ```
   
   For these two ways, what do you think?



-- 
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