Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/14963 )
Change subject: IMPALA-9010: Add builtin mask functions ...................................................................... Patch Set 1: Code-Review+1 (9 comments) Great work, found a couple of nits, but other than that lgtm. http://gerrit.cloudera.org:8080/#/c/14963/1/be/src/exprs/expr-test.cc File be/src/exprs/expr-test.cc: http://gerrit.cloudera.org:8080/#/c/14963/1/be/src/exprs/expr-test.cc@10147 PS1, Line 10147: valid invalid? or 'invalid digit' maybe? http://gerrit.cloudera.org:8080/#/c/14963/1/be/src/exprs/mask-functions-ir.cc File be/src/exprs/mask-functions-ir.cc: http://gerrit.cloudera.org:8080/#/c/14963/1/be/src/exprs/mask-functions-ir.cc@46 PS1, Line 46: static uint8_t MaskTransform(uint8_t val, int masked_upper_char, int masked_lower_char, It's only called from the loop of MaskSubStr, maybe it'd be beneficial to mark it as 'inline', or even 'ALWAYS_INLINE'. http://gerrit.cloudera.org:8080/#/c/14963/1/be/src/exprs/mask-functions-ir.cc@141 PS1, Line 141: return num_digits; What about the value 0? Shouldn't it have one digit? Maybe not because 'mask(0)' in hive returns 0, while any other values are masked out, but it seems strange to me. http://gerrit.cloudera.org:8080/#/c/14963/1/be/src/exprs/mask-functions-ir.cc@145 PS1, Line 145: keep nit: 'be kept'? http://gerrit.cloudera.org:8080/#/c/14963/1/be/src/exprs/mask-functions-ir.cc@171 PS1, Line 171: keep nit: 'be kept' http://gerrit.cloudera.org:8080/#/c/14963/1/be/src/exprs/mask-functions-ir.cc@242 PS1, Line 242: GetCharFromString maybe 'GetFirstChar()' would be more precise and shorter? http://gerrit.cloudera.org:8080/#/c/14963/1/be/src/exprs/mask-functions-ir.cc@260 PS1, Line 260: / nit: too many slashes http://gerrit.cloudera.org:8080/#/c/14963/1/be/src/exprs/mask-functions-ir.cc@304 PS1, Line 304: / nit: too many slashes http://gerrit.cloudera.org:8080/#/c/14963/1/be/src/exprs/mask-functions.h File be/src/exprs/mask-functions.h: http://gerrit.cloudera.org:8080/#/c/14963/1/be/src/exprs/mask-functions.h@18 PS1, Line 18: nit: extra blank line -- To view, visit http://gerrit.cloudera.org:8080/14963 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ica779a1bf63a085d51f3b533f654cbaac102a664 Gerrit-Change-Number: 14963 Gerrit-PatchSet: 1 Gerrit-Owner: Quanlong Huang <[email protected]> Gerrit-Reviewer: Fang-Yu Rao <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Kurt Deschler <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Mon, 06 Jan 2020 14:45:48 +0000 Gerrit-HasComments: Yes
