[FLINK-9292] [core] Remove TypeInfoParser (part 2) #5970

yanghua · 2018-05-09T09:19:40Z

What is the purpose of the change

This pull request removed class TypeInfoParser and refactored specific test case

Brief change log

Removed class TypeInfoParser
Refactor code where use TypeInfoParser
Refactor test code where use TypeInfoParser

Verifying this change

This change is already covered by existing tests*.

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): (yes / no)
The public API, i.e., is any changed class annotated with @Public(Evolving): (yes / no)
The serializers: (yes / no / don't know)
The runtime per-record code paths (performance sensitive): (yes / no / don't know)
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (yes / no / don't know)
The S3 file system connector: (yes / no / don't know)

Documentation

Does this pull request introduce a new feature? (yes / no)
If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)

yanghua · 2018-05-09T09:20:16Z

hi @StephanEwen this issues part 2, please review it

yanghua · 2018-05-10T11:46:02Z

cc @StephanEwen fixed a incompatibility error

StephanEwen

Thanks, this looks mostly good.

Some inline comments, most important the one about not changing the one test that must use TypeHint rather than Types.

StephanEwen · 2018-05-11T11:36:59Z

flink-core/src/test/java/org/apache/flink/api/common/typeinfo/TypeInformationTest.java

@@ -61,7 +61,7 @@ public void testOfGenericClassForGenericType() {
 	@Test
 	public void testOfTypeHint() {
 		assertEquals(BasicTypeInfo.STRING_TYPE_INFO, TypeInformation.of(String.class));
-		assertEquals(BasicTypeInfo.STRING_TYPE_INFO, TypeInformation.of(new TypeHint<String>(){}));
+		assertEquals(BasicTypeInfo.STRING_TYPE_INFO, Types.STRING);


Please undo this one, the test explicitly tests the TypeHint use.

OK, sorry maybe batch replace, will revert it

StephanEwen · 2018-05-11T11:47:09Z

flink-java/src/test/java/org/apache/flink/api/java/sca/UdfAnalyzerTest.java

@@ -109,7 +112,7 @@ public String map(MyPojo value) throws Exception {
 	@Test
 	public void testForwardWithGenericTypePublicAttrAccess() {
 		compareAnalyzerResultWithAnnotationsSingleInput(MapFunction.class, Map4.class,
-				"org.apache.flink.api.java.sca.UdfAnalyzerTest$MyPojo", "String");
+			TypeInformation.of(new TypeHint<GenericTypeInfo<MyPojo>>(){}), Types.STRING);


The GenericTypeInfo here seems to be not correct.

@StephanEwen it is a problem, will trigger test error, because the class will be parsed as a PoJo type, when I debug it.

Then the code needs to be new GenericTypeInfo<>(MyPojo.class). Otherwise it will try and determine why the Type Info for GenericType is and it just happens to be a generic type ;-)

StephanEwen · 2018-05-11T11:53:14Z

flink-java/src/test/java/org/apache/flink/api/java/sca/UdfAnalyzerTest.java

-		compareAnalyzerResultWithAnnotationsSingleInput(MapFunction.class, Map14.class, "Tuple2<String,Integer>",
-				"Tuple2<String,String>");
+		compareAnalyzerResultWithAnnotationsSingleInput(MapFunction.class, Map14.class,
+			TypeInformation.of(new TypeHint<Tuple2<String, Integer>>(){}), TypeInformation.of(new TypeHint<Tuple2<String, String>>(){}));


There is so much use of the types Tuple2<String, Integer>> and Tuple2<String, String>, it would make sense to factor these out into a static field and reference them from there.

accept this suggestion

yanghua · 2018-05-11T15:03:46Z

cc @StephanEwen refactored based on your suggestion, the travis build failed because of yarn test, seems not about this PR's change.

yanghua · 2018-05-13T01:40:04Z

cc @StephanEwen

StephanEwen · 2018-05-13T11:25:27Z

flink-java/src/test/java/org/apache/flink/api/java/sca/UdfAnalyzerTest.java

@@ -62,6 +62,10 @@
 @SuppressWarnings("serial")
 public class UdfAnalyzerTest {

+	private static TypeInformation<Tuple2<String, Integer>> stringIntTuple2TypeInfo = TypeInformation.of(new TypeHint<Tuple2<String, Integer>>(){});


These fields are constants, so they should be final. Can you add the modified and rename the fields to match the naming convention?

StephanEwen · 2018-05-13T11:26:06Z

One minor style comment, otherwise this is good to go!

yanghua · 2018-05-13T11:44:29Z

cc @StephanEwen refactored base on your suggestion.

StephanEwen · 2018-05-13T13:02:28Z

Looks good, thanks.

+1 to merge

yanghua · 2018-05-15T08:13:07Z

cc @tillrohrmann this can be merged thanks!

yanghua · 2018-05-16T06:44:52Z

cc @tzulitai

StephanEwen · 2018-05-16T18:24:30Z

Merging this...

This changes apache#5970

zentol · 2018-05-22T07:53:21Z

@yanghua The PR has been merged, could you close it?

yanghua · 2018-05-22T08:32:17Z

@zentol OK, closing after merged~

This changes apache#5970

yanghua force-pushed the FLINK-9292 branch from eb34f0b to 40aa3ae Compare May 10, 2018 09:33

StephanEwen requested changes May 11, 2018

View reviewed changes

yanghua force-pushed the FLINK-9292 branch from 40aa3ae to ca919a8 Compare May 11, 2018 14:04

StephanEwen reviewed May 13, 2018

View reviewed changes

yanghua added 4 commits May 13, 2018 19:42

[FLINK-9292] [core] Remove TypeInfoParser (part 2)

e715c28

fixed a incompatibility error reported by japicmp plugin

7efd18f

refactor code and fixed a type error

fe6fa2f

changed fields to constants

50bc1f2

yanghua force-pushed the FLINK-9292 branch from ca919a8 to 50bc1f2 Compare May 13, 2018 11:43

StephanEwen pushed a commit to StephanEwen/flink that referenced this pull request May 16, 2018

[FLINK-9292] [core] Remove TypeInfoParser (part 2)

09ed785

This changes apache#5970

StephanEwen pushed a commit to StephanEwen/flink that referenced this pull request May 17, 2018

[FLINK-9292] [core] Remove TypeInfoParser (part 2)

9ddb978

This changes apache#5970

cjolif pushed a commit to cjolif/flink that referenced this pull request May 19, 2018

[FLINK-9292] [core] Remove TypeInfoParser (part 2)

2065679

This changes apache#5970

yanghua closed this May 22, 2018

sampathBhat pushed a commit to sampathBhat/flink that referenced this pull request Jul 26, 2018

[FLINK-9292] [core] Remove TypeInfoParser (part 2)

3bee8e8

This changes apache#5970

rmetzger added the component=<none> label Mar 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-9292] [core] Remove TypeInfoParser (part 2) #5970

[FLINK-9292] [core] Remove TypeInfoParser (part 2) #5970

yanghua commented May 9, 2018

yanghua commented May 9, 2018

yanghua commented May 10, 2018

StephanEwen left a comment

StephanEwen May 11, 2018

yanghua May 11, 2018

StephanEwen May 11, 2018

yanghua May 11, 2018

StephanEwen May 11, 2018

StephanEwen May 11, 2018

yanghua May 11, 2018

yanghua commented May 11, 2018

yanghua commented May 13, 2018

StephanEwen May 13, 2018

StephanEwen commented May 13, 2018

yanghua commented May 13, 2018

StephanEwen commented May 13, 2018

yanghua commented May 15, 2018

yanghua commented May 16, 2018

StephanEwen commented May 16, 2018

zentol commented May 22, 2018

yanghua commented May 22, 2018

[FLINK-9292] [core] Remove TypeInfoParser (part 2) #5970

[FLINK-9292] [core] Remove TypeInfoParser (part 2) #5970

Conversation

yanghua commented May 9, 2018

What is the purpose of the change

Brief change log

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

yanghua commented May 9, 2018

yanghua commented May 10, 2018

StephanEwen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanghua commented May 11, 2018

yanghua commented May 13, 2018

Choose a reason for hiding this comment

StephanEwen commented May 13, 2018

yanghua commented May 13, 2018

StephanEwen commented May 13, 2018

yanghua commented May 15, 2018

yanghua commented May 16, 2018

StephanEwen commented May 16, 2018

zentol commented May 22, 2018

yanghua commented May 22, 2018