hbase-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Apekshit Sharma (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HBASE-6919) Remove unnecessary cast from Bytes.readVLong
Date Thu, 26 Mar 2015 20:37:53 GMT

    [ https://issues.apache.org/jira/browse/HBASE-6919?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14382619#comment-14382619
] 

Apekshit Sharma commented on HBASE-6919:
----------------------------------------

Here are few change suggestions I came up with, requesting community's views on them.

1) deprecate IOException in readVLong:
(From offline discussions with Jon) it seems that code can never throw IOException. Should
we remove the exception? Since this thread is from before compatibility standards, it might
be a bit more difficult to do this change now. One possible way is:
a) add new function, say toVLong(...) (to keep name consistent with ~40 other toXYZ() functions)
without exception and deprecate current readVLong() in release in 1.1
b) Remove the deprecated function in 2.0 release

2) Rename bytesToInt() to toVInt() :
- This is the only function in Bytes class with name like bytesToXX(). I think we should rename
it to toVInt() to be consistent with other functions.
- Also, I am confused as to why it returns a long. Although technically correct, shouldn't
it return an int instead? Should we change it or is there a reason it's that way?

3) Add following functions. Alternatively we can use the names writeVLong / writeVInt, any
preferences?
{code}
int putVLong(byte[] bytes, int offset, long val);
int putVInt(byte[] bytes, int offset, int val);
{code}

4) code duplication: I see code duplication in lots of functions (grouped together below)
{code}
byte [] vintToBytes(final long vint);
void WritableUtils.writeVInt(DataOutput stream, int i);
(proposed above) int putVLong(byte[] bytes, int offset, long val);
(proposed above) int putVInt(byte[] bytes, int offset, int val);
{code}
{code}
long readVLong(final byte [] buffer, final int offset);
long bytesToVint(final byte [] buffer);
int WritableUtils.readVInt((DataInput stream) ;
{code}
(From discussion with Jon) It's possible that the duplication is for efficiency, however I
don't see any comments suggesting that. What do others think? Should we change them to use
WritableUtils class' functions which do exactly same thing but accept DataInput/DataOutput
as params instead of byte[].

5) Description above suggests to add vlongFromBytes(ImmutableBytesWritable ptr) and vintFromBytes(ImmutableBytesWritable
ptr). However I think that it'll be redundant code as one can simply use toVLong(ptr.get())
or toVInt(ptr.get()) instead. Should we still have them?

> Remove unnecessary cast from Bytes.readVLong
> --------------------------------------------
>
>                 Key: HBASE-6919
>                 URL: https://issues.apache.org/jira/browse/HBASE-6919
>             Project: HBase
>          Issue Type: Improvement
>            Reporter: James Taylor
>            Priority: Minor
>              Labels: beginners
>
> Remove the throws IOException so that caller doesn't have to catch and ignore.
> {code}
>   public static long readVLong(final byte [] buffer, final int offset)
>   throws IOException
> {code}
> Also, add
> {code}
>   public static int readVInt(final byte [] buffer, final int offset)
>   throws IOException {
>     return (int)readVLong(buffer,offset);
>   }
> {code}
> and these are useful too:
> {code}
>     /**
>      * Put long as variable length encoded number at the offset in
>      * the result byte array.
>      * @param vint Integer to make a vint of.
>      * @param result buffer to put vint into
>      * @return Vint length in bytes of vint
>      */
>     public static int vintToBytes(byte[] result, int offset, final long vint) {
>       long i = vint;
>       if (i >= -112 && i <= 127) {
>         result[offset] = (byte) i;
>         return 1;
>       }
>       int len = -112;
>       if (i < 0) {
>         i ^= -1L; // take one's complement'
>         len = -120;
>       }
>       long tmp = i;
>       while (tmp != 0) {
>         tmp = tmp >> 8;
>         len--;
>       }
>       result[offset++] = (byte) len;
>       len = (len < -120) ? -(len + 120) : -(len + 112);
>       for (int idx = len; idx != 0; idx--) {
>         int shiftbits = (idx - 1) * 8;
>         long mask = 0xFFL << shiftbits;
>         result[offset++] = (byte)((i & mask) >> shiftbits);
>       }
>       return len + 1;
>     }
>     /**
>      * Decode a vint from the buffer pointed at to by ptr and
>      * increment the offset of the ptr by the length of the
>      * vint.
>      * @param ptr a pointer to a byte array buffer
>      * @return the decoded vint value as an int
>      */
>     public static int vintFromBytes(ImmutableBytesWritable ptr) {
>         return (int) vlongFromBytes(ptr);
>     }
>     
>     /**
>      * Decode a vint from the buffer pointed at to by ptr and
>      * increment the offset of the ptr by the length of the
>      * vint.
>      * @param ptr a pointer to a byte array buffer
>      * @return the decoded vint value as a long
>      */
>     public static long vlongFromBytes(ImmutableBytesWritable ptr) {
>         final byte [] buffer = ptr.get();
>         final int offset = ptr.getOffset();
>         byte firstByte = buffer[offset];
>         int len = WritableUtils.decodeVIntSize(firstByte);
>         if (len == 1) {
>             ptr.set(buffer, offset+1, ptr.getLength());
>             return firstByte;
>         }
>         long i = 0;
>         for (int idx = 0; idx < len-1; idx++) {
>             byte b = buffer[offset + 1 + idx];
>             i = i << 8;
>             i = i | (b & 0xFF);
>         }
>         ptr.set(buffer, offset+len, ptr.getLength());
>         return (WritableUtils.isNegativeVInt(firstByte) ? ~i : i);
>     }
> {code}



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Mime
View raw message