You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2020/07/22 16:33:19 UTC
[GitHub] [flink] gaoyunhaii commented on a change in pull request #8357: [FLINK-12175] Change filling of typeHierarchy in analyzePojo, for cor…
gaoyunhaii commented on a change in pull request #8357:
URL: https://github.com/apache/flink/pull/8357#discussion_r458902742
##########
File path: flink-core/src/test/java/org/apache/flink/api/java/typeutils/PojoParametrizedTypeExtractionTest.java
##########
@@ -0,0 +1,101 @@
+package org.apache.flink.api.java.typeutils;
Review comment:
Needs the licence.
##########
File path: flink-core/src/test/java/org/apache/flink/api/java/typeutils/PojoParametrizedTypeExtractionTest.java
##########
@@ -0,0 +1,101 @@
+package org.apache.flink.api.java.typeutils;
+
+import org.apache.flink.api.common.functions.MapFunction;
+import org.apache.flink.api.common.typeinfo.TypeInformation;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.io.Serializable;
+
+/**
+ * Tests concerning type extraction of Parametrized Pojo and its superclasses.
+ */
+public class PojoParametrizedTypeExtractionTest {
+
+ @Test
+ public void testCreateTypeInfo(){
+ /// Init
+ final TypeInformation<ParametrizedParentImpl> directTypeInfo = TypeExtractor.createTypeInfo(ParametrizedParentImpl.class);
+
+ /// Check
+ Assert.assertTrue(directTypeInfo instanceof PojoTypeInfo);
+ }
+
+ @Test
+ public void testFieldExtraction(){
+ /// Init
+ final TypeInformation<ParametrizedParentImpl> directTypeInfo = TypeExtractor.createTypeInfo(ParametrizedParentImpl.class);
+
+ // Extract
+ TypeInformation<?> directPojoFieldTypeInfo = ((PojoTypeInfo) directTypeInfo)
+ .getPojoFieldAt(0)
+ .getTypeInformation();
+
+ // Check
+ Assert.assertTrue(directPojoFieldTypeInfo instanceof PojoTypeInfo);
+ }
+
+ @Test
+ public void testMapReturnTypeInfo(){
+ /// Init
+ final TypeInformation<ParametrizedParentImpl> directTypeInfo = TypeExtractor.createTypeInfo(ParametrizedParentImpl.class);
+
+ // Extract
+ TypeInformation<ParametrizedParentImpl> mapReturnTypeInfo = TypeExtractor
+ .getMapReturnTypes(new ConcreteMapFunction(), directTypeInfo);
+
+ // Check
+ Assert.assertTrue(mapReturnTypeInfo instanceof PojoTypeInfo);
+ Assert.assertEquals(directTypeInfo, mapReturnTypeInfo);
+ }
+
+ @Test
+ public void testMapReturnFieldTypeInfo(){
+ /// Init
+ final TypeInformation<ParametrizedParentImpl> directTypeInfo = TypeExtractor.createTypeInfo(ParametrizedParentImpl.class);
+
+ // Extract
+ TypeInformation<ParametrizedParentImpl> mapReturnTypeInfo = TypeExtractor
+ .getMapReturnTypes(new ConcreteMapFunction(), directTypeInfo);
+ TypeInformation<?> mapReturnPojoFieldTypeInfo = ((PojoTypeInfo) mapReturnTypeInfo)
+ .getPojoFieldAt(0)
+ .getTypeInformation();
+
+ // Check
+ Assert.assertTrue(mapReturnPojoFieldTypeInfo instanceof PojoTypeInfo);
+
+ }
+
+ /**
+ * Representation of Pojo class with 2 fields.
+ */
+ public static class Pojo implements Serializable {
+ public int digits;
+ public String letters;
+ }
+
+ /**
+ * Representation of class which is parametrized by some pojo.
+ */
+ public static class ParameterizedParent<T extends Serializable> implements Serializable {
+ public T pojoField;
+ }
+
+ /**
+ * Implementation of ParametrizedParent parametrized by Pojo.
+ */
+ public static class ParametrizedParentImpl extends ParameterizedParent<Pojo> {
+ public double precise;
+ }
+
+ /**
+ * Representation of map function for type extraction.
+ */
+ public static class ConcreteMapFunction implements MapFunction<ParametrizedParentImpl, ParametrizedParentImpl> {
+ @Override
+ public ParametrizedParentImpl map(ParametrizedParentImpl value) throws Exception {
+ return null;
+ }
+ }
+
Review comment:
remove the blank lines
##########
File path: flink-core/src/main/java/org/apache/flink/api/java/typeutils/TypeExtractor.java
##########
@@ -1837,8 +1837,8 @@ private boolean isValidPojoField(Field f, Class<?> clazz, ArrayList<Type> typeHi
if (parameterizedType != null) {
getTypeHierarchy(typeHierarchy, parameterizedType, Object.class);
}
- // create a type hierarchy, if the incoming only contains the most bottom one or none.
- else if (typeHierarchy.size() <= 1) {
+ // create a type hierarchy, for fields types extraction
Review comment:
change to `create a type hierarchy for type extraction of fields` ?
##########
File path: flink-core/src/test/java/org/apache/flink/api/java/typeutils/PojoParametrizedTypeExtractionTest.java
##########
@@ -0,0 +1,101 @@
+package org.apache.flink.api.java.typeutils;
+
+import org.apache.flink.api.common.functions.MapFunction;
+import org.apache.flink.api.common.typeinfo.TypeInformation;
Review comment:
blank lines required.
##########
File path: flink-core/src/test/java/org/apache/flink/api/java/typeutils/PojoParametrizedTypeExtractionTest.java
##########
@@ -0,0 +1,101 @@
+package org.apache.flink.api.java.typeutils;
+
+import org.apache.flink.api.common.functions.MapFunction;
+import org.apache.flink.api.common.typeinfo.TypeInformation;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.io.Serializable;
+
+/**
+ * Tests concerning type extraction of Parametrized Pojo and its superclasses.
+ */
+public class PojoParametrizedTypeExtractionTest {
+
+ @Test
+ public void testCreateTypeInfo(){
+ /// Init
+ final TypeInformation<ParametrizedParentImpl> directTypeInfo = TypeExtractor.createTypeInfo(ParametrizedParentImpl.class);
+
+ /// Check
+ Assert.assertTrue(directTypeInfo instanceof PojoTypeInfo);
+ }
+
+ @Test
+ public void testFieldExtraction(){
+ /// Init
+ final TypeInformation<ParametrizedParentImpl> directTypeInfo = TypeExtractor.createTypeInfo(ParametrizedParentImpl.class);
+
+ // Extract
+ TypeInformation<?> directPojoFieldTypeInfo = ((PojoTypeInfo) directTypeInfo)
+ .getPojoFieldAt(0)
+ .getTypeInformation();
+
+ // Check
+ Assert.assertTrue(directPojoFieldTypeInfo instanceof PojoTypeInfo);
+ }
+
+ @Test
+ public void testMapReturnTypeInfo(){
+ /// Init
+ final TypeInformation<ParametrizedParentImpl> directTypeInfo = TypeExtractor.createTypeInfo(ParametrizedParentImpl.class);
+
+ // Extract
+ TypeInformation<ParametrizedParentImpl> mapReturnTypeInfo = TypeExtractor
+ .getMapReturnTypes(new ConcreteMapFunction(), directTypeInfo);
+
+ // Check
+ Assert.assertTrue(mapReturnTypeInfo instanceof PojoTypeInfo);
+ Assert.assertEquals(directTypeInfo, mapReturnTypeInfo);
+ }
+
+ @Test
+ public void testMapReturnFieldTypeInfo(){
+ /// Init
+ final TypeInformation<ParametrizedParentImpl> directTypeInfo = TypeExtractor.createTypeInfo(ParametrizedParentImpl.class);
+
+ // Extract
+ TypeInformation<ParametrizedParentImpl> mapReturnTypeInfo = TypeExtractor
+ .getMapReturnTypes(new ConcreteMapFunction(), directTypeInfo);
+ TypeInformation<?> mapReturnPojoFieldTypeInfo = ((PojoTypeInfo) mapReturnTypeInfo)
+ .getPojoFieldAt(0)
+ .getTypeInformation();
+
+ // Check
+ Assert.assertTrue(mapReturnPojoFieldTypeInfo instanceof PojoTypeInfo);
+
+ }
+
+ /**
+ * Representation of Pojo class with 2 fields.
+ */
+ public static class Pojo implements Serializable {
Review comment:
How about removing the ` implements Serializable` ? Since it should be irrelevant to this test.
##########
File path: flink-core/src/test/java/org/apache/flink/api/java/typeutils/PojoParametrizedTypeExtractionTest.java
##########
@@ -0,0 +1,101 @@
+package org.apache.flink.api.java.typeutils;
+
+import org.apache.flink.api.common.functions.MapFunction;
+import org.apache.flink.api.common.typeinfo.TypeInformation;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.io.Serializable;
+
+/**
+ * Tests concerning type extraction of Parametrized Pojo and its superclasses.
+ */
+public class PojoParametrizedTypeExtractionTest {
+
+ @Test
+ public void testCreateTypeInfo(){
+ /// Init
+ final TypeInformation<ParametrizedParentImpl> directTypeInfo = TypeExtractor.createTypeInfo(ParametrizedParentImpl.class);
+
+ /// Check
+ Assert.assertTrue(directTypeInfo instanceof PojoTypeInfo);
+ }
+
+ @Test
+ public void testFieldExtraction(){
+ /// Init
+ final TypeInformation<ParametrizedParentImpl> directTypeInfo = TypeExtractor.createTypeInfo(ParametrizedParentImpl.class);
+
+ // Extract
+ TypeInformation<?> directPojoFieldTypeInfo = ((PojoTypeInfo) directTypeInfo)
+ .getPojoFieldAt(0)
+ .getTypeInformation();
+
+ // Check
+ Assert.assertTrue(directPojoFieldTypeInfo instanceof PojoTypeInfo);
+ }
+
+ @Test
+ public void testMapReturnTypeInfo(){
+ /// Init
+ final TypeInformation<ParametrizedParentImpl> directTypeInfo = TypeExtractor.createTypeInfo(ParametrizedParentImpl.class);
+
+ // Extract
+ TypeInformation<ParametrizedParentImpl> mapReturnTypeInfo = TypeExtractor
+ .getMapReturnTypes(new ConcreteMapFunction(), directTypeInfo);
+
+ // Check
+ Assert.assertTrue(mapReturnTypeInfo instanceof PojoTypeInfo);
+ Assert.assertEquals(directTypeInfo, mapReturnTypeInfo);
+ }
+
+ @Test
+ public void testMapReturnFieldTypeInfo(){
+ /// Init
+ final TypeInformation<ParametrizedParentImpl> directTypeInfo = TypeExtractor.createTypeInfo(ParametrizedParentImpl.class);
+
+ // Extract
+ TypeInformation<ParametrizedParentImpl> mapReturnTypeInfo = TypeExtractor
+ .getMapReturnTypes(new ConcreteMapFunction(), directTypeInfo);
+ TypeInformation<?> mapReturnPojoFieldTypeInfo = ((PojoTypeInfo) mapReturnTypeInfo)
+ .getPojoFieldAt(0)
+ .getTypeInformation();
+
+ // Check
+ Assert.assertTrue(mapReturnPojoFieldTypeInfo instanceof PojoTypeInfo);
+
+ }
+
+ /**
+ * Representation of Pojo class with 2 fields.
+ */
+ public static class Pojo implements Serializable {
+ public int digits;
+ public String letters;
+ }
+
+ /**
+ * Representation of class which is parametrized by some pojo.
+ */
+ public static class ParameterizedParent<T extends Serializable> implements Serializable {
Review comment:
And also remove the `extends Serializable` and ` implements Serializable` here ? (Together with the comment in Line 72).
##########
File path: flink-core/src/test/java/org/apache/flink/api/java/typeutils/PojoParametrizedTypeExtractionTest.java
##########
@@ -0,0 +1,101 @@
+package org.apache.flink.api.java.typeutils;
+
+import org.apache.flink.api.common.functions.MapFunction;
+import org.apache.flink.api.common.typeinfo.TypeInformation;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.io.Serializable;
+
+/**
+ * Tests concerning type extraction of Parametrized Pojo and its superclasses.
+ */
+public class PojoParametrizedTypeExtractionTest {
+
+ @Test
+ public void testCreateTypeInfo(){
+ /// Init
+ final TypeInformation<ParametrizedParentImpl> directTypeInfo = TypeExtractor.createTypeInfo(ParametrizedParentImpl.class);
+
+ /// Check
+ Assert.assertTrue(directTypeInfo instanceof PojoTypeInfo);
+ }
+
+ @Test
+ public void testFieldExtraction(){
+ /// Init
+ final TypeInformation<ParametrizedParentImpl> directTypeInfo = TypeExtractor.createTypeInfo(ParametrizedParentImpl.class);
+
+ // Extract
+ TypeInformation<?> directPojoFieldTypeInfo = ((PojoTypeInfo) directTypeInfo)
+ .getPojoFieldAt(0)
+ .getTypeInformation();
+
+ // Check
+ Assert.assertTrue(directPojoFieldTypeInfo instanceof PojoTypeInfo);
+ }
+
+ @Test
+ public void testMapReturnTypeInfo(){
+ /// Init
+ final TypeInformation<ParametrizedParentImpl> directTypeInfo = TypeExtractor.createTypeInfo(ParametrizedParentImpl.class);
+
+ // Extract
+ TypeInformation<ParametrizedParentImpl> mapReturnTypeInfo = TypeExtractor
+ .getMapReturnTypes(new ConcreteMapFunction(), directTypeInfo);
+
+ // Check
+ Assert.assertTrue(mapReturnTypeInfo instanceof PojoTypeInfo);
+ Assert.assertEquals(directTypeInfo, mapReturnTypeInfo);
+ }
+
+ @Test
+ public void testMapReturnFieldTypeInfo(){
Review comment:
Similarly, I think we might merge `testMapReturnFieldTypeInfo` with the above `testMapReturnTypeInfo`, as commented in Line 25.
##########
File path: flink-core/src/test/java/org/apache/flink/api/java/typeutils/PojoParametrizedTypeExtractionTest.java
##########
@@ -0,0 +1,101 @@
+package org.apache.flink.api.java.typeutils;
+
+import org.apache.flink.api.common.functions.MapFunction;
+import org.apache.flink.api.common.typeinfo.TypeInformation;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.io.Serializable;
+
+/**
+ * Tests concerning type extraction of Parametrized Pojo and its superclasses.
+ */
+public class PojoParametrizedTypeExtractionTest {
+
+ @Test
+ public void testCreateTypeInfo(){
+ /// Init
Review comment:
I tend to remove the comments and the following `Init`, `Extract` and `Check` comments, since the current tests seem to be not very complex and easy to understand.
##########
File path: flink-core/src/test/java/org/apache/flink/api/java/typeutils/PojoParametrizedTypeExtractionTest.java
##########
@@ -0,0 +1,101 @@
+package org.apache.flink.api.java.typeutils;
+
+import org.apache.flink.api.common.functions.MapFunction;
+import org.apache.flink.api.common.typeinfo.TypeInformation;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.io.Serializable;
+
+/**
+ * Tests concerning type extraction of Parametrized Pojo and its superclasses.
+ */
+public class PojoParametrizedTypeExtractionTest {
+
+ @Test
+ public void testCreateTypeInfo(){
+ /// Init
+ final TypeInformation<ParametrizedParentImpl> directTypeInfo = TypeExtractor.createTypeInfo(ParametrizedParentImpl.class);
+
+ /// Check
+ Assert.assertTrue(directTypeInfo instanceof PojoTypeInfo);
+ }
+
+ @Test
+ public void testFieldExtraction(){
Review comment:
I tend to merge this PR with the above `testCreateTypeInfo` and rename to `testDirectlyCreateTypeInfo`, since here seems to have the same action with the above test and with more asserts. This makes them more like the same test.
If we merge the two tests, we may create an explicit `PojoTypeInfo` object as the expected result and use the `assertEquals` to compare the result with the expected result.
##########
File path: flink-core/src/test/java/org/apache/flink/api/java/typeutils/PojoParametrizedTypeExtractionTest.java
##########
@@ -0,0 +1,101 @@
+package org.apache.flink.api.java.typeutils;
+
+import org.apache.flink.api.common.functions.MapFunction;
+import org.apache.flink.api.common.typeinfo.TypeInformation;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.io.Serializable;
+
+/**
+ * Tests concerning type extraction of Parametrized Pojo and its superclasses.
+ */
+public class PojoParametrizedTypeExtractionTest {
+
+ @Test
+ public void testCreateTypeInfo(){
+ /// Init
+ final TypeInformation<ParametrizedParentImpl> directTypeInfo = TypeExtractor.createTypeInfo(ParametrizedParentImpl.class);
+
+ /// Check
+ Assert.assertTrue(directTypeInfo instanceof PojoTypeInfo);
Review comment:
`assertTrue` better also give the `message field`.
##########
File path: flink-core/src/test/java/org/apache/flink/api/java/typeutils/PojoParametrizedTypeExtractionTest.java
##########
@@ -0,0 +1,101 @@
+package org.apache.flink.api.java.typeutils;
+
+import org.apache.flink.api.common.functions.MapFunction;
+import org.apache.flink.api.common.typeinfo.TypeInformation;
+import org.junit.Assert;
+import org.junit.Test;
+
+import java.io.Serializable;
+
+/**
+ * Tests concerning type extraction of Parametrized Pojo and its superclasses.
+ */
+public class PojoParametrizedTypeExtractionTest {
+
+ @Test
+ public void testCreateTypeInfo(){
+ /// Init
+ final TypeInformation<ParametrizedParentImpl> directTypeInfo = TypeExtractor.createTypeInfo(ParametrizedParentImpl.class);
+
+ /// Check
+ Assert.assertTrue(directTypeInfo instanceof PojoTypeInfo);
+ }
+
+ @Test
+ public void testFieldExtraction(){
+ /// Init
+ final TypeInformation<ParametrizedParentImpl> directTypeInfo = TypeExtractor.createTypeInfo(ParametrizedParentImpl.class);
+
+ // Extract
+ TypeInformation<?> directPojoFieldTypeInfo = ((PojoTypeInfo) directTypeInfo)
+ .getPojoFieldAt(0)
+ .getTypeInformation();
+
+ // Check
+ Assert.assertTrue(directPojoFieldTypeInfo instanceof PojoTypeInfo);
+ }
+
+ @Test
+ public void testMapReturnTypeInfo(){
+ /// Init
+ final TypeInformation<ParametrizedParentImpl> directTypeInfo = TypeExtractor.createTypeInfo(ParametrizedParentImpl.class);
+
+ // Extract
+ TypeInformation<ParametrizedParentImpl> mapReturnTypeInfo = TypeExtractor
+ .getMapReturnTypes(new ConcreteMapFunction(), directTypeInfo);
+
+ // Check
+ Assert.assertTrue(mapReturnTypeInfo instanceof PojoTypeInfo);
+ Assert.assertEquals(directTypeInfo, mapReturnTypeInfo);
Review comment:
I tend to use an explicit `PojoTypeInfo` as the expected result (namely we create a PojoTypeInfo directly). Since both the two type extraction logic are based on similar logic, it is possible that some bad implementation may cause the same errors for the two processes.
Besides, if we use explicit expected result, we could also remove the previous `instanceof` assert.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org