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 <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-HasComments: Yes
