You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2021/08/16 16:06:13 UTC
[GitHub] [calcite] asolimando opened a new pull request #2488: [CALCITE-2489] Order of fields in JavaTypeFactoryImpl#createStructType is unstable
asolimando opened a new pull request #2488:
URL: https://github.com/apache/calcite/pull/2488
### Summary:
The PR follows the approach suggested in the ML discussion [discussion](https://mail-archives.apache.org/mod_mbox/calcite-dev/202102.mbox/%3CCAFtOckKUyaqBHe3q66svZWyX77Zwj0Kc_D4%2B49m75SthuPj%2BGQ%40mail.gmail.com%3E) from several months ago.
In a nutshell, the output of "Class.getFields()" is JVM implementation-specific without any guarantees on the field ordering, the PR aims at providing a deterministic and stable ordering.
`ReflectiveSchema` is where users can define a schema starting from a Java class. Now the schema descriptor has an extra parameter for specifying the sought parameter ordering, among the followings:
- JVM, // same as Class.getFields()
- ALPHABETICAL, // alphabetical order
- ALPHABETICAL_AND_HIERARCHY, // alphabetical order but inherited fields come first
- CONSTRUCTOR, // order is taken from the constructor matching the highest number of fields
- EXPLICIT, // field ordering is explicitly provided
- EXPLICIT_TOLERANT // field ordering is explicitly provided, but can be incomplete
`CONSTRUCTOR` is being used as default ordering (it generally matches the order of the declared fields, which is what all the JVM implementations I could test use, thus ensuring the least number of diff in existing unit tests), and `ALPHABETICAL_AND_HIERARCHY` as a fallback ordering for classes lacking the name of the parameters in their constructor(s).
### Walkthrough of the commits in the PR (from "older" to "newer"):
1. implementation of the replacement for `Class.getFields()` for `ReflectiveSchema` and the auxiliary methods in `Types` helper class
2. added unit tests covering the aforementioned changes
3. replacing `Class.getFields()` and `Class.getDeclaredFields()` anywhere in the code where the "order" of such fields matters
4. added `-parameters` compilation parameter to enrich Calcite's classes with parameters names for their constructors (more details are in the aforementioned ML discussion) and constructor-based field ordering as default
5. adapted few existing unit tests due to changes in field order coming from (4)
### How the PR was tested:
Several unit tests have been added, covering all the new or changed features, tests are passing locally and in CI.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] asolimando commented on pull request #2488: [CALCITE-2489] Order of fields in JavaTypeFactoryImpl#createStructType is unstable
Posted by GitBox <gi...@apache.org>.
asolimando commented on pull request #2488:
URL: https://github.com/apache/calcite/pull/2488#issuecomment-950176140
Resolved conflicts with current master
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2488: [CALCITE-2489] Order of fields in JavaTypeFactoryImpl#createStructType is unstable
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2488:
URL: https://github.com/apache/calcite/pull/2488#discussion_r690596413
##########
File path: build.gradle.kts
##########
@@ -71,10 +71,10 @@ val enableSpotBugs = props.bool("spotbugs")
val enableCheckerframework by props()
val enableErrorprone by props()
val enableDependencyAnalysis by props()
-val enableParameters by props(true)
val skipJandex by props()
val skipCheckstyle by props()
val skipAutostyle by props()
+val skipJavacParameterNames by props(true)
Review comment:
I would go with adding parameter names by default.
```suggestion
val skipJavacParameterNames by props()
```
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2488: [CALCITE-2489] Order of fields in JavaTypeFactoryImpl#createStructType is unstable
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2488:
URL: https://github.com/apache/calcite/pull/2488#discussion_r690596778
##########
File path: build.gradle.kts
##########
@@ -693,7 +693,7 @@ allprojects {
if (enableCheckerframework) {
options.forkOptions.memoryMaximumSize = "2g"
}
- if (enableParameters) {
+ if (skipJavacParameterNames) {
Review comment:
```suggestion
if (!skipJavacParameterNames) {
```
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2488: [CALCITE-2489] Order of fields in JavaTypeFactoryImpl#createStructType is unstable
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2488:
URL: https://github.com/apache/calcite/pull/2488#discussion_r690091227
##########
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 + "]");
Review comment:
"expected but found" message will be automatically added by `assertEquals`, so please remove it from the message.
assertion message should clarify **why** certain value was expected.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2488: [CALCITE-2489] Order of fields in JavaTypeFactoryImpl#createStructType is unstable
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2488:
URL: https://github.com/apache/calcite/pull/2488#discussion_r689734307
##########
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:
Do you think [ClassValue](https://docs.oracle.com/javase/8/docs/api/java/lang/ClassValue.html) would be a better fit?
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2488: [CALCITE-2489] Order of fields in JavaTypeFactoryImpl#createStructType is unstable
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2488:
URL: https://github.com/apache/calcite/pull/2488#discussion_r689730188
##########
File path: build.gradle.kts
##########
@@ -71,6 +71,7 @@ val enableSpotBugs = props.bool("spotbugs")
val enableCheckerframework by props()
val enableErrorprone by props()
val enableDependencyAnalysis by props()
+val enableParameters by props(true)
Review comment:
The intention was enabled by default features were named `skip*` and disabled by default were named `enable*`.
So I would suggest rephrasing the flag as `skipJavacParameterNames` or something like that.
WDYT?
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2488: [CALCITE-2489] Order of fields in JavaTypeFactoryImpl#createStructType is unstable
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2488:
URL: https://github.com/apache/calcite/pull/2488#discussion_r690106000
##########
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:
Please refrain from `assertTrue(...complexExpression...)` since the failure would look like `expected true got false` which often is puzzling for maintainers :-/
There are several ways to approach it:
a) `assertThat(actual, contains(substring))` from Hamcreset. I don't really like "hamcreset all the way", however, trivial comparisons like `contains` are more-or-less readable and they produce reasonable messages
b) `if (!thrown.getMessage().contains(...)) { throw new AssertionException("Types.getClassFields(AC, true, " + explicitFieldsOrdering + ", " + classFieldsMap + ") should throw incomplete list of fields exception since field ... was not listed in parameter ...", thrown); }`
c) `assertTrue(expression, () -> "clarifying message"...)`
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] asolimando commented on a change in pull request #2488: [CALCITE-2489] Order of fields in JavaTypeFactoryImpl#createStructType is unstable
Posted by GitBox <gi...@apache.org>.
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?
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] asolimando commented on a change in pull request #2488: [CALCITE-2489] Order of fields in JavaTypeFactoryImpl#createStructType is unstable
Posted by GitBox <gi...@apache.org>.
asolimando commented on a change in pull request #2488:
URL: https://github.com/apache/calcite/pull/2488#discussion_r690572665
##########
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2488: [CALCITE-2489] Order of fields in JavaTypeFactoryImpl#createStructType is unstable
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2488:
URL: https://github.com/apache/calcite/pull/2488#discussion_r690596413
##########
File path: build.gradle.kts
##########
@@ -71,10 +71,10 @@ val enableSpotBugs = props.bool("spotbugs")
val enableCheckerframework by props()
val enableErrorprone by props()
val enableDependencyAnalysis by props()
-val enableParameters by props(true)
val skipJandex by props()
val skipCheckstyle by props()
val skipAutostyle by props()
+val skipJavacParameterNames by props(true)
Review comment:
I would go with adding parameter names by default.
```suggestion
val skipJavacParameterNames by props()
```
##########
File path: build.gradle.kts
##########
@@ -693,7 +693,7 @@ allprojects {
if (enableCheckerframework) {
options.forkOptions.memoryMaximumSize = "2g"
}
- if (enableParameters) {
+ if (skipJavacParameterNames) {
Review comment:
```suggestion
if (!skipJavacParameterNames) {
```
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2488: [CALCITE-2489] Order of fields in JavaTypeFactoryImpl#createStructType is unstable
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2488:
URL: https://github.com/apache/calcite/pull/2488#discussion_r690091227
##########
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 + "]");
Review comment:
"expected but found" message will be automatically added by `assertEquals`, so please remove it from the message.
assertion message should be there, and it should clarify **why** certain value was expected
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] asolimando commented on a change in pull request #2488: [CALCITE-2489] Order of fields in JavaTypeFactoryImpl#createStructType is unstable
Posted by GitBox <gi...@apache.org>.
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2488: [CALCITE-2489] Order of fields in JavaTypeFactoryImpl#createStructType is unstable
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2488:
URL: https://github.com/apache/calcite/pull/2488#discussion_r689730188
##########
File path: build.gradle.kts
##########
@@ -71,6 +71,7 @@ val enableSpotBugs = props.bool("spotbugs")
val enableCheckerframework by props()
val enableErrorprone by props()
val enableDependencyAnalysis by props()
+val enableParameters by props(true)
Review comment:
The intention was enabled by default features are named `skip*` and disabled by default are named `enable*`.
So I would suggest rephrasing the flag as `skipJavacParameterNames` or something like that.
WDYT?
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org