Dan Hecht has posted comments on this change. Change subject: IMPALA-2107: Add Base64 encoder/decoder ......................................................................
Patch Set 2: (5 comments) could you also test the error paths? http://gerrit.cloudera.org:8080/#/c/2633/2/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: Line 112: * spacing http://gerrit.cloudera.org:8080/#/c/2633/2/be/src/exprs/string-functions.cc File be/src/exprs/string-functions.cc: Line 804: if (UNLIKELY(outmax > static_cast<unsigned>(std::numeric_limits<int>::max()))) { does this catch all possible overflows? what if str.len == max or max-1? i guess this should catch that. is there some value of str.len that leads to outmax of 0? I guess not given the 4:3 ratio but haven't thought very hard. Also, should be out_max, out_len etc. Line 814: outmax maybe use result.len here and line below. but feel free to ignore if you find it clearer with outmax. Line 840: outmax out_max Line 849: outlen out_len -- 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: 2 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
