Mailing-List: contact dev-help@crunch.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@crunch.apache.org
MIME-Version: 1.0
From: Sean Owen <sowen@cloudera.com>
Date: Tue, 19 May 2015 22:54:24 +0100
Message-ID: 
 <CAMAsSdJd=AeAZwC4cvaGKouzYnKKz3=S0qjtHWveme-oDWAjug@mail.gmail.com>
Subject: Another Coverity scan?
To: dev@crunch.apache.org
Content-Type: multipart/alternative; boundary=001a113746ccc5c2060516765b1d

--001a113746ccc5c2060516765b1d
Content-Type: text/plain; charset=UTF-8

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.

--001a113746ccc5c2060516765b1d--