From af8553b9feb9fca2f9624512d046d383d45a2584 Mon Sep 17 00:00:00 2001
From: Thomas White <tomwhite@apache.org>
Date: Fri, 1 Feb 2013 15:03:35 +0000
Subject: [PATCH] HADOOP-9124. SortedMapWritable violates contract of Map
 interface for equals() and hashCode(). Contributed by Surenkumar Nihalani

git-svn-id: https://svn.apache.org/repos/asf/hadoop/common/trunk@1441475 13f79535-47bb-0310-9956-ffa450edef68
---
 .../hadoop-common/CHANGES.txt                 |  3 +
 .../apache/hadoop/io/SortedMapWritable.java   | 23 ++++++
 .../hadoop/io/TestSortedMapWritable.java      | 71 ++++++++++++++++++-
 3 files changed, 94 insertions(+), 3 deletions(-)

diff --git a/hadoop-common-project/hadoop-common/CHANGES.txt b/hadoop-common-project/hadoop-common/CHANGES.txt
index 43a8e483345..b1cc950a747 100644
--- a/hadoop-common-project/hadoop-common/CHANGES.txt
+++ b/hadoop-common-project/hadoop-common/CHANGES.txt
@@ -592,6 +592,9 @@ Release 2.0.3-alpha - Unreleased
     HADOOP-9221. Convert remaining xdocs to APT. (Andy Isaacson via atm)
 
     HADOOP-8981. TestMetricsSystemImpl fails on Windows. (Xuan Gong via suresh)
+    
+    HADOOP-9124. SortedMapWritable violates contract of Map interface for
+    equals() and hashCode(). (Surenkumar Nihalani via tomwhite)
 
 Release 2.0.2-alpha - 2012-09-07 
 
diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/SortedMapWritable.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/SortedMapWritable.java
index eee744ec6a2..c80af15c9e2 100644
--- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/SortedMapWritable.java
+++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/io/SortedMapWritable.java
@@ -203,4 +203,27 @@ public class SortedMapWritable extends AbstractMapWritable
       e.getValue().write(out);
     }
   }
+
+  @Override
+  public boolean equals(Object obj) {
+    if (this == obj) {
+      return true;
+    }
+
+    if (obj instanceof SortedMapWritable) {
+      Map map = (Map) obj;
+      if (size() != map.size()) {
+        return false;
+      }
+
+      return entrySet().equals(map.entrySet());
+    }
+
+    return false;
+  }
+
+  @Override
+  public int hashCode() {
+    return instance.hashCode();
+  }
 }
diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/TestSortedMapWritable.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/TestSortedMapWritable.java
index 927bfc1f42d..1fbfcad7627 100644
--- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/TestSortedMapWritable.java
+++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/io/TestSortedMapWritable.java
@@ -17,15 +17,20 @@
  */
 package org.apache.hadoop.io;
 
-import java.util.Map;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertTrue;
 
-import junit.framework.TestCase;
+import java.util.Map;
+import org.junit.Test;
 
 /**
  * Tests SortedMapWritable
  */
-public class TestSortedMapWritable extends TestCase {
+public class TestSortedMapWritable {
   /** the test */
+  @Test
   @SuppressWarnings("unchecked")
   public void testSortedMapWritable() {
     Text[] keys = {
@@ -90,6 +95,7 @@ public class TestSortedMapWritable extends TestCase {
   /**
    * Test that number of "unknown" classes is propagated across multiple copies.
    */
+  @Test
   @SuppressWarnings("deprecation")
   public void testForeignClass() {
     SortedMapWritable inMap = new SortedMapWritable();
@@ -99,4 +105,63 @@ public class TestSortedMapWritable extends TestCase {
     SortedMapWritable copyOfCopy = new SortedMapWritable(outMap);
     assertEquals(1, copyOfCopy.getNewClasses());
   }
+  
+  /**
+   * Tests if equal and hashCode method still hold the contract.
+   */
+  @Test
+  public void testEqualsAndHashCode() {
+    String failureReason;
+    SortedMapWritable mapA = new SortedMapWritable();
+    SortedMapWritable mapB = new SortedMapWritable();
+    
+    // Sanity checks
+    failureReason = "SortedMapWritable couldn't be initialized. Got null reference";
+    assertNotNull(failureReason, mapA);
+    assertNotNull(failureReason, mapB);
+    
+    // Basic null check
+    assertFalse("equals method returns true when passed null", mapA.equals(null));
+    
+    // When entry set is empty, they should be equal
+    assertTrue("Two empty SortedMapWritables are no longer equal", mapA.equals(mapB));
+    
+    // Setup
+    Text[] keys = {
+        new Text("key1"),
+        new Text("key2")
+    };
+    
+    BytesWritable[] values = {
+        new BytesWritable("value1".getBytes()),
+        new BytesWritable("value2".getBytes())
+    };
+    
+    mapA.put(keys[0], values[0]);
+    mapB.put(keys[1], values[1]);
+    
+    // entrySets are different
+    failureReason = "Two SortedMapWritables with different data are now equal";
+    assertTrue(failureReason, mapA.hashCode() != mapB.hashCode());
+    assertTrue(failureReason, !mapA.equals(mapB));
+    assertTrue(failureReason, !mapB.equals(mapA));
+    
+    mapA.put(keys[1], values[1]);
+    mapB.put(keys[0], values[0]);
+    
+    // entrySets are now same
+    failureReason = "Two SortedMapWritables with same entry sets formed in different order are now different";
+    assertEquals(failureReason, mapA.hashCode(), mapB.hashCode());
+    assertTrue(failureReason, mapA.equals(mapB));
+    assertTrue(failureReason, mapB.equals(mapA));
+    
+    // Let's check if entry sets of same keys but different values
+    mapA.put(keys[0], values[1]);
+    mapA.put(keys[1], values[0]);
+    
+    failureReason = "Two SortedMapWritables with different content are now equal";
+    assertTrue(failureReason, mapA.hashCode() != mapB.hashCode());
+    assertTrue(failureReason, !mapA.equals(mapB));
+    assertTrue(failureReason, !mapB.equals(mapA));
+  }
 }