[GitHub] flink pull request #2337: [FLINK-3042] [FLINK-3060] [types] Define a way to ...
Github user asfgit closed the pull request at: https://github.com/apache/flink/pull/2337 --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request #2337: [FLINK-3042] [FLINK-3060] [types] Define a way to ...
Github user twalthr commented on a diff in the pull request:
https://github.com/apache/flink/pull/2337#discussion_r77984002
--- Diff:
flink-core/src/main/java/org/apache/flink/api/java/typeutils/TypeExtractor.java
---
@@ -792,12 +832,40 @@ else if (t instanceof Class) {
return null;
}
-
+
+ @SuppressWarnings({"unchecked", "rawtypes"})
private TypeInformation
createTypeInfoFromInput(TypeVariable returnTypeVar, ArrayList
inputTypeHierarchy, Type inType, TypeInformation inTypeInfo) {
TypeInformation info = null;
-
+
+ // use a factory to find corresponding type information to type
variable
+ final ArrayList factoryHierarchy = new
ArrayList<>(inputTypeHierarchy);
+ final TypeInfoFactory factory =
getClosestFactory(factoryHierarchy, inType);
+ if (factory != null) {
+ // the type that defines the factory is last in factory
hierarchy
+ final Type factoryDefiningType =
factoryHierarchy.get(factoryHierarchy.size() - 1);
+ // defining type has generics, the factory need to be
asked for a mapping of subtypes to type information
+ if (factoryDefiningType instanceof ParameterizedType) {
--- End diff --
Yes, because we try to create type information from the input in this
method. This only works if a type variable connects input with output.
`Map, OutType>`
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2337: [FLINK-3042] [FLINK-3060] [types] Define a way to ...
Github user twalthr commented on a diff in the pull request:
https://github.com/apache/flink/pull/2337#discussion_r77981435
--- Diff:
flink-core/src/main/java/org/apache/flink/api/common/typeinfo/TypeInfoFactory.java
---
@@ -0,0 +1,55 @@
+/*
+ * 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.common.typeinfo;
+
+import java.lang.reflect.Type;
+import java.util.Map;
+import org.apache.flink.annotation.Public;
+import org.apache.flink.api.java.typeutils.TypeExtractor;
+
+/**
+ * Base class for implementing a type information factory. A type
information factory allows for
+ * plugging-in user-defined {@link TypeInformation} into the Flink type
system. The factory is
+ * called during the type extraction phase if the corresponding type has
been annotated with
+ * {@link TypeInfo}. In a hierarchy of types the closest factory will be
chosen while traversing
+ * upwards, however, a globally registered factory has highest precedence
+ * (see {@link TypeExtractor#registerFactory(Type, Class)}).
+ *
+ * @param type for which {@link TypeInformation} is created
+ */
+@Public
+public abstract class TypeInfoFactory {
+
+ public TypeInfoFactory() {
+ // default constructor
--- End diff --
There is no reason for it. I will remove it.
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2337: [FLINK-3042] [FLINK-3060] [types] Define a way to ...
Github user greghogan commented on a diff in the pull request:
https://github.com/apache/flink/pull/2337#discussion_r77842627
--- Diff:
flink-core/src/main/java/org/apache/flink/api/java/typeutils/TypeExtractor.java
---
@@ -792,12 +832,40 @@ else if (t instanceof Class) {
return null;
}
-
+
+ @SuppressWarnings({"unchecked", "rawtypes"})
private TypeInformation
createTypeInfoFromInput(TypeVariable returnTypeVar, ArrayList
inputTypeHierarchy, Type inType, TypeInformation inTypeInfo) {
TypeInformation info = null;
-
+
+ // use a factory to find corresponding type information to type
variable
+ final ArrayList factoryHierarchy = new
ArrayList<>(inputTypeHierarchy);
+ final TypeInfoFactory factory =
getClosestFactory(factoryHierarchy, inType);
+ if (factory != null) {
+ // the type that defines the factory is last in factory
hierarchy
+ final Type factoryDefiningType =
factoryHierarchy.get(factoryHierarchy.size() - 1);
+ // defining type has generics, the factory need to be
asked for a mapping of subtypes to type information
+ if (factoryDefiningType instanceof ParameterizedType) {
--- End diff --
A TypeInformation is created here only with factories of parameterized
types?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2337: [FLINK-3042] [FLINK-3060] [types] Define a way to ...
Github user greghogan commented on a diff in the pull request:
https://github.com/apache/flink/pull/2337#discussion_r77827675
--- Diff:
flink-core/src/main/java/org/apache/flink/api/common/typeinfo/TypeInfoFactory.java
---
@@ -0,0 +1,55 @@
+/*
+ * 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.common.typeinfo;
+
+import java.lang.reflect.Type;
+import java.util.Map;
+import org.apache.flink.annotation.Public;
+import org.apache.flink.api.java.typeutils.TypeExtractor;
+
+/**
+ * Base class for implementing a type information factory. A type
information factory allows for
+ * plugging-in user-defined {@link TypeInformation} into the Flink type
system. The factory is
+ * called during the type extraction phase if the corresponding type has
been annotated with
+ * {@link TypeInfo}. In a hierarchy of types the closest factory will be
chosen while traversing
+ * upwards, however, a globally registered factory has highest precedence
+ * (see {@link TypeExtractor#registerFactory(Type, Class)}).
+ *
+ * @param type for which {@link TypeInformation} is created
+ */
+@Public
+public abstract class TypeInfoFactory {
+
+ public TypeInfoFactory() {
+ // default constructor
--- End diff --
What is the reason for the empty no-arg 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2337: [FLINK-3042] [FLINK-3060] [types] Define a way to ...
Github user twalthr commented on a diff in the pull request:
https://github.com/apache/flink/pull/2337#discussion_r7227
--- Diff:
flink-core/src/main/java/org/apache/flink/api/common/typeinfo/TypeInformation.java
---
@@ -122,14 +123,25 @@
public abstract Class getTypeClass();
/**
-* Returns the generic parameters of this type.
+* Optional method for giving Flink's type extraction system
information about the mapping
+* of a generic type parameter to the type information of a subtype.
This information is necessary
+* in cases where type information should be deduced from an input type.
*
-* @return The list of generic parameters. This list can be empty.
+* For instance, a method for a {@link Tuple2} would look like this:
+*
+* Map m = new HashMap();
+* m.put("T0", this.getTypeAt(0));
+* m.put("T1", this.getTypeAt(1));
+* return m;
+*
+*
+* @return map of inferred subtypes; it must not contain all generic
parameters as key;
--- End diff --
You are right, "it doesn't have to contain..." would be better. What I
wanted to say is: It is also ok to just supply partial type information for
generic parameters (so returning an empty map is always acceptable).
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2337: [FLINK-3042] [FLINK-3060] [types] Define a way to ...
Github user greghogan commented on a diff in the pull request:
https://github.com/apache/flink/pull/2337#discussion_r77714875
--- Diff:
flink-core/src/main/java/org/apache/flink/api/common/typeinfo/TypeInformation.java
---
@@ -122,14 +123,25 @@
public abstract Class getTypeClass();
/**
-* Returns the generic parameters of this type.
+* Optional method for giving Flink's type extraction system
information about the mapping
+* of a generic type parameter to the type information of a subtype.
This information is necessary
+* in cases where type information should be deduced from an input type.
*
-* @return The list of generic parameters. This list can be empty.
+* For instance, a method for a {@link Tuple2} would look like this:
+*
+* Map m = new HashMap();
+* m.put("T0", this.getTypeAt(0));
+* m.put("T1", this.getTypeAt(1));
+* return m;
+*
+*
+* @return map of inferred subtypes; it must not contain all generic
parameters as key;
--- End diff --
What is meant by "it must not contain all generic parameters as key"?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2337: [FLINK-3042] [FLINK-3060] [types] Define a way to ...
Github user StephanEwen commented on a diff in the pull request: https://github.com/apache/flink/pull/2337#discussion_r75097330 --- Diff: flink-core/src/test/java/org/apache/flink/api/java/typeutils/TypeInfoFactoryTest.java --- @@ -0,0 +1,482 @@ +/* + * 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 java.lang.reflect.Type; +import java.util.HashMap; +import java.util.Map; +import org.apache.flink.api.common.ExecutionConfig; +import org.apache.flink.api.common.functions.InvalidTypesException; +import org.apache.flink.api.common.functions.MapFunction; +import static org.apache.flink.api.common.typeinfo.BasicTypeInfo.BOOLEAN_TYPE_INFO; --- End diff -- Minor nitpick: Most code styles (through not enforced here) put all static imports below the regular imports. --- 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request #2337: [FLINK-3042] [FLINK-3060] [types] Define a way to ...
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/2337#discussion_r75097127
--- Diff:
flink-core/src/test/java/org/apache/flink/api/java/typeutils/TypeInfoFactoryTest.java
---
@@ -0,0 +1,482 @@
+/*
+ * 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 java.lang.reflect.Type;
+import java.util.HashMap;
+import java.util.Map;
+import org.apache.flink.api.common.ExecutionConfig;
+import org.apache.flink.api.common.functions.InvalidTypesException;
+import org.apache.flink.api.common.functions.MapFunction;
+import static
org.apache.flink.api.common.typeinfo.BasicTypeInfo.BOOLEAN_TYPE_INFO;
+import static
org.apache.flink.api.common.typeinfo.BasicTypeInfo.DOUBLE_TYPE_INFO;
+import static
org.apache.flink.api.common.typeinfo.BasicTypeInfo.FLOAT_TYPE_INFO;
+import static
org.apache.flink.api.common.typeinfo.BasicTypeInfo.INT_TYPE_INFO;
+import static
org.apache.flink.api.common.typeinfo.BasicTypeInfo.STRING_TYPE_INFO;
+import org.apache.flink.api.common.typeinfo.TypeInfo;
+import org.apache.flink.api.common.typeinfo.TypeInfoFactory;
+import org.apache.flink.api.common.typeinfo.TypeInformation;
+import org.apache.flink.api.common.typeutils.TypeSerializer;
+import org.apache.flink.api.java.tuple.Tuple1;
+import org.apache.flink.api.java.tuple.Tuple2;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+import org.junit.Test;
+
+/**
+ * Tests for extracting {@link
org.apache.flink.api.common.typeinfo.TypeInformation} from types
+ * using a {@link org.apache.flink.api.common.typeinfo.TypeInfoFactory}
+ */
+@SuppressWarnings({"unchecked", "rawtypes"})
+public class TypeInfoFactoryTest {
+
+ @Test
+ public void testSimpleType() {
+ TypeInformation ti =
TypeExtractor.createTypeInfo(IntLike.class);
+ assertEquals(INT_TYPE_INFO, ti);
+
+ ti = TypeExtractor.getForClass(IntLike.class);
+ assertEquals(INT_TYPE_INFO, ti);
+
+ ti = TypeExtractor.getForObject(new IntLike());
+ assertEquals(INT_TYPE_INFO, ti);
+ }
+
+ @Test
+ public void testMyEitherGenericType() {
+ MapFunction f = new MyEitherMapper();
+ TypeInformation ti = TypeExtractor.getMapReturnTypes(f,
(TypeInformation) BOOLEAN_TYPE_INFO);
+ assertTrue(ti instanceof EitherTypeInfo);
+ EitherTypeInfo eti = (EitherTypeInfo) ti;
+ assertEquals(BOOLEAN_TYPE_INFO, eti.getLeftType());
+ assertEquals(STRING_TYPE_INFO, eti.getRightType());
+ }
+
+ @Test
+ public void testMyOptionGenericType() {
+ TypeInformation inTypeInfo = new MyOptionTypeInfo(new
TupleTypeInfo(BOOLEAN_TYPE_INFO, STRING_TYPE_INFO));
+ MapFunction f = new MyOptionMapper();
+ TypeInformation ti = TypeExtractor.getMapReturnTypes(f,
inTypeInfo);
+ assertTrue(ti instanceof MyOptionTypeInfo);
+ MyOptionTypeInfo oti = (MyOptionTypeInfo) ti;
+ assertTrue(oti.getInnerType() instanceof TupleTypeInfo);
+ TupleTypeInfo tti = (TupleTypeInfo) oti.getInnerType();
+ assertEquals(BOOLEAN_TYPE_INFO, tti.getTypeAt(0));
+ assertEquals(BOOLEAN_TYPE_INFO, tti.getTypeAt(1));
+ }
+
+ @Test
+ public void testMyTuple2() {
+ TypeInformation inTypeInfo = new TupleTypeInfo(new
MyTupleTypeInfo(DOUBLE_TYPE_INFO, STRING_TYPE_INFO));
+ MapFunction f = new MyTupleMapperL2();
+ TypeInformation ti = TypeExtractor.getMapReturnTypes(f,
inTypeInfo);
+ assertTrue(ti instanceof TupleTypeInfo);
+ TupleTypeInfo tti = (TupleTypeInfo) ti;
+ assertTrue(tti.getTypeAt(0) instanceof MyTupleTypeInfo);
+ MyTupleTypeInfo mtti = (MyTupleTypeInfo) tti.getTypeAt(0);
+
[GitHub] flink pull request #2337: [FLINK-3042] [FLINK-3060] [types] Define a way to ...
Github user StephanEwen commented on a diff in the pull request:
https://github.com/apache/flink/pull/2337#discussion_r75097072
--- Diff:
flink-core/src/test/java/org/apache/flink/api/java/typeutils/TypeInfoFactoryTest.java
---
@@ -0,0 +1,482 @@
+/*
+ * 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 java.lang.reflect.Type;
+import java.util.HashMap;
+import java.util.Map;
+import org.apache.flink.api.common.ExecutionConfig;
+import org.apache.flink.api.common.functions.InvalidTypesException;
+import org.apache.flink.api.common.functions.MapFunction;
+import static
org.apache.flink.api.common.typeinfo.BasicTypeInfo.BOOLEAN_TYPE_INFO;
+import static
org.apache.flink.api.common.typeinfo.BasicTypeInfo.DOUBLE_TYPE_INFO;
+import static
org.apache.flink.api.common.typeinfo.BasicTypeInfo.FLOAT_TYPE_INFO;
+import static
org.apache.flink.api.common.typeinfo.BasicTypeInfo.INT_TYPE_INFO;
+import static
org.apache.flink.api.common.typeinfo.BasicTypeInfo.STRING_TYPE_INFO;
+import org.apache.flink.api.common.typeinfo.TypeInfo;
+import org.apache.flink.api.common.typeinfo.TypeInfoFactory;
+import org.apache.flink.api.common.typeinfo.TypeInformation;
+import org.apache.flink.api.common.typeutils.TypeSerializer;
+import org.apache.flink.api.java.tuple.Tuple1;
+import org.apache.flink.api.java.tuple.Tuple2;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+import org.junit.Test;
+
+/**
+ * Tests for extracting {@link
org.apache.flink.api.common.typeinfo.TypeInformation} from types
+ * using a {@link org.apache.flink.api.common.typeinfo.TypeInfoFactory}
+ */
+@SuppressWarnings({"unchecked", "rawtypes"})
+public class TypeInfoFactoryTest {
+
+ @Test
+ public void testSimpleType() {
+ TypeInformation ti =
TypeExtractor.createTypeInfo(IntLike.class);
--- End diff --
I would prefer to not use raw types unless really necessary. Can't you use
`TypeInformation`?
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #2337: [FLINK-3042] [FLINK-3060] [types] Define a way to ...
GitHub user twalthr opened a pull request:
https://github.com/apache/flink/pull/2337
[FLINK-3042] [FLINK-3060] [types] Define a way to let types create their
own TypeInformation
Thanks for contributing to Apache Flink. Before you open your pull request,
please take the following check list into consideration.
If your changes take all of the items into account, feel free to open your
pull request. For more information and/or questions please refer to the [How To
Contribute guide](http://flink.apache.org/how-to-contribute.html).
In addition to going through the list, please provide a meaningful
description of your changes.
- [x] General
- The pull request references the related JIRA issue ("[FLINK-XXX] Jira
title text")
- The pull request addresses only one issue
- Each commit in the PR has a meaningful commit message (including the
JIRA id)
- [ ] Documentation
- Documentation has been added for new functionality
- Old documentation affected by the pull request has been updated
- JavaDoc for public methods has been added
- [x] Tests & Build
- Functionality added by the pull request is covered by tests
- `mvn clean verify` has been executed successfully locally or a Travis
build has passed
This PR introduces so-called `TypeInfoFactory`s. A type information factory
allows for plugging-in user-defined TypeInformation into the Flink type system.
The factory is called during the type extraction phase if the corresponding
type has been annotated with `TypeInfo` or registered globally using
`TypeExtractor.registerFactory(Type, Class)`.
In a hierarchy of types the closest factory will be chosen while traversing
upwards, however, a globally registered factory has highest precedence.
Although this PR further increases the complexity of the `TypeExtractor`.
`TypeInfoFactory`s could simplify the code in future. We could implement
factories for all Flink built-in types and thus split up the extraction code
into type specific factories.
I haven't updated the doc yet. I will add documentation after I got
feedback.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/twalthr/flink FLINK-3042
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/2337.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 #2337
commit 3481cf4da45292fcbf541e0ecd885a663373837b
Author: twalthr
Date: 2016-08-04T15:01:08Z
[FLINK-3042] [FLINK-3060] [types] Define a way to let types create their
own TypeInformation
---
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
