asolimando commented on a change in pull request #2488:
URL: https://github.com/apache/calcite/pull/2488#discussion_r690572316
##########
File path:
core/src/main/java/org/apache/calcite/adapter/java/ReflectiveSchema.java
##########
@@ -75,6 +78,8 @@
extends AbstractSchema {
private final Class clazz;
private final Object target;
+ private Types.FieldsOrdering fieldsOrdering;
+ private @Nullable Map<Class, List<Field>> classFieldsMap;
Review comment:
I was not familiar with the `ClassValue` interface, from what I
understood by looking it up, it looks beneficial when the `Map` could be the
only impediment to the disposal of classes by the GC.
I think I would anyway need to pass (via the constructor of `MyClassValue`)
and store a Map internally to implement the `computeValue` method, so it won’t
probably help much. I also expect the list of classes in the map to be of a
reasonable size, since it’s coming from the user.
What do you think?
##########
File path: linq4j/src/test/java/org/apache/calcite/linq4j/tree/TypeTest.java
##########
@@ -62,4 +83,236 @@
assertEquals(Object.class, Types.gcd(String.class, int.class));
java.io.Serializable o = true ? "x" : 1;
}
+
+ private static void assertFields(List<Field> expected, List<Field> computed)
{
+ assertEquals(expected, computed, "Expected field(s) ["
+ + expected + "] but found [" + computed + "]");
+ }
+
+ @Test void testExcludedStaticFieldsAndViaAnnotation() throws
NoSuchFieldException {
+ List<Field> fieldNames =
Collections.singletonList(A.class.getField("strField"));
+
+ assertFields(fieldNames, Types.getClassFields(A.class));
+ }
+
+ @Test void testAlphabeticalAndHierarchicalOrder() throws
NoSuchFieldException {
+ List<Field> fieldNames = Arrays.asList(AB.class.getField("aField"),
+ A.class.getField("strField"),
+ AB.class.getField("zField"));
+
+ assertFields(
+ fieldNames, Types.getClassFields(AB.class, true,
+ Types.FieldsOrdering.ALPHABETICAL, null));
+ }
+
+ @Test void testOrderViaFieldNamesAndClassHierarchy() throws
NoSuchFieldException {
+ List<Field> fieldNames = Arrays.asList(A.class.getField("strField"),
+ AB.class.getField("aField"),
+ AB.class.getField("zField"));
+
+ assertFields(
+ fieldNames, Types.getClassFields(AB.class, true,
+ Types.FieldsOrdering.ALPHABETICAL_AND_HIERARCHY, null));
+ }
+
+ @Test void testNoHierarchyConstructorAlphabetical() throws
NoSuchFieldException {
+ List<Field> fieldNames = Arrays.asList(AB.class.getField("aField"),
+ AB.class.getField("zField"));
+
+ assertFields(
+ fieldNames, Types.getClassFields(AB.class, false,
+ Types.FieldsOrdering.CONSTRUCTOR, null));
+ }
+
+ @TestIfParameterNames(clazz = AC.class)
+ void testNoHierarchyConstructorInDeclarationOrder() throws
NoSuchFieldException {
+ List<Field> fieldNames = Arrays.asList(AC.class.getField("zField"),
+ AC.class.getField("aField"));
+
+ assertFields(
+ fieldNames, Types.getClassFields(AC.class, false,
+ Types.FieldsOrdering.CONSTRUCTOR, null));
+ }
+
+
+ @TestIfParameterNames(clazz = AB.class)
+ void testHierarchyConstructorAlphabetical() throws NoSuchFieldException {
+ List<Field> fieldNames = Arrays.asList(AB.class.getField("aField"),
+ AB.class.getField("zField"),
+ A.class.getField("strField"));
+
+ assertFields(
+ fieldNames, Types.getClassFields(AB.class, true,
+ Types.FieldsOrdering.CONSTRUCTOR, null));
+ }
+
+ @TestIfParameterNames(clazz = AC.class)
+ void testHierarchyConstructorInDeclarationOrder() throws
NoSuchFieldException {
+ List<Field> fieldNames = Arrays.asList(A.class.getField("strField"),
+ AC.class.getField("zField"),
+ AC.class.getField("aField"));
+
+ assertFields(
+ fieldNames, Types.getClassFields(AC.class, true,
+ Types.FieldsOrdering.CONSTRUCTOR, null));
+ }
+
+ @TestIfParameterNames(clazz = AD.class)
+ void testIncompleteConstructors() throws NoSuchFieldException {
+ List<Field> fieldNames = Arrays.asList(A.class.getField("strField"),
+ AD.class.getField("zField"),
+ AD.class.getField("aField"));
+
+ assertFields(
+ fieldNames, Types.getClassFields(AD.class, true,
+ Types.FieldsOrdering.CONSTRUCTOR, null));
+ }
+
+ @Test void testExplicitFieldNames() throws NoSuchFieldException {
+ Map<Class, List<Field>> classFieldsMap = new HashMap<>();
+ List<Field> fieldNames = Arrays.asList(AC.class.getField("zField"),
+ A.class.getField("strField"),
+ AC.class.getField("aField"));
+ classFieldsMap.put(AC.class, fieldNames);
+
+ assertFields(
+ fieldNames, Types.getClassFields(AC.class, true,
+ Types.FieldsOrdering.EXPLICIT, classFieldsMap));
+ }
+
+ @Test void testExplicitFieldNamesIncompleteKO() throws NoSuchFieldException {
+ Map<Class, List<Field>> classFieldsMap = new HashMap<>();
+ List<Field> fieldNames = Arrays.asList(AC.class.getField("zField"),
+ A.class.getField("strField"));
+ classFieldsMap.put(AC.class, fieldNames);
+
+ Types.FieldsOrdering explicitFieldsOrdering =
Types.FieldsOrdering.EXPLICIT;
+
+ IllegalArgumentException thrown = assertThrows(
+ IllegalArgumentException.class,
+ () -> Types.getClassFields(AC.class, true, explicitFieldsOrdering,
classFieldsMap),
+ "Expected 'Types.getClassFields' to throw on incomplete field list"
+ );
+
+ String expectedErrorPrefix = "Incomplete list of fields is not compatible
with \""
+ + explicitFieldsOrdering + "\"";
+ assertTrue(thrown.getMessage().contains(expectedErrorPrefix));
Review comment:
Thanks Vladimir, I have improved the error message above and opted for
option (a) for the latest assertion
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
