Dan Hecht has posted comments on this change. Change subject: IMPALA-2107: Add Base64 encoder/decoder ......................................................................
Patch Set 4: (8 comments) http://gerrit.cloudera.org:8080/#/c/2633/4/be/src/exprs/string-functions.cc File be/src/exprs/string-functions.cc: Line 799: characters how about saying "bytes" here to help differentiate between input (which can be arbitrary bytes) and output (which is one of 64 characters). Line 817: AnyValUtil::ToString(str) Might be really long. I'm a bit worried about printing the whole thing. what do you think? Line 829: characters likewise, how about saying "bytes" here. Line 830: (which represents 6-bits) Line 831: . (for a total of 8 bits, so there is one less byte of output). just to help explain things to readers who aren't so familiar with base64 encoding. Line 832: character byte Line 836: ss << "Invalid base64 string \"" << AnyValUtil::ToString(str) << "\""; maybe be more specific about the reason (length not a multiple of 4). Also, str might be really long. Maybe just print the length? Line 854: AnyValUtil::ToString(str) same thing. this might be really long, right? -- 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: 4 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
