[jira] [Commented] (FLINK-4673) TypeInfoFactory for Either type

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/FLINK-4673?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15815277#comment-15815277
 ] 

ASF GitHub Bot commented on FLINK-4673:
---

Github user greghogan commented on a diff in the pull request:

https://github.com/apache/flink/pull/2545#discussion_r95386388
  
--- Diff: 
flink-core/src/main/java/org/apache/flink/api/java/typeutils/TypeExtractor.java 
---
@@ -675,38 +673,6 @@ else if (isClassType(t) && 
Tuple.class.isAssignableFrom(typeToClass(t))) {
return new TupleTypeInfo(typeToClass(t), subTypesInfo);

}
-   // check if type is a subclass of Either
-   else if (isClassType(t) && 
Either.class.isAssignableFrom(typeToClass(t))) {
-   Type curT = t;
-
-   // go up the hierarchy until we reach Either (with or 
without generics)
-   // collect the types while moving up for a later 
top-down
-   while (!(isClassType(curT) && 
typeToClass(curT).equals(Either.class))) {
-   typeHierarchy.add(curT);
-   curT = typeToClass(curT).getGenericSuperclass();
-   }
-
-   // check if Either has generics
-   if (curT instanceof Class) {
-   throw new InvalidTypesException("Either needs 
to be parameterized by using generics.");
-   }
-
-   typeHierarchy.add(curT);
-
-   // create the type information for the subtypes
-   final TypeInformation[] subTypesInfo = 
createSubTypesInfo(t, (ParameterizedType) curT, typeHierarchy, in1Type, 
in2Type, false);
-   // type needs to be treated a pojo due to additional 
fields
-   if (subTypesInfo == null) {
-   if (t instanceof ParameterizedType) {
-   return (TypeInformation) 
analyzePojo(typeToClass(t), new ArrayList(typeHierarchy), 
(ParameterizedType) t, in1Type, in2Type);
--- End diff --

@twalthr thanks for checking this! Glad to hear that your factories 
implementation has exceeded expectations :)


> TypeInfoFactory for Either type
> ---
>
> Key: FLINK-4673
> URL: https://issues.apache.org/jira/browse/FLINK-4673
> Project: Flink
>  Issue Type: Improvement
>  Components: Core
>Affects Versions: 1.2.0
>Reporter: Greg Hogan
>Assignee: Greg Hogan
>Priority: Minor
> Fix For: 1.3.0
>
>
> I was able to resolve the requirement to specify an explicit 
> {{TypeInformation}} in the pull request for FLINK-4624 by creating a 
> {{TypeInfoFactory}} for the {{Either}} type.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (FLINK-4673) TypeInfoFactory for Either type

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/FLINK-4673?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15815094#comment-15815094
 ] 

ASF GitHub Bot commented on FLINK-4673:
---

Github user asfgit closed the pull request at:

https://github.com/apache/flink/pull/2545


> TypeInfoFactory for Either type
> ---
>
> Key: FLINK-4673
> URL: https://issues.apache.org/jira/browse/FLINK-4673
> Project: Flink
>  Issue Type: Improvement
>  Components: Core
>Affects Versions: 1.2.0
>Reporter: Greg Hogan
>Assignee: Greg Hogan
>Priority: Minor
> Fix For: 1.3.0
>
>
> I was able to resolve the requirement to specify an explicit 
> {{TypeInformation}} in the pull request for FLINK-4624 by creating a 
> {{TypeInfoFactory}} for the {{Either}} type.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (FLINK-4673) TypeInfoFactory for Either type

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/FLINK-4673?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15815006#comment-15815006
 ] 

ASF GitHub Bot commented on FLINK-4673:
---

Github user twalthr commented on a diff in the pull request:

https://github.com/apache/flink/pull/2545#discussion_r95365908
  
--- Diff: 
flink-core/src/main/java/org/apache/flink/api/java/typeutils/TypeExtractor.java 
---
@@ -675,38 +673,6 @@ else if (isClassType(t) && 
Tuple.class.isAssignableFrom(typeToClass(t))) {
return new TupleTypeInfo(typeToClass(t), subTypesInfo);

}
-   // check if type is a subclass of Either
-   else if (isClassType(t) && 
Either.class.isAssignableFrom(typeToClass(t))) {
-   Type curT = t;
-
-   // go up the hierarchy until we reach Either (with or 
without generics)
-   // collect the types while moving up for a later 
top-down
-   while (!(isClassType(curT) && 
typeToClass(curT).equals(Either.class))) {
-   typeHierarchy.add(curT);
-   curT = typeToClass(curT).getGenericSuperclass();
-   }
-
-   // check if Either has generics
-   if (curT instanceof Class) {
-   throw new InvalidTypesException("Either needs 
to be parameterized by using generics.");
-   }
-
-   typeHierarchy.add(curT);
-
-   // create the type information for the subtypes
-   final TypeInformation[] subTypesInfo = 
createSubTypesInfo(t, (ParameterizedType) curT, typeHierarchy, in1Type, 
in2Type, false);
-   // type needs to be treated a pojo due to additional 
fields
-   if (subTypesInfo == null) {
-   if (t instanceof ParameterizedType) {
-   return (TypeInformation) 
analyzePojo(typeToClass(t), new ArrayList(typeHierarchy), 
(ParameterizedType) t, in1Type, in2Type);
--- End diff --

Sorry for not writing back earlier. You are right, all tests still work. 
The factories work better than I expected. We lose the input validation in this 
PR but I think this is ok. I will merge this.


> TypeInfoFactory for Either type
> ---
>
> Key: FLINK-4673
> URL: https://issues.apache.org/jira/browse/FLINK-4673
> Project: Flink
>  Issue Type: Improvement
>  Components: Core
>Affects Versions: 1.2.0
>Reporter: Greg Hogan
>Assignee: Greg Hogan
>Priority: Minor
>
> I was able to resolve the requirement to specify an explicit 
> {{TypeInformation}} in the pull request for FLINK-4624 by creating a 
> {{TypeInfoFactory}} for the {{Either}} type.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (FLINK-4673) TypeInfoFactory for Either type

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/FLINK-4673?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15562254#comment-15562254
 ] 

ASF GitHub Bot commented on FLINK-4673:
---

Github user twalthr commented on a diff in the pull request:

https://github.com/apache/flink/pull/2545#discussion_r82598976
  
--- Diff: 
flink-core/src/main/java/org/apache/flink/api/java/typeutils/TypeExtractor.java 
---
@@ -675,38 +673,6 @@ else if (isClassType(t) && 
Tuple.class.isAssignableFrom(typeToClass(t))) {
return new TupleTypeInfo(typeToClass(t), subTypesInfo);

}
-   // check if type is a subclass of Either
-   else if (isClassType(t) && 
Either.class.isAssignableFrom(typeToClass(t))) {
-   Type curT = t;
-
-   // go up the hierarchy until we reach Either (with or 
without generics)
-   // collect the types while moving up for a later 
top-down
-   while (!(isClassType(curT) && 
typeToClass(curT).equals(Either.class))) {
-   typeHierarchy.add(curT);
-   curT = typeToClass(curT).getGenericSuperclass();
-   }
-
-   // check if Either has generics
-   if (curT instanceof Class) {
-   throw new InvalidTypesException("Either needs 
to be parameterized by using generics.");
-   }
-
-   typeHierarchy.add(curT);
-
-   // create the type information for the subtypes
-   final TypeInformation[] subTypesInfo = 
createSubTypesInfo(t, (ParameterizedType) curT, typeHierarchy, in1Type, 
in2Type, false);
-   // type needs to be treated a pojo due to additional 
fields
-   if (subTypesInfo == null) {
-   if (t instanceof ParameterizedType) {
-   return (TypeInformation) 
analyzePojo(typeToClass(t), new ArrayList(typeHierarchy), 
(ParameterizedType) t, in1Type, in2Type);
--- End diff --

Sorry, for the delay. I will have a look at it again.


> TypeInfoFactory for Either type
> ---
>
> Key: FLINK-4673
> URL: https://issues.apache.org/jira/browse/FLINK-4673
> Project: Flink
>  Issue Type: Improvement
>  Components: Core
>Affects Versions: 1.2.0
>Reporter: Greg Hogan
>Assignee: Greg Hogan
>Priority: Minor
>
> I was able to resolve the requirement to specify an explicit 
> {{TypeInformation}} in the pull request for FLINK-4624 by creating a 
> {{TypeInfoFactory}} for the {{Either}} type.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (FLINK-4673) TypeInfoFactory for Either type

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/FLINK-4673?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15548914#comment-15548914
 ] 

ASF GitHub Bot commented on FLINK-4673:
---

Github user greghogan commented on a diff in the pull request:

https://github.com/apache/flink/pull/2545#discussion_r81987517
  
--- Diff: 
flink-core/src/main/java/org/apache/flink/api/java/typeutils/TypeExtractor.java 
---
@@ -675,38 +673,6 @@ else if (isClassType(t) && 
Tuple.class.isAssignableFrom(typeToClass(t))) {
return new TupleTypeInfo(typeToClass(t), subTypesInfo);

}
-   // check if type is a subclass of Either
-   else if (isClassType(t) && 
Either.class.isAssignableFrom(typeToClass(t))) {
-   Type curT = t;
-
-   // go up the hierarchy until we reach Either (with or 
without generics)
-   // collect the types while moving up for a later 
top-down
-   while (!(isClassType(curT) && 
typeToClass(curT).equals(Either.class))) {
-   typeHierarchy.add(curT);
-   curT = typeToClass(curT).getGenericSuperclass();
-   }
-
-   // check if Either has generics
-   if (curT instanceof Class) {
-   throw new InvalidTypesException("Either needs 
to be parameterized by using generics.");
-   }
-
-   typeHierarchy.add(curT);
-
-   // create the type information for the subtypes
-   final TypeInformation[] subTypesInfo = 
createSubTypesInfo(t, (ParameterizedType) curT, typeHierarchy, in1Type, 
in2Type, false);
-   // type needs to be treated a pojo due to additional 
fields
-   if (subTypesInfo == null) {
-   if (t instanceof ParameterizedType) {
-   return (TypeInformation) 
analyzePojo(typeToClass(t), new ArrayList(typeHierarchy), 
(ParameterizedType) t, in1Type, in2Type);
--- End diff --

`Either` is overridden in `TypeExtractorTest` without any test errors. 
Could you give an example that would break the current PR?


> TypeInfoFactory for Either type
> ---
>
> Key: FLINK-4673
> URL: https://issues.apache.org/jira/browse/FLINK-4673
> Project: Flink
>  Issue Type: Improvement
>  Components: Core
>Affects Versions: 1.2.0
>Reporter: Greg Hogan
>Assignee: Greg Hogan
>Priority: Minor
>
> I was able to resolve the requirement to specify an explicit 
> {{TypeInformation}} in the pull request for FLINK-4624 by creating a 
> {{TypeInfoFactory}} for the {{Either}} type.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (FLINK-4673) TypeInfoFactory for Either type

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/FLINK-4673?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15522591#comment-15522591
 ] 

ASF GitHub Bot commented on FLINK-4673:
---

Github user twalthr commented on a diff in the pull request:

https://github.com/apache/flink/pull/2545#discussion_r80437853
  
--- Diff: 
flink-core/src/main/java/org/apache/flink/api/java/typeutils/TypeExtractor.java 
---
@@ -675,38 +673,6 @@ else if (isClassType(t) && 
Tuple.class.isAssignableFrom(typeToClass(t))) {
return new TupleTypeInfo(typeToClass(t), subTypesInfo);

}
-   // check if type is a subclass of Either
-   else if (isClassType(t) && 
Either.class.isAssignableFrom(typeToClass(t))) {
-   Type curT = t;
-
-   // go up the hierarchy until we reach Either (with or 
without generics)
-   // collect the types while moving up for a later 
top-down
-   while (!(isClassType(curT) && 
typeToClass(curT).equals(Either.class))) {
-   typeHierarchy.add(curT);
-   curT = typeToClass(curT).getGenericSuperclass();
-   }
-
-   // check if Either has generics
-   if (curT instanceof Class) {
-   throw new InvalidTypesException("Either needs 
to be parameterized by using generics.");
-   }
-
-   typeHierarchy.add(curT);
-
-   // create the type information for the subtypes
-   final TypeInformation[] subTypesInfo = 
createSubTypesInfo(t, (ParameterizedType) curT, typeHierarchy, in1Type, 
in2Type, false);
-   // type needs to be treated a pojo due to additional 
fields
-   if (subTypesInfo == null) {
-   if (t instanceof ParameterizedType) {
-   return (TypeInformation) 
analyzePojo(typeToClass(t), new ArrayList(typeHierarchy), 
(ParameterizedType) t, in1Type, in2Type);
--- End diff --

What happens if Either class is extended? The whole checking is missing 
when using a factory. It need to go into the `createTypeInfo` method. What I 
just recognized, actually the `EitherTypeInfo` needs a second constructor that 
takes the subclass similar to `TupleTypeInfo`. The `t` parameter of 
`createTypeInfo` can than passed to this constructor.


> TypeInfoFactory for Either type
> ---
>
> Key: FLINK-4673
> URL: https://issues.apache.org/jira/browse/FLINK-4673
> Project: Flink
>  Issue Type: Improvement
>  Components: Core
>Affects Versions: 1.2.0
>Reporter: Greg Hogan
>Assignee: Greg Hogan
>Priority: Minor
>
> I was able to resolve the requirement to specify an explicit 
> {{TypeInformation}} in the pull request for FLINK-4624 by creating a 
> {{TypeInfoFactory}} for the {{Either}} type.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (FLINK-4673) TypeInfoFactory for Either type

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/FLINK-4673?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15522590#comment-15522590
 ] 

ASF GitHub Bot commented on FLINK-4673:
---

Github user twalthr commented on a diff in the pull request:

https://github.com/apache/flink/pull/2545#discussion_r80439161
  
--- Diff: 
flink-core/src/test/java/org/apache/flink/api/java/typeutils/TypeExtractorTest.java
 ---
@@ -1857,12 +1856,6 @@ public void testEitherHierarchy() {
Assert.assertEquals(expected, ti);
}
 
-   @Test(expected=InvalidTypesException.class)
-   public void testEitherFromObjectException() {
-   Either either = Either.Left("test");
-   TypeExtractor.getForObject(either);
--- End diff --

This test is necessary. The exception is not thrown anymore but the type 
information that is currently generated is invalid (the right type information 
is `null`, which should never happen).


> TypeInfoFactory for Either type
> ---
>
> Key: FLINK-4673
> URL: https://issues.apache.org/jira/browse/FLINK-4673
> Project: Flink
>  Issue Type: Improvement
>  Components: Core
>Affects Versions: 1.2.0
>Reporter: Greg Hogan
>Assignee: Greg Hogan
>Priority: Minor
>
> I was able to resolve the requirement to specify an explicit 
> {{TypeInformation}} in the pull request for FLINK-4624 by creating a 
> {{TypeInfoFactory}} for the {{Either}} type.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (FLINK-4673) TypeInfoFactory for Either type

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/FLINK-4673?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15522592#comment-15522592
 ] 

ASF GitHub Bot commented on FLINK-4673:
---

Github user twalthr commented on a diff in the pull request:

https://github.com/apache/flink/pull/2545#discussion_r80424649
  
--- Diff: 
flink-core/src/main/java/org/apache/flink/api/java/typeutils/EitherTypeInfoFactory.java
 ---
@@ -0,0 +1,34 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.api.java.typeutils;
+
+import org.apache.flink.api.common.typeinfo.TypeInfoFactory;
+import org.apache.flink.api.common.typeinfo.TypeInformation;
+import org.apache.flink.types.Either;
+
+import java.lang.reflect.Type;
+import java.util.Map;
+
+public class EitherTypeInfoFactory extends TypeInfoFactory> {
+
+   @Override
+   public TypeInformation> createTypeInfo(Type t, Map genericParameters) {
+   return new EitherTypeInfo(genericParameters.get("L"), 
genericParameters.get("R"));
--- End diff --

Can check here if the key is not null? Btw. we should add a null check to 
the constructor of EitherTypeInfo.


> TypeInfoFactory for Either type
> ---
>
> Key: FLINK-4673
> URL: https://issues.apache.org/jira/browse/FLINK-4673
> Project: Flink
>  Issue Type: Improvement
>  Components: Core
>Affects Versions: 1.2.0
>Reporter: Greg Hogan
>Assignee: Greg Hogan
>Priority: Minor
>
> I was able to resolve the requirement to specify an explicit 
> {{TypeInformation}} in the pull request for FLINK-4624 by creating a 
> {{TypeInfoFactory}} for the {{Either}} type.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (FLINK-4673) TypeInfoFactory for Either type

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/FLINK-4673?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15522384#comment-15522384
 ] 

ASF GitHub Bot commented on FLINK-4673:
---

Github user twalthr commented on the issue:

https://github.com/apache/flink/pull/2545
  
I will review this PR soon.


> TypeInfoFactory for Either type
> ---
>
> Key: FLINK-4673
> URL: https://issues.apache.org/jira/browse/FLINK-4673
> Project: Flink
>  Issue Type: Improvement
>  Components: Core
>Affects Versions: 1.2.0
>Reporter: Greg Hogan
>Assignee: Greg Hogan
>Priority: Minor
>
> I was able to resolve the requirement to specify an explicit 
> {{TypeInformation}} in the pull request for FLINK-4624 by creating a 
> {{TypeInfoFactory}} for the {{Either}} type.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

[jira] [Commented] (FLINK-4673) TypeInfoFactory for Either type

[ASF GitHub Bot (JIRA)]

[ 
https://issues.apache.org/jira/browse/FLINK-4673?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=15518971#comment-15518971
 ] 

ASF GitHub Bot commented on FLINK-4673:
---

GitHub user greghogan opened a pull request:

https://github.com/apache/flink/pull/2545

[FLINK-4673] [core] TypeInfoFactory for Either type

Removes from TypeExtractor the explicit parsing for Either and adds an 
EitherTypeInfoFactory.

A test was removed that was expected to fail but now looks to succeed.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/greghogan/flink 
4673_typeinfofactory_for_either_type

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/flink/pull/2545.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #2545


commit a7fe93d8cbe10eb1c327cc659202e10c34efead9
Author: Greg Hogan 
Date:   2016-09-23T18:59:36Z

[FLINK-4673] [core] TypeInfoFactory for Either type

Removes from TypeExtractor the explicit parsing for Either and adds an
EitherTypeInfoFactory.




> TypeInfoFactory for Either type
> ---
>
> Key: FLINK-4673
> URL: https://issues.apache.org/jira/browse/FLINK-4673
> Project: Flink
>  Issue Type: Improvement
>  Components: Core
>Affects Versions: 1.2.0
>Reporter: Greg Hogan
>Assignee: Greg Hogan
>Priority: Minor
>
> I was able to resolve the requirement to specify an explicit 
> {{TypeInformation}} in the pull request for FLINK-4624 by creating a 
> {{TypeInfoFactory}} for the {{Either}} type.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)