[GitHub] spark issue #19553: [SPARK-22330][CORE] Linear containsKey operation for ser...
Github user Whoosh commented on the issue: https://github.com/apache/spark/pull/19553 retest this please --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #19553: [SPARK-22330][CORE] Linear containsKey operation for ser...
Github user Whoosh commented on the issue: https://github.com/apache/spark/pull/19553 Ye, why not, let's continue this circle ;) --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #19553: [SPARK-22330][CORE] Linear containsKey operation for ser...
Github user Whoosh commented on the issue: https://github.com/apache/spark/pull/19553 retest this please --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19553: [SPARK-22330][CORE] Linear containsKey operation ...
Github user Whoosh commented on a diff in the pull request:
https://github.com/apache/spark/pull/19553#discussion_r148453286
--- Diff: core/src/main/scala/org/apache/spark/api/java/JavaUtils.scala ---
@@ -43,10 +43,17 @@ private[spark] object JavaUtils {
override def size: Int = underlying.size
-override def get(key: AnyRef): B = try {
- underlying.getOrElse(key.asInstanceOf[A], null.asInstanceOf[B])
-} catch {
- case ex: ClassCastException => null.asInstanceOf[B]
+// Delegate to implementation because AbstractMap implementation
iterates over whole key set
+override def containsKey(key: AnyRef): Boolean = {
+ underlying.contains(key.asInstanceOf[A])
--- End diff --
Neveminde, maybe it's my local issues, i will commit suggested chagnes,
lets look at jenkins afterall.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #19553: [SPARK-22330][CORE] Linear containsKey operation for ser...
Github user Whoosh commented on the issue: https://github.com/apache/spark/pull/19553 retest this, please --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19553: [SPARK-22330][CORE] Linear containsKey operation ...
Github user Whoosh commented on a diff in the pull request:
https://github.com/apache/spark/pull/19553#discussion_r148451557
--- Diff: core/src/main/scala/org/apache/spark/api/java/JavaUtils.scala ---
@@ -43,10 +43,17 @@ private[spark] object JavaUtils {
override def size: Int = underlying.size
-override def get(key: AnyRef): B = try {
- underlying.getOrElse(key.asInstanceOf[A], null.asInstanceOf[B])
-} catch {
- case ex: ClassCastException => null.asInstanceOf[B]
+// Delegate to implementation because AbstractMap implementation
iterates over whole key set
+override def containsKey(key: AnyRef): Boolean = {
+ underlying.contains(key.asInstanceOf[A])
--- End diff --
@cloud-fan @srowen
So, let's have decided what we want for containsKey() here first, before i
will do next commit -)
Because suggested (key.isInstanceOf[A] &&
underlying.contains(key.asInstanceOf[A]))
will not work, it cause compile-time error with "abstract type A is
unchecked since it is eliminated by erasure" message.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark issue #19553: [SPARK-22330][CORE] Linear containsKey operation for ser...
Github user Whoosh commented on the issue: https://github.com/apache/spark/pull/19553 @cloud-fan I've checked all core tests, it was fine, should I do smt in addition to? --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19553: [SPARK-22330][CORE] Linear containsKey operation ...
Github user Whoosh commented on a diff in the pull request:
https://github.com/apache/spark/pull/19553#discussion_r147591080
--- Diff: core/src/main/scala/org/apache/spark/api/java/JavaUtils.scala ---
@@ -43,10 +43,15 @@ private[spark] object JavaUtils {
override def size: Int = underlying.size
-override def get(key: AnyRef): B = try {
- underlying.getOrElse(key.asInstanceOf[A], null.asInstanceOf[B])
-} catch {
- case ex: ClassCastException => null.asInstanceOf[B]
+// Delegate to implementation because AbstractMap implementation
iterates over whole key set
+override def containsKey(key: AnyRef): Boolean = key match {
+ case key: A => underlying.contains(key)
--- End diff --
@srowen
It can't be so. Will cause "abstract type A is unchecked since it is
eliminated by erasure" compile-time error.
As I guess, there is no need any type checking before a get because it'll
have cast to Object anyway and get(key) it's only compiling issues, please
correct me if I'm wrong, I've added a simple test for this.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19553: [SPARK-22330][CORE] Linear containsKey operation ...
Github user Whoosh commented on a diff in the pull request:
https://github.com/apache/spark/pull/19553#discussion_r147008530
--- Diff: core/src/test/scala/org/apache/spark/util/JavaUtils.scala ---
@@ -0,0 +1,37 @@
+/*
+ * 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.spark.util
+
+import org.mockito.Mockito._
+
+import org.apache.spark.SparkFunSuite
+import org.apache.spark.api.java.JavaUtils
+
+class JavaUtils extends SparkFunSuite {
+
+ test("containsKey implementation without iteratively entrySet call") {
--- End diff --
@srowen Added addition assertion, I guess, it's enough.
Btw, sorry for my slow answers(that you're have pinged me), haven't time
all three days =(
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19553: [SPARK-22330][CORE] Linear containsKey operation ...
Github user Whoosh commented on a diff in the pull request:
https://github.com/apache/spark/pull/19553#discussion_r147007902
--- Diff: core/src/main/scala/org/apache/spark/api/java/JavaUtils.scala ---
@@ -43,6 +43,13 @@ private[spark] object JavaUtils {
override def size: Int = underlying.size
+// Delegate to implementation because AbstractMap implementation
iterates over whole key set
+override def containsKey(key: AnyRef): Boolean = try {
--- End diff --
@srowen
Yeah, you're right -) I have been focused on code around. Scala isn't my
language haven't known that construction, thx.
> What about other methods -- anything worth plumbing directly to the
underlying implementation?
Yep, checked, remove(key) still plumbing, actually, I don't know how to
simply fix it, cuz there are no methods to remove it without recreating a new
Map.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19553: [SPARK-22330][CORE] Linear containsKey operation ...
Github user Whoosh commented on a diff in the pull request: https://github.com/apache/spark/pull/19553#discussion_r147006646 --- Diff: core/src/test/scala/org/apache/spark/util/JavaUtils.scala --- @@ -0,0 +1,37 @@ +/* --- End diff -- Yep, done. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #19553: [SPARK-22330][CORE] Linear containsKey operation ...
GitHub user Whoosh opened a pull request: https://github.com/apache/spark/pull/19553 [SPARK-22330][CORE] Linear containsKey operation for serialized maps â¦alization. ## What changes were proposed in this pull request? Use non-linear containsKey operation for serialized maps, lookup into underlying map. ## How was this patch tested? unit tests You can merge this pull request into a Git repository by running: $ git pull https://github.com/Whoosh/spark SPARK-22330 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/spark/pull/19553.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 #19553 commit 518b301d32a77a44812235d42f07302edcc4fda2 Author: Alexander Istomin <isto...@rutarget.ru> Date: 2017-10-22T21:46:50Z SPARK-22330 use underlying contains instead of default AbstractMap realization. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
