libenchao commented on code in PR #23984:
URL: https://github.com/apache/flink/pull/23984#discussion_r1439188114
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/CodeGenUtils.scala:
##########
@@ -124,14 +124,27 @@ object CodeGenUtils {
private val nameCounter = new AtomicLong
- def newName(name: String): String = {
- s"$name$$${nameCounter.getAndIncrement}"
+ def newName(context: CodeGeneratorContext = null, name: String): String = {
+ if (context == null || context.getNameCounter == null) {
+ // Add an 'i' in the middle to distinguish from nameCounter in
CodeGeneratorContext
+ // and avoid naming conflicts.
+ s"$name$$i${nameCounter.getAndIncrement}"
+ } else {
+ s"$name$$${context.getNameCounter.getAndIncrement}"
+ }
}
- def newNames(names: String*): Seq[String] = {
+ def newNames(context: CodeGeneratorContext, names: String*): Seq[String] = {
require(names.toSet.size == names.length, "Duplicated names")
- val newId = nameCounter.getAndIncrement
- names.map(name => s"$name$$$newId")
+ if (context == null || context.getNameCounter == null) {
Review Comment:
I'm wondering that if `newNames` can be optimized to something like
`names.map(name => newName(context, name))`?
##########
flink-table/flink-table-planner/src/test/scala/org/apache/flink/table/planner/codegen/CodeGenUtilsTest.scala:
##########
@@ -0,0 +1,53 @@
+/*
+ * 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.flink.table.planner.codegen
+
+import org.apache.flink.configuration.Configuration
+
+import org.junit.jupiter.api.Assertions.assertEquals
+import org.junit.jupiter.api.Test
+
+import scala.collection.mutable.ArrayBuffer
+
+class CodeGenUtilsTest {
+ private val classLoader = Thread.currentThread().getContextClassLoader
+
+ @Test
+ def testNewName(): Unit = {
Review Comment:
Could you also add a test for `parentCtx`?
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/CodeGeneratorContext.scala:
##########
@@ -37,15 +37,28 @@ import org.apache.flink.util.InstantiationUtil
import java.time.ZoneId
import java.util.TimeZone
+import java.util.concurrent.atomic.AtomicLong
import java.util.function.{Supplier => JSupplier}
import scala.collection.mutable
/**
* The context for code generator, maintaining various reusable statements
that could be insert into
* different code sections in the final generated class.
+ *
+ * Caution: As we use nameCounter in each CodeGeneratorContext, we must ensure
that a unique
+ * CodeGeneratorContext(or contexts share the same ancestors) is used
throughout the entire class to
+ * avoid naming conflicts. So when we create a context for a class, we can set
parentCtx to null.
+ * However, when we create a context for a code block, we must ensure that all
contexts for code
+ * blocks in a class share a common ancestor by setting parentCtx.
*/
-class CodeGeneratorContext(val tableConfig: ReadableConfig, val classLoader:
ClassLoader) {
+class CodeGeneratorContext(
+ val tableConfig: ReadableConfig,
+ val classLoader: ClassLoader,
+ parentCtx: CodeGeneratorContext) {
Review Comment:
Can this be `final` too?
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/CodeGenUtils.scala:
##########
@@ -124,14 +124,27 @@ object CodeGenUtils {
private val nameCounter = new AtomicLong
- def newName(name: String): String = {
- s"$name$$${nameCounter.getAndIncrement}"
+ def newName(context: CodeGeneratorContext = null, name: String): String = {
Review Comment:
This one has a default value for `context`, but `newNames` does not have, is
there any rational behind this?
--
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
