I totally agree with you about the ease of maintenance. That was actually
my initial refactoring. But then I thought that if we pushed the check
condition down to the base class then it will cost some stack operations
for passing parameters. If we times this with millions of cells to convert
then that could cause some performance hit. So I decided to leave the check
conditions at the descent classes instead of the base class. Plus if some
one wonder about if the condition (start < length) has been checked or not,
they will have the answer right there instead of having to go down one more
step to the base class. But maybe it's me over thinking about performance.
;-) Anyway, I pushed the new refactored code to the repo. The null check
(start < length) has to be before the nonNumeric check otherwise it defeats
the purpose of the check.


On Mon, Jan 13, 2014 at 10:15 PM, Christopher Nguyen <
notifications@github.com> wrote:

> In serde/src/java/org/apache/hadoop/hive/serde2/lazy/LazyLong.java:
>
> > @@ -51,6 +51,10 @@ public LazyLong(LazyLong copy) {
> >
> >    @Override
> >    public void init(ByteArrayRef bytes, int start, int length) {
> > +    if (start >= length || isNonNumeric(bytes.getData(), start)) {
>
> I would refactor this snippet out to the base class LazyPrimitive, and
> make the check simpler/easier to maintain:
>
> if (isNull(bytes, start)) return;
>
> Also I would put the isNonNumeric() check before the start >= lengthcheck as the probability of
> isNonNumeric() is likely far higher.
>
> —
> Reply to this email directly or view it on GitHub<https://github.com/adatao/hive/pull/1/files#r8850283>
> .
>