This is an automated email from the ASF dual-hosted git repository.
lincoln pushed a commit to branch release-1.20
in repository https://gitbox.apache.org/repos/asf/flink.git
The following commit(s) were added to refs/heads/release-1.20 by this push:
new ce29a69bcbe [FLINK-35498][table] Fix unexpected argument name
conflicts when extracting method parameter names from UDF
ce29a69bcbe is described below
commit ce29a69bcbe384cdfaec7db98ce4447b760ce3a2
Author: Xuyang <xyzhong...@163.com>
AuthorDate: Thu Jul 4 21:13:48 2024 +0800
[FLINK-35498][table] Fix unexpected argument name conflicts when extracting
method parameter names from UDF
This closes #25019
---
.../table/types/extraction/ExtractionUtils.java | 49 ++++++++-
.../types/extraction/ExtractionUtilsTest.java | 110 +++++++++++++++++++++
.../planner/runtime/stream/sql/FunctionITCase.java | 43 ++++++++
3 files changed, 198 insertions(+), 4 deletions(-)
diff --git
a/flink-table/flink-table-common/src/main/java/org/apache/flink/table/types/extraction/ExtractionUtils.java
b/flink-table/flink-table-common/src/main/java/org/apache/flink/table/types/extraction/ExtractionUtils.java
index 9854e62eb0c..5968b41fc84 100644
---
a/flink-table/flink-table-common/src/main/java/org/apache/flink/table/types/extraction/ExtractionUtils.java
+++
b/flink-table/flink-table-common/src/main/java/org/apache/flink/table/types/extraction/ExtractionUtils.java
@@ -19,6 +19,7 @@
package org.apache.flink.table.types.extraction;
import org.apache.flink.annotation.Internal;
+import org.apache.flink.annotation.VisibleForTesting;
import org.apache.flink.api.common.typeutils.TypeSerializer;
import org.apache.flink.table.annotation.ArgumentHint;
import org.apache.flink.table.api.DataTypes;
@@ -58,6 +59,7 @@ import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
+import java.util.TreeMap;
import java.util.function.Function;
import java.util.regex.Pattern;
import java.util.stream.Collectors;
@@ -747,7 +749,8 @@ public final class ExtractionUtils {
return fieldNames;
}
- private static @Nullable List<String> extractExecutableNames(Executable
executable) {
+ @VisibleForTesting
+ static @Nullable List<String> extractExecutableNames(Executable
executable) {
final int offset;
if (!Modifier.isStatic(executable.getModifiers())) {
// remove "this" as first parameter
@@ -824,6 +827,40 @@ public final class ExtractionUtils {
* <localVar:index=2 , name=otherLocal2 , desc=J, sig=null, start=L1,
end=L2>
* }
* }</pre>
+ *
+ * <p>If a constructor or method has multiple identical local variables
that are not initialized
+ * like:
+ *
+ * <pre>{@code
+ * String localVariable;
+ * if (generic == null) {
+ * localVariable = "null";
+ * } else if (generic < 0) {
+ * localVariable = "negative";
+ * } else if (generic > 0) {
+ * localVariable = "positive";
+ * } else {
+ * localVariable = "zero";
+ * }
+ * }</pre>
+ *
+ * <p>Its local variable table is as follows:
+ *
+ * <pre>{@code
+ * Start Length Slot Name Signature
+ * 7 3 2 localVariable Ljava/lang/String;
+ * 22 3 2 localVariable Ljava/lang/String;
+ * 37 3 2 localVariable Ljava/lang/String;
+ * 0 69 0 this ...;
+ * 0 69 1 generic Ljava/lang/Long;
+ * 43 26 2 localVariable Ljava/lang/String;
+ * }</pre>
+ *
+ * <p>The method parameters are always at the head in the 'slot' list.
+ *
+ * <p>NOTE: the first parameter may be "this" if the function is not
static. See more at <a
+ *
href="https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-3.html";>3.6.
Receiving
+ * Arguments</a>
*/
private static class ParameterExtractor extends ClassVisitor {
@@ -831,7 +868,7 @@ public final class ExtractionUtils {
private final String methodDescriptor;
- private final List<String> parameterNames = new ArrayList<>();
+ private final Map<Integer, String> parameterNamesWithIndex = new
TreeMap<>();
ParameterExtractor(Constructor<?> constructor) {
super(OPCODE);
@@ -844,7 +881,11 @@ public final class ExtractionUtils {
}
List<String> getParameterNames() {
- return parameterNames;
+ // method parameters are always at the head in the 'index' list
+ // NOTE: the first parameter may be "this" if the function is not
static
+ // See more at Chapter "3.6. Receiving Arguments" in
+ // https://docs.oracle.com/javase/specs/jvms/se8/html/jvms-3.html
+ return new ArrayList<>(parameterNamesWithIndex.values());
}
@Override
@@ -860,7 +901,7 @@ public final class ExtractionUtils {
Label start,
Label end,
int index) {
- parameterNames.add(name);
+ parameterNamesWithIndex.put(index, name);
}
};
}
diff --git
a/flink-table/flink-table-common/src/test/java/org/apache/flink/table/types/extraction/ExtractionUtilsTest.java
b/flink-table/flink-table-common/src/test/java/org/apache/flink/table/types/extraction/ExtractionUtilsTest.java
index 80b12fb9a95..c8149345fa9 100644
---
a/flink-table/flink-table-common/src/test/java/org/apache/flink/table/types/extraction/ExtractionUtilsTest.java
+++
b/flink-table/flink-table-common/src/test/java/org/apache/flink/table/types/extraction/ExtractionUtilsTest.java
@@ -18,6 +18,8 @@
package org.apache.flink.table.types.extraction;
+import org.apache.flink.shaded.guava31.com.google.common.collect.ImmutableList;
+
import org.junit.jupiter.api.Test;
import java.lang.reflect.GenericArrayType;
@@ -93,6 +95,41 @@ public class ExtractionUtilsTest {
assertThat(innerFuture.getActualTypeArguments()[0]).isEqualTo(Long.class);
}
+ @Test
+ void testExtractExecutableNamesWithMultiLocalVariableBlocks() {
+ List<String> expectedParameterNames =
+ ImmutableList.of("generic", "genericFuture",
"listOfGenericFuture", "array");
+
+ // test the local variable is not initialized at first
+ List<Method> methods =
+ ExtractionUtils.collectMethods(
+ MultiLocalVariableWithoutInitializationClass.class,
"method");
+ Method method = methods.get(0);
+ List<String> parameterNames =
ExtractionUtils.extractExecutableNames(method);
+ assertThat(parameterNames).isEqualTo(expectedParameterNames);
+
+ // test the local variable is initialized at first
+ methods =
+ ExtractionUtils.collectMethods(
+ MultiLocalVariableBlocksWithInitializationClass.class,
"method");
+ method = methods.get(0);
+ parameterNames = ExtractionUtils.extractExecutableNames(method);
+ assertThat(parameterNames).isEqualTo(expectedParameterNames);
+ }
+
+ @Test
+ void testExtractExecutableNamesWithParameterNameShadowed() {
+ List<String> expectedParameterNames =
+ ImmutableList.of(
+ "generic", "result", "genericFuture",
"listOfGenericFuture", "array");
+ // test the local variable is not initialized at first
+ List<Method> methods =
+
ExtractionUtils.collectMethods(ParameterNameShadowedClass.class, "method");
+ Method method = methods.get(0);
+ List<String> parameterNames =
ExtractionUtils.extractExecutableNames(method);
+ assertThat(parameterNames).isEqualTo(expectedParameterNames);
+ }
+
/** Test function. */
public static class ClassBase<T> {
@@ -114,4 +151,77 @@ public class ExtractionUtilsTest {
/** Test function. */
public static class FutureClass extends
ClassBase2<CompletableFuture<Long>> {}
+
+ /**
+ * A test function that contains multi local variable blocks without
initialization at first.
+ */
+ public static class MultiLocalVariableWithoutInitializationClass extends
ClassBase<Long> {
+
+ @Override
+ public void method(
+ Long generic,
+ CompletableFuture<Long> genericFuture,
+ List<CompletableFuture<Long>> listOfGenericFuture,
+ Long[] array) {
+ // don't initialize the local variable
+ String localVariable;
+
+ if (generic == null) {
+ localVariable = "null";
+ } else if (generic < 0) {
+ localVariable = "negative";
+ } else if (generic > 0) {
+ localVariable = "positive";
+ } else {
+ localVariable = "zero";
+ }
+
+ // use the local variable
+ System.err.println("localVariable: " + localVariable);
+ }
+ }
+
+ /** A test function that contains multi local variable blocks with
initialization at first. */
+ public static class MultiLocalVariableBlocksWithInitializationClass
extends ClassBase<Long> {
+
+ @Override
+ public void method(
+ Long generic,
+ CompletableFuture<Long> genericFuture,
+ List<CompletableFuture<Long>> listOfGenericFuture,
+ Long[] array) {
+ // initialize the local variable
+ String localVariable = "";
+
+ if (generic == null) {
+ localVariable = "null";
+ } else if (generic < 0) {
+ localVariable = "negative";
+ } else if (generic > 0) {
+ localVariable = "positive";
+ } else {
+ localVariable = "zero";
+ }
+
+ // use the local variable
+ System.err.println("localVariable: " + localVariable);
+ }
+ }
+
+ /**
+ * A test function where one function parameter has the same name as a
class member variable
+ * within another complex function parameter.
+ */
+ public static class ParameterNameShadowedClass {
+
+ @SuppressWarnings("unused")
+ public void method(
+ Long generic,
+ // this `result` has the same name as the class member
variable in
+ // `CompletableFuture`
+ Object result,
+ CompletableFuture<Long> genericFuture,
+ List<CompletableFuture<Long>> listOfGenericFuture,
+ Long[] array) {}
+ }
}
diff --git
a/flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/runtime/stream/sql/FunctionITCase.java
b/flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/runtime/stream/sql/FunctionITCase.java
index f0623717681..199f2266022 100644
---
a/flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/runtime/stream/sql/FunctionITCase.java
+++
b/flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/runtime/stream/sql/FunctionITCase.java
@@ -1542,6 +1542,26 @@ public class FunctionITCase extends StreamingTestBase {
"drop function lowerUdf");
}
+ @Test
+ void testUdfWithMultiLocalVariables() {
+ List<Row> sourceData = Arrays.asList(Row.of(1L, 2L), Row.of(2L, 3L));
+ TestCollectionTableFactory.reset();
+ TestCollectionTableFactory.initData(sourceData);
+
+ tEnv().executeSql(
+ "CREATE TABLE SourceTable(x BIGINT, y BIGINT) WITH
('connector' = 'COLLECTION')");
+ tEnv().executeSql(
+ "CREATE FUNCTION MultiLocalVariables AS '"
+ + MultiLocalVariableBlocksClass.class.getName()
+ + "'");
+
+ List<Row> actualRows =
+ CollectionUtil.iteratorToList(
+ tEnv().executeSql("SELECT MultiLocalVariables(x, y)
FROM SourceTable")
+ .collect());
+ assertThat(actualRows).isEqualTo(Arrays.asList(Row.of(2L),
Row.of(6L)));
+ }
+
//
--------------------------------------------------------------------------------------------
// Test functions
//
--------------------------------------------------------------------------------------------
@@ -2139,6 +2159,29 @@ public class FunctionITCase extends StreamingTestBase {
}
}
+ /** A function that contains a local variable with multi blocks. */
+ public static class MultiLocalVariableBlocksClass extends ScalarFunction {
+
+ public Long eval(Long a, Long b) {
+ long localVariable;
+ if (a == null) {
+ // block 1
+ localVariable = 0;
+ } else if (a == 0) {
+ // block 2
+ localVariable = -1;
+ } else if (b < 1) {
+ // block 3
+ localVariable = -1L * a;
+ } else {
+ // block 4
+ localVariable = a;
+ }
+
+ return localVariable * Optional.ofNullable(b).orElse(0L);
+ }
+ }
+
private interface FunctionCreator {
void createFunction(TableEnvironment environment);
}
