[FLINK-33792] Generate the same code for the same logic #23984

zoudan · 2023-12-22T07:39:18Z

What is the purpose of the change

This pull request ensure that we generate the same code for the same logic, it is a precondition for sharing generated classes between different jobs.

Brief change log

add a name counter in each CodeGeneratorContext and use it when we generate names for variables.

Verifying this change

Please make sure both new and modified tests in this PR follows the conventions defined in our code quality guide: https://flink.apache.org/contributing/code-style-and-quality-common.html#testing

This change added tests and can be verified as follows:

Add a new test: CodeGenUtilsTest#testNewName
Add tests in existing class: HashCodeGeneratorTest#testHashWithIndependentNameCounter and ProjectionCodeGeneratorTest#testHashWithIndependentNameCounter

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): (no)
The public API, i.e., is any changed class annotated with @Public(Evolving): (no)
The serializers: (no)
The runtime per-record code paths (performance sensitive): (no)
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (no)
The S3 file system connector: (no)

Documentation

Does this pull request introduce a new feature? (yes)
If yes, how is the feature documented? (docs)

flinkbot · 2023-12-22T07:46:50Z

CI report:

5424149 Azure: SUCCESS

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run azure re-run the last Azure build

libenchao

@zoudan Thanks for the PR, it looks great. My main point is that we don't need to add a config for it, it can be done by default.

libenchao · 2023-12-22T08:40:45Z

...flink-table-api-java/src/main/java/org/apache/flink/table/api/config/TableConfigOptions.java

@@ -233,6 +233,16 @@ private TableConfigOptions() {}
                    .withDescription(
                            "Specifies a threshold where class members of generated code will be grouped into arrays by types.");

+    @Documentation.TableOption(execMode = Documentation.ExecMode.BATCH_STREAMING)
+    public static final ConfigOption<Boolean> INDEPENDENT_NAME_COUNTER_ENABLED =


Why do we need this config, I think it's ok to change the behavior by default, it's no meaning to expose it to users.

libenchao · 2023-12-22T09:00:57Z

...-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/HashCodeGenerator.scala

@@ -80,23 +80,25 @@ object HashCodeGenerator {
      }
    """.stripMargin

+    System.out.println(code)


zoudan · 2023-12-25T02:40:43Z

@libenchao Thanks for reviewing, I have remove the configuration, please take a look when you have time.

libenchao · 2023-12-25T09:37:34Z

@zoudan The PR looks good to me, except that the CI is failing, could you fix that?

zoudan · 2023-12-28T09:20:28Z

@flinkbot run azure

zoudan · 2023-12-29T02:01:47Z

@flinkbot run azure

zoudan · 2023-12-29T08:19:20Z

@libenchao CI is passed, please take a look when you have time.

libenchao

@zoudan Thanks for the updating, using 'parent context' is a great idea. I'll left a few minor comments, others looks good to me.

...ble-planner/src/main/scala/org/apache/flink/table/planner/codegen/CodeGeneratorContext.scala

libenchao · 2024-01-02T06:46:35Z

...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 = {


This one has a default value for context, but newNames does not have, is there any rational behind this?

Parameter section with *-parameter is not allowed to have default arguments

My point is that do you really need to have a default value, IIUC, we want all places to pass context?

Get it, removed

libenchao · 2024-01-02T06:48:44Z

...flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/CodeGenUtils.scala

    require(names.toSet.size == names.length, "Duplicated names")
-    val newId = nameCounter.getAndIncrement
-    names.map(name => s"$name$$$newId")
+    if (context == null || context.getNameCounter == null) {


I'm wondering that if newNames can be optimized to something like names.map(name => newName(context, name))?

The returned new names use the same index, I keep it the same as before. So, I did not call newName

...k-table-planner/src/test/scala/org/apache/flink/table/planner/codegen/CodeGenUtilsTest.scala

lsyldliu

@zoudan Thanks for contribution, I will take a look in the next few days.

zoudan · 2024-01-02T11:52:34Z

@flinkbot run azure

zoudan · 2024-01-03T02:11:26Z

@flinkbot run azure

libenchao

+1 from my side

lsyldliu

@zoudan Thanks for contribution, LGTM overall, I just left some minor comments.

lsyldliu · 2024-01-08T12:35:04Z

...le-planner/src/main/scala/org/apache/flink/table/planner/codegen/LongHashJoinGenerator.scala

@@ -87,10 +90,11 @@ object LongHashJoinGenerator {
  def genProjection(
      tableConfig: ReadableConfig,
      classLoader: ClassLoader,
-      types: Array[LogicalType]): GeneratedProjection = {
+      types: Array[LogicalType],
+      parentCtx: CodeGeneratorContext): GeneratedProjection = {


We can simplify the method signature to

def genProjection( types: Array[LogicalType], parentCtx: CodeGeneratorContext): GeneratedProjection

Sorry, I do not get your point, I think tableConfig and classLoader are needed in this method. And I only add the parameter parentCtx without change anything else.

I just mean that we can get the tableConfig and classLoader from parentCtx, but this isn't an important point, ignore it now.

lsyldliu · 2024-01-08T12:56:07Z

...flink-table-planner/src/main/scala/org/apache/flink/table/planner/codegen/CodeGenUtils.scala

-    names.map(name => s"$name$$$newId")
+    if (context == null || context.getNameCounter == null) {
+      val newId = nameCounter.getAndIncrement
+      // Add an 'i' in the middle to distinguish from nameCounter in CodeGeneratorContext


Why here will have conflicts?

There may be some scenarios where it is not convenient for us to obtain class level CodeGeneratorContext, and we could use the nameCounter in CodeGenUtils to generate new names. In these cases we may use nameCounter from CodeGenUtils and CodeGeneratorContext in the same class.

lsyldliu · 2024-01-08T13:04:35Z

...k-table-planner/src/main/java/org/apache/flink/table/planner/functions/casting/CastRule.java

+                @Override
+                @Nullable
+                public CodeGeneratorContext getCodeGeneratorContext() {
+                    return null;


Can you add some annotation to explain why here returns null?

Change it to non null

zoudan · 2024-01-09T03:26:05Z

@flinkbot run azure

zoudan · 2024-01-09T09:42:33Z

@flinkbot run azure

zoudan · 2024-01-09T11:29:14Z

@flinkbot run azure

zoudan · 2024-01-10T02:20:43Z

@flinkbot run azure

zoudan · 2024-01-11T11:37:18Z

@flinkbot run azure

zoudan · 2024-01-11T11:45:44Z

@flinkbot run azure

zoudan · 2024-01-12T02:07:31Z

@flinkbot run azure

zoudan · 2024-01-12T03:04:37Z

@lsyldliu I have updated my code, please have a took when you have time.

lsyldliu

LGTM

libenchao requested changes Dec 22, 2023

View reviewed changes

libenchao reviewed Jan 2, 2024

View reviewed changes

lsyldliu reviewed Jan 2, 2024

View reviewed changes

libenchao approved these changes Jan 3, 2024

View reviewed changes

lsyldliu reviewed Jan 8, 2024

View reviewed changes

zoudan force-pushed the FLINK-33792 branch from 3875042 to c407012 Compare January 11, 2024 11:45

zoudan added 10 commits January 12, 2024 10:06

[FLINK-33792] Generate the same code for the same logic

d28be96

remove the configuration

34b8908

remove unused import

2d8014f

add parent context to avoid naming conflicts

172886e

keep name counter in CodeGenUtils

e833630

add UT for parentCtx

47cf0da

remove default parameter value for newName

65908aa

add CodeGeneratorContext in CastRule.Context

a21a509

empty commit to trigger CI

4858e75

empty commit to trigger CI

5424149

zoudan force-pushed the FLINK-33792 branch from c407012 to 5424149 Compare January 12, 2024 02:07

lsyldliu approved these changes Jan 12, 2024

View reviewed changes

libenchao closed this in d26c1b6 Jan 15, 2024

flinkbot added the component=TableSQL/Planner label Apr 4, 2024

[FLINK-33792] Generate the same code for the same logic #23984

[FLINK-33792] Generate the same code for the same logic #23984

Conversation

zoudan commented Dec 22, 2023

What is the purpose of the change

Brief change log

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

flinkbot commented Dec 22, 2023 • edited

CI report:

libenchao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zoudan commented Dec 25, 2023

libenchao commented Dec 25, 2023

zoudan commented Dec 28, 2023

zoudan commented Dec 29, 2023

zoudan commented Dec 29, 2023

libenchao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lsyldliu left a comment

Choose a reason for hiding this comment

zoudan commented Jan 2, 2024

zoudan commented Jan 3, 2024

libenchao left a comment

Choose a reason for hiding this comment

lsyldliu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lsyldliu Jan 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zoudan commented Jan 9, 2024

zoudan commented Jan 9, 2024

zoudan commented Jan 9, 2024

zoudan commented Jan 10, 2024

zoudan commented Jan 11, 2024

zoudan commented Jan 11, 2024

zoudan commented Jan 12, 2024

zoudan commented Jan 12, 2024

lsyldliu left a comment

Choose a reason for hiding this comment

flinkbot commented Dec 22, 2023 •

edited

lsyldliu Jan 8, 2024 •

edited