impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-CR](cdh5-trunk) IMPALA-2107: Add Base64 encoder/decoder
Date Tue, 12 Apr 2016 00:20:46 GMT
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 <jbapple@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Jim Apple <jbapple@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message