You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by tillrohrmann <gi...@git.apache.org> on 2015/09/15 19:13:05 UTC
[GitHub] flink pull request: [FLINK-2637] [api-breaking] [scala, types] Add...
GitHub user tillrohrmann opened a pull request:
https://github.com/apache/flink/pull/1134
[FLINK-2637] [api-breaking] [scala, types] Adds equals and hashCode method to TypeInformations and TypeSerializers
Adds abstract `equals`, `hashCode`, `canEqual` and `toString` methods to `TypeInformation`. Adds missing implementations to subtypes. The `canEqual(Object obj)` method returns true iff the `obj` can be equaled with the called instance.
Adds abstract `equals`, `hashCode` and `canEqual` methods to `TypeSerializer`.
Makes `CompositeType` subtypes serializable by removing non-serializable fields which were only used for the comparator construction. The comparator construction is now realized within a builder object which keeps the intermediate state. Consequently, the PR #943 is now obsolete and can be closed.
Refactors the `ObjectArrayTypeInfo` so that the type extraction logic now happens within the `TypeExtractor` and no longer in the `TypeInformation` subtype.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/tillrohrmann/flink fixOptionTypeInfo
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/1134.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 #1134
----
commit 35a18b3f5148ec3fdddc318ea4d5971427fffda3
Author: Till Rohrmann <tr...@apache.org>
Date: 2015-09-07T23:12:09Z
[FLINK-2637] [api-breaking] [scala, types] Adds equals and hashCode method to TypeInformations and TypeSerializers. Fixes ObjectArrayTypeInfo. Makes CompositeTypes serializable.
Adds test for equality relation's symmetric property
----
---
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: [FLINK-2637] [api-breaking] [scala, types] Add...
Posted by rmetzger <gi...@git.apache.org>.
Github user rmetzger commented on the pull request:
https://github.com/apache/flink/pull/1134#issuecomment-141167625
Hey @rxin, I'm really sorry that this happened. Max told me that he stopped the application on AppEngine as soon as he learned that the tool is not working as expected.
As far as I can see the JIRA activity of Max's user account, the tool stopped changing Spark issues 3 hours ago.
Please let me know if more Spark JIRAs are affected.
I'll ask @mxm to remove the automated comments again and change the links back to their old value.
---
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: [FLINK-2637] [api-breaking] [scala, types] Add...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/flink/pull/1134#issuecomment-141163582
I understand you took the bot from Spark, but can you change the JIRA link? It is spamming the Spark JIRA right now.
https://issues.apache.org/jira/secure/ViewProfile.jspa?name=mxm
---
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: [FLINK-2637] [api-breaking] [scala, types] Add...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/1134#discussion_r39634683
--- Diff: flink-java/src/test/java/org/apache/flink/api/java/typeutils/runtime/ValueTypeInfoTest.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.runtime;
--- End diff --
Good catch. Have removed 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2637] [api-breaking] [scala, types] Add...
Posted by chiwanpark <gi...@git.apache.org>.
Github user chiwanpark commented on a diff in the pull request:
https://github.com/apache/flink/pull/1134#discussion_r39650722
--- Diff: flink-core/src/main/java/org/apache/flink/api/common/typeinfo/IntegerTypeInfo.java ---
@@ -18,15 +18,33 @@
package org.apache.flink.api.common.typeinfo;
+import com.google.common.base.Preconditions;
+import com.google.common.collect.Sets;
import org.apache.flink.api.common.typeutils.TypeComparator;
import org.apache.flink.api.common.typeutils.TypeSerializer;
+import java.util.Set;
+
/**
- * Type information for numeric primitive types (int, long, double, byte, ...).
+ * Type information for numeric integer primitive types: int, long, byte, short, boolean, character.
*/
public class IntegerTypeInfo<T> extends NumericTypeInfo<T> {
+ private static final long serialVersionUID = -8068827354966766955L;
+
+ private static final Set<Class<?>> integerTypes = Sets.<Class<?>>newHashSet(
+ Integer.class,
+ Long.class,
+ Byte.class,
+ Short.class,
+ Boolean.class,
--- End diff --
Is `Boolean` numeric?
---
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: [FLINK-2637] [api-breaking] [scala, types] Add...
Posted by mxm <gi...@git.apache.org>.
Github user mxm commented on the pull request:
https://github.com/apache/flink/pull/1134#issuecomment-141231467
Hi @rxin, I'm really sorry for bothering. I was trying out the Spark Pull Request Dashboard with all the settings adapted to the Flink JIRA and Flink Github repository. Only when it was too late, I saw that there were some hard-coded URLs in the the source code that also needed to be adapted. This caused some unexpected behavior (Flink issue number in Flink pull requests being associated with Spark issues numbers). I've removed all comments and links and apologize very much for the inconvenience this caused. I've definitely learned a lesson out of this one.
Thanks for your patience and understanding!
---
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: [FLINK-2637] [api-breaking] [scala, types] Add...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on the pull request:
https://github.com/apache/flink/pull/1134#issuecomment-140771931
Thanks for the fast update, LGTM.
Will merge this tomorrow unless somebody speaks up.
---
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: [FLINK-2637] [api-breaking] [scala, types] Add...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/1134#discussion_r39633196
--- Diff: flink-java/src/main/java/org/apache/flink/api/java/typeutils/PojoField.java ---
@@ -68,4 +72,24 @@ private void readObject(ObjectInputStream in)
public String toString() {
return "PojoField " + field.getDeclaringClass() + "." + field.getName() + " (" + type + ")";
}
+
+ @Override
+ public boolean equals(Object obj) {
+ if (obj instanceof PojoField) {
--- End diff --
You're right. Will refactor the public access to the fields and include the field in the `equals` and `hashCode` calculation.
---
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: [FLINK-2637] [api-breaking] [scala, types] Add...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/1134#discussion_r39634911
--- Diff: flink-staging/flink-table/src/main/scala/org/apache/flink/api/table/typeinfo/RenamingProxyTypeInfo.scala ---
@@ -31,7 +32,8 @@ import org.apache.flink.api.common.typeutils.{CompositeType, TypeComparator, Typ
*/
class RenamingProxyTypeInfo[T](
tpe: CompositeType[T],
--- End diff --
Yes, will the `vals`.
---
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: [FLINK-2637] [api-breaking] [scala, types] Add...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on a diff in the pull request:
https://github.com/apache/flink/pull/1134#discussion_r39612554
--- Diff: flink-core/src/main/java/org/apache/flink/api/common/typeinfo/NumericTypeInfo.java ---
@@ -18,16 +18,36 @@
package org.apache.flink.api.common.typeinfo;
+import com.google.common.base.Preconditions;
+import com.google.common.collect.Sets;
import org.apache.flink.api.common.typeutils.TypeComparator;
import org.apache.flink.api.common.typeutils.TypeSerializer;
+import java.util.Set;
+
/**
- * Type information for numeric primitive types (int, long, double, byte, ...).
+ * Type information for numeric primitive types: int, long, double, byte, short, float, char,
+ * boolean.
--- End diff --
Boolean?
---
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: [FLINK-2637] [api-breaking] [scala, types] Add...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/1134#discussion_r39632768
--- Diff: flink-core/src/main/java/org/apache/flink/api/common/typeinfo/NumericTypeInfo.java ---
@@ -18,16 +18,36 @@
package org.apache.flink.api.common.typeinfo;
+import com.google.common.base.Preconditions;
+import com.google.common.collect.Sets;
import org.apache.flink.api.common.typeutils.TypeComparator;
import org.apache.flink.api.common.typeutils.TypeSerializer;
+import java.util.Set;
+
/**
- * Type information for numeric primitive types (int, long, double, byte, ...).
+ * Type information for numeric primitive types: int, long, double, byte, short, float, char,
+ * boolean.
--- End diff --
Good catch. 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2637] [api-breaking] [scala, types] Add...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on a diff in the pull request:
https://github.com/apache/flink/pull/1134#discussion_r39630227
--- Diff: flink-scala/src/main/scala/org/apache/flink/api/scala/typeutils/OptionSerializer.scala ---
@@ -20,6 +20,8 @@ package org.apache.flink.api.scala.typeutils
import org.apache.flink.api.common.typeutils.TypeSerializer
import org.apache.flink.core.memory.{DataOutputView, DataInputView}
+import scala.xml.dtd.ContentModel._labelT
--- End diff --
?
---
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: [FLINK-2637] [api-breaking] [scala, types] Add...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/1134#discussion_r39725082
--- Diff: flink-core/src/main/java/org/apache/flink/api/common/typeinfo/IntegerTypeInfo.java ---
@@ -18,15 +18,33 @@
package org.apache.flink.api.common.typeinfo;
+import com.google.common.base.Preconditions;
+import com.google.common.collect.Sets;
import org.apache.flink.api.common.typeutils.TypeComparator;
import org.apache.flink.api.common.typeutils.TypeSerializer;
+import java.util.Set;
+
/**
- * Type information for numeric primitive types (int, long, double, byte, ...).
+ * Type information for numeric integer primitive types: int, long, byte, short, boolean, character.
*/
public class IntegerTypeInfo<T> extends NumericTypeInfo<T> {
+ private static final long serialVersionUID = -8068827354966766955L;
+
+ private static final Set<Class<?>> integerTypes = Sets.<Class<?>>newHashSet(
+ Integer.class,
+ Long.class,
+ Byte.class,
+ Short.class,
+ Boolean.class,
--- End diff --
Nope, I missed that. 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request: [FLINK-2637] [api-breaking] [scala, types] Add...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on a diff in the pull request:
https://github.com/apache/flink/pull/1134#discussion_r39630864
--- Diff: flink-staging/flink-table/src/main/scala/org/apache/flink/api/table/typeinfo/RenamingProxyTypeInfo.scala ---
@@ -31,7 +32,8 @@ import org.apache.flink.api.common.typeutils.{CompositeType, TypeComparator, Typ
*/
class RenamingProxyTypeInfo[T](
tpe: CompositeType[T],
--- End diff --
val?
---
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: [FLINK-2637] [api-breaking] [scala, types] Add...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/flink/pull/1134
---
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: [FLINK-2637] [api-breaking] [scala, types] Add...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on a diff in the pull request:
https://github.com/apache/flink/pull/1134#discussion_r39629488
--- Diff: flink-java/src/test/java/org/apache/flink/api/java/typeutils/runtime/ValueTypeInfoTest.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.runtime;
--- End diff --
This *TypeInfoTest is in a *.runtime package. Wrong class name or package?
---
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: [FLINK-2637] [api-breaking] [scala, types] Add...
Posted by rmetzger <gi...@git.apache.org>.
Github user rmetzger commented on the pull request:
https://github.com/apache/flink/pull/1134#issuecomment-140663091
Big change, but I didn't spot anything suspicious.
+1 to merge
---
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: [FLINK-2637] [api-breaking] [scala, types] Add...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/1134#discussion_r39634744
--- Diff: flink-scala/src/main/scala/org/apache/flink/api/scala/typeutils/OptionSerializer.scala ---
@@ -20,6 +20,8 @@ package org.apache.flink.api.scala.typeutils
import org.apache.flink.api.common.typeutils.TypeSerializer
import org.apache.flink.core.memory.{DataOutputView, DataInputView}
+import scala.xml.dtd.ContentModel._labelT
--- End diff --
Good catch 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: [FLINK-2637] [api-breaking] [scala, types] Add...
Posted by tillrohrmann <gi...@git.apache.org>.
Github user tillrohrmann commented on the pull request:
https://github.com/apache/flink/pull/1134#issuecomment-140756486
Thanks for reviewing @fhueske and @rmetzger. I addressed the comments and updated the 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 pull request: [FLINK-2637] [api-breaking] [scala, types] Add...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on a diff in the pull request:
https://github.com/apache/flink/pull/1134#discussion_r39625961
--- Diff: flink-java/src/main/java/org/apache/flink/api/java/typeutils/PojoField.java ---
@@ -68,4 +72,24 @@ private void readObject(ObjectInputStream in)
public String toString() {
return "PojoField " + field.getDeclaringClass() + "." + field.getName() + " (" + type + ")";
}
+
+ @Override
+ public boolean equals(Object obj) {
+ if (obj instanceof PojoField) {
--- End diff --
Shouldn't `equals()` and `hashcode()` include `field`?
---
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: [FLINK-2637] [api-breaking] [scala, types] Add...
Posted by fhueske <gi...@git.apache.org>.
Github user fhueske commented on the pull request:
https://github.com/apache/flink/pull/1134#issuecomment-140748369
Very good refactoring! I had just a few inline comments.
Thanks for this effort!
---
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.
---