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:
[email protected]
