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 <[email protected]> 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 <[email protected]> Authored: Thu Apr 24 10:20:20 2014 +0100 Committer: Gabriel Reid <[email protected]> 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;
