[crunch] 01/02: CRUNCH-684: Fix .equals and .hashCode for Targets

[jw...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

jwills pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/crunch.git

commit 869aac60c9d3b5bef10b4e907ec3840be2d8c20e
Author: Andrew Olson <ao...@cerner.com>
AuthorDate: Wed May 1 16:20:17 2019 -0500

    CRUNCH-684: Fix .equals and .hashCode for Targets
    
    Signed-off-by: Josh Wills <jw...@apache.org>
---
 .../org/apache/crunch/io/impl/FileTargetImpl.java  |   5 +-
 .../apache/crunch/io/impl/FileTargetImplTest.java  | 114 +++++++++++++++++++++
 .../org/apache/crunch/io/hbase/HBaseTarget.java    |   5 +-
 .../apache/crunch/io/hbase/HBaseTargetTest.java    |  41 ++++++++
 4 files changed, 161 insertions(+), 4 deletions(-)

diff --git a/crunch-core/src/main/java/org/apache/crunch/io/impl/FileTargetImpl.java b/crunch-core/src/main/java/org/apache/crunch/io/impl/FileTargetImpl.java
index 9c9aaef..d48ac31 100644
--- a/crunch-core/src/main/java/org/apache/crunch/io/impl/FileTargetImpl.java
+++ b/crunch-core/src/main/java/org/apache/crunch/io/impl/FileTargetImpl.java
@@ -23,6 +23,7 @@ import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
+import java.util.Objects;
 import java.util.concurrent.Callable;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.Executors;
@@ -361,12 +362,12 @@ public class FileTargetImpl implements PathTarget {
       return false;
     }
     FileTargetImpl o = (FileTargetImpl) other;
-    return path.equals(o.path);
+    return Objects.equals(path, o.path) && Objects.equals(formatBundle, o.formatBundle);
   }
 
   @Override
   public int hashCode() {
-    return new HashCodeBuilder().append(path).toHashCode();
+    return new HashCodeBuilder().append(path).append(formatBundle).toHashCode();
   }
 
   @Override
diff --git a/crunch-core/src/test/java/org/apache/crunch/io/impl/FileTargetImplTest.java b/crunch-core/src/test/java/org/apache/crunch/io/impl/FileTargetImplTest.java
index 981e20a..3a44703 100644
--- a/crunch-core/src/test/java/org/apache/crunch/io/impl/FileTargetImplTest.java
+++ b/crunch-core/src/test/java/org/apache/crunch/io/impl/FileTargetImplTest.java
@@ -19,8 +19,10 @@ package org.apache.crunch.io.impl;
 
 
 import org.apache.commons.io.FileUtils;
+import org.apache.crunch.Target;
 import org.apache.crunch.io.SequentialFileNamingScheme;
 import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FileSystem;
 import org.apache.hadoop.fs.Path;
 import org.apache.hadoop.mapreduce.lib.output.SequenceFileOutputFormat;
 import org.junit.Rule;
@@ -28,6 +30,7 @@ import org.junit.Test;
 import org.junit.rules.TemporaryFolder;
 
 import java.io.File;
+import java.net.URI;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.HashMap;
@@ -36,9 +39,12 @@ import java.util.Map;
 import java.util.Set;
 
 import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.CoreMatchers.not;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertThat;
 import static org.junit.matchers.JUnitMatchers.hasItem;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
 
 public class FileTargetImplTest {
 
@@ -77,4 +83,112 @@ public class FileTargetImplTest {
       assertThat(fileContents, hasItem(content));
     }
   }
+
+  @Test
+  public void testEquality() {
+    Target target = new FileTargetImpl(new Path("/path"), SequenceFileOutputFormat.class,
+        SequentialFileNamingScheme.getInstance());
+    Target target2 = new FileTargetImpl(new Path("/path"), SequenceFileOutputFormat.class,
+        SequentialFileNamingScheme.getInstance());
+
+    assertEquals(target, target2);
+    assertEquals(target.hashCode(), target2.hashCode());
+  }
+
+  @Test
+  public void testEqualityWithExtraConf() {
+    Target target = new FileTargetImpl(new Path("/path"), SequenceFileOutputFormat.class,
+        SequentialFileNamingScheme.getInstance()).outputConf("key", "value");
+    Target target2 = new FileTargetImpl(new Path("/path"), SequenceFileOutputFormat.class,
+        SequentialFileNamingScheme.getInstance()).outputConf("key", "value");
+
+    assertEquals(target, target2);
+    assertEquals(target.hashCode(), target2.hashCode());
+  }
+
+  @Test
+  public void testEqualityWithFileSystem() {
+    Path path = new Path("/path");
+    Path qualifiedPath = path.makeQualified(URI.create("scheme://cluster"), new Path("/"));
+    FileSystem fs = mock(FileSystem.class);
+    when(fs.makeQualified(path)).thenReturn(qualifiedPath);
+    Configuration conf = new Configuration(false);
+    conf.set("key", "value");
+    when(fs.getConf()).thenReturn(conf);
+
+    Target target = new FileTargetImpl(path, SequenceFileOutputFormat.class,
+        SequentialFileNamingScheme.getInstance()).fileSystem(fs);
+    Target target2 = new FileTargetImpl(path, SequenceFileOutputFormat.class,
+        SequentialFileNamingScheme.getInstance()).fileSystem(fs);
+
+    assertEquals(target, target2);
+    assertEquals(target.hashCode(), target2.hashCode());
+  }
+
+  @Test
+  public void testInequality() {
+    Target target = new FileTargetImpl(new Path("/path"), SequenceFileOutputFormat.class,
+        SequentialFileNamingScheme.getInstance());
+    Target target2 = new FileTargetImpl(new Path("/path2"), SequenceFileOutputFormat.class,
+        SequentialFileNamingScheme.getInstance());
+
+    assertThat(target, is(not(target2)));
+    assertThat(target.hashCode(), is(not(target2.hashCode())));
+  }
+
+  @Test
+  public void testInequalityWithExtraConf() {
+    Target target = new FileTargetImpl(new Path("/path"), SequenceFileOutputFormat.class,
+        SequentialFileNamingScheme.getInstance()).outputConf("key", "value");
+    Target target2 = new FileTargetImpl(new Path("/path"), SequenceFileOutputFormat.class,
+        SequentialFileNamingScheme.getInstance()).outputConf("key", "value2");
+
+    assertThat(target, is(not(target2)));
+    assertThat(target.hashCode(), is(not(target2.hashCode())));
+  }
+
+  @Test
+  public void testInequalityWithFileSystemURI() {
+    Path path = new Path("/path");
+    Path qualifiedPath = path.makeQualified(URI.create("scheme://cluster"), new Path("/"));
+    Path qualifiedPath2 = path.makeQualified(URI.create("scheme://cluster2"), new Path("/"));
+    FileSystem fs = mock(FileSystem.class);
+    FileSystem fs2 = mock(FileSystem.class);
+    when(fs.makeQualified(path)).thenReturn(qualifiedPath);
+    when(fs2.makeQualified(path)).thenReturn(qualifiedPath2);
+    when(fs.getConf()).thenReturn(new Configuration(false));
+    when(fs2.getConf()).thenReturn(new Configuration(false));
+
+    Target target = new FileTargetImpl(path, SequenceFileOutputFormat.class,
+        SequentialFileNamingScheme.getInstance()).fileSystem(fs);
+    Target target2 = new FileTargetImpl(path, SequenceFileOutputFormat.class,
+        SequentialFileNamingScheme.getInstance()).fileSystem(fs2);
+
+    assertThat(target, is(not(target2)));
+    assertThat(target.hashCode(), is(not(target2.hashCode())));
+  }
+
+  @Test
+  public void testInequalityWithFileSystemConf() {
+    Path path = new Path("/path");
+    Path qualifiedPath = path.makeQualified(URI.create("scheme://cluster"), new Path("/"));
+    FileSystem fs = mock(FileSystem.class);
+    FileSystem fs2 = mock(FileSystem.class);
+    when(fs.makeQualified(path)).thenReturn(qualifiedPath);
+    when(fs2.makeQualified(path)).thenReturn(qualifiedPath);
+    Configuration conf = new Configuration(false);
+    conf.set("key", "value");
+    Configuration conf2 = new Configuration(false);
+    conf2.set("key", "value2");
+    when(fs.getConf()).thenReturn(conf);
+    when(fs2.getConf()).thenReturn(conf2);
+
+    Target target = new FileTargetImpl(path, SequenceFileOutputFormat.class,
+        SequentialFileNamingScheme.getInstance()).fileSystem(fs);
+    Target target2 = new FileTargetImpl(path, SequenceFileOutputFormat.class,
+        SequentialFileNamingScheme.getInstance()).fileSystem(fs2);
+
+    assertThat(target, is(not(target2)));
+    assertThat(target.hashCode(), is(not(target2.hashCode())));
+  }
 }
\ No newline at end of file
diff --git a/crunch-hbase/src/main/java/org/apache/crunch/io/hbase/HBaseTarget.java b/crunch-hbase/src/main/java/org/apache/crunch/io/hbase/HBaseTarget.java
index d51dee0..87ef45c 100644
--- a/crunch-hbase/src/main/java/org/apache/crunch/io/hbase/HBaseTarget.java
+++ b/crunch-hbase/src/main/java/org/apache/crunch/io/hbase/HBaseTarget.java
@@ -19,6 +19,7 @@ package org.apache.crunch.io.hbase;
 
 import java.io.IOException;
 import java.util.Map;
+import java.util.Objects;
 
 import com.google.common.collect.Maps;
 import org.apache.commons.lang.builder.HashCodeBuilder;
@@ -75,13 +76,13 @@ public class HBaseTarget implements MapReduceTarget {
     if (!other.getClass().equals(getClass()))
       return false;
     HBaseTarget o = (HBaseTarget) other;
-    return table.equals(o.table);
+    return Objects.equals(table, o.table) && Objects.equals(extraConf, o.extraConf);
   }
 
   @Override
   public int hashCode() {
     HashCodeBuilder hcb = new HashCodeBuilder();
-    return hcb.append(table).toHashCode();
+    return hcb.append(table).append(extraConf).toHashCode();
   }
 
   @Override
diff --git a/crunch-hbase/src/test/java/org/apache/crunch/io/hbase/HBaseTargetTest.java b/crunch-hbase/src/test/java/org/apache/crunch/io/hbase/HBaseTargetTest.java
index 8faa27d..728732c 100644
--- a/crunch-hbase/src/test/java/org/apache/crunch/io/hbase/HBaseTargetTest.java
+++ b/crunch-hbase/src/test/java/org/apache/crunch/io/hbase/HBaseTargetTest.java
@@ -17,9 +17,14 @@
  */
 package org.apache.crunch.io.hbase;
 
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.CoreMatchers.not;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertThat;
 
 import java.io.IOException;
+
+import org.apache.crunch.Target;
 import org.apache.hadoop.fs.Path;
 import org.apache.hadoop.mapreduce.Job;
 import org.junit.Test;
@@ -38,4 +43,40 @@ public class HBaseTargetTest {
 
     assertEquals("12345", job.getConfiguration().get("hbase.client.scanner.timeout.period"));
   }
+
+  @Test
+  public void testEquality() {
+    Target target = new HBaseTarget("testTable");
+    Target target2 = new HBaseTarget("testTable");
+
+    assertEquals(target, target2);
+    assertEquals(target.hashCode(), target2.hashCode());
+  }
+
+  @Test
+  public void testEqualityWithExtraConf() {
+    Target target = new HBaseTarget("testTable").outputConf("key", "value");
+    Target target2 = new HBaseTarget("testTable").outputConf("key", "value");
+
+    assertEquals(target, target2);
+    assertEquals(target.hashCode(), target2.hashCode());
+  }
+
+  @Test
+  public void testInequality() {
+    Target target = new HBaseTarget("testTable");
+    Target target2 = new HBaseTarget("testTable2");
+
+    assertThat(target, is(not(target2)));
+    assertThat(target.hashCode(), is(not(target2.hashCode())));
+  }
+
+  @Test
+  public void testInequalityWithExtraConf() {
+    Target target = new HBaseTarget("testTable").outputConf("key", "value");
+    Target target2 = new HBaseTarget("testTable").outputConf("key", "value2");
+
+    assertThat(target, is(not(target2)));
+    assertThat(target.hashCode(), is(not(target2.hashCode())));
+  }
 }