Hi all, I'd like to run by you all a little bit of analysis I did a while ago on Crunch (https://issues.apache.org/jira/browse/CRUNCH-380) and wanted to try again. I've been experimenting again with static analysis, in particular Coverity, and gave it another pass over the Crunch code.
It turned up a few minor potential issues, and a number of reasonable touch-ups, so I wanted to propose another omnibus change to zap several of them. You can see the current state at https://scan.coverity.com/projects/1983?tab=overview but it does require login. As a preview, the minor touch-ups are: - Replace use of old junit.framework.* with org.unit.* for consistency - Remove some unused imports - String.getBytes() -> String.getBytes(Charset) to avoid platform dependence - Remove a few dead stores - Replace one Map.keySet() + many get()s with Map.entrySet() iteration - Remove a few @Nullable on method args that can't be (immediately dereferenced) - Closing some objects in a finally block that are Closable - Math.abs is technically a bad idea for something that can == Integer.MIN_VALUE There are a few changes that might be minor bug fixes: Aggregators:1059 "maxInputLength > 0 && next.length() > maxInputLength" also needs a check for next != null, but doesn't the second clause also need parentheses? TupleWritable:337 The call to skip() doesn't check that the expected number of bytes were skipped. OrcWritable Missing hashCode for equals WritableGroupedTableType:97 options is checked for null but is always dereferenced at the end CrunchOutputs:201 baseContext can't be null at this point because of line 192 SparkRuntime:342 Not a bug but redundant I think since this occurs inside a block also guarded by "if (t instance MapReduceTarget) {" Does some of this sound worth a look at a patch? I've actually got it ready and can make a JIRA.
