[GitHub] flink pull request: [FLINK-2565] Support primitive Arrays as keys
Github user StephanEwen commented on the pull request: https://github.com/apache/flink/pull/1043#issuecomment-134639235 Looks good +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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] flink pull request: [FLINK-2565] Support primitive Arrays as keys
Github user zentol commented on the pull request: https://github.com/apache/flink/pull/1043#issuecomment-134618042 @StephanEwen I've reimplemented hashCode() and compare() accordingly. --- 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: [FLINK-2565] Support primitive Arrays as keys
Github user StephanEwen commented on the pull request: https://github.com/apache/flink/pull/1043#issuecomment-134603166 I think you currently pay quite a performance price for implementing the compare methods in such a ways that they delegate to the `ByteComparator` and `FloatComparator`. In order to call those, every single byte or float need to be boxed. That can be avoided if the method directly implements the array comparisons on the primitive 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: [FLINK-2565] Support primitive Arrays as keys
Github user fhueske commented on the pull request: https://github.com/apache/flink/pull/1043#issuecomment-134665239 Thanks! Good to merge, IMO. --- 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: [FLINK-2565] Support primitive Arrays as keys
Github user zentol commented on the pull request: https://github.com/apache/flink/pull/1043#issuecomment-134663435 @fhueske Added the test you requested. --- 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: [FLINK-2565] Support primitive Arrays as keys
Github user tillrohrmann commented on a diff in the pull request:
https://github.com/apache/flink/pull/1043#discussion_r37842526
--- Diff:
flink-java/src/test/java/org/apache/flink/api/java/operator/GroupingTest.java
---
@@ -127,6 +128,15 @@ public void testGroupByKeyFields5() {
}
@Test
+ public void testGroupByKeyFields6() {
--- End diff --
A more descriptive name or a comment would be helpful to understand the
test case.
---
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: [FLINK-2565] Support primitive Arrays as keys
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/1043#discussion_r37844225
--- Diff:
flink-java/src/test/java/org/apache/flink/api/java/operator/GroupingTest.java
---
@@ -127,6 +128,15 @@ public void testGroupByKeyFields5() {
}
@Test
+ public void testGroupByKeyFields6() {
--- End diff --
fixed
---
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: [FLINK-2565] Support primitive Arrays as keys
Github user fhueske commented on the pull request: https://github.com/apache/flink/pull/1043#issuecomment-134536795 Looks good. Maybe add one more test that executes one of the comparators, for example in GroupReduceITCase? --- 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: [FLINK-2565] Support primitive Arrays as keys
Github user zentol commented on the pull request: https://github.com/apache/flink/pull/1043#issuecomment-134538956 The Python API will heavily rely on this, so actual use-cases should be sufficiently covered there. --- 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: [FLINK-2565] Support primitive Arrays as keys
Github user fhueske commented on the pull request: https://github.com/apache/flink/pull/1043#issuecomment-134563069 This change touches the DataSet API, so there should be a test there as well, IMO. Also this feature would not be tested until the Python API is adapted. --- 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: [FLINK-2565] Support primitive Arrays as keys
Github user zentol commented on the pull request: https://github.com/apache/flink/pull/1043#issuecomment-134565718 Fair enough, I'll add another test. --- 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: [FLINK-2565] Support primitive Arrays as keys
Github user tillrohrmann commented on the pull request: https://github.com/apache/flink/pull/1043#issuecomment-134167882 Good work @zentol. Could we also add a test case where we actually use a primitive array as a key for some operation? Other than that, LGTM +1. --- 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: [FLINK-2565] Support primitive Arrays as keys
Github user zentol commented on the pull request: https://github.com/apache/flink/pull/1043#issuecomment-134345186 I've added a test case to make sure a primitive array is accepted as a key. is that what you had in mind @tillrohrmann ? --- 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: [FLINK-2565] Support primitive Arrays as keys
Github user rmetzger commented on a diff in the pull request:
https://github.com/apache/flink/pull/1043#discussion_r37729380
--- Diff:
flink-core/src/main/java/org/apache/flink/api/common/typeutils/base/array/BooleanPrimitiveArrayComparator.java
---
@@ -0,0 +1,56 @@
+/*
+ * 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.typeutils.base.array;
+
+import static java.lang.Math.min;
+import org.apache.flink.api.common.typeutils.TypeComparator;
+import org.apache.flink.api.common.typeutils.base.BooleanComparator;
+
+public class BooleanPrimitiveArrayComparator extends
PrimitiveArrayComparatorboolean[], BooleanComparator {
+ public BooleanPrimitiveArrayComparator(boolean ascending) {
+ super(ascending, new BooleanComparator(ascending));
+ }
+
+ @Override
+ public int hash(boolean[] record) {
+ int result = 0;
+ for (boolean field : record) {
+ result += comparator.hash(field);
+ }
+ return result;
+ }
+
+ @Override
+ public int compare(boolean[] first, boolean[] second) {
+ for (int x = 0; x min(first.length, second.length); x++) {
+ int cmp = comparator.compare(first[x], second[x]);
+ if (cmp != 0) {
+ return cmp;
--- End diff --
I don't know the semantics of the comparators out of the top of my head,
but are you sure you don't have to invert depending on ascending here?
---
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: [FLINK-2565] Support primitive Arrays as keys
Github user rmetzger commented on a diff in the pull request:
https://github.com/apache/flink/pull/1043#discussion_r37729602
--- Diff:
flink-core/src/main/java/org/apache/flink/api/common/typeutils/base/array/BooleanPrimitiveArrayComparator.java
---
@@ -0,0 +1,56 @@
+/*
+ * 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.typeutils.base.array;
+
+import static java.lang.Math.min;
+import org.apache.flink.api.common.typeutils.TypeComparator;
+import org.apache.flink.api.common.typeutils.base.BooleanComparator;
+
+public class BooleanPrimitiveArrayComparator extends
PrimitiveArrayComparatorboolean[], BooleanComparator {
+ public BooleanPrimitiveArrayComparator(boolean ascending) {
+ super(ascending, new BooleanComparator(ascending));
+ }
+
+ @Override
+ public int hash(boolean[] record) {
+ int result = 0;
+ for (boolean field : record) {
+ result += comparator.hash(field);
+ }
+ return result;
+ }
+
+ @Override
+ public int compare(boolean[] first, boolean[] second) {
+ for (int x = 0; x min(first.length, second.length); x++) {
+ int cmp = comparator.compare(first[x], second[x]);
+ if (cmp != 0) {
+ return cmp;
--- End diff --
Okay, makes sense.
---
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: [FLINK-2565] Support primitive Arrays as keys
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/1043#discussion_r37729559
--- Diff:
flink-core/src/main/java/org/apache/flink/api/common/typeutils/base/array/BooleanPrimitiveArrayComparator.java
---
@@ -0,0 +1,56 @@
+/*
+ * 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.typeutils.base.array;
+
+import static java.lang.Math.min;
+import org.apache.flink.api.common.typeutils.TypeComparator;
+import org.apache.flink.api.common.typeutils.base.BooleanComparator;
+
+public class BooleanPrimitiveArrayComparator extends
PrimitiveArrayComparatorboolean[], BooleanComparator {
+ public BooleanPrimitiveArrayComparator(boolean ascending) {
+ super(ascending, new BooleanComparator(ascending));
+ }
+
+ @Override
+ public int hash(boolean[] record) {
+ int result = 0;
+ for (boolean field : record) {
+ result += comparator.hash(field);
+ }
+ return result;
+ }
+
+ @Override
+ public int compare(boolean[] first, boolean[] second) {
+ for (int x = 0; x min(first.length, second.length); x++) {
+ int cmp = comparator.compare(first[x], second[x]);
+ if (cmp != 0) {
+ return cmp;
--- End diff --
no, because the underlying comparator has used the ascending flag already.
It's always a headache thinking about the comparator logic :(
---
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: [FLINK-2565] Support primitive Arrays as keys
Github user StephanEwen commented on the pull request: https://github.com/apache/flink/pull/1043#issuecomment-134397104 Looks good! --- 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: [FLINK-2565] Support primitive Arrays as keys
GitHub user zentol opened a pull request: https://github.com/apache/flink/pull/1043 [FLINK-2565] Support primitive Arrays as keys Adds a comparator and test for every primitive array type. Modifies the CustomType2 class in GroupingTest to retain a field with an unsupported type. You can merge this pull request into a Git repository by running: $ git pull https://github.com/zentol/flink 2565_arrayKey Alternatively you can review and apply these changes as the patch at: https://github.com/apache/flink/pull/1043.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 #1043 commit 7551a47e60186a91ecc1df364f1a3ae0c9474a3f Author: zentol s.mo...@web.de Date: 2015-08-23T13:36:47Z [FLINK-2565] Support primitive Arrays as keys --- 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. ---
