This is an automated email from the ASF dual-hosted git repository. jhyde pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/calcite.git
commit 54192eb678d8db95c1f1b41c1e172be064bae0ba Author: Julian Hyde <[email protected]> AuthorDate: Mon Jun 19 14:52:26 2023 -0700 [CALCITE-5788] Order of metadata handler methods is inconsistent in different java versions Solve the problem by sorting methods, and handler methods, by method name. We require that method names are unique, and that methods and handlers have the same name. Besides method name, there are other checks to ensure that handler methods have the right form (e.g. they have two extra arguments, which are RelMetadataQuery and a subclass of RelNode), which used to be asserts and are now calls to Preconditions.checkArgument. --- .../apache/calcite/rel/metadata/MetadataDef.java | 45 +++++++++++++++------- .../calcite/rel/metadata/MetadataHandler.java | 22 ++++++++--- .../janino/RelMetadataHandlerGeneratorUtil.java | 42 ++++++++++---------- .../calcite/rel/metadata/MetadataHandlerTest.java | 32 +++++++++------ 4 files changed, 91 insertions(+), 50 deletions(-) diff --git a/core/src/main/java/org/apache/calcite/rel/metadata/MetadataDef.java b/core/src/main/java/org/apache/calcite/rel/metadata/MetadataDef.java index 30ccea2360..f85b1b557e 100644 --- a/core/src/main/java/org/apache/calcite/rel/metadata/MetadataDef.java +++ b/core/src/main/java/org/apache/calcite/rel/metadata/MetadataDef.java @@ -18,12 +18,17 @@ package org.apache.calcite.rel.metadata; import org.apache.calcite.rel.RelNode; import org.apache.calcite.util.Pair; +import org.apache.calcite.util.Util; import com.google.common.collect.ImmutableList; import java.lang.reflect.Method; import java.util.Arrays; +import java.util.Comparator; import java.util.List; +import java.util.SortedMap; + +import static com.google.common.base.Preconditions.checkArgument; /** * Definition of metadata. @@ -39,22 +44,36 @@ public class MetadataDef<M extends Metadata> { Class<? extends MetadataHandler<M>> handlerClass, Method... methods) { this.metadataClass = metadataClass; this.handlerClass = handlerClass; - this.methods = ImmutableList.copyOf(methods); - final Method[] handlerMethods = MetadataHandler.handlerMethods(handlerClass); + this.methods = + Arrays.stream(methods) + .sorted(Comparator.comparing(Method::getName)) + .collect(Util.toImmutableList()); + final SortedMap<String, Method> handlerMethods = + MetadataHandler.handlerMethods(handlerClass); // Handler must have the same methods as Metadata, each method having // additional "subclass-of-RelNode, RelMetadataQuery" parameters. - assert handlerMethods.length == methods.length; - for (Pair<Method, Method> pair : Pair.zip(methods, handlerMethods)) { - final List<Class<?>> leftTypes = - Arrays.asList(pair.left.getParameterTypes()); - final List<Class<?>> rightTypes = - Arrays.asList(pair.right.getParameterTypes()); - assert leftTypes.size() + 2 == rightTypes.size(); - assert RelNode.class.isAssignableFrom(rightTypes.get(0)); - assert RelMetadataQuery.class == rightTypes.get(1); - assert leftTypes.equals(rightTypes.subList(2, rightTypes.size())); - } + checkArgument(this.methods.size() == handlerMethods.size(), + "handlerMethods.length = methods.length", this.methods, handlerMethods); + Pair.forEach(this.methods, handlerMethods.values(), + (method, handlerMethod) -> { + final List<Class<?>> methodTypes = + Arrays.asList(method.getParameterTypes()); + final List<Class<?>> handlerTypes = + Arrays.asList(handlerMethod.getParameterTypes()); + checkArgument(methodTypes.size() + 2 == handlerTypes.size(), + "methodTypes.size + 2 == handlerTypes.size", handlerMethod, + methodTypes, handlerTypes); + checkArgument(RelNode.class.isAssignableFrom(handlerTypes.get(0)), + "RelNode.assignableFrom(handlerType[0])", handlerMethod, + handlerTypes); + checkArgument(RelMetadataQuery.class == handlerTypes.get(1), + "handlerTypes[1] == RelMetadataQuery", handlerMethod, + handlerTypes); + checkArgument(methodTypes.equals(Util.skip(handlerTypes, 2)), + "methodTypes == handlerTypes.skip(2)", handlerMethod, methodTypes, + handlerTypes); + }); } /** Creates a {@link org.apache.calcite.rel.metadata.MetadataDef}. */ diff --git a/core/src/main/java/org/apache/calcite/rel/metadata/MetadataHandler.java b/core/src/main/java/org/apache/calcite/rel/metadata/MetadataHandler.java index d543580be1..b89c54b3e9 100644 --- a/core/src/main/java/org/apache/calcite/rel/metadata/MetadataHandler.java +++ b/core/src/main/java/org/apache/calcite/rel/metadata/MetadataHandler.java @@ -16,9 +16,12 @@ */ package org.apache.calcite.rel.metadata; +import com.google.common.collect.ImmutableSortedMap; + import java.lang.reflect.Method; import java.lang.reflect.Modifier; import java.util.Arrays; +import java.util.SortedMap; /** * Marker interface for a handler of metadata. @@ -29,17 +32,26 @@ public interface MetadataHandler<M extends Metadata> { MetadataDef<M> getDef(); /** - * Finds handler methods defined by a {@link MetadataHandler}. Static and synthetic methods - * are ignored. + * Finds handler methods defined by a {@link MetadataHandler}, + * and returns a map keyed by method name. + * + * <p>Ignores static and synthetic methods, + * and the {@link MetadataHandler#getDef()} method. + * + * <p>Methods must have unique names. * * @param handlerClass the handler class to inspect * @return handler methods */ - static Method[] handlerMethods(Class<? extends MetadataHandler<?>> handlerClass) { - return Arrays.stream(handlerClass.getDeclaredMethods()) + static SortedMap<String, Method> handlerMethods( + Class<? extends MetadataHandler<?>> handlerClass) { + final ImmutableSortedMap.Builder<String, Method> map = + ImmutableSortedMap.naturalOrder(); + Arrays.stream(handlerClass.getDeclaredMethods()) .filter(m -> !m.getName().equals("getDef")) .filter(m -> !m.isSynthetic()) .filter(m -> !Modifier.isStatic(m.getModifiers())) - .toArray(Method[]::new); + .forEach(m -> map.put(m.getName(), m)); + return map.build(); } } diff --git a/core/src/main/java/org/apache/calcite/rel/metadata/janino/RelMetadataHandlerGeneratorUtil.java b/core/src/main/java/org/apache/calcite/rel/metadata/janino/RelMetadataHandlerGeneratorUtil.java index 5dd1a5a423..1d3ef55836 100644 --- a/core/src/main/java/org/apache/calcite/rel/metadata/janino/RelMetadataHandlerGeneratorUtil.java +++ b/core/src/main/java/org/apache/calcite/rel/metadata/janino/RelMetadataHandlerGeneratorUtil.java @@ -16,6 +16,7 @@ */ package org.apache.calcite.rel.metadata.janino; +import org.apache.calcite.linq4j.Ord; import org.apache.calcite.rel.metadata.MetadataDef; import org.apache.calcite.rel.metadata.MetadataHandler; @@ -23,11 +24,10 @@ import org.checkerframework.checker.nullness.qual.Nullable; import org.immutables.value.Value; import java.lang.reflect.Method; -import java.util.Arrays; -import java.util.Comparator; import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.SortedMap; import static org.apache.calcite.linq4j.Nullness.castNonNull; @@ -58,12 +58,13 @@ public class RelMetadataHandlerGeneratorUtil { public static HandlerNameAndGeneratedCode generateHandler( Class<? extends MetadataHandler<?>> handlerClass, List<? extends MetadataHandler<?>> handlers) { - final String classPackage = castNonNull(RelMetadataHandlerGeneratorUtil.class.getPackage()) - .getName(); + final String classPackage = + castNonNull(RelMetadataHandlerGeneratorUtil.class.getPackage()) + .getName(); final String name = "GeneratedMetadata_" + simpleNameForHandler(handlerClass); - final Method[] declaredMethods = MetadataHandler.handlerMethods(handlerClass); - Arrays.sort(declaredMethods, Comparator.comparing(Method::getName)); + final SortedMap<String, Method> declaredMethods = + MetadataHandler.handlerMethods(handlerClass); final Map<MetadataHandler<?>, String> handlerToName = new LinkedHashMap<>(); for (MetadataHandler<?> provider : handlers) { @@ -76,20 +77,19 @@ public class RelMetadataHandlerGeneratorUtil { buff.append(LICENSE) .append("package ").append(classPackage).append(";\n\n"); - //Class definition + // Class definition buff.append("public final class ").append(name).append("\n") .append(" implements ").append(handlerClass.getCanonicalName()).append(" {\n"); - //PROPERTIES - for (int i = 0; i < declaredMethods.length; i++) { - CacheGeneratorUtil.cacheProperties(buff, declaredMethods[i], i); - } + // PROPERTIES + Ord.forEach(declaredMethods.values(), (declaredMethod, i) -> + CacheGeneratorUtil.cacheProperties(buff, declaredMethod, i)); for (Map.Entry<MetadataHandler<?>, String> handlerAndName : handlerToName.entrySet()) { buff.append(" public final ").append(handlerAndName.getKey().getClass().getName()) .append(' ').append(handlerAndName.getValue()).append(";\n"); } - //CONSTRUCTOR + // CONSTRUCTOR buff.append(" public ").append(name).append("(\n"); for (Map.Entry<MetadataHandler<?>, String> handlerAndName : handlerToName.entrySet()) { buff.append(" ") @@ -108,7 +108,7 @@ public class RelMetadataHandlerGeneratorUtil { } buff.append(" }\n"); - //METHODS + // METHODS getDefMethod(buff, handlerToName.values() .stream() @@ -116,11 +116,11 @@ public class RelMetadataHandlerGeneratorUtil { .orElse(null)); DispatchGenerator dispatchGenerator = new DispatchGenerator(handlerToName); - for (int i = 0; i < declaredMethods.length; i++) { - CacheGeneratorUtil.cachedMethod(buff, declaredMethods[i], i); - dispatchGenerator.dispatchMethod(buff, declaredMethods[i], handlers); - } - //End of Class + Ord.forEach(declaredMethods.values(), (declaredMethod, i) -> { + CacheGeneratorUtil.cachedMethod(buff, declaredMethod, i); + dispatchGenerator.dispatchMethod(buff, declaredMethod, handlers); + }); + // End of Class buff.append("\n}\n"); return ImmutableHandlerNameAndGeneratedCode.builder() .withHandlerName(classPackage + "." + name) @@ -145,10 +145,10 @@ public class RelMetadataHandlerGeneratorUtil { private static String simpleNameForHandler(Class<? extends MetadataHandler<?>> clazz) { String simpleName = clazz.getSimpleName(); - //Previously the pattern was to have a nested in class named Handler - //So we need to add the parents class to get a unique name + // Previously the pattern was to have a nested in class named Handler + // So we need to add the parents class to get a unique name if (simpleName.equals("Handler")) { - String[] parts = clazz.getName().split("\\.|\\$"); + String[] parts = clazz.getName().split("[.$]"); return parts[parts.length - 2] + parts[parts.length - 1]; } else { return simpleName; diff --git a/core/src/test/java/org/apache/calcite/rel/metadata/MetadataHandlerTest.java b/core/src/test/java/org/apache/calcite/rel/metadata/MetadataHandlerTest.java index b84c6e50b2..1a1867a95d 100644 --- a/core/src/test/java/org/apache/calcite/rel/metadata/MetadataHandlerTest.java +++ b/core/src/test/java/org/apache/calcite/rel/metadata/MetadataHandlerTest.java @@ -16,46 +16,56 @@ */ package org.apache.calcite.rel.metadata; +import com.google.common.collect.ImmutableList; + import org.junit.jupiter.api.Test; import java.lang.reflect.Method; +import java.util.List; +import java.util.SortedMap; import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.MatcherAssert.assertThat; -import static org.hamcrest.Matchers.emptyArray; +import static org.hamcrest.Matchers.empty; +import static org.hamcrest.Matchers.hasSize; /** * Tests for {@link MetadataHandler}. */ class MetadataHandlerTest { @Test void findsHandlerMethods() { - Method[] methods = MetadataHandler.handlerMethods(TestMetadataHandler.class); + final SortedMap<String, Method> map = + MetadataHandler.handlerMethods(TestMetadataHandler.class); + final List<Method> methods = ImmutableList.copyOf(map.values()); - assertThat(methods.length, is(1)); - assertThat(methods[0].getName(), is("getTestMetadata")); + assertThat(methods, hasSize(1)); + assertThat(methods.get(0).getName(), is("getTestMetadata")); } @Test void getDefMethodInHandlerIsIgnored() { - Method[] methods = + final SortedMap<String, Method> map = MetadataHandler.handlerMethods( MetadataHandlerWithGetDefMethodOnly.class); + final List<Method> methods = ImmutableList.copyOf(map.values()); - assertThat(methods, is(emptyArray())); + assertThat(methods, empty()); } @Test void staticMethodInHandlerIsIgnored() { - Method[] methods = + final SortedMap<String, Method> map = MetadataHandler.handlerMethods(MetadataHandlerWithStaticMethod.class); + final List<Method> methods = ImmutableList.copyOf(map.values()); - assertThat(methods, is(emptyArray())); + assertThat(methods, empty()); } - @Test void synthenticMethodInHandlerIsIgnored() { - Method[] methods = + @Test void syntheticMethodInHandlerIsIgnored() { + final SortedMap<String, Method> map = MetadataHandler.handlerMethods( TestMetadataHandlers.handlerClassWithSyntheticMethod()); + final List<Method> methods = ImmutableList.copyOf(map.values()); - assertThat(methods, is(emptyArray())); + assertThat(methods, empty()); } /**
