Jim Apple 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? It doesn't need it, but I have added it for efficiency and clarity. Line 803: uint8_t* const out = ctx->Allocate(outmax); > FWIW, looks like Translate() already does that. I'll use the Translate() method and just adjust .len. Thanks for pointing that out! I've added error checking for if the StringVal allocation fails. Line 807: - > spacing Done Line 807: (SASL_OK != encode_result) || (outlen != outmax-1) > two sets of extraneous parenthesis. Done Line 825: bits > bits or characters? if not characters, why would outmax be adjusted? also, 1. bits. 2. Not sure I understand the question about outmax adjusting if the answer is "bits, not characters". Could you elaborate? 3. I've removed the reallocate. I can remove the adjustment to outmax if you think it is worth it, but I think it is useful error checking. Line 828: // output. > why does = at the end have this behavior? For compatibility with other base64 encoding systems. The original reason for it is not very clear in the RFC: "In the general case, when assumptions about the size of transported data cannot be made, padding is required to yield correct decoded data." Line 829: (str.len/4) > should this round up also? spacing is also off. str.len should always be a multiple of four. I've added error checking. -- 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
