-
Notifications
You must be signed in to change notification settings - Fork 13.8k
[FLINK-35498][table] Fix unexpected argument name conflicts when extracting method parameter names from UDF #24890
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
lincoln-lil
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xuyangzhong Thanks for fixing this! Overall looks good to me, I've left some comments, PTAL.
...link-table-common/src/main/java/org/apache/flink/table/types/extraction/ExtractionUtils.java
Outdated
Show resolved
Hide resolved
...link-table-common/src/main/java/org/apache/flink/table/types/extraction/ExtractionUtils.java
Show resolved
Hide resolved
...-planner/src/test/java/org/apache/flink/table/planner/runtime/stream/sql/FunctionITCase.java
Outdated
Show resolved
Hide resolved
…acting method parameters from UDF
|
Hi, @lincoln-lil I have updated this pr to resolve your comments and rebase the master. I would be grateful if you have time to review it again. |
lincoln-lil
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xuyangzhong Thanks for your updates! LGTM +1
Could you also create backport pr to both 1.20 & 1.19 branches as this affects the availability of some UDFs?
…acting method parameter names from UDF This closes apache#24890 (cherry picked from commit 84165b2)
…acting method parameter names from UDF This closes apache#24890 (cherry picked from commit 84165b2)
…acting method parameter names from UDF This closes apache#24890
…acting method parameter names from UDF This closes apache#24890
What is the purpose of the change
Currently, we depend on asm to extract method parameter names. We custom a MethodVisitor to visit local variable tables in file
.class, and then cut the firstNlocal variable names as the method parameter names.However, if there are multi blocks about one local variable, the first
Nlocal variable names could be same, and then wrongly be extracted, and crashed when validating the conflict of them(the new logic added in FLINK-1.19).This pr will use the slot index in
.classfile to extract the method parameter names, because method parameter names are always at the head in the 'slot index' list(https://docs.oracle.com/javase/specs/jvms/se17/html/jvms-3.html)Take the test function
ExtractionUtilsTest#MultiLocalVariableBlocksWithoutInitializationClassas an example:[localVariable, localVariable, localVariable, this, generic, genericFuture, listOfGenericFuture, array, localVariable]thisin index 0)[localVariable, localVariable, this, generic]This is because in local variable table, there are multi
localVariablewith different lifecycle in different blocks.[this, generic, genericFuture, listOfGenericFuture, array, localVariable]thisin index 0)[generic, genericFuture, listOfGenericFuture, array]That's what we expected.
Further more, if the local variable has been initialized before
if statement, its lifecycle is across allifblocks, and there is no need to init new local variablelocalVariablefor it. Otherwize,localVariablein multi blocks are actually not visible although it is declared at first, and jvm will create multilocalVariablefor different blocks and add it in local variable table(You can see in local variable table, although theselocalVariable's slots are same, but their start label and end label are different).Brief change log
Verifying this change
Some tests are added to verity it.
Does this pull request potentially affect one of the following parts:
@Public(Evolving): noDocumentation