You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by greghogan <gi...@git.apache.org> on 2016/09/24 12:27:16 UTC
[GitHub] flink pull request #2545: [FLINK-4673] [core] TypeInfoFactory for Either typ...
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 <co...@greghogan.com>
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.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2545: [FLINK-4673] [core] TypeInfoFactory for Either typ...
Posted by twalthr <gi...@git.apache.org>.
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<OUT>) analyzePojo(typeToClass(t), new ArrayList<Type>(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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2545: [FLINK-4673] [core] TypeInfoFactory for Either typ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/2545
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2545: [FLINK-4673] [core] TypeInfoFactory for Either typ...
Posted by twalthr <gi...@git.apache.org>.
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<L, R> extends TypeInfoFactory<Either<L, R>> {
+
+ @Override
+ public TypeInformation<Either<L, R>> createTypeInfo(Type t, Map<String, TypeInformation<?>> 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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2545: [FLINK-4673] [core] TypeInfoFactory for Either typ...
Posted by greghogan <gi...@git.apache.org>.
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<OUT>) analyzePojo(typeToClass(t), new ArrayList<Type>(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?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #2545: [FLINK-4673] [core] TypeInfoFactory for Either type
Posted by twalthr <gi...@git.apache.org>.
Github user twalthr commented on the issue:
https://github.com/apache/flink/pull/2545
I will review this PR soon.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2545: [FLINK-4673] [core] TypeInfoFactory for Either typ...
Posted by twalthr <gi...@git.apache.org>.
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<OUT>) analyzePojo(typeToClass(t), new ArrayList<Type>(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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2545: [FLINK-4673] [core] TypeInfoFactory for Either typ...
Posted by twalthr <gi...@git.apache.org>.
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<OUT>) analyzePojo(typeToClass(t), new ArrayList<Type>(typeHierarchy), (ParameterizedType) t, in1Type, in2Type);
--- End diff --
Sorry, for the delay. I will have a look at it again.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2545: [FLINK-4673] [core] TypeInfoFactory for Either typ...
Posted by twalthr <gi...@git.apache.org>.
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<String, Tuple1<Integer>> 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).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2545: [FLINK-4673] [core] TypeInfoFactory for Either typ...
Posted by greghogan <gi...@git.apache.org>.
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<OUT>) analyzePojo(typeToClass(t), new ArrayList<Type>(typeHierarchy), (ParameterizedType) t, in1Type, in2Type);
--- End diff --
@twalthr thanks for checking this! Glad to hear that your factories implementation has exceeded expectations :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---