You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@crunch.apache.org by gr...@apache.org on 2014/04/25 22:13:38 UTC
git commit: CRUNCH-380 Improvements based on Coverity scan
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 <gr...@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 <so...@cloudera.com>
Authored: Thu Apr 24 10:20:20 2014 +0100
Committer: Gabriel Reid <gr...@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;