crunch-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From gr...@apache.org
Subject git commit: CRUNCH-380 Improvements based on Coverity scan
Date Fri, 25 Apr 2014 20:13:38 GMT
Repository: crunch
Updated Branches:
  refs/heads/master 1e142de03 -> 7d908c7c8


CRUNCH-380 Improvements based on Coverity scan

* Avoid corner case in negating Comparator return value of Integer.MIN_VALUE
* Remove dead stores
* Avoid unnecessary Math.abs on hashCode() values; it does not always return
  nonnegative values and is caught by the masking anyway
* Miscellaneous small inspection changes

Signed-off-by: Gabriel Reid <greid@apache.org>


Project: http://git-wip-us.apache.org/repos/asf/crunch/repo
Commit: http://git-wip-us.apache.org/repos/asf/crunch/commit/7d908c7c
Tree: http://git-wip-us.apache.org/repos/asf/crunch/tree/7d908c7c
Diff: http://git-wip-us.apache.org/repos/asf/crunch/diff/7d908c7c

Branch: refs/heads/master
Commit: 7d908c7c8f456d5b6acb49162dabf2a6f7a4c7ff
Parents: 1e142de
Author: Sean Owen <sowen@cloudera.com>
Authored: Thu Apr 24 10:20:20 2014 +0100
Committer: Gabriel Reid <greid@apache.org>
Committed: Fri Apr 25 17:15:20 2014 +0200

----------------------------------------------------------------------
 .../crunch/contrib/io/jdbc/DataBaseSource.java  |  2 --
 .../crunch/io/text/TextFileReaderFactory.java   |  3 +-
 .../java/org/apache/crunch/lib/Aggregate.java   |  6 +++-
 .../org/apache/crunch/lib/SecondarySort.java    |  2 --
 .../org/apache/crunch/lib/join/JoinUtils.java   |  4 +--
 .../lib/sort/ReverseWritableComparator.java     | 13 ++++++--
 .../apache/crunch/types/writable/Writables.java |  2 +-
 .../lib/AvroIndexedRecordPartitionerTest.java   |  2 +-
 .../lib/TupleWritablePartitionerTest.java       |  2 +-
 .../org/apache/crunch/io/hbase/HFileUtils.java  | 32 +++++++++++---------
 .../apache/crunch/io/hbase/HTableIterator.java  |  2 +-
 11 files changed, 42 insertions(+), 28 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/crunch/blob/7d908c7c/crunch-contrib/src/main/java/org/apache/crunch/contrib/io/jdbc/DataBaseSource.java
----------------------------------------------------------------------
diff --git a/crunch-contrib/src/main/java/org/apache/crunch/contrib/io/jdbc/DataBaseSource.java
b/crunch-contrib/src/main/java/org/apache/crunch/contrib/io/jdbc/DataBaseSource.java
index 2c51b84..d5f43f4 100644
--- a/crunch-contrib/src/main/java/org/apache/crunch/contrib/io/jdbc/DataBaseSource.java
+++ b/crunch-contrib/src/main/java/org/apache/crunch/contrib/io/jdbc/DataBaseSource.java
@@ -71,8 +71,6 @@ public class DataBaseSource<T extends DBWritable & Writable> extends
FileSourceI
     private String selectClause;
     public String countClause;
 
-    private DataBaseSource<T> dataBaseSource;
-
     public Builder(Class<T> inputClass) {
       this.inputClass = inputClass;
     }

http://git-wip-us.apache.org/repos/asf/crunch/blob/7d908c7c/crunch-core/src/main/java/org/apache/crunch/io/text/TextFileReaderFactory.java
----------------------------------------------------------------------
diff --git a/crunch-core/src/main/java/org/apache/crunch/io/text/TextFileReaderFactory.java
b/crunch-core/src/main/java/org/apache/crunch/io/text/TextFileReaderFactory.java
index 851d199..3077cb4 100644
--- a/crunch-core/src/main/java/org/apache/crunch/io/text/TextFileReaderFactory.java
+++ b/crunch-core/src/main/java/org/apache/crunch/io/text/TextFileReaderFactory.java
@@ -20,6 +20,7 @@ package org.apache.crunch.io.text;
 import java.io.BufferedReader;
 import java.io.IOException;
 import java.io.InputStreamReader;
+import java.nio.charset.Charset;
 import java.util.Iterator;
 
 import org.apache.commons.logging.Log;
@@ -60,7 +61,7 @@ public class TextFileReaderFactory<T> implements FileReaderFactory<T>
{
       return Iterators.emptyIterator();
     }
 
-    final BufferedReader reader = new BufferedReader(new InputStreamReader(is));
+    final BufferedReader reader = new BufferedReader(new InputStreamReader(is, Charset.forName("UTF-8")));
     return new AutoClosingIterator<T>(reader, new UnmodifiableIterator<T>() {
       boolean nextChecked = false;
       private String nextLine;

http://git-wip-us.apache.org/repos/asf/crunch/blob/7d908c7c/crunch-core/src/main/java/org/apache/crunch/lib/Aggregate.java
----------------------------------------------------------------------
diff --git a/crunch-core/src/main/java/org/apache/crunch/lib/Aggregate.java b/crunch-core/src/main/java/org/apache/crunch/lib/Aggregate.java
index 3d132d4..df9310f 100644
--- a/crunch-core/src/main/java/org/apache/crunch/lib/Aggregate.java
+++ b/crunch-core/src/main/java/org/apache/crunch/lib/Aggregate.java
@@ -102,7 +102,11 @@ public class Aggregate {
     @Override
     public int compare(Pair<K, V> left, Pair<K, V> right) {
       int cmp = ((Comparable<V>) left.second()).compareTo(right.second());
-      return ascending ? cmp : -cmp;
+      if (ascending) {
+        return cmp;
+      } else {
+        return cmp == Integer.MIN_VALUE ? Integer.MAX_VALUE : -cmp;
+      }
     }
   }
 

http://git-wip-us.apache.org/repos/asf/crunch/blob/7d908c7c/crunch-core/src/main/java/org/apache/crunch/lib/SecondarySort.java
----------------------------------------------------------------------
diff --git a/crunch-core/src/main/java/org/apache/crunch/lib/SecondarySort.java b/crunch-core/src/main/java/org/apache/crunch/lib/SecondarySort.java
index 1c89cb9..a3ee840 100644
--- a/crunch-core/src/main/java/org/apache/crunch/lib/SecondarySort.java
+++ b/crunch-core/src/main/java/org/apache/crunch/lib/SecondarySort.java
@@ -100,8 +100,6 @@ public class SecondarySort {
     PTableType<Pair<K, V1>, Pair<V1, V2>> inter = ptf.tableOf(
         ptf.pairs(input.getKeyType(), valueType.getSubTypes().get(0)),
         valueType);
-    PTableType<K, Collection<Pair<V1, V2>>> out = ptf.tableOf(input.getKeyType(),
-        ptf.collections(input.getValueType()));
     GroupingOptions.Builder gob = GroupingOptions.builder()
         .requireSortedKeys()
         .groupingComparatorClass(JoinUtils.getGroupingComparator(ptf))

http://git-wip-us.apache.org/repos/asf/crunch/blob/7d908c7c/crunch-core/src/main/java/org/apache/crunch/lib/join/JoinUtils.java
----------------------------------------------------------------------
diff --git a/crunch-core/src/main/java/org/apache/crunch/lib/join/JoinUtils.java b/crunch-core/src/main/java/org/apache/crunch/lib/join/JoinUtils.java
index c717ab7..b4829be 100644
--- a/crunch-core/src/main/java/org/apache/crunch/lib/join/JoinUtils.java
+++ b/crunch-core/src/main/java/org/apache/crunch/lib/join/JoinUtils.java
@@ -60,7 +60,7 @@ public class JoinUtils {
   public static class TupleWritablePartitioner extends Partitioner<TupleWritable, Writable>
{
     @Override
     public int getPartition(TupleWritable key, Writable value, int numPartitions) {
-      return (Math.abs(key.get(0).hashCode()) & Integer.MAX_VALUE) % numPartitions;
+      return (key.get(0).hashCode() & Integer.MAX_VALUE) % numPartitions;
     }
   }
 
@@ -102,7 +102,7 @@ public class JoinUtils {
       } else {
         throw new UnsupportedOperationException("Unknown avro key type: " + key);
       }
-      return (Math.abs(record.get(0).hashCode()) & Integer.MAX_VALUE) % numPartitions;
+      return (record.get(0).hashCode() & Integer.MAX_VALUE) % numPartitions;
     }
   }
 

http://git-wip-us.apache.org/repos/asf/crunch/blob/7d908c7c/crunch-core/src/main/java/org/apache/crunch/lib/sort/ReverseWritableComparator.java
----------------------------------------------------------------------
diff --git a/crunch-core/src/main/java/org/apache/crunch/lib/sort/ReverseWritableComparator.java
b/crunch-core/src/main/java/org/apache/crunch/lib/sort/ReverseWritableComparator.java
index f356fc5..d578c7a 100644
--- a/crunch-core/src/main/java/org/apache/crunch/lib/sort/ReverseWritableComparator.java
+++ b/crunch-core/src/main/java/org/apache/crunch/lib/sort/ReverseWritableComparator.java
@@ -40,11 +40,20 @@ public class ReverseWritableComparator<T> extends Configured implements
RawCompa
 
   @Override
   public int compare(byte[] arg0, int arg1, int arg2, byte[] arg3, int arg4, int arg5) {
-    return -comparator.compare(arg0, arg1, arg2, arg3, arg4, arg5);
+    return safeNegate(comparator.compare(arg0, arg1, arg2, arg3, arg4, arg5));
   }
 
   @Override
   public int compare(T o1, T o2) {
-    return -comparator.compare(o1, o2);
+    return safeNegate(comparator.compare(o1, o2));
   }
+
+  /**
+   * @return an {@code int} definitely of the opposite sign as its argument. This is {@code
-i}
+   *  unless {@code i == Integer.MIN_VALUE} in which case it's {@code Integer.MAX_VALUE}
+   */
+  private static int safeNegate(int i) {
+    return i == Integer.MIN_VALUE ? Integer.MAX_VALUE : -i;
+  }
+
 }
\ No newline at end of file

http://git-wip-us.apache.org/repos/asf/crunch/blob/7d908c7c/crunch-core/src/main/java/org/apache/crunch/types/writable/Writables.java
----------------------------------------------------------------------
diff --git a/crunch-core/src/main/java/org/apache/crunch/types/writable/Writables.java b/crunch-core/src/main/java/org/apache/crunch/types/writable/Writables.java
index 89464ac..f571f2f 100644
--- a/crunch-core/src/main/java/org/apache/crunch/types/writable/Writables.java
+++ b/crunch-core/src/main/java/org/apache/crunch/types/writable/Writables.java
@@ -242,7 +242,7 @@ public class Writables {
   private static final MapFn<Boolean, BooleanWritable> BOOLEAN_TO_BW = new MapFn<Boolean,
BooleanWritable>() {
     @Override
     public BooleanWritable map(Boolean input) {
-      return input == Boolean.TRUE ? TRUE : FALSE;
+      return input ? TRUE : FALSE;
     }
   };
 

http://git-wip-us.apache.org/repos/asf/crunch/blob/7d908c7c/crunch-core/src/test/java/org/apache/crunch/lib/AvroIndexedRecordPartitionerTest.java
----------------------------------------------------------------------
diff --git a/crunch-core/src/test/java/org/apache/crunch/lib/AvroIndexedRecordPartitionerTest.java
b/crunch-core/src/test/java/org/apache/crunch/lib/AvroIndexedRecordPartitionerTest.java
index 0dfed32..ac8b89c 100644
--- a/crunch-core/src/test/java/org/apache/crunch/lib/AvroIndexedRecordPartitionerTest.java
+++ b/crunch-core/src/test/java/org/apache/crunch/lib/AvroIndexedRecordPartitionerTest.java
@@ -50,7 +50,7 @@ public class AvroIndexedRecordPartitionerTest {
     IndexedRecord indexedRecord = new MockIndexedRecord(-3);
     AvroKey<IndexedRecord> avroKey = new AvroKey<IndexedRecord>(indexedRecord);
 
-    assertEquals(3, avroPartitioner.getPartition(avroKey, new AvroValue<Object>(),
5));
+    assertEquals(0, avroPartitioner.getPartition(avroKey, new AvroValue<Object>(),
5));
     assertEquals(1, avroPartitioner.getPartition(avroKey, new AvroValue<Object>(),
2));
   }
 

http://git-wip-us.apache.org/repos/asf/crunch/blob/7d908c7c/crunch-core/src/test/java/org/apache/crunch/lib/TupleWritablePartitionerTest.java
----------------------------------------------------------------------
diff --git a/crunch-core/src/test/java/org/apache/crunch/lib/TupleWritablePartitionerTest.java
b/crunch-core/src/test/java/org/apache/crunch/lib/TupleWritablePartitionerTest.java
index aee185a..86c7437 100644
--- a/crunch-core/src/test/java/org/apache/crunch/lib/TupleWritablePartitionerTest.java
+++ b/crunch-core/src/test/java/org/apache/crunch/lib/TupleWritablePartitionerTest.java
@@ -43,7 +43,7 @@ public class TupleWritablePartitionerTest {
     IntWritable intWritable = new IntWritable(3);
     BytesWritable bw = new BytesWritable(WritableUtils.toByteArray(intWritable));
     TupleWritable key = new TupleWritable(new Writable[] { bw });
-    assertEquals(1, tupleWritableParitioner.getPartition(key, NullWritable.get(), 5));
+    assertEquals(2, tupleWritableParitioner.getPartition(key, NullWritable.get(), 5));
     assertEquals(0, tupleWritableParitioner.getPartition(key, NullWritable.get(), 2));
   }
 }

http://git-wip-us.apache.org/repos/asf/crunch/blob/7d908c7c/crunch-hbase/src/main/java/org/apache/crunch/io/hbase/HFileUtils.java
----------------------------------------------------------------------
diff --git a/crunch-hbase/src/main/java/org/apache/crunch/io/hbase/HFileUtils.java b/crunch-hbase/src/main/java/org/apache/crunch/io/hbase/HFileUtils.java
index 9fcd747..cb82dd8 100644
--- a/crunch-hbase/src/main/java/org/apache/crunch/io/hbase/HFileUtils.java
+++ b/crunch-hbase/src/main/java/org/apache/crunch/io/hbase/HFileUtils.java
@@ -17,6 +17,21 @@
  */
 package org.apache.crunch.io.hbase;
 
+import static org.apache.crunch.types.writable.Writables.bytes;
+import static org.apache.crunch.types.writable.Writables.nulls;
+import static org.apache.crunch.types.writable.Writables.tableOf;
+
+import java.io.IOException;
+import java.io.Serializable;
+import java.nio.ByteBuffer;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.List;
+import java.util.Map;
+import java.util.NavigableSet;
+import java.util.Set;
+
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Lists;
@@ -50,19 +65,6 @@ import org.apache.hadoop.io.NullWritable;
 import org.apache.hadoop.io.RawComparator;
 import org.apache.hadoop.io.SequenceFile;
 
-import java.io.IOException;
-import java.io.Serializable;
-import java.nio.ByteBuffer;
-import java.util.Arrays;
-import java.util.Collections;
-import java.util.Comparator;
-import java.util.List;
-import java.util.Map;
-import java.util.NavigableSet;
-import java.util.Set;
-
-import static org.apache.crunch.types.writable.Writables.*;
-
 public final class HFileUtils {
 
   private static final Log LOG = LogFactory.getLog(HFileUtils.class);
@@ -100,7 +102,9 @@ public final class HFileUtils {
     }
 
     private int compareTimestamp(KeyValue l, KeyValue r) {
-      return -Longs.compare(l.getTimestamp(), r.getTimestamp());
+      // These arguments are intentionally reversed, with r then l, to sort
+      // the timestamps in descending order as is expected by HBase
+      return Longs.compare(r.getTimestamp(), l.getTimestamp());
     }
 
     private int compareType(KeyValue l, KeyValue r) {

http://git-wip-us.apache.org/repos/asf/crunch/blob/7d908c7c/crunch-hbase/src/main/java/org/apache/crunch/io/hbase/HTableIterator.java
----------------------------------------------------------------------
diff --git a/crunch-hbase/src/main/java/org/apache/crunch/io/hbase/HTableIterator.java b/crunch-hbase/src/main/java/org/apache/crunch/io/hbase/HTableIterator.java
index daa4a48..bfe1439 100644
--- a/crunch-hbase/src/main/java/org/apache/crunch/io/hbase/HTableIterator.java
+++ b/crunch-hbase/src/main/java/org/apache/crunch/io/hbase/HTableIterator.java
@@ -51,7 +51,7 @@ class HTableIterator implements Iterator<Pair<ImmutableBytesWritable,
Result>> {
       try {
         table.close();
       } catch (IOException e) {
-        LOG.error("Exception closing HTable: " + table.getTableName(), e);
+        LOG.error("Exception closing HTable: " + table.getName(), e);
       }
     }
     return hasNext;


Mime
View raw message