Skip to content

Commit

Permalink
[CALCITE-5788] Order of metadata handler methods is inconsistent in d…
Browse files Browse the repository at this point in the history 
…ifferent 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.
  • Loading branch information
@julianhyde
julianhyde committed Jun 21, 2023
1 parent 181dd40 commit 54192eb
Show file tree
Hide file tree
Showing 4 changed files with 91 additions and 50 deletions.
45 changes: 32 additions & 13 deletions core/src/main/java/org/apache/calcite/rel/metadata/MetadataDef.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,17 @@

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.
Expand All @@ -39,22 +44,36 @@ private MetadataDef(Class<M> metadataClass,
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}. */
Expand Down
22 changes: 17 additions & 5 deletions core/src/main/java/org/apache/calcite/rel/metadata/MetadataHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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();
}
}
42 changes: 21 additions & 21 deletions ...src/main/java/org/apache/calcite/rel/metadata/janino/RelMetadataHandlerGeneratorUtil.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,18 +16,18 @@
*/
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;

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;

Expand Down Expand Up @@ -58,12 +58,13 @@ private 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) {
Expand All @@ -76,20 +77,19 @@ public static HandlerNameAndGeneratedCode generateHandler(
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(" ")
Expand All @@ -108,19 +108,19 @@ public static HandlerNameAndGeneratedCode generateHandler(
}
buff.append(" }\n");

//METHODS
// METHODS
getDefMethod(buff,
handlerToName.values()
.stream()
.findFirst()
.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)
Expand All @@ -145,10 +145,10 @@ private static void getDefMethod(StringBuilder buff, @Nullable String handlerNam

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;
Expand Down
32 changes: 21 additions & 11 deletions core/src/test/java/org/apache/calcite/rel/metadata/MetadataHandlerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}

/**
Expand Down

0 comments on commit 54192eb

Please sign in to comment.