[Impala-CR](cdh5-trunk) IMPALA-2107: Add Base64 encoder/decoder

["Dan Hecht (Code Review)" <ge...@cloudera.org>]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-2107: Add Base64 encoder/decoder
......................................................................


Patch Set 1:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/2633/1/be/src/exprs/string-functions.cc
File be/src/exprs/string-functions.cc:

Line 797:   if (str.is_null) return StringVal::null();
does encode need the len==0 check like you have in decode?


Line 803:   uint8_t* const out = ctx->Allocate(outmax);
> It would be possible to do this without two allocation/deallocation pairs a
FWIW, looks like Translate() already does that. 
In any case, if you keep this scratch allocation, you need to check for Allocate() returning NULL.


Line 807: -
spacing


Line 807: (SASL_OK != encode_result) || (outlen != outmax-1)
two sets of extraneous parenthesis.


Line 825: bits
bits or characters? if not characters, why would outmax be adjusted?  also, why is it worth adjusting outmax anyway, if we're just going to reallocate the proper sized string.


Line 828:   // output.
why does = at the end have this behavior?


Line 829: (str.len/4)
should this round up also?  spacing is also off.


-- 
To view, visit http://gerrit.cloudera.org:8080/2633
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I911451c5d68e8ae9d352abfcf4d5ff36484f0bf3
Gerrit-PatchSet: 1
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Jim Apple <jb...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes